Skip to content

Commit 373e133

Browse files
JoostKkara
JoostK
authored and
kara
committed
fix(ngcc): consistently use outer declaration for classes (#32539)
In ngcc's reflection hosts for compiled JS bundles, such as ESM2015, special care needs to be taken for classes as there may be an outer declaration (referred to as "declaration") and an inner declaration (referred to as "implementation") for a given class. Therefore, there will also be two `ts.Symbol`s bound per class, and ngcc needs to switch between those declarations and symbols depending on where certain information can be found. Prior to this commit, the `NgccReflectionHost` interface had methods `getClassSymbol` and `findClassSymbols` that would return a `ts.Symbol`. These class symbols would be used to kick off compilation of components using ngtsc, so it is important for these symbols to correspond with the publicly visible outer declaration of the class. However, the ESM2015 reflection host used to return the `ts.Symbol` for the inner declaration, if the class was declared as follows: ```javascript var MyClass = class MyClass {}; ``` For the above code, `Esm2015ReflectionHost.getClassSymbol` would return the `ts.Symbol` corresponding with the `class MyClass {}` declaration, whereas it should have corresponded with the `var MyClass` declaration. As a consequence, no `NgModule` could be resolved for the component, so no components/directives would be in scope for the component. This resulted in errors during runtime. This commit resolves the issue by introducing a `NgccClassSymbol` that contains references to both the outer and inner `ts.Symbol`, instead of just a single `ts.Symbol`. This avoids the unclarity of whether a `ts.Symbol` corresponds with the outer or inner declaration. More details can be found here: https://hackmd.io/7nkgWOFWQlSRAuIW_8KPPw Fixes #32078 Closes FW-1507 PR Close #32539
1 parent 2279cb8 commit 373e133

12 files changed

Lines changed: 509 additions & 291 deletions

File tree

‎packages/compiler-cli/ngcc/src/analysis/module_with_providers_analyzer.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ export class ModuleWithProvidersAnalyzer {
8080
let dtsFn: ts.Declaration|null = null;
8181
const containerClass = fn.container && this.host.getClassSymbol(fn.container);
8282
if (containerClass) {
83-
const dtsClass = this.host.getDtsDeclaration(containerClass.valueDeclaration);
83+
const dtsClass = this.host.getDtsDeclaration(containerClass.declaration.valueDeclaration);
8484
// Get the declaration of the matching static method
8585
dtsFn = dtsClass && ts.isClassDeclaration(dtsClass) ?
8686
dtsClass.members

‎packages/compiler-cli/ngcc/src/analysis/util.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,9 @@ export function isWithinPackage(packagePath: AbsoluteFsPath, sourceFile: ts.Sour
2020
}
2121

2222
export function analyzeDecorators(
23-
symbol: NgccClassSymbol, decorators: Decorator[] | null,
23+
classSymbol: NgccClassSymbol, decorators: Decorator[] | null,
2424
handlers: DecoratorHandler<any, any>[]): AnalyzedClass|null {
25-
const declaration = symbol.valueDeclaration;
25+
const declaration = classSymbol.declaration.valueDeclaration;
2626
const matchingHandlers = handlers
2727
.map(handler => {
2828
const detected = handler.detect(declaration, decorators);
@@ -78,7 +78,7 @@ export function analyzeDecorators(
7878
}
7979
}
8080
return {
81-
name: symbol.name,
81+
name: classSymbol.name,
8282
declaration,
8383
decorators,
8484
matches,

‎packages/compiler-cli/ngcc/src/host/commonjs_host.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
6464
if (esm5HelperCalls.length > 0) {
6565
return esm5HelperCalls;
6666
} else {
67-
const sourceFile = classSymbol.valueDeclaration.getSourceFile();
67+
const sourceFile = classSymbol.declaration.valueDeclaration.getSourceFile();
6868
return this.getTopLevelHelperCalls(sourceFile, helperName);
6969
}
7070
}

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

Lines changed: 131 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ import {Logger} from '../logging/logger';
1515
import {BundleProgram} from '../packages/bundle_program';
1616
import {findAll, getNameText, hasNameIdentifier, isDefined, stripDollarSuffix} from '../utils';
1717

18-
import {ModuleWithProvidersFunction, NgccClassSymbol, NgccReflectionHost, PRE_R3_MARKER, SwitchableVariableDeclaration, isSwitchableVariableDeclaration} from './ngcc_host';
18+
import {ClassSymbol, ModuleWithProvidersFunction, NgccClassSymbol, NgccReflectionHost, PRE_R3_MARKER, SwitchableVariableDeclaration, isSwitchableVariableDeclaration} from './ngcc_host';
1919

2020
export const DECORATORS = 'decorators' as ts.__String;
2121
export const PROP_DECORATORS = 'propDecorators' as ts.__String;
@@ -91,7 +91,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
9191
}
9292

9393
/**
94-
* Find the declaration of a node that we think is a class.
94+
* Find a symbol for a node that we think is a class.
9595
* Classes should have a `name` identifier, because they may need to be referenced in other parts
9696
* of the program.
9797
*
@@ -104,22 +104,111 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
104104
* Here, the intermediate `MyClass_1` assignment is optional. In the above example, the
105105
* `class MyClass {}` node is returned as declaration of `MyClass`.
106106
*
107-
* @param node the node that represents the class whose declaration we are finding.
108-
* @returns the declaration of the class or `undefined` if it is not a "class".
107+
* @param declaration the declaration node whose symbol we are finding.
108+
* @returns the symbol for the node or `undefined` if it is not a "class" or has no symbol.
109109
*/
110-
getClassDeclaration(node: ts.Node): ClassDeclaration|undefined {
111-
return getInnerClassDeclaration(node) || undefined;
110+
getClassSymbol(declaration: ts.Node): NgccClassSymbol|undefined {
111+
const symbol = this.getClassSymbolFromOuterDeclaration(declaration);
112+
if (symbol !== undefined) {
113+
return symbol;
114+
}
115+
116+
return this.getClassSymbolFromInnerDeclaration(declaration);
112117
}
113118

114119
/**
115-
* Find a symbol for a node that we think is a class.
116-
* @param node the node whose symbol we are finding.
117-
* @returns the symbol for the node or `undefined` if it is not a "class" or has no symbol.
120+
* In ES2015, a class may be declared using a variable declaration of the following structure:
121+
*
122+
* ```
123+
* var MyClass = MyClass_1 = class MyClass {};
124+
* ```
125+
*
126+
* This method extracts the `NgccClassSymbol` for `MyClass` when provided with the `var MyClass`
127+
* declaration node. When the `class MyClass {}` node or any other node is given, this method will
128+
* return undefined instead.
129+
*
130+
* @param declaration the declaration whose symbol we are finding.
131+
* @returns the symbol for the node or `undefined` if it does not represent an outer declaration
132+
* of a class.
118133
*/
119-
getClassSymbol(declaration: ts.Node): NgccClassSymbol|undefined {
120-
const classDeclaration = this.getClassDeclaration(declaration);
121-
return classDeclaration &&
122-
this.checker.getSymbolAtLocation(classDeclaration.name) as NgccClassSymbol;
134+
protected getClassSymbolFromOuterDeclaration(declaration: ts.Node): NgccClassSymbol|undefined {
135+
// Create a symbol without inner declaration if the declaration is a regular class declaration.
136+
if (ts.isClassDeclaration(declaration) && hasNameIdentifier(declaration)) {
137+
return this.createClassSymbol(declaration, null);
138+
}
139+
140+
// Otherwise, the declaration may be a variable declaration, in which case it must be
141+
// initialized using a class expression as inner declaration.
142+
if (ts.isVariableDeclaration(declaration) && hasNameIdentifier(declaration)) {
143+
const innerDeclaration = getInnerClassDeclaration(declaration);
144+
if (innerDeclaration !== null) {
145+
return this.createClassSymbol(declaration, innerDeclaration);
146+
}
147+
}
148+
149+
return undefined;
150+
}
151+
152+
/**
153+
* In ES2015, a class may be declared using a variable declaration of the following structure:
154+
*
155+
* ```
156+
* var MyClass = MyClass_1 = class MyClass {};
157+
* ```
158+
*
159+
* This method extracts the `NgccClassSymbol` for `MyClass` when provided with the
160+
* `class MyClass {}` declaration node. When the `var MyClass` node or any other node is given,
161+
* this method will return undefined instead.
162+
*
163+
* @param declaration the declaration whose symbol we are finding.
164+
* @returns the symbol for the node or `undefined` if it does not represent an inner declaration
165+
* of a class.
166+
*/
167+
protected getClassSymbolFromInnerDeclaration(declaration: ts.Node): NgccClassSymbol|undefined {
168+
if (!ts.isClassExpression(declaration) || !hasNameIdentifier(declaration)) {
169+
return undefined;
170+
}
171+
172+
const outerDeclaration = getVariableDeclarationOfDeclaration(declaration);
173+
if (outerDeclaration === undefined || !hasNameIdentifier(outerDeclaration)) {
174+
return undefined;
175+
}
176+
177+
return this.createClassSymbol(outerDeclaration, declaration);
178+
}
179+
180+
/**
181+
* Creates an `NgccClassSymbol` from an outer and inner declaration. If a class only has an outer
182+
* declaration, the "implementation" symbol of the created `NgccClassSymbol` will be set equal to
183+
* the "declaration" symbol.
184+
*
185+
* @param outerDeclaration The outer declaration node of the class.
186+
* @param innerDeclaration The inner declaration node of the class, or undefined if no inner
187+
* declaration is present.
188+
* @returns the `NgccClassSymbol` representing the class, or undefined if a `ts.Symbol` for any of
189+
* the declarations could not be resolved.
190+
*/
191+
protected createClassSymbol(
192+
outerDeclaration: ClassDeclaration, innerDeclaration: ClassDeclaration|null): NgccClassSymbol
193+
|undefined {
194+
const declarationSymbol =
195+
this.checker.getSymbolAtLocation(outerDeclaration.name) as ClassSymbol | undefined;
196+
if (declarationSymbol === undefined) {
197+
return undefined;
198+
}
199+
200+
const implementationSymbol = innerDeclaration !== null ?
201+
this.checker.getSymbolAtLocation(innerDeclaration.name) :
202+
declarationSymbol;
203+
if (implementationSymbol === undefined) {
204+
return undefined;
205+
}
206+
207+
return {
208+
name: declarationSymbol.name,
209+
declaration: declarationSymbol,
210+
implementation: implementationSymbol,
211+
};
123212
}
124213

125214
/**
@@ -221,7 +310,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
221310
* Check whether the given node actually represents a class.
222311
*/
223312
isClass(node: ts.Node): node is ClassDeclaration {
224-
return super.isClass(node) || !!this.getClassDeclaration(node);
313+
return super.isClass(node) || this.getClassSymbol(node) !== undefined;
225314
}
226315

227316
/**
@@ -549,7 +638,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
549638
*/
550639
protected getStaticProperty(symbol: NgccClassSymbol, propertyName: ts.__String): ts.Symbol
551640
|undefined {
552-
return symbol.exports && symbol.exports.get(propertyName);
641+
return symbol.implementation.exports && symbol.implementation.exports.get(propertyName);
553642
}
554643

555644
/**
@@ -561,8 +650,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
561650
* @returns all information of the decorators on the class.
562651
*/
563652
protected acquireDecoratorInfo(classSymbol: NgccClassSymbol): DecoratorInfo {
564-
if (this.decoratorCache.has(classSymbol.valueDeclaration)) {
565-
return this.decoratorCache.get(classSymbol.valueDeclaration) !;
653+
const decl = classSymbol.declaration.valueDeclaration;
654+
if (this.decoratorCache.has(decl)) {
655+
return this.decoratorCache.get(decl) !;
566656
}
567657

568658
// First attempt extracting decorators from static properties.
@@ -572,7 +662,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
572662
decoratorInfo = this.computeDecoratorInfoFromHelperCalls(classSymbol);
573663
}
574664

575-
this.decoratorCache.set(classSymbol.valueDeclaration, decoratorInfo);
665+
this.decoratorCache.set(decl, decoratorInfo);
576666
return decoratorInfo;
577667
}
578668

@@ -665,8 +755,8 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
665755

666756
// The member map contains all the method (instance and static); and any instance properties
667757
// that are initialized in the class.
668-
if (symbol.members) {
669-
symbol.members.forEach((value, key) => {
758+
if (symbol.implementation.members) {
759+
symbol.implementation.members.forEach((value, key) => {
670760
const decorators = decoratorsMap.get(key as string);
671761
const reflectedMembers = this.reflectMembers(value, decorators);
672762
if (reflectedMembers) {
@@ -677,8 +767,8 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
677767
}
678768

679769
// The static property map contains all the static properties
680-
if (symbol.exports) {
681-
symbol.exports.forEach((value, key) => {
770+
if (symbol.implementation.exports) {
771+
symbol.implementation.exports.forEach((value, key) => {
682772
const decorators = decoratorsMap.get(key as string);
683773
const reflectedMembers = this.reflectMembers(value, decorators, true);
684774
if (reflectedMembers) {
@@ -697,11 +787,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
697787
// }
698788
// MyClass.staticProperty = ...;
699789
// ```
700-
const variableDeclaration = getVariableDeclarationOfDeclaration(symbol.valueDeclaration);
701-
if (variableDeclaration !== undefined) {
702-
const variableSymbol = this.checker.getSymbolAtLocation(variableDeclaration.name);
703-
if (variableSymbol && variableSymbol.exports) {
704-
variableSymbol.exports.forEach((value, key) => {
790+
if (ts.isVariableDeclaration(symbol.declaration.valueDeclaration)) {
791+
if (symbol.declaration.exports) {
792+
symbol.declaration.exports.forEach((value, key) => {
705793
const decorators = decoratorsMap.get(key as string);
706794
const reflectedMembers = this.reflectMembers(value, decorators, true);
707795
if (reflectedMembers) {
@@ -1182,8 +1270,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
11821270
*/
11831271
protected getConstructorParameterDeclarations(classSymbol: NgccClassSymbol):
11841272
ts.ParameterDeclaration[]|null {
1185-
if (classSymbol.members && classSymbol.members.has(CONSTRUCTOR)) {
1186-
const constructorSymbol = classSymbol.members.get(CONSTRUCTOR) !;
1273+
const members = classSymbol.implementation.members;
1274+
if (members && members.has(CONSTRUCTOR)) {
1275+
const constructorSymbol = members.get(CONSTRUCTOR) !;
11871276
// For some reason the constructor does not have a `valueDeclaration` ?!?
11881277
const constructor = constructorSymbol.declarations &&
11891278
constructorSymbol.declarations[0] as ts.ConstructorDeclaration | undefined;
@@ -1314,7 +1403,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
13141403
* @returns an array of statements that may contain helper calls.
13151404
*/
13161405
protected getStatementsForClass(classSymbol: NgccClassSymbol): ts.Statement[] {
1317-
return Array.from(classSymbol.valueDeclaration.getSourceFile().statements);
1406+
return Array.from(classSymbol.declaration.valueDeclaration.getSourceFile().statements);
13181407
}
13191408

13201409
/**
@@ -1637,28 +1726,29 @@ function getCalleeName(call: ts.CallExpression): string|null {
16371726
* ```
16381727
*
16391728
* Here, the intermediate `MyClass_1` assignment is optional. In the above example, the
1640-
* `class MyClass {}` expression is returned as declaration of `MyClass`. Note that if `node`
1641-
* represents a regular class declaration, it will be returned as-is.
1729+
* `class MyClass {}` expression is returned as declaration of `var MyClass`. If the variable
1730+
* is not initialized using a class expression, null is returned.
16421731
*
16431732
* @param node the node that represents the class whose declaration we are finding.
16441733
* @returns the declaration of the class or `null` if it is not a "class".
16451734
*/
1646-
function getInnerClassDeclaration(node: ts.Node):
1647-
ClassDeclaration<ts.ClassDeclaration|ts.ClassExpression>|null {
1735+
function getInnerClassDeclaration(node: ts.Node): ClassDeclaration<ts.ClassExpression>|null {
1736+
if (!ts.isVariableDeclaration(node) || node.initializer === undefined) {
1737+
return null;
1738+
}
1739+
16481740
// Recognize a variable declaration of the form `var MyClass = class MyClass {}` or
16491741
// `var MyClass = MyClass_1 = class MyClass {};`
1650-
if (ts.isVariableDeclaration(node) && node.initializer !== undefined) {
1651-
node = node.initializer;
1652-
while (isAssignment(node)) {
1653-
node = node.right;
1654-
}
1742+
let expression = node.initializer;
1743+
while (isAssignment(expression)) {
1744+
expression = expression.right;
16551745
}
16561746

1657-
if (!ts.isClassDeclaration(node) && !ts.isClassExpression(node)) {
1747+
if (!ts.isClassExpression(expression) || !hasNameIdentifier(expression)) {
16581748
return null;
16591749
}
16601750

1661-
return hasNameIdentifier(node) ? node : null;
1751+
return expression;
16621752
}
16631753

16641754
function getDecoratorArgs(node: ts.ObjectLiteralExpression): ts.Expression[] {

0 commit comments

Comments
 (0)