fix(ivy): ngcc - render namespaced imported decorators correctly (#31426)

The support for decorators that were imported via a namespace,
e.g. `import * as core from `@angular/core` was implemented
piecemeal. This meant that it was easy to miss situations where
a decorator identifier needed to be handled as a namepsaced
import rather than a direct import.

One such issue was that UMD processing of decorators was not
correct: the namespace was being omitted from references to
decorators.

Now the types have been modified to make it clear that a
`Decorator.identifier` could hold a namespaced identifier,
and the corresponding code that uses these types has been
fixed.

Fixes #31394

PR Close #31426

integration/ngcc/test.sh

@@ -67,6 +67,9 @@ if [[ $? != 0 ]]; then exit 1; fi
 grep "_MatMenuBase.ngBaseDef = ɵngcc0.ɵɵdefineBase({ inputs: {" node_modules/@angular/material/esm5/menu.es5.js
 if [[ $? != 0 ]]; then exit 1; fi
 
+# Did it handle namespace imported decorators in UMD?
+grep "type: core.Injectable," node_modules/@angular/cdk/bundles/cdk-a11y.umd.js
+
 # Can it be safely run again (as a noop)?
 # And check that it logged skipping compilation as expected
 ivy-ngcc -l debug | grep 'Skipping'

packages/compiler-cli/ngcc/src/host/esm2015_host.ts

@@ -9,7 +9,7 @@
 import * as ts from 'typescript';
 
 import {AbsoluteFsPath} from '../../../src/ngtsc/file_system';
-import {ClassDeclaration, ClassMember, ClassMemberKind, ClassSymbol, CtorParameter, Declaration, Decorator, Import, TypeScriptReflectionHost, reflectObjectLiteral} from '../../../src/ngtsc/reflection';
+import {ClassDeclaration, ClassMember, ClassMemberKind, ClassSymbol, CtorParameter, Declaration, Decorator, Import, TypeScriptReflectionHost, isDecoratorIdentifier, reflectObjectLiteral} from '../../../src/ngtsc/reflection';
 import {Logger} from '../logging/logger';
 import {BundleProgram} from '../packages/bundle_program';
 import {findAll, getNameText, hasNameIdentifier, isDefined} from '../utils';
@@ -803,12 +803,11 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
    * is not a valid decorator call.
    */
   protected reflectDecoratorCall(call: ts.CallExpression): Decorator|null {
-    // The call could be of the form `Decorator(...)` or `namespace_1.Decorator(...)`
-    const decoratorExpression =
-        ts.isPropertyAccessExpression(call.expression) ? call.expression.name : call.expression;
-    if (ts.isIdentifier(decoratorExpression)) {
+    const decoratorExpression = call.expression;
+    if (isDecoratorIdentifier(decoratorExpression)) {
       // We found a decorator!
-      const decoratorIdentifier = decoratorExpression;
+      const decoratorIdentifier =
+          ts.isIdentifier(decoratorExpression) ? decoratorExpression : decoratorExpression.name;
       return {
         name: decoratorIdentifier.text,
         identifier: decoratorIdentifier,
@@ -872,16 +871,14 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
 
           // Is the value of the `type` property an identifier?
           if (decorator.has('type')) {
-            let typeIdentifier = decorator.get('type') !;
-            if (ts.isPropertyAccessExpression(typeIdentifier)) {
-              // the type is in a namespace, e.g. `core.Directive`
-              typeIdentifier = typeIdentifier.name;
-            }
-            if (ts.isIdentifier(typeIdentifier)) {
+            let decoratorType = decorator.get('type') !;
+            if (isDecoratorIdentifier(decoratorType)) {
+              const decoratorIdentifier =
+                  ts.isIdentifier(decoratorType) ? decoratorType : decoratorType.name;
               decorators.push({
-                name: typeIdentifier.text,
-                identifier: typeIdentifier,
-                import: this.getImportOfIdentifier(typeIdentifier), node,
+                name: decoratorIdentifier.text,
+                identifier: decoratorType,
+                import: this.getImportOfIdentifier(decoratorIdentifier), node,
                 args: getDecoratorArgs(node),
               });
             }
@@ -1223,51 +1220,6 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
     return Array.from(classSymbol.valueDeclaration.getSourceFile().statements);
   }
 
-  /**
-   * Try to get the import info for this identifier as though it is a namespaced import.
-   * For example, if the identifier is the `__metadata` part of a property access chain like:
-   *
-   * ```
-   * tslib_1.__metadata
-   * ```
-   *
-   * then it might be that `tslib_1` is a namespace import such as:
-   *
-   * ```
-   * import * as tslib_1 from 'tslib';
-   * ```
-   * @param id the TypeScript identifier to find the import info for.
-   * @returns The import info if this is a namespaced import or `null`.
-   */
-  protected getImportOfNamespacedIdentifier(id: ts.Identifier): Import|null {
-    if (!(ts.isPropertyAccessExpression(id.parent) && id.parent.name === id)) {
-      return null;
-    }
-
-    const namespaceIdentifier = getFarLeftIdentifier(id.parent);
-    const namespaceSymbol =
-        namespaceIdentifier && this.checker.getSymbolAtLocation(namespaceIdentifier);
-    const declaration = namespaceSymbol && namespaceSymbol.declarations.length === 1 ?
-        namespaceSymbol.declarations[0] :
-        null;
-    const namespaceDeclaration =
-        declaration && ts.isNamespaceImport(declaration) ? declaration : null;
-    if (!namespaceDeclaration) {
-      return null;
-    }
-
-    const importDeclaration = namespaceDeclaration.parent.parent;
-    if (!ts.isStringLiteral(importDeclaration.moduleSpecifier)) {
-      // Should not happen as this would be invalid TypesScript
-      return null;
-    }
-
-    return {
-      from: importDeclaration.moduleSpecifier.text,
-      name: id.text,
-    };
-  }
-
   /**
    * Test whether a decorator was imported from `@angular/core`.
    *
@@ -1546,19 +1498,6 @@ function isClassMemberType(node: ts.Declaration): node is ts.ClassElement|
   return ts.isClassElement(node) || isPropertyAccess(node) || ts.isBinaryExpression(node);
 }
 
-/**
- * Compute the left most identifier in a property access chain. E.g. the `a` of `a.b.c.d`.
- * @param propertyAccess The starting property access expression from which we want to compute
- * the left most identifier.
- * @returns the left most identifier in the chain or `null` if it is not an identifier.
- */
-function getFarLeftIdentifier(propertyAccess: ts.PropertyAccessExpression): ts.Identifier|null {
-  while (ts.isPropertyAccessExpression(propertyAccess.expression)) {
-    propertyAccess = propertyAccess.expression;
-  }
-  return ts.isIdentifier(propertyAccess.expression) ? propertyAccess.expression : null;
-}
-
 /**
  * Collect mappings between exported declarations in a source file and its associated
  * declaration in the typings program.

packages/compiler-cli/src/ngtsc/annotations/src/metadata.ts

@@ -124,7 +124,7 @@ function classMemberToMetadata(
 function decoratorToMetadata(decorator: Decorator): ts.ObjectLiteralExpression {
   // Decorators have a type.
   const properties: ts.ObjectLiteralElementLike[] = [
-    ts.createPropertyAssignment('type', ts.updateIdentifier(decorator.identifier)),
+    ts.createPropertyAssignment('type', ts.getMutableClone(decorator.identifier)),
   ];
   // Sometimes they have arguments.
   if (decorator.args !== null && decorator.args.length > 0) {

packages/compiler-cli/src/ngtsc/annotations/test/metadata_spec.ts

@@ -26,6 +26,16 @@ runInEachFileSystem(() => {
           `/*@__PURE__*/ i0.ɵsetClassMetadata(Target, [{ type: Component, args: ['metadata'] }], null, null);`);
     });
 
+    it('should convert namespaced decorated class metadata', () => {
+      const res = compileAndPrint(`
+    import * as core from '@angular/core';
+
+    @core.Component('metadata') class Target {}
+    `);
+      expect(res).toEqual(
+          `/*@__PURE__*/ i0.ɵsetClassMetadata(Target, [{ type: core.Component, args: ['metadata'] }], null, null);`);
+    });
+
     it('should convert decorated class constructor parameter metadata', () => {
       const res = compileAndPrint(`
     import {Component, Inject, Injector} from '@angular/core';

packages/compiler-cli/src/ngtsc/reflection/src/host.ts

@@ -21,9 +21,9 @@ export interface Decorator {
   name: string;
 
   /**
-   * Identifier which refers to the decorator in source.
+   * Identifier which refers to the decorator in the user's code.
    */
-  identifier: ts.Identifier;
+  identifier: DecoratorIdentifier;
 
   /**
    * `Import` by which the decorator was brought into the module in which it was invoked, or `null`
@@ -42,6 +42,17 @@ export interface Decorator {
   args: ts.Expression[]|null;
 }
 
+/**
+ * A decorator is identified by either a simple identifier (e.g. `Decorator`) or, in some cases,
+ * a namespaced property access (e.g. `core.Decorator`).
+ */
+export type DecoratorIdentifier = ts.Identifier | NamespacedIdentifier;
+export type NamespacedIdentifier = ts.PropertyAccessExpression & {expression: ts.Identifier};
+export function isDecoratorIdentifier(exp: ts.Expression): exp is DecoratorIdentifier {
+  return ts.isIdentifier(exp) ||
+      ts.isPropertyAccessExpression(exp) && ts.isIdentifier(exp.expression);
+}
+
 /**
  * The `ts.Declaration` of a "class".
  *
@@ -371,7 +382,7 @@ export interface ReflectionHost {
    * result of an IIFE execution.
    *
    * @returns an array of `Decorator` metadata if decorators are present on the declaration, or
-   * `null` if either no decorators were present or if the declaration is not of a decorable type.
+   * `null` if either no decorators were present or if the declaration is not of a decoratable type.
    */
   getDecoratorsOfDeclaration(declaration: ts.Declaration): Decorator[]|null;
 

packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts

@@ -8,7 +8,7 @@
 
 import * as ts from 'typescript';
 
-import {ClassDeclaration, ClassMember, ClassMemberKind, CtorParameter, Declaration, Decorator, FunctionDefinition, Import, ReflectionHost, TsHelperFn} from './host';
+import {ClassDeclaration, ClassMember, ClassMemberKind, CtorParameter, Declaration, Decorator, FunctionDefinition, Import, ReflectionHost, isDecoratorIdentifier} from './host';
 import {typeToValue} from './type_to_value';
 
 /**
@@ -54,7 +54,7 @@ export class TypeScriptReflectionHost implements ReflectionHost {
       let typeNode = originalTypeNode;
 
       // Check if we are dealing with a simple nullable union type e.g. `foo: Foo|null`
-      // and extract the type. More complext union types e.g. `foo: Foo|Bar` are not supported.
+      // and extract the type. More complex union types e.g. `foo: Foo|Bar` are not supported.
       // We also don't need to support `foo: Foo|undefined` because Angular's DI injects `null` for
       // optional tokes that don't have providers.
       if (typeNode && ts.isUnionTypeNode(typeNode)) {
@@ -79,7 +79,16 @@ export class TypeScriptReflectionHost implements ReflectionHost {
   }
 
   getImportOfIdentifier(id: ts.Identifier): Import|null {
-    return this.getDirectImportOfIdentifier(id) || this.getImportOfNamespacedIdentifier(id);
+    const directImport = this.getDirectImportOfIdentifier(id);
+    if (directImport !== null) {
+      return directImport;
+    } else if (ts.isQualifiedName(id.parent) && id.parent.right === id) {
+      return this.getImportOfNamespacedIdentifier(id, getQualifiedNameRoot(id.parent));
+    } else if (ts.isPropertyAccessExpression(id.parent) && id.parent.name === id) {
+      return this.getImportOfNamespacedIdentifier(id, getFarLeftIdentifier(id.parent));
+    } else {
+      return null;
+    }
   }
 
   getExportsOfModule(node: ts.Node): Map<string, Declaration>|null {
@@ -190,6 +199,7 @@ export class TypeScriptReflectionHost implements ReflectionHost {
 
   /**
    * Try to get the import info for this identifier as though it is a namespaced import.
+   *
    * For example, if the identifier is the `Directive` part of a qualified type chain like:
    *
    * ```
@@ -205,12 +215,9 @@ export class TypeScriptReflectionHost implements ReflectionHost {
    * @param id the TypeScript identifier to find the import info for.
    * @returns The import info if this is a namespaced import or `null`.
    */
-  protected getImportOfNamespacedIdentifier(id: ts.Identifier): Import|null {
-    if (!(ts.isQualifiedName(id.parent) && id.parent.right === id)) {
-      return null;
-    }
-    const namespaceIdentifier = getQualifiedNameRoot(id.parent);
-    if (!namespaceIdentifier) {
+  protected getImportOfNamespacedIdentifier(
+      id: ts.Identifier, namespaceIdentifier: ts.Identifier|null): Import|null {
+    if (namespaceIdentifier === null) {
       return null;
     }
     const namespaceSymbol = this.checker.getSymbolAtLocation(namespaceIdentifier);
@@ -300,14 +307,15 @@ export class TypeScriptReflectionHost implements ReflectionHost {
 
     // The final resolved decorator should be a `ts.Identifier` - if it's not, then something is
     // wrong and the decorator can't be resolved statically.
-    if (!ts.isIdentifier(decoratorExpr)) {
+    if (!isDecoratorIdentifier(decoratorExpr)) {
       return null;
     }
 
-    const importDecl = this.getImportOfIdentifier(decoratorExpr);
+    const decoratorIdentifier = ts.isIdentifier(decoratorExpr) ? decoratorExpr : decoratorExpr.name;
+    const importDecl = this.getImportOfIdentifier(decoratorIdentifier);
 
     return {
-      name: decoratorExpr.text,
+      name: decoratorIdentifier.text,
       identifier: decoratorExpr,
       import: importDecl, node, args,
     };
@@ -500,3 +508,16 @@ function getQualifiedNameRoot(qualifiedName: ts.QualifiedName): ts.Identifier|nu
   }
   return ts.isIdentifier(qualifiedName.left) ? qualifiedName.left : null;
 }
+
+/**
+ * Compute the left most identifier in a property access chain. E.g. the `a` of `a.b.c.d`.
+ * @param propertyAccess The starting property access expression from which we want to compute
+ * the left most identifier.
+ * @returns the left most identifier in the chain or `null` if it is not an identifier.
+ */
+function getFarLeftIdentifier(propertyAccess: ts.PropertyAccessExpression): ts.Identifier|null {
+  while (ts.isPropertyAccessExpression(propertyAccess.expression)) {
+    propertyAccess = propertyAccess.expression;
+  }
+  return ts.isIdentifier(propertyAccess.expression) ? propertyAccess.expression : null;
+}