Skip to content
Permalink
Browse files

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
  • Loading branch information...
JoostK authored and kara committed Sep 3, 2019
1 parent 2279cb8 commit 373e1337de26a4e0ed3fa63bcb027bd3a2adf2c8
@@ -80,7 +80,7 @@ export class ModuleWithProvidersAnalyzer {
let dtsFn: ts.Declaration|null = null;
const containerClass = fn.container && this.host.getClassSymbol(fn.container);
if (containerClass) {
const dtsClass = this.host.getDtsDeclaration(containerClass.valueDeclaration);
const dtsClass = this.host.getDtsDeclaration(containerClass.declaration.valueDeclaration);
// Get the declaration of the matching static method
dtsFn = dtsClass && ts.isClassDeclaration(dtsClass) ?
dtsClass.members
@@ -20,9 +20,9 @@ export function isWithinPackage(packagePath: AbsoluteFsPath, sourceFile: ts.Sour
}

export function analyzeDecorators(
symbol: NgccClassSymbol, decorators: Decorator[] | null,
classSymbol: NgccClassSymbol, decorators: Decorator[] | null,
handlers: DecoratorHandler<any, any>[]): AnalyzedClass|null {
const declaration = symbol.valueDeclaration;
const declaration = classSymbol.declaration.valueDeclaration;
const matchingHandlers = handlers
.map(handler => {
const detected = handler.detect(declaration, decorators);
@@ -78,7 +78,7 @@ export function analyzeDecorators(
}
}
return {
name: symbol.name,
name: classSymbol.name,
declaration,
decorators,
matches,
@@ -64,7 +64,7 @@ export class CommonJsReflectionHost extends Esm5ReflectionHost {
if (esm5HelperCalls.length > 0) {
return esm5HelperCalls;
} else {
const sourceFile = classSymbol.valueDeclaration.getSourceFile();
const sourceFile = classSymbol.declaration.valueDeclaration.getSourceFile();
return this.getTopLevelHelperCalls(sourceFile, helperName);
}
}
@@ -15,7 +15,7 @@ import {Logger} from '../logging/logger';
import {BundleProgram} from '../packages/bundle_program';
import {findAll, getNameText, hasNameIdentifier, isDefined, stripDollarSuffix} from '../utils';

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

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

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

return this.getClassSymbolFromInnerDeclaration(declaration);
}

/**
* Find a symbol for a node that we think is a class.
* @param node the node whose symbol we are finding.
* @returns the symbol for the node or `undefined` if it is not a "class" or has no symbol.
* In ES2015, a class may be declared using a variable declaration of the following structure:
*
* ```
* var MyClass = MyClass_1 = class MyClass {};
* ```
*
* This method extracts the `NgccClassSymbol` for `MyClass` when provided with the `var MyClass`
* declaration node. When the `class MyClass {}` node or any other node is given, this method will
* return undefined instead.
*
* @param declaration the declaration whose symbol we are finding.
* @returns the symbol for the node or `undefined` if it does not represent an outer declaration
* of a class.
*/
getClassSymbol(declaration: ts.Node): NgccClassSymbol|undefined {
const classDeclaration = this.getClassDeclaration(declaration);
return classDeclaration &&
this.checker.getSymbolAtLocation(classDeclaration.name) as NgccClassSymbol;
protected getClassSymbolFromOuterDeclaration(declaration: ts.Node): NgccClassSymbol|undefined {
// Create a symbol without inner declaration if the declaration is a regular class declaration.
if (ts.isClassDeclaration(declaration) && hasNameIdentifier(declaration)) {
return this.createClassSymbol(declaration, null);
}

// Otherwise, the declaration may be a variable declaration, in which case it must be
// initialized using a class expression as inner declaration.
if (ts.isVariableDeclaration(declaration) && hasNameIdentifier(declaration)) {
const innerDeclaration = getInnerClassDeclaration(declaration);
if (innerDeclaration !== null) {
return this.createClassSymbol(declaration, innerDeclaration);
}
}

return undefined;
}

/**
* In ES2015, a class may be declared using a variable declaration of the following structure:
*
* ```
* var MyClass = MyClass_1 = class MyClass {};
* ```
*
* This method extracts the `NgccClassSymbol` for `MyClass` when provided with the
* `class MyClass {}` declaration node. When the `var MyClass` node or any other node is given,
* this method will return undefined instead.
*
* @param declaration the declaration whose symbol we are finding.
* @returns the symbol for the node or `undefined` if it does not represent an inner declaration
* of a class.
*/
protected getClassSymbolFromInnerDeclaration(declaration: ts.Node): NgccClassSymbol|undefined {
if (!ts.isClassExpression(declaration) || !hasNameIdentifier(declaration)) {
return undefined;
}

const outerDeclaration = getVariableDeclarationOfDeclaration(declaration);
if (outerDeclaration === undefined || !hasNameIdentifier(outerDeclaration)) {
return undefined;
}

return this.createClassSymbol(outerDeclaration, declaration);
}

/**
* Creates an `NgccClassSymbol` from an outer and inner declaration. If a class only has an outer
* declaration, the "implementation" symbol of the created `NgccClassSymbol` will be set equal to
* the "declaration" symbol.
*
* @param outerDeclaration The outer declaration node of the class.
* @param innerDeclaration The inner declaration node of the class, or undefined if no inner
* declaration is present.
* @returns the `NgccClassSymbol` representing the class, or undefined if a `ts.Symbol` for any of
* the declarations could not be resolved.
*/
protected createClassSymbol(
outerDeclaration: ClassDeclaration, innerDeclaration: ClassDeclaration|null): NgccClassSymbol
|undefined {
const declarationSymbol =
this.checker.getSymbolAtLocation(outerDeclaration.name) as ClassSymbol | undefined;
if (declarationSymbol === undefined) {
return undefined;
}

const implementationSymbol = innerDeclaration !== null ?
this.checker.getSymbolAtLocation(innerDeclaration.name) :
declarationSymbol;
if (implementationSymbol === undefined) {
return undefined;
}

return {
name: declarationSymbol.name,
declaration: declarationSymbol,
implementation: implementationSymbol,
};
}

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

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

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

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

this.decoratorCache.set(classSymbol.valueDeclaration, decoratorInfo);
this.decoratorCache.set(decl, decoratorInfo);
return decoratorInfo;
}

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

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

// The static property map contains all the static properties
if (symbol.exports) {
symbol.exports.forEach((value, key) => {
if (symbol.implementation.exports) {
symbol.implementation.exports.forEach((value, key) => {
const decorators = decoratorsMap.get(key as string);
const reflectedMembers = this.reflectMembers(value, decorators, true);
if (reflectedMembers) {
@@ -697,11 +787,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
// }
// MyClass.staticProperty = ...;
// ```
const variableDeclaration = getVariableDeclarationOfDeclaration(symbol.valueDeclaration);
if (variableDeclaration !== undefined) {
const variableSymbol = this.checker.getSymbolAtLocation(variableDeclaration.name);
if (variableSymbol && variableSymbol.exports) {
variableSymbol.exports.forEach((value, key) => {
if (ts.isVariableDeclaration(symbol.declaration.valueDeclaration)) {
if (symbol.declaration.exports) {
symbol.declaration.exports.forEach((value, key) => {
const decorators = decoratorsMap.get(key as string);
const reflectedMembers = this.reflectMembers(value, decorators, true);
if (reflectedMembers) {
@@ -1182,8 +1270,9 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
*/
protected getConstructorParameterDeclarations(classSymbol: NgccClassSymbol):
ts.ParameterDeclaration[]|null {
if (classSymbol.members && classSymbol.members.has(CONSTRUCTOR)) {
const constructorSymbol = classSymbol.members.get(CONSTRUCTOR) !;
const members = classSymbol.implementation.members;
if (members && members.has(CONSTRUCTOR)) {
const constructorSymbol = members.get(CONSTRUCTOR) !;
// For some reason the constructor does not have a `valueDeclaration` ?!?
const constructor = constructorSymbol.declarations &&
constructorSymbol.declarations[0] as ts.ConstructorDeclaration | undefined;
@@ -1314,7 +1403,7 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
* @returns an array of statements that may contain helper calls.
*/
protected getStatementsForClass(classSymbol: NgccClassSymbol): ts.Statement[] {
return Array.from(classSymbol.valueDeclaration.getSourceFile().statements);
return Array.from(classSymbol.declaration.valueDeclaration.getSourceFile().statements);
}

/**
@@ -1637,28 +1726,29 @@ function getCalleeName(call: ts.CallExpression): string|null {
* ```
*
* Here, the intermediate `MyClass_1` assignment is optional. In the above example, the
* `class MyClass {}` expression is returned as declaration of `MyClass`. Note that if `node`
* represents a regular class declaration, it will be returned as-is.
* `class MyClass {}` expression is returned as declaration of `var MyClass`. If the variable
* is not initialized using a class expression, null is returned.
*
* @param node the node that represents the class whose declaration we are finding.
* @returns the declaration of the class or `null` if it is not a "class".
*/
function getInnerClassDeclaration(node: ts.Node):
ClassDeclaration<ts.ClassDeclaration|ts.ClassExpression>|null {
function getInnerClassDeclaration(node: ts.Node): ClassDeclaration<ts.ClassExpression>|null {
if (!ts.isVariableDeclaration(node) || node.initializer === undefined) {
return null;
}

// Recognize a variable declaration of the form `var MyClass = class MyClass {}` or
// `var MyClass = MyClass_1 = class MyClass {};`
if (ts.isVariableDeclaration(node) && node.initializer !== undefined) {
node = node.initializer;
while (isAssignment(node)) {
node = node.right;
}
let expression = node.initializer;
while (isAssignment(expression)) {
expression = expression.right;
}

if (!ts.isClassDeclaration(node) && !ts.isClassExpression(node)) {
if (!ts.isClassExpression(expression) || !hasNameIdentifier(expression)) {
return null;
}

return hasNameIdentifier(node) ? node : null;
return expression;
}

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

0 comments on commit 373e133

Please sign in to comment.
You can’t perform that action at this time.