Use unique declaration references #192

valerybugakov · 2022-10-27T06:57:44Z

Fixes #152

The too many references issue was caused by the fact that ts has multiple definitions in the same file, and we used this information to create a reference for each namespace ts block.

This PR removes redundant references by filtering out duplicated SCIP symbol values.
This PR also fixes SCIP symbol generation for namespace imports. Covered by the updated snapshot test.

Test plan

See the updated snapshots/input/syntax/src/namespace.d.ts spanshot test.

package.json

varungandhi-src · 2022-10-27T11:56:36Z

@valerybugakov I think you accidentally committed generated files. I've opened a PR to fix the .gitignore. #193

valerybugakov · 2022-10-27T12:01:30Z

@varungandhi-src, approved your PR 👍

src/FileIndexer.ts

olafurpg · 2022-10-27T12:35:20Z

Please wait with merging until I've reviewed.

src/FileIndexer.ts

olafurpg

@valerybugakov Thank you for this contribution! There are several independent changes in this PR that make it harder to review than necessary. I opened a separate PR #194 to demonstrate how the issue can be fixed with a smaller diff. In the future, I encourage you to try and open separate PRs for independent changes. It's especially important that functional changes like the new ts.isNamespaceImport(node) condition are documented with a minimized diff in the git history.

olafurpg · 2022-10-27T13:43:19Z

package.json

@@ -11,8 +11,8 @@
    "tslint": "tslint -p tsconfig.json --format stylish",
    "eslint": "eslint --cache '**/*.ts?(x)'",
    "build": "node ./node_modules/typescript/bin/tsc -b .",
-    "test": "uvu -r ts-node/register --ignore dist",
-    "update-snapshots": "uvu -r ts-node/register --ignore dist --update-snapshots",
+    "test": "TS_NODE_TRANSPILE_ONLY=true uvu -r ts-node/register --ignore dist",


I added new test-fast and update-snapshots-fast scripts. I prefer to get the type errors in the terminal because typechecking is reasonably fast in this codebase and debugging runtime exceptions is slower than debugging type errors.

Sounds good! To clarify the reasoning behind this: while iterating on changes locally, I often have type errors because I want to test some logic but don't want to invest time into making Typescript happy about it because there's no intent to commit it. Type-checking slows down the process of trying different ideas on existing tests.

I'm +1 to adding these scripts to package.json as long as we keep the typechecking variants as well. For sg/sg, I appreciate that sg start doesn't typecheck because it's a big codebase slowing down the edit/reload/debug feedback loop. In scip-typescript, typechecking has been fast enough for my needs.

src/FileIndexer.ts

olafurpg · 2022-10-27T14:05:58Z

src/FileIndexer.ts

+    if (
+      ts.isImportSpecifier(node) ||
+      ts.isImportClause(node) ||
+      ts.isNamespaceImport(node)


Is this change related to de-duplication? It would be nice to isolate this change and have a diff in the snapshot tests to better understand the exact motivation for this change.

olafurpg · 2022-10-27T14:16:32Z

src/FileIndexer.ts

+      return []
+    }
+
+    return Array.from(


This expression is fairly expensive considering it's in a very hot loop and that we can achieve a similar results with a single if condition, as is done in
#194

It's very important that we try to keep performance parity with tsc -b and the fewer redundant collections we allocate the better.

olafurpg · 2022-10-27T14:23:30Z

src/FileIndexer.ts

-    const isDefinition = this.declarationName(node.parent) === node
-    if (isDefinition) {
-      role |= scip.scip.SymbolRole.Definition
+  private visitDefinition(node: ts.Node, sym: ts.Symbol): void {


I'm not convinced this refactoring is an improvement in maintainability. In the future, I want to add more symbol roles such as import and write and then the symbol_roles bitmask will need to be updated in two different places.

olafurpg · 2022-10-27T14:25:52Z

src/FileIndexer.ts

@@ -65,7 +65,21 @@ export class FileIndexer {
    if (ts.isIdentifier(node) || ts.isStringLiteralLike(node)) {
      const sym = this.getTSSymbolAtLocation(node)
      if (sym) {
-        this.visitSymbolOccurrence(node, sym)
+        /**


I agree it's not super clear why this.declarationName(node.parent) === node identifies a definition. To improve readability, I'd prefer to add a function private isDefinition(...): boolean with a docstring than adding an inline comment like this.

Use unique declaration references

74c59b9

valerybugakov requested review from olafurpg and varungandhi-src October 27, 2022 06:57

valerybugakov self-assigned this Oct 27, 2022