From 97e13991c573146460ec830d29ced87cc61c07ec Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Tue, 19 May 2020 20:42:54 +0100 Subject: [PATCH] fix(ngcc): identifier ModuleWithProviders functions in IIFE wrapped classes (#37206) In ES2015 IIFE wrapped classes, the identifier that would reference the class of the NgModule may be an alias variable. Previously the `Esm2015ReflectionHost` was not able to match this alias to the original class declaration. This resulted in failing to identify some `ModuleWithProviders` functions in such case. These IIFE wrapped classes were introduced in TypeScript 3.9, which is why this issue is only recently appearing. Since 9.1.x does not support TS 3.9 there is no reason to backport this commit to that branch. Fixes #37189 PR Close #37206 --- .../ngcc/src/host/esm2015_host.ts | 29 +++-- .../module_with_providers_analyzer_spec.ts | 41 ++++++- .../ngcc/test/host/esm2015_host_spec.ts | 108 ++++++++++++++---- 3 files changed, 147 insertions(+), 31 deletions(-) diff --git a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts index da1f8d8034..98ec68548c 100644 --- a/packages/compiler-cli/ngcc/src/host/esm2015_host.ts +++ b/packages/compiler-cli/ngcc/src/host/esm2015_host.ts @@ -132,6 +132,13 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N return symbol; } + if (declaration.parent !== undefined && isNamedVariableDeclaration(declaration.parent)) { + const variableValue = this.getVariableValue(declaration.parent); + if (variableValue !== null) { + declaration = variableValue; + } + } + return this.getClassSymbolFromInnerDeclaration(declaration); } @@ -294,8 +301,15 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N if (superDeclaration.known !== null || superDeclaration.identity !== null) { return superDeclaration; } + let declarationNode: ts.Node = superDeclaration.node; + if (isNamedVariableDeclaration(superDeclaration.node) && !isTopLevel(superDeclaration.node)) { + const variableValue = this.getVariableValue(superDeclaration.node); + if (variableValue !== null && ts.isClassExpression(variableValue)) { + declarationNode = getContainingStatement(variableValue); + } + } - const outerClassNode = getClassDeclarationFromInnerDeclaration(superDeclaration.node); + const outerClassNode = getClassDeclarationFromInnerDeclaration(declarationNode); const declaration = outerClassNode !== null ? this.getDeclarationOfIdentifier(outerClassNode.name) : superDeclaration; @@ -2525,19 +2539,20 @@ function isTopLevel(node: ts.Node): boolean { /** * Get the actual (outer) declaration of a class. * - * In ES5, the implementation of a class is a function expression that is hidden inside an IIFE and + * Sometimes, the implementation of a class is an expression that is hidden inside an IIFE and * returned to be assigned to a variable outside the IIFE, which is what the rest of the program * interacts with. * - * Given the inner function declaration, we want to get to the declaration of the outer variable - * that represents the class. + * Given the inner declaration, we want to get to the declaration of the outer variable that + * represents the class. * - * @param node a node that could be the function expression inside an ES5 class IIFE. - * @returns the outer variable declaration or `undefined` if it is not a "class". + * @param node a node that could be the inner declaration inside an IIFE. + * @returns the outer variable declaration or `null` if it is not a "class". */ export function getClassDeclarationFromInnerDeclaration(node: ts.Node): ClassDeclaration|null { - if (ts.isFunctionDeclaration(node) || ts.isClassDeclaration(node)) { + if (ts.isFunctionDeclaration(node) || ts.isClassDeclaration(node) || + ts.isVariableStatement(node)) { // It might be the function expression inside the IIFE. We need to go 5 levels up... // - IIFE body. diff --git a/packages/compiler-cli/ngcc/test/analysis/module_with_providers_analyzer_spec.ts b/packages/compiler-cli/ngcc/test/analysis/module_with_providers_analyzer_spec.ts index 51ca221319..0c8ebd6e94 100644 --- a/packages/compiler-cli/ngcc/test/analysis/module_with_providers_analyzer_spec.ts +++ b/packages/compiler-cli/ngcc/test/analysis/module_with_providers_analyzer_spec.ts @@ -9,7 +9,7 @@ import * as ts from 'typescript'; import {absoluteFrom, AbsoluteFsPath, getSourceFileOrError} from '../../../src/ngtsc/file_system'; import {runInEachFileSystem, TestFile} from '../../../src/ngtsc/file_system/testing'; -import {isNamedClassDeclaration} from '../../../src/ngtsc/reflection'; +import {isNamedClassDeclaration, isNamedVariableDeclaration} from '../../../src/ngtsc/reflection'; import {getDeclaration} from '../../../src/ngtsc/testing'; import {loadTestFiles} from '../../../test/helpers'; import {ModuleWithProvidersAnalyses, ModuleWithProvidersAnalyzer} from '../../src/analysis/module_with_providers_analyzer'; @@ -41,6 +41,7 @@ runInEachFileSystem(() => { export * from './no-providers'; export * from './module'; export * from './delegated'; + export * from './iife-wrapped'; ` }, { @@ -309,6 +310,25 @@ runInEachFileSystem(() => { name: _('/node_modules/some-library/index.d.ts'), contents: 'export declare class LibraryModule {}' }, + { + name: _('/node_modules/test-package/src/iife-wrapped.js'), + contents: ` + import {NgModule} from './core'; + let WrappedClass = (() => { + var WrappedClass_Alias; + let AdjacentWrappedClass = WrappedClass_Alias = class InnerWrappedClass { + static forRoot() { + return { + ngModule: WrappedClass_Alias, + providers: [] + }; + } + }; + AdjacentWrappedClass = WrappedClass_Alias = __decorate([], AdjacentWrappedClass); + return AdjacentWrappedClass; + })(); + export {WrappedClass};` + }, ]; const TEST_DTS_PROGRAM: TestFile[] = [ { @@ -320,6 +340,7 @@ runInEachFileSystem(() => { export * from './no-providers'; export * from './module'; export * from './delegated'; + export * from './iife-wrapped'; ` }, { @@ -453,6 +474,14 @@ runInEachFileSystem(() => { name: _('/node_modules/some-library/index.d.ts'), contents: 'export declare class LibraryModule {}' }, + { + name: _('/node_modules/test-package/typings/iife-wrapped.d.ts'), + contents: ` + import {ModuleWithProviders} from './core'; + export declare class WrappedClass { + static forRoot(): ModuleWithProviders; + }` + }, ]; loadTestFiles(TEST_PROGRAM); loadTestFiles(TEST_DTS_PROGRAM); @@ -613,6 +642,12 @@ runInEachFileSystem(() => { ]); }); + it('should find declarations that reference an aliased IIFE wrapped class', () => { + const analysis = getAnalysisDescription( + analyses, _('/node_modules/test-package/typings/iife-wrapped.d.ts')); + expect(analysis).toContain(['WrappedClass.forRoot', 'WrappedClass', null]); + }); + function getAnalysisDescription( analyses: ModuleWithProvidersAnalyses, fileName: AbsoluteFsPath) { const file = getSourceFileOrError(dtsProgram.program, fileName); @@ -626,7 +661,9 @@ runInEachFileSystem(() => { } function getName(node: ts.Declaration|null): string { - return node && isNamedClassDeclaration(node) ? `${node.name.text}.` : ''; + return node && (isNamedVariableDeclaration(node) || isNamedClassDeclaration(node)) ? + `${node.name.text}.` : + ''; } }); }); diff --git a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts b/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts index 41954bd539..4a76026333 100644 --- a/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts +++ b/packages/compiler-cli/ngcc/test/host/esm2015_host_spec.ts @@ -12,6 +12,7 @@ import {absoluteFrom, getFileSystem, getSourceFileOrError} from '../../../src/ng import {runInEachFileSystem, TestFile} from '../../../src/ngtsc/file_system/testing'; import {ClassMemberKind, ConcreteDeclaration, CtorParameter, DownleveledEnum, isNamedClassDeclaration, isNamedFunctionDeclaration, isNamedVariableDeclaration, TypeScriptReflectionHost} from '../../../src/ngtsc/reflection'; import {getDeclaration} from '../../../src/ngtsc/testing'; +import {walkForDeclaration} from '../../../src/ngtsc/testing/src/utils'; import {loadFakeCore, loadTestFiles} from '../../../test/helpers'; import {DelegatingReflectionHost} from '../../src/host/delegating_host'; import {Esm2015ReflectionHost} from '../../src/host/esm2015_host'; @@ -139,7 +140,7 @@ runInEachFileSystem(() => { })(); let AliasedWrappedClass = /** @class */ (() => { let AliasedWrappedClassAdjacent = class AliasedWrappedClassInner {}; - return SimpleWrappedClassAdjacent; + return AliasWrappedClassAdjacent; })(); `, }; @@ -176,11 +177,17 @@ runInEachFileSystem(() => { })(); let usageOfWrappedClass = AliasedWrappedClass_1; let DecoratedWrappedClass = /** @class */ (() => { - let DecoratedWrappedClass_1 = class DecoratedWrappedClass {} - // ... add decorations ... - return DecoratedWrappedClass_1; - })(); - let usageOfDecorated = DecoratedWrappedClass_1; + var DecoratedWrappedClass_1; + let AdjacentDecoratedWrappedClass = DecoratedWrappedClass_1 = class InnerDecoratedWrappedClass { + static forRoot() { + return new DecoratedWrappedClass_1(); + } + }; + AdjacentDecoratedWrappedClass = DecoratedWrappedClass_1 = __decorate([ + Decorator() + ], AdjacentDecoratedWrappedClass); + return AdjacentDecoratedWrappedClass; + })(); `, }; @@ -1623,7 +1630,7 @@ runInEachFileSystem(() => { .toBe(classDeclaration); }); - it('should return the original declaration of an aliased class', () => { + it('should return the original declaration of a wrapped aliased class', () => { loadTestFiles([WRAPPED_CLASS_EXPRESSION_FILE]); const bundle = makeTestBundleProgram(WRAPPED_CLASS_EXPRESSION_FILE.name); const host = createHost(bundle, new Esm2015ReflectionHost(new MockLogger(), false, bundle)); @@ -1663,6 +1670,27 @@ runInEachFileSystem(() => { expect(host.getDeclarationOfIdentifier(innerIdentifier)!.node).toBe(outerDeclaration); }); + it('should return the correct declaration for an aliased class identifier inside an IIFE', + () => { + loadTestFiles([WRAPPED_CLASS_EXPRESSION_FILE]); + const bundle = makeTestBundleProgram(WRAPPED_CLASS_EXPRESSION_FILE.name); + const host = + createHost(bundle, new Esm2015ReflectionHost(new MockLogger(), false, bundle)); + const classDeclaration = getDeclaration( + bundle.program, WRAPPED_CLASS_EXPRESSION_FILE.name, 'DecoratedWrappedClass', + ts.isVariableDeclaration); + const innerClassDeclaration = + walkForDeclaration('InnerDecoratedWrappedClass', classDeclaration); + if (innerClassDeclaration === null) { + throw new Error('Expected InnerDecoratedWrappedClass to exist'); + } + const aliasedClassIdentifier = + (innerClassDeclaration.parent as ts.BinaryExpression).left as ts.Identifier; + expect(aliasedClassIdentifier.text).toBe('DecoratedWrappedClass_1'); + const d = host.getDeclarationOfIdentifier(aliasedClassIdentifier); + expect(d!.node).toBe(classDeclaration); + }); + it('should recognize enum declarations with string values', () => { const testFile: TestFile = { name: _('/node_modules/test-package/some/file.js'), @@ -1944,7 +1972,7 @@ runInEachFileSystem(() => { expect(innerSymbol.implementation).toBe(outerSymbol.implementation); }); - it('should return the class symbol for a decorated wrapped class expression (outer variable declaration)', + it('should return the class symbol for a decorated wrapped class expression (from the outer variable declaration)', () => { loadTestFiles([WRAPPED_CLASS_EXPRESSION_FILE]); const bundle = makeTestBundleProgram(WRAPPED_CLASS_EXPRESSION_FILE.name); @@ -1965,16 +1993,17 @@ runInEachFileSystem(() => { return fail('Expected a named class declaration'); } expect(classSymbol.implementation.valueDeclaration.name!.text) - .toBe('DecoratedWrappedClass'); + .toBe('InnerDecoratedWrappedClass'); if (classSymbol.adjacent === undefined || !isNamedVariableDeclaration(classSymbol.adjacent.valueDeclaration)) { return fail('Expected a named variable declaration for the adjacent symbol'); } - expect(classSymbol.adjacent.valueDeclaration.name.text).toBe('DecoratedWrappedClass_1'); + expect(classSymbol.adjacent.valueDeclaration.name.text) + .toBe('AdjacentDecoratedWrappedClass'); }); - it('should return the class symbol for a decorated wrapped class expression (inner class expression)', + it('should return the class symbol for a decorated wrapped class expression (from the inner class expression)', () => { loadTestFiles([WRAPPED_CLASS_EXPRESSION_FILE]); const bundle = makeTestBundleProgram(WRAPPED_CLASS_EXPRESSION_FILE.name); @@ -1983,11 +2012,10 @@ runInEachFileSystem(() => { const outerNode = getDeclaration( bundle.program, WRAPPED_CLASS_EXPRESSION_FILE.name, 'DecoratedWrappedClass', isNamedVariableDeclaration); - const innerNode: ts.ClassExpression = - (outerNode as any) - .initializer.expression.expression.body.statements[0] - .declarationList.declarations[0] - .initializer; + const innerNode = walkForDeclaration('InnerDecoratedWrappedClass', outerNode); + if (innerNode === null) { + throw new Error('Expected to find InnerDecoratedWrappedClass'); + } const classSymbol = host.getClassSymbol(innerNode); if (classSymbol === undefined) { @@ -2001,7 +2029,44 @@ runInEachFileSystem(() => { !isNamedVariableDeclaration(classSymbol.adjacent.valueDeclaration)) { return fail('Expected a named variable declaration for the adjacent symbol'); } - expect(classSymbol.adjacent.valueDeclaration.name.text).toBe('DecoratedWrappedClass_1'); + expect(classSymbol.adjacent.valueDeclaration.name.text) + .toBe('AdjacentDecoratedWrappedClass'); + }); + + + it('should return the class symbol for a decorated wrapped class expression (from the adjacent class expression)', + () => { + loadTestFiles([WRAPPED_CLASS_EXPRESSION_FILE]); + const bundle = makeTestBundleProgram(WRAPPED_CLASS_EXPRESSION_FILE.name); + const host = + createHost(bundle, new Esm2015ReflectionHost(new MockLogger(), false, bundle)); + const outerNode = getDeclaration( + bundle.program, WRAPPED_CLASS_EXPRESSION_FILE.name, 'DecoratedWrappedClass', + isNamedVariableDeclaration); + const innerNode = walkForDeclaration('InnerDecoratedWrappedClass', outerNode); + if (innerNode === null) { + throw new Error('Expected to find InnerDecoratedWrappedClass'); + } + const adjacentNode: ts.ClassExpression = + (outerNode as any) + .initializer.expression.expression.body.statements[0] + .declarationList.declarations[0] + .name; + const classSymbol = host.getClassSymbol(adjacentNode); + + if (classSymbol === undefined) { + return fail('Expected classSymbol to be defined'); + } + expect(classSymbol.name).toEqual('DecoratedWrappedClass'); + expect(classSymbol.declaration.valueDeclaration).toBe(outerNode); + expect(classSymbol.implementation.valueDeclaration).toBe(innerNode); + + if (classSymbol.adjacent === undefined || + !isNamedVariableDeclaration(classSymbol.adjacent.valueDeclaration)) { + return fail('Expected a named variable declaration for the adjacent symbol'); + } + expect(classSymbol.adjacent.valueDeclaration.name.text) + .toBe('AdjacentDecoratedWrappedClass'); }); it('should return the same class symbol (of the outer declaration) for decorated wrapped outer and inner declarations', @@ -2013,11 +2078,10 @@ runInEachFileSystem(() => { const outerNode = getDeclaration( bundle.program, WRAPPED_CLASS_EXPRESSION_FILE.name, 'DecoratedWrappedClass', isNamedVariableDeclaration); - const innerNode: ts.ClassExpression = - (outerNode as any) - .initializer.expression.expression.body.statements[0] - .declarationList.declarations[0] - .initializer; + const innerNode = walkForDeclaration('InnerDecoratedWrappedClass', outerNode); + if (innerNode === null) { + throw new Error('Expected to find InnerDecoratedWrappedClass'); + } const innerSymbol = host.getClassSymbol(innerNode)!; const outerSymbol = host.getClassSymbol(outerNode)!;