fix(compiler-cli): set TS original node on imported namespace identif…

…iers (#40711)

This commit causes imports added by ngtsc's `ImportManager` to have their
TypeScript "original node" set to the generated `ts.ImportDeclaration`
statement.

In g3, the tsickle transformer runs after the Angular transformer and post-
processes Angular's compilation output. One of its post-processing tasks is
to transform generated imports and references to imported symbols from the
commonjs module system to the g3 module system. Part of this transformation
involves recognizing modules with specific metadata and altering references
to symbols from those modules accordingly.

Normally, tsickle can rely on TypeScript's binding for an imported symbol to
find its origin module and thus the correct metadata for the symbol. However
the Angular transform generates new synthetic imports which don't have such
binding information. Angular's imports are always namespace imports of the
form:

```
import * as qualifier 'module/specifier';
```

References to such an import are then of the form `qualifier.SymbolName`.

To process such imports properly, tsickle needs to be able to associate the
reference to `qualifier` in the expression `qualifer.SymbolName` with the
`ts.ImportDeclaration` statement that defines it. It expects to do this by
looking at the `ts.getOriginalNode()` for the `qualifier` reference, which
should be the `ts.ImportDeclaration`. This commit changes ngtsc's import
generation mechanism to set the original node on `qualifier` identifiers
according to this expectation.

This commit is not tested in the direct compiler tests, since:

1) there is no observable behavior externally from setting the original node
2) we don't have tests that intercept transformer operations (which could be
   used to directly assert against the AST nodes)
3) tsickle's published version does not (yet) contain the g3-specific
   transformations which rely on the original node and would thus allow the
   behavior to be observed.

Instead, we rely on the g3 testing suite to validate the correctness of this
fix. Breaking this functionality would cause g3 compilation errors for
targets, since tsickle would be unable to transform imports correctly.

PR Close #40711

packages/compiler-cli/ngcc/src/rendering/commonjs_rendering_formatter.ts

@@ -39,7 +39,7 @@ export class CommonJsRenderingFormatter extends Esm5RenderingFormatter {
 
     const insertionPoint = this.findEndOfImports(file);
     const renderedImports =
-        imports.map(i => `var ${i.qualifier} = require('${i.specifier}');\n`).join('');
+        imports.map(i => `var ${i.qualifier.text} = require('${i.specifier}');\n`).join('');
     output.appendLeft(insertionPoint, renderedImports);
   }
 

packages/compiler-cli/ngcc/src/rendering/esm_rendering_formatter.ts

@@ -42,7 +42,7 @@ export class EsmRenderingFormatter implements RenderingFormatter {
 
     const insertionPoint = this.findEndOfImports(sf);
     const renderedImports =
-        imports.map(i => `import * as ${i.qualifier} from '${i.specifier}';\n`).join('');
+        imports.map(i => `import * as ${i.qualifier.text} from '${i.specifier}';\n`).join('');
     output.appendLeft(insertionPoint, renderedImports);
   }
 

packages/compiler-cli/ngcc/src/rendering/umd_rendering_formatter.ts

@@ -225,7 +225,7 @@ function renderFactoryParameters(
   }
 
   const parameters = factoryFunction.parameters;
-  const parameterString = imports.map(i => i.qualifier).join(',');
+  const parameterString = imports.map(i => i.qualifier.text).join(',');
   if (parameters.length > 0) {
     const injectionPoint = parameters[0].getFullStart();
     output.appendLeft(injectionPoint, parameterString + ',');

packages/compiler-cli/ngcc/test/rendering/commonjs_rendering_formatter_spec.ts

@@ -177,8 +177,8 @@ exports.D = D;
         renderer.addImports(
             output,
             [
-              {specifier: '@angular/core', qualifier: 'i0'},
-              {specifier: '@angular/common', qualifier: 'i1'}
+              {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')},
+              {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')}
             ],
             sourceFile);
         expect(output.toString()).toContain(`/* A copyright notice */
@@ -257,7 +257,8 @@ var A = (function() {`);
         const file = getSourceFileOrError(program, _('/node_modules/test-package/some/file.js'));
         const output = new MagicString(PROGRAM.contents);
         renderer.addConstants(output, 'var x = 3;', file);
-        renderer.addImports(output, [{specifier: '@angular/core', qualifier: 'i0'}], file);
+        renderer.addImports(
+            output, [{specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}], file);
         expect(output.toString()).toContain(`
 var core = require('@angular/core');
 var i0 = require('@angular/core');

packages/compiler-cli/ngcc/test/rendering/esm5_rendering_formatter_spec.ts

@@ -188,8 +188,8 @@ export { F };
         renderer.addImports(
             output,
             [
-              {specifier: '@angular/core', qualifier: 'i0'},
-              {specifier: '@angular/common', qualifier: 'i1'}
+              {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')},
+              {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')}
             ],
             sourceFile);
         expect(output.toString()).toContain(`/* A copyright notice */
@@ -263,7 +263,8 @@ var A = (function() {`);
         const file = getSourceFileOrError(program, _('/node_modules/test-package/some/file.js'));
         const output = new MagicString(PROGRAM.contents);
         renderer.addConstants(output, 'var x = 3;', file);
-        renderer.addImports(output, [{specifier: '@angular/core', qualifier: 'i0'}], file);
+        renderer.addImports(
+            output, [{specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}], file);
         expect(output.toString()).toContain(`
 import {Directive} from '@angular/core';
 import * as i0 from '@angular/core';

packages/compiler-cli/ngcc/test/rendering/esm_rendering_formatter_spec.ts

@@ -156,8 +156,8 @@ runInEachFileSystem(() => {
           renderer.addImports(
               output,
               [
-                {specifier: '@angular/core', qualifier: 'i0'},
-                {specifier: '@angular/common', qualifier: 'i1'}
+                {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')},
+                {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')}
               ],
               sourceFile);
           expect(output.toString()).toContain(`/* A copyright notice */
@@ -231,7 +231,8 @@ const x = 3;
           const file = getSourceFileOrError(program, _('/node_modules/test-package/some/file.js'));
           const output = new MagicString(PROGRAM.contents);
           renderer.addConstants(output, 'const x = 3;', file);
-          renderer.addImports(output, [{specifier: '@angular/core', qualifier: 'i0'}], file);
+          renderer.addImports(
+              output, [{specifier: '@angular/core', qualifier: ts.createIdentifier('i0')}], file);
           expect(output.toString()).toContain(`
 import {Directive} from '@angular/core';
 import * as i0 from '@angular/core';

packages/compiler-cli/ngcc/test/rendering/renderer_spec.ts

@@ -276,7 +276,7 @@ A.ɵcmp = ɵngcc0.ɵɵdefineComponent({ type: A, selectors: [["a"]], decls: 1, v
              const addImportsSpy = testFormatter.addImports as jasmine.Spy;
              expect(addImportsSpy.calls.first().args[0].toString()).toEqual(RENDERED_CONTENTS);
              expect(addImportsSpy.calls.first().args[1]).toEqual([
-               {specifier: '@angular/core', qualifier: 'ɵngcc0'}
+               {specifier: '@angular/core', qualifier: jasmine.objectContaining({text: 'ɵngcc0'})}
              ]);
            });
 
@@ -693,7 +693,7 @@ UndecoratedBase.ɵdir = ɵngcc0.ɵɵdefineDirective({ type: UndecoratedBase, vie
                   `function () { (typeof ngDevMode === "undefined" || ngDevMode) && ɵngcc0.setClassMetadata(`);
           const addImportsSpy = testFormatter.addImports as jasmine.Spy;
           expect(addImportsSpy.calls.first().args[1]).toEqual([
-            {specifier: './r3_symbols', qualifier: 'ɵngcc0'}
+            {specifier: './r3_symbols', qualifier: jasmine.objectContaining({text: 'ɵngcc0'})}
           ]);
         });
 

packages/compiler-cli/ngcc/test/rendering/umd_rendering_formatter_spec.ts

@@ -200,8 +200,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib',
         renderer.addImports(
             output,
             [
-              {specifier: '@angular/core', qualifier: 'i0'},
-              {specifier: '@angular/common', qualifier: 'i1'}
+              {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')},
+              {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')}
             ],
             file);
         expect(output.toString())
@@ -217,8 +217,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib',
         renderer.addImports(
             output,
             [
-              {specifier: '@angular/core', qualifier: 'i0'},
-              {specifier: '@angular/common', qualifier: 'i1'}
+              {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')},
+              {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')}
             ],
             file);
         expect(output.toString())
@@ -233,8 +233,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib',
         renderer.addImports(
             output,
             [
-              {specifier: '@angular/core', qualifier: 'i0'},
-              {specifier: '@angular/common', qualifier: 'i1'}
+              {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')},
+              {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')}
             ],
             file);
         expect(output.toString())
@@ -249,10 +249,12 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib',
         renderer.addImports(
             output,
             [
-              {specifier: '@ngrx/store', qualifier: 'i0'},
-              {specifier: '@angular/platform-browser-dynamic', qualifier: 'i1'},
-              {specifier: '@angular/common/testing', qualifier: 'i2'},
-              {specifier: '@angular-foo/package', qualifier: 'i3'}
+              {specifier: '@ngrx/store', qualifier: ts.createIdentifier('i0')}, {
+                specifier: '@angular/platform-browser-dynamic',
+                qualifier: ts.createIdentifier('i1')
+              },
+              {specifier: '@angular/common/testing', qualifier: ts.createIdentifier('i2')},
+              {specifier: '@angular-foo/package', qualifier: ts.createIdentifier('i3')}
             ],
             file);
         expect(output.toString())
@@ -270,8 +272,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib',
            renderer.addImports(
                output,
                [
-                 {specifier: '@angular/core', qualifier: 'i0'},
-                 {specifier: '@angular/common', qualifier: 'i1'}
+                 {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')},
+                 {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')}
                ],
                file);
            expect(output.toString())
@@ -287,8 +289,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib',
            renderer.addImports(
                output,
                [
-                 {specifier: '@angular/core', qualifier: 'i0'},
-                 {specifier: '@angular/common', qualifier: 'i1'}
+                 {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')},
+                 {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')}
                ],
                file);
            expect(output.toString())
@@ -315,8 +317,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib',
         renderer.addImports(
             output,
             [
-              {specifier: '@angular/core', qualifier: 'i0'},
-              {specifier: '@angular/common', qualifier: 'i1'}
+              {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')},
+              {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')}
             ],
             file);
         const outputSrc = output.toString();
@@ -361,8 +363,8 @@ typeof define === 'function' && define.amd ? define('file', ['exports','/tslib',
         renderer.addImports(
             output,
             [
-              {specifier: '@angular/core', qualifier: 'i0'},
-              {specifier: '@angular/common', qualifier: 'i1'}
+              {specifier: '@angular/core', qualifier: ts.createIdentifier('i0')},
+              {specifier: '@angular/common', qualifier: ts.createIdentifier('i1')}
             ],
             file);
         const outputSrc = output.toString();

packages/compiler-cli/src/ngtsc/transform/src/utils.ts

@@ -19,15 +19,27 @@ export function addImports(
     extraStatements: ts.Statement[] = []): ts.SourceFile {
   // Generate the import statements to prepend.
   const addedImports = importManager.getAllImports(sf.fileName).map(i => {
-    const qualifier = ts.createIdentifier(i.qualifier);
+    const qualifier = ts.createIdentifier(i.qualifier.text);
     const importClause = ts.createImportClause(
         /* name */ undefined,
         /* namedBindings */ ts.createNamespaceImport(qualifier));
-    return ts.createImportDeclaration(
+    const decl = ts.createImportDeclaration(
         /* decorators */ undefined,
         /* modifiers */ undefined,
         /* importClause */ importClause,
         /* moduleSpecifier */ ts.createLiteral(i.specifier));
+
+    // Set the qualifier's original TS node to the `ts.ImportDeclaration`. This allows downstream
+    // transforms such as tsickle to properly process references to this import.
+    //
+    // This operation is load-bearing in g3 as some imported modules contain special metadata
+    // generated by clutz, which tsickle uses to transform imports and references to those imports.
+    //
+    // TODO(alxhub): add a test for this when tsickle is updated externally to depend on this
+    // behavior.
+    ts.setOriginalNode(i.qualifier, decl);
+
+    return decl;
   });
 
   // Filter out the existing imports and the source file body. All new statements

packages/compiler-cli/src/ngtsc/translator/index.ts

@@ -7,9 +7,9 @@
  */
 
 export {AstFactory, BinaryOperator, LeadingComment, ObjectLiteralProperty, SourceMapLocation, SourceMapRange, TemplateElement, TemplateLiteral, UnaryOperator, VariableDeclarationType} from './src/api/ast_factory';
-export {Import, ImportGenerator, NamedImport} from './src/api/import_generator';
+export {ImportGenerator, NamedImport} from './src/api/import_generator';
 export {Context} from './src/context';
-export {ImportManager} from './src/import_manager';
+export {Import, ImportManager} from './src/import_manager';
 export {ExpressionTranslatorVisitor, RecordWrappedNodeExprFn, TranslatorOptions} from './src/translator';
 export {translateType} from './src/type_translator';
 export {attachComments, createTemplateMiddle, createTemplateTail, TypeScriptAstFactory} from './src/typescript_ast_factory';

packages/compiler-cli/src/ngtsc/translator/src/api/import_generator.ts

@@ -6,16 +6,6 @@
  * found in the LICENSE file at https://angular.io/license
  */
 
-/**
- * Information about an import that has been added to a module.
- */
-export interface Import {
-  /** The name of the module that has been imported. */
-  specifier: string;
-  /** The alias of the imported module. */
-  qualifier: string;
-}
-
 /**
  * The symbol name and import namespace of an imported symbol,
  * which has been registered through the ImportGenerator.

packages/compiler-cli/src/ngtsc/translator/src/import_manager.ts

@@ -7,7 +7,17 @@
  */
 import * as ts from 'typescript';
 import {ImportRewriter, NoopImportRewriter} from '../../imports';
-import {Import, ImportGenerator, NamedImport} from './api/import_generator';
+import {ImportGenerator, NamedImport} from './api/import_generator';
+
+/**
+ * Information about an import that has been added to a module.
+ */
+export interface Import {
+  /** The name of the module that has been imported. */
+  specifier: string;
+  /** The `ts.Identifer` by which the imported module is known. */
+  qualifier: ts.Identifier;
+}
 
 export class ImportManager implements ImportGenerator<ts.Identifier> {
   private specifierToIdentifier = new Map<string, ts.Identifier>();
@@ -42,11 +52,14 @@ export class ImportManager implements ImportGenerator<ts.Identifier> {
   }
 
   getAllImports(contextPath: string): Import[] {
-    const imports: {specifier: string, qualifier: string}[] = [];
-    this.specifierToIdentifier.forEach((qualifier, specifier) => {
-      specifier = this.rewriter.rewriteSpecifier(specifier, contextPath);
-      imports.push({specifier, qualifier: qualifier.text});
-    });
+    const imports: Import[] = [];
+    for (const [originalSpecifier, qualifier] of this.specifierToIdentifier) {
+      const specifier = this.rewriter.rewriteSpecifier(originalSpecifier, contextPath);
+      imports.push({
+        specifier,
+        qualifier,
+      });
+    }
     return imports;
   }
 }

packages/compiler-cli/src/ngtsc/typecheck/src/context.ts

@@ -355,7 +355,7 @@ export class TypeCheckContextImpl implements TypeCheckContext {
 
     // Write out the imports that need to be added to the beginning of the file.
     let imports = importManager.getAllImports(sf.fileName)
-                      .map(i => `import * as ${i.qualifier} from '${i.specifier}';`)
+                      .map(i => `import * as ${i.qualifier.text} from '${i.specifier}';`)
                       .join('\n');
     code = imports + '\n' + code;
 

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_file.ts

@@ -51,7 +51,7 @@ export class TypeCheckFile extends Environment {
 
   render(): string {
     let source: string = this.importManager.getAllImports(this.contextFile.fileName)
-                             .map(i => `import * as ${i.qualifier} from '${i.specifier}';`)
+                             .map(i => `import * as ${i.qualifier.text} from '${i.specifier}';`)
                              .join('\n') +
         '\n\n';
     const printer = ts.createPrinter();