Skip to content
Permalink
Browse files

fix(ngcc): properly detect origin of constructor param types (#33901)

The ReflectionHost supports enumeration of constructor parameters, and one
piece of information it returns describes the origin of the parameter's
type. Parameter types come in two flavors: local (the type is not imported
from anywhere) or non-local (the type comes via an import).

ngcc incorrectly classified all type parameters as 'local', because in the
source files that ngcc processes the type parameter is a real ts.Identifer.
However, that identifier may still have come from an import and thus might
be non-local.

This commit changes ngcc's ReflectionHost(s) to properly recognize and
report these non-local type references.

Fixes #33677

PR Close #33901
  • Loading branch information
alxhub committed Nov 18, 2019
1 parent c126a6e commit 05d5c4fc76cb962028c518ba87b4bd23403f224b
@@ -9,7 +9,7 @@
import * as ts from 'typescript';

import {AbsoluteFsPath} from '../../../src/ngtsc/file_system';
import {ClassDeclaration, ClassMember, ClassMemberKind, ConcreteDeclaration, CtorParameter, Declaration, Decorator, TypeScriptReflectionHost, isDecoratorIdentifier, reflectObjectLiteral} from '../../../src/ngtsc/reflection';
import {ClassDeclaration, ClassMember, ClassMemberKind, ConcreteDeclaration, CtorParameter, Declaration, Decorator, TypeScriptReflectionHost, TypeValueReference, isDecoratorIdentifier, reflectObjectLiteral} from '../../../src/ngtsc/reflection';
import {isWithinPackage} from '../analysis/util';
import {Logger} from '../logging/logger';
import {BundleProgram} from '../packages/bundle_program';
@@ -1357,12 +1357,34 @@ export class Esm2015ReflectionHost extends TypeScriptReflectionHost implements N
constructorParamInfo[index] :
{decorators: null, typeExpression: null};
const nameNode = node.name;

let typeValueReference: TypeValueReference|null = null;
if (typeExpression !== null) {
// `typeExpression` is an expression in a "type" context. Resolve it to a declared value.
// Either it's a reference to an imported type, or a type declared locally. Distinguish the
// two cases with `getDeclarationOfExpression`.
const decl = this.getDeclarationOfExpression(typeExpression);
if (decl !== null && decl.node !== null && decl.viaModule !== null &&
isNamedDeclaration(decl.node)) {
typeValueReference = {
local: false,
valueDeclaration: decl.node,
moduleName: decl.viaModule,
name: decl.node.name.text,
};
} else {
typeValueReference = {
local: true,
expression: typeExpression,
defaultImportStatement: null,
};
}
}

return {
name: getNameText(nameNode),
nameNode,
typeValueReference: typeExpression !== null ?
{local: true as true, expression: typeExpression, defaultImportStatement: null} :
null,
typeValueReference,
typeNode: null, decorators
};
});
@@ -54,11 +54,10 @@ runInEachFileSystem(() => {
SOME_DIRECTIVE_FILE = {
name: _('/some_directive.js'),
contents: `
import { Directive, Inject, InjectionToken, Input, HostListener, HostBinding } from '@angular/core';
import { Directive, Inject, InjectionToken, Input, HostListener, HostBinding, ViewContainerRef, TemplateRef } from '@angular/core';
const INJECTED_TOKEN = new InjectionToken('injected');
const ViewContainerRef = {};
const TemplateRef = {};
const TestToken = {};
class SomeDirective {
constructor(_viewContainer, _template, injected) {
@@ -196,6 +195,7 @@ runInEachFileSystem(() => {
class NoArgsProperty {
}
NoArgsProperty.decorators = [
{ type: Directive },
];
@@ -1099,6 +1099,7 @@ runInEachFileSystem(() => {

describe('getConstructorParameters()', () => {
it('should find the decorated constructor parameters', () => {
loadFakeCore(getFileSystem());
loadTestFiles([SOME_DIRECTIVE_FILE]);
const {program} = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name);
const host = new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker());
@@ -1111,7 +1112,7 @@ runInEachFileSystem(() => {
'_viewContainer', '_template', 'injected'
]);
expectTypeValueReferencesForParameters(
parameters, ['ViewContainerRef', 'TemplateRef', null]);
parameters, ['ViewContainerRef', 'TemplateRef', null], '@angular/core');
});

it('should accept `ctorParameters` as an array', () => {
@@ -1499,23 +1500,15 @@ runInEachFileSystem(() => {

describe('getDeclarationOfIdentifier()', () => {
it('should return the declaration of a locally defined identifier', () => {
loadFakeCore(getFileSystem());
loadTestFiles([SOME_DIRECTIVE_FILE]);
const {program} = makeTestBundleProgram(SOME_DIRECTIVE_FILE.name);
const host = new Esm2015ReflectionHost(new MockLogger(), false, program.getTypeChecker());
const classNode = getDeclaration(
program, SOME_DIRECTIVE_FILE.name, 'SomeDirective', isNamedClassDeclaration);
const ctrDecorators = host.getConstructorParameters(classNode) !;
const identifierOfViewContainerRef = (ctrDecorators[0].typeValueReference !as{
local: true,
expression: ts.Identifier,
defaultImportStatement: null,
}).expression;

const expectedDeclarationNode = getDeclaration(
program, SOME_DIRECTIVE_FILE.name, 'ViewContainerRef', isNamedVariableDeclaration);
const actualDeclaration = host.getDeclarationOfIdentifier(identifierOfViewContainerRef);
const actualDeclaration = host.getDeclarationOfIdentifier(classNode.name);
expect(actualDeclaration).not.toBe(null);
expect(actualDeclaration !.node).toBe(expectedDeclarationNode);
expect(actualDeclaration !.node).toBe(classNode);
expect(actualDeclaration !.viaModule).toBe(null);
});

@@ -1560,9 +1560,6 @@ runInEachFileSystem(() => {

expect(decorators.length).toEqual(1);
expect(decorators[0].import).toBe(mockImportInfo);

const typeIdentifier = spy.calls.mostRecent().args[0] as ts.Identifier;
expect(typeIdentifier.text).toBe('Inject');
});
});

@@ -14,15 +14,26 @@ import {CtorParameter} from '../../../src/ngtsc/reflection';
* names.
*/
export function expectTypeValueReferencesForParameters(
parameters: CtorParameter[], expectedParams: (string | null)[]) {
parameters: CtorParameter[], expectedParams: (string | null)[],
fromModule: string | null = null) {
parameters !.forEach((param, idx) => {
const expected = expectedParams[idx];
if (expected !== null) {
if (param.typeValueReference === null || !param.typeValueReference.local ||
!ts.isIdentifier(param.typeValueReference.expression)) {
if (param.typeValueReference === null) {
fail(`Incorrect typeValueReference generated, expected ${expected}`);
} else {
expect(param.typeValueReference.expression.text).toEqual(expected);
} else if (param.typeValueReference.local && fromModule !== null) {
fail(`Incorrect typeValueReference generated, expected non-local`);
} else if (!param.typeValueReference.local && fromModule === null) {
fail(`Incorrect typeValueReference generated, expected local`);
} else if (param.typeValueReference.local) {
if (!ts.isIdentifier(param.typeValueReference.expression)) {
fail(`Incorrect typeValueReference generated, expected identifer`);
} else {
expect(param.typeValueReference.expression.text).toEqual(expected);
}
} else if (param.typeValueReference !== null) {
expect(param.typeValueReference.moduleName).toBe(fromModule !);
expect(param.typeValueReference.name).toBe(expected);
}
}
});

0 comments on commit 05d5c4f

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