Repository: kythe/kythe
Pull request #5527 (2023-02-24 22:35:43)

feat(typescript_indexer): implement ref inlining for imported symbols
The goal of this PR is to connect usages of imported symbol to the its declaration from original file. Today indexer produces the following graph:

```ts
// foo.ts
//- @ANSWER defines/binding AnswerOrig
export const ANSWER = 42;


// bar.ts
//- @ANSWER ref/imports AnswerOrig
//- @ANSWER defines/binding AnswerLocal
import {ANSWER} from './foo';

//- @ANSWER ref AnswerLocal
console.log(ANSWER);
```

After this PR the graph will look like this:

```ts
// foo.ts
//- @ANSWER defines/binding AnswerOrig
export const ANSWER = 42;


// bar.ts
//- @ANSWER ref/imports AnswerOrig
import {ANSWER} from './foo';

//- @ANSWER ref AnswerOrig
console.log(ANSWER);
```

Note that `AnswerLocal` is gone. 

The change is guarded by `enableImportsEdgeReassignment` flag. This will allow us to easier roll it out at Google. 

This is related to #4021.

nbeloglazov: @jaysachs bump

Patch:
diff --git a/.gitignore b/.gitignore
index 2b06bb933..8e225d69a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -14,6 +14,7 @@ MANIFEST-*
 .vscode
 .clangd
 .cache
+.idea
 
 # Binary files
 *.class
diff --git a/kythe/typescript/indexer.ts b/kythe/typescript/indexer.ts
index 1e609a352..6a1133278 100644
--- a/kythe/typescript/indexer.ts
+++ b/kythe/typescript/indexer.ts
@@ -76,6 +76,12 @@ export interface IndexingOptions {
    * is used.
    */
   readFile?: (path: string) => Buffer;
+
+  /**
+   * Enables post processing that "inlines" refs to imported symbols to point instead
+   * to the original definition.
+   */
+  enableImportsEdgeReassignment?: boolean;
 }
 
 /**
@@ -245,6 +251,14 @@ function todo(sourceRoot: string, node: ts.Node, message: string) {
   console.warn(`TODO: ${file}:${line}:${character}: ${message}`);
 }
 
+/**
+ * Convert VName to a string that can be used as key in Maps.
+ */
+function vnameToString(vname: VName): string {
+  return `(${vname.corpus},${vname.language},${vname.path},${vname.root},${
+      vname.signature})`;
+}
+
 type NamespaceAndContext = string&{__brand: 'nsctx'};
 /**
  * A SymbolVNameStore stores a mapping of symbols to the (many) VNames it may
@@ -793,6 +807,18 @@ class Visitor {
   /** Cached anchor nodes. Signature is used as key. */
   private readonly anchors = new Map<string, VName>();
 
+  /**
+   * Mapping from imported values to their remote definitions. For cases like
+   *
+   * import {Foo} from './dep';
+   * new Foo();
+   *
+   * We don't produce a defition to Foo symbol. Instead all refs ot it will
+   * point to the remote Foo from dep.ts. This map contains mapping from
+   * VName of local Foo to the VName of the remote Foo.
+   */
+  private readonly localSymbolToRemoteReassignMap = new Map<string, VName>();
+
   constructor(
       private readonly host: IndexerHost,
       private file: ts.SourceFile,
@@ -860,6 +886,7 @@ class Visitor {
 
   /** emitEdge emits a new edge entry, relating two VNames. */
   emitEdge(source: VName, kind: EdgeKind|OrdinalEdge, target: VName) {
+    target = this.localSymbolToRemoteReassignMap.get(vnameToString(target)) ?? target;
     this.host.options.emit({
       source,
       edge_kind: kind,
@@ -1288,11 +1315,13 @@ class Visitor {
    *
    * @param name TypeScript import declaration node
    * @param bindingAnchor anchor that "defines/binding" the local import
-   *     definition
+   *     definition. If the bindingAnchor is null then the local definition
+   *     won't be created and refs to the local definition will be reassigned
+   *     to the remote definition.
    * @param refAnchor anchor that "ref" the import's remote declaration
    */
   visitImport(
-      name: ts.Node, bindingAnchor: Readonly<VName>,
+      name: ts.Node, bindingAnchor: Readonly<VName>|null,
       refAnchor: Readonly<VName>) {
     // An import both aliases a symbol from another module
     // (call it the remote symbol) and it defines a local symbol.
@@ -1325,15 +1354,17 @@ class Visitor {
       const kLocalValue = this.host.getSymbolName(localSym, TSNamespace.VALUE);
       if (!kRemoteValue || !kLocalValue) return;
 
-      // The local import value is a "variable" with an "import" subkind, and
-      // aliases its remote definition.
-      this.emitNode(kLocalValue, NodeKind.VARIABLE);
-      this.emitFact(kLocalValue, FactName.SUBKIND, Subkind.IMPORT);
-      this.emitEdge(kLocalValue, EdgeKind.ALIASES, kRemoteValue);
-
-      // Emit edges from the binding and referencing anchors to the import's
-      // local and remote definition, respectively.
-      this.emitEdge(bindingAnchor, EdgeKind.DEFINES_BINDING, kLocalValue);
+      if (bindingAnchor == null) {
+        this.localSymbolToRemoteReassignMap.set(vnameToString(kLocalValue), kRemoteValue);
+      } else {
+        // The local import value is a "variable" with an "import" subkind, and
+        // aliases its remote definition.
+        this.emitNode(kLocalValue, NodeKind.VARIABLE);
+        this.emitFact(kLocalValue, FactName.SUBKIND, Subkind.IMPORT);
+        this.emitEdge(kLocalValue, EdgeKind.ALIASES, kRemoteValue);
+        this.emitEdge(bindingAnchor, EdgeKind.DEFINES_BINDING, kLocalValue);
+      }
+      // Emit edges from the referencing anchor to the import's remote definition.
       this.emitEdge(refAnchor, EdgeKind.REF_IMPORTS, kRemoteValue);
     }
     if (remoteSym.flags & ts.SymbolFlags.Type) {
@@ -1341,15 +1372,17 @@ class Visitor {
       const kLocalType = this.host.getSymbolName(localSym, TSNamespace.TYPE);
       if (!kRemoteType || !kLocalType) return;
 
-      // The local import value is a "talias" (type alias) with an "import"
-      // subkind, and aliases its remote definition.
-      this.emitNode(kLocalType, NodeKind.TALIAS);
-      this.emitFact(kLocalType, FactName.SUBKIND, Subkind.IMPORT);
-      this.emitEdge(kLocalType, EdgeKind.ALIASES, kRemoteType);
-
-      // Emit edges from the binding and referencing anchors to the import's
-      // local and remote definition, respectively.
-      this.emitEdge(bindingAnchor, EdgeKind.DEFINES_BINDING, kLocalType);
+      if (bindingAnchor == null) {
+        this.localSymbolToRemoteReassignMap.set(vnameToString(kLocalType), kRemoteType);
+      } else {
+        // The local import value is a "talias" (type alias) with an "import"
+        // subkind, and aliases its remote definition.
+        this.emitNode(kLocalType, NodeKind.TALIAS);
+        this.emitFact(kLocalType, FactName.SUBKIND, Subkind.IMPORT);
+        this.emitEdge(kLocalType, EdgeKind.ALIASES, kRemoteType);
+        this.emitEdge(bindingAnchor, EdgeKind.DEFINES_BINDING, kLocalType);
+      }
+      // Emit edges from the referencing anchor to the import's remote definition.
       this.emitEdge(refAnchor, EdgeKind.REF_IMPORTS, kRemoteType);
     }
   }
@@ -1438,13 +1471,15 @@ class Visitor {
     const importTextAnchor =
         this.newAnchor(decl, importTextSpan.start, importTextSpan.end);
 
+    const enableReassignment = this.host.options.enableImportsEdgeReassignment;
     if (ts.isImportEqualsDeclaration(decl)) {
       // This is an equals import, e.g.:
       //   import foo = require('./bar');
       //
       // TODO(#4021): Bind the local definition and reference the remote
       // definition on the import name.
-      this.visitImport(decl.name, importTextAnchor, this.newAnchor(decl.name));
+      const refAnchor = this.newAnchor(decl.name)
+      this.visitImport(decl.name, enableReassignment ? null : importTextAnchor, refAnchor);
       return;
     }
 
@@ -1461,8 +1496,9 @@ class Visitor {
       //
       // TODO(#4021): Bind the local definition and reference the remote
       // definition on the import name.
+      const refAnchor = this.newAnchor(clause.name);
       this.visitImport(
-          clause.name, importTextAnchor, this.newAnchor(clause.name));
+          clause.name, enableReassignment ? null : importTextAnchor, refAnchor);
       return;
     }
 
@@ -1507,8 +1543,8 @@ class Visitor {
             bindingAnchor = this.newAnchor(imp.name);
             refAnchor = this.newAnchor(imp.propertyName);
           } else {
-            bindingAnchor = importTextAnchor;
             refAnchor = this.newAnchor(imp.name);
+            bindingAnchor = enableReassignment ? null : importTextAnchor;
           }
           this.visitImport(imp.name, bindingAnchor, refAnchor);
         }


Test patch:
diff --git a/kythe/typescript/test.ts b/kythe/typescript/test.ts
index cdaa57534..e83559579 100644
--- a/kythe/typescript/test.ts
+++ b/kythe/typescript/test.ts
@@ -109,6 +109,7 @@ function verify(
   for (const file of testFiles) {
     fileVNames.set(file, {...rootVName, path: file});
   }
+  const enableEdgeReassignment = testCase.name.includes('enable_edge_reassignment');
 
   try {
     const compilationUnit: indexer.CompilationUnit = {
@@ -124,6 +125,7 @@ function verify(
         verifier.stdin.write(JSON.stringify(obj) + '\n');
       },
       plugins,
+      enableImportsEdgeReassignment: enableEdgeReassignment,
     });
   } finally {
     // Ensure we close stdin on the verifier even on crashes, or otherwise
diff --git a/kythe/typescript/testdata/imports_enable_edge_reassignment_group/export.ts b/kythe/typescript/testdata/imports_enable_edge_reassignment_group/export.ts
new file mode 100644
index 000000000..a42fab165
--- /dev/null
+++ b/kythe/typescript/testdata/imports_enable_edge_reassignment_group/export.ts
@@ -0,0 +1,12 @@
+// Syntax 1: exporting a value from another module.
+//- @"'./module'" ref/imports Mod
+export {value} from './module';
+
+// Syntax 2: a bare "export" statement.
+//- @local defines/binding Local
+const local = 3;
+//- @local ref Local
+export {local};
+//- @local ref Local
+//- @aliasedLocal ref Local
+export {local as aliasedLocal};
diff --git a/kythe/typescript/testdata/imports_enable_edge_reassignment_group/import.ts b/kythe/typescript/testdata/imports_enable_edge_reassignment_group/import.ts
new file mode 100644
index 000000000..90a873251
--- /dev/null
+++ b/kythe/typescript/testdata/imports_enable_edge_reassignment_group/import.ts
@@ -0,0 +1,47 @@
+// This test imports the same value in a variety of names and ensures
+// they all resolve to the same VName.
+
+//- @"'./module'" ref/imports Mod
+import './module';
+
+//- @mod_imp defines/binding ModLocal
+//- @"'./module'" ref/imports Mod
+import * as mod_imp from './module';
+
+// Importing from a module gets a VName that refers into the other module.
+//- @value ref/imports Val
+//- @"'./module'" ref/imports Mod
+import {value} from './module';
+
+// Importing from a module gets a VName that refers into the other module,.
+//- @value ref/imports Val
+//- @renamedValue defines/binding RenamedValue
+//- RenamedValue.node/kind variable
+//- RenamedValue.subkind import
+//- RenamedValue aliases Val
+import {value as renamedValue} from './module';
+
+// Ensure the various names of the imported value link together.
+
+//- @value ref Val
+value;
+
+//- @mod_imp ref ModLocal
+//- @value ref Val
+mod_imp.value;
+
+//- @renamedValue ref RenamedValue
+renamedValue;
+
+//- @value ref/imports Val
+import {value as exportedValue} from './export';
+//- @local ref/imports Local
+import {local} from './export';
+//- @aliasedLocal ref/imports Local
+import {aliasedLocal} from './export';
+
+// Importing a type from another module.
+//- @MyType ref/imports MyType
+import {MyType} from './module';
+//- @MyType ref MyType
+let x: MyType;
diff --git a/kythe/typescript/testdata/imports_enable_edge_reassignment_group/module.ts b/kythe/typescript/testdata/imports_enable_edge_reassignment_group/module.ts
new file mode 100644
index 000000000..fb39b6089
--- /dev/null
+++ b/kythe/typescript/testdata/imports_enable_edge_reassignment_group/module.ts
@@ -0,0 +1,18 @@
+// All modules define a 'module' record representing the file as a module.
+// The signature is 'module' and the path is the module path (the filename
+// without an extension).  See the discussion of "Module name" in README.md.
+
+//- Mod=vname("module", _, _, "testdata/imports_enable_edge_reassignment_group/module", _).node/kind record
+
+// The first letter in the module is tagged as defining the module.
+// See discussion in emitModuleAnchor().
+//- ModAnchor.node/kind anchor
+//- ModAnchor./kythe/loc/start 0
+//- ModAnchor./kythe/loc/end 1
+//- ModAnchor defines/binding Mod
+
+//- @value defines/binding Val
+export let value = 3;
+
+//- @MyType defines/binding MyType
+export type MyType = string;