From 8bdcca1e08eaeb031a940f95e8afe91081cd2482 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Thu, 20 May 2021 16:39:17 -0400 Subject: [PATCH] fix(compiler-cli): better detect classes that are indirectly exported (#42207) The compiler flag `compileNonExportedClasses` allows the Angular compiler to process classes which are not exported at the top level of a source file. This is often used to allow for AOT compilation of test classes inside `it()` test blocks, for example. Previously, the compiler would identify exported classes by looking for an `export` modifier on the class declaration itself. This works for the trivial case, but fails for indirectly exported classes: ```typescript // Component is declared unexported. @Component({...}) class FooCmp {...} // Indirect export of FooCmp export {FooCmp}; ``` This is not an immediate problem for most application builds, since the default value for `compileNonExportedClasses` is `true` and therefore such classes get compiled regardless. However, in the Angular Language Service now, `compileNonExportedClasses` is forcibly overridden to `false`. That's because the tsconfig used by the IDE and Language Service is often far broader than the application build's configuration, and pulls in test files that can contain unexported classes not designed with AOT compilation in mind. Therefore, the Language Service has trouble working with such structures. In this commit, the `ReflectionHost` gains a new API for detecting whether a class is exported. The implementation of this method now not only considers the `export` modifier, but also scans the `ts.SourceFile` for indirect exports like the example above. This ensures the above case will be processed directly in the Language Service. This new operation is cached using an expando symbol on the `ts.SourceFile`, ensuring good performance even when scanning large source files with lots of exports (e.g. a FESM file under `ngcc`). Fixes #42184. PR Close #42207 --- .../ngcc/src/host/delegating_host.ts | 4 + .../src/ngtsc/reflection/src/host.ts | 10 ++ .../src/ngtsc/reflection/src/typescript.ts | 104 ++++++++++++++++++ .../src/ngtsc/transform/src/compilation.ts | 2 +- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 18 +++ 5 files changed, 137 insertions(+), 1 deletion(-) diff --git a/packages/compiler-cli/ngcc/src/host/delegating_host.ts b/packages/compiler-cli/ngcc/src/host/delegating_host.ts index e72a40a94d01a..8352520f59c76 100644 --- a/packages/compiler-cli/ngcc/src/host/delegating_host.ts +++ b/packages/compiler-cli/ngcc/src/host/delegating_host.ts @@ -161,4 +161,8 @@ export class DelegatingReflectionHost implements NgccReflectionHost { detectKnownDeclaration(decl: T): T { return this.ngccHost.detectKnownDeclaration(decl); } + + isStaticallyExported(clazz: ClassDeclaration): boolean { + return this.ngccHost.isStaticallyExported(clazz); + } } diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/host.ts b/packages/compiler-cli/src/ngtsc/reflection/src/host.ts index 7b6adeadbb65d..851880f5ef3aa 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/host.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/host.ts @@ -856,4 +856,14 @@ export interface ReflectionHost { * have a different name than it does externally. */ getAdjacentNameOfClass(clazz: ClassDeclaration): ts.Identifier; + + /** + * Returns `true` if a class is exported from the module in which it's defined. + * + * Not all mechanisms by which a class is exported can be statically detected, especially when + * processing already compiled JavaScript. A `false` result does not indicate that the class is + * never visible outside its module, only that it was not exported via one of the export + * mechanisms that the `ReflectionHost` is capable of statically checking. + */ + isStaticallyExported(clazz: ClassDeclaration): boolean; } diff --git a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts index 451d0c9ae3fd8..3fa2636790333 100644 --- a/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts +++ b/packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts @@ -199,6 +199,35 @@ export class TypeScriptReflectionHost implements ReflectionHost { return clazz.name; } + isStaticallyExported(clazz: ClassDeclaration): boolean { + // First check if there's an `export` modifier directly on the class declaration. + let topLevel: ts.Node = clazz; + if (ts.isVariableDeclaration(clazz) && ts.isVariableDeclarationList(clazz.parent)) { + topLevel = clazz.parent.parent; + } + if (topLevel.modifiers !== undefined && + topLevel.modifiers.some(modifier => modifier.kind === ts.SyntaxKind.ExportKeyword)) { + // The node is part of a declaration that's directly exported. + return true; + } + + // If `topLevel` is not directly exported via a modifier, then it might be indirectly exported, + // e.g.: + // + // class Foo {} + // export {Foo}; + // + // The only way to check this is to look at the module level for exports of the class. As a + // performance optimization, this check is only performed if the class is actually declared at + // the top level of the file and thus eligible for exporting in the first place. + if (topLevel.parent === undefined || !ts.isSourceFile(topLevel.parent)) { + return false; + } + + const localExports = this.getLocalExportedClassesOfSourceFile(clazz.getSourceFile()); + return localExports.has(clazz); + } + protected getDirectImportOfIdentifier(id: ts.Identifier): Import|null { const symbol = this.checker.getSymbolAtLocation(id); @@ -413,6 +442,58 @@ export class TypeScriptReflectionHost implements ReflectionHost { isStatic, }; } + + /** + * Get the set of classes declared in `file` which are exported. + */ + private getLocalExportedClassesOfSourceFile(file: ts.SourceFile): Set { + const cacheSf: SourceFileWithCachedExports = file as SourceFileWithCachedExports; + if (cacheSf[LocalExportedClasses] !== undefined) { + // TS does not currently narrow symbol-keyed fields, hence the non-null assert is needed. + return cacheSf[LocalExportedClasses]!; + } + + const exportSet = new Set(); + cacheSf[LocalExportedClasses] = exportSet; + + const sfSymbol = this.checker.getSymbolAtLocation(cacheSf); + + if (sfSymbol === undefined || sfSymbol.exports === undefined) { + return exportSet; + } + + // Scan the exported symbol of the `ts.SourceFile` for the original `symbol` of the class + // declaration. + // + // Note: when checking multiple classes declared in the same file, this repeats some operations. + // In theory, this could be expensive if run in the context of a massive input file (like a + // large FESM in ngcc). If performance does become an issue here, it should be possible to + // create a `Set<>` + + // Unfortunately, `ts.Iterator` doesn't implement the iterator protocol, so iteration here is + // done manually. + const iter = sfSymbol.exports.values(); + let item = iter.next(); + while (item.done !== true) { + let exportedSymbol = item.value; + + // If this exported symbol comes from an `export {Foo}` statement, then the symbol is actually + // for the export declaration, not the original declaration. Such a symbol will be an alias, + // so unwrap aliasing if necessary. + if (exportedSymbol.flags & ts.SymbolFlags.Alias) { + exportedSymbol = this.checker.getAliasedSymbol(exportedSymbol); + } + + if (exportedSymbol.valueDeclaration !== undefined && + exportedSymbol.valueDeclaration.getSourceFile() === file && + this.isClass(exportedSymbol.valueDeclaration)) { + exportSet.add(exportedSymbol.valueDeclaration); + } + item = iter.next(); + } + + return exportSet; + } } export function reflectNameOfDeclaration(decl: ts.Declaration): string|null { @@ -593,3 +674,26 @@ function getExportedName(decl: ts.Declaration, originalId: ts.Identifier): strin (decl.propertyName !== undefined ? decl.propertyName : decl.name).text : originalId.text; } + +const LocalExportedClasses = Symbol('LocalExportedClasses'); + +/** + * A `ts.SourceFile` expando which includes a cached `Set` of local `ClassDeclarations` that are + * exported either directly (`export class ...`) or indirectly (via `export {...}`). + * + * This cache does not cause memory leaks as: + * + * 1. The only references cached here are local to the `ts.SourceFile`, and thus also available in + * `this.statements`. + * + * 2. The only way this `Set` could change is if the source file itself was changed, which would + * invalidate the entire `ts.SourceFile` object in favor of a new version. Thus, changing the + * source file also invalidates this cache. + */ +interface SourceFileWithCachedExports extends ts.SourceFile { + /** + * Cached `Set` of `ClassDeclaration`s which are locally declared in this file and are exported + * either directly or indirectly. + */ + [LocalExportedClasses]?: Set; +} diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index aabc0cf5a78ce..6c8ba0da9ccfc 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -211,7 +211,7 @@ export class TraitCompiler implements ProgramTypeCheckAdapter { private scanClassForTraits(clazz: ClassDeclaration): PendingTrait[]|null { - if (!this.compileNonExportedClasses && !isExported(clazz)) { + if (!this.compileNonExportedClasses && !this.reflector.isStaticallyExported(clazz)) { return null; } diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index f98377ea0f8e6..494b6045de76c 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -7395,6 +7395,24 @@ export const Foo = Foo__PRE_R3__; expect(jsContents).not.toContain('defineNgModule('); expect(jsContents).toContain('NgModule({'); }); + + it('should still compile a class that is indirectly exported', () => { + env.write('test.ts', ` + import {Component} from '@angular/core'; + + @Component({ + selector: 'test-cmp', + template: 'Test Cmp', + }) + class TestCmp {} + + export {TestCmp}; + `); + env.driveMain(); + const jsContents = env.getContents('test.js'); + + expect(jsContents).toContain('defineComponent'); + }); }); describe('undecorated providers', () => {