cargdev/angular
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(ivy): never use imported type references as values (#29111)

Browse Source 
ngtsc occasionally converts a type reference (such as the type of a
parameter in a constructor) to a value reference (argument to a
directiveInject call). TypeScript has a bad habit of sometimes removing
the import statement associated with this type reference, because it's a
type only import when it initially looks at the file.

A solution to this is to always add an import to refer to a type position
value that's imported, and not rely on the existing import.

PR Close #29111
This commit is contained in:
Alex Rickabaugh
2019-03-04 11:43:55 -08:00
committed by Andrew Kushnir
parent 20a9dbef8e
commit 881807dc36
15 changed files with 308 additions and 87 deletions
Download Patch File Download Diff File Expand all files Collapse all files

41
packages/compiler-cli/src/ngtsc/reflection/src/host.ts
View File

@ -151,6 +151,31 @@ export interface ClassMember {
decorators: Decorator[]|null;
}
/**
* A reference to a value that originated from a type position.
*
* For example, a constructor parameter could be declared as `foo: Foo`. A `TypeValueReference`
* extracted from this would refer to the value of the class `Foo` (assuming it was actually a
* type).
*
* There are two kinds of such references. A reference with `local: false` refers to a type that was
* imported, and gives the symbol `name` and the `moduleName` of the import. Note that this
* `moduleName` may be a relative path, and thus is likely only valid within the context of the file
* which contained the original type reference.
*
* A reference with `local: true` refers to any other kind of type via a `ts.Expression` that's
* valid within the local file where the type was referenced.
*/
export type TypeValueReference = {
local: true; expression: ts.Expression;
} |
{
local: false;
name: string;
moduleName: string;
valueDeclaration: ts.Declaration;
};
/**
* A parameter to a constructor.
*/
@ -172,18 +197,22 @@ export interface CtorParameter {
nameNode: ts.BindingName;
/**
* TypeScript `ts.Expression` representing the type value of the parameter, if the type is a
* simple
* expression type that can be converted to a value.
* Reference to the value of the parameter's type annotation, if it's possible to refer to the
* parameter's type as a value.
*
* If the type is not present or cannot be represented as an expression, `type` is `null`.
* This can either be a reference to a local value, in which case it has `local` set to `true` and
* contains a `ts.Expression`, or it's a reference to an imported value, in which case `local` is
* set to `false` and the symbol and module name of the imported value are provided instead.
*
* If the type is not present or cannot be represented as an expression, `typeValueReference` is
* `null`.
*/
typeExpression: ts.Expression|null;
typeValueReference: TypeValueReference|null;
/**
* TypeScript `ts.TypeNode` representing the type node found in the type position.
*
* This field can be used for diagnostics reporting if `typeExpression` is `null`.
* This field can be used for diagnostics reporting if `typeValueReference` is `null`.
*
* Can be null, if the param has no type declared.
*/

115
packages/compiler-cli/src/ngtsc/reflection/src/typescript.ts
View File

@ -8,7 +8,7 @@
import * as ts from 'typescript';
import {ClassMember, ClassMemberKind, CtorParameter, Declaration, Decorator, FunctionDefinition, Import, ReflectionHost} from './host';
import {ClassMember, ClassMemberKind, CtorParameter, Declaration, Decorator, FunctionDefinition, Import, ReflectionHost, TypeValueReference} from './host';
/**
* reflector.ts implements static reflection of declarations using the TypeScript `ts.TypeChecker`.
@ -48,7 +48,7 @@ export class TypeScriptReflectionHost implements ReflectionHost {
// It may or may not be possible to write an expression that refers to the value side of the
// type named for the parameter.
let typeValueExpr: ts.Expression|null = null;
let typeValueExpr: TypeValueReference|null = null;
let originalTypeNode = node.type || null;
let typeNode = originalTypeNode;
@ -69,27 +69,57 @@ export class TypeScriptReflectionHost implements ReflectionHost {
// It's not possible to get a value expression if the parameter doesn't even have a type.
if (typeNode && ts.isTypeReferenceNode(typeNode)) {
// It's only valid to convert a type reference to a value reference if the type actually has
// a value declaration associated with it.
let symbol: ts.Symbol|undefined = this.checker.getSymbolAtLocation(typeNode.typeName);
if (symbol !== undefined) {
let resolvedSymbol = symbol;
if (symbol.flags & ts.SymbolFlags.Alias) {
resolvedSymbol = this.checker.getAliasedSymbol(symbol);
}
if (resolvedSymbol.valueDeclaration !== undefined) {
const symbols = resolveTypeSymbols(typeNode, this.checker);
if (symbols !== null) {
const {local, decl} = symbols;
// It's only valid to convert a type reference to a value reference if the type actually
// has a value declaration associated with it.
if (decl.valueDeclaration !== undefined) {
// The type points to a valid value declaration. Rewrite the TypeReference into an
// Expression which references the value pointed to by the TypeReference, if possible.
const firstDecl = symbol.declarations && symbol.declarations[0];
// Look at the local `ts.Symbol`'s declarations and see if it comes from an import
// statement. If so, extract the module specifier and the name of the imported type.
const firstDecl = local.declarations && local.declarations[0];
if (firstDecl && ts.isImportSpecifier(firstDecl)) {
// Making sure TS produces the necessary imports in case a symbol was declared in a
// different script and imported. To do that we check symbol's first declaration and
// if it's an import - use its identifier. The `Identifier` from the `ImportSpecifier`
// knows it could be a value reference, and will emit as one if needed.
typeValueExpr = ts.updateIdentifier(firstDecl.name);
// The symbol was imported by name, in a ts.ImportSpecifier.
const name = (firstDecl.propertyName || firstDecl.name).text;
const moduleSpecifier = firstDecl.parent.parent.parent.moduleSpecifier;
if (!ts.isStringLiteral(moduleSpecifier)) {
throw new Error('not a module specifier');
}
const moduleName = moduleSpecifier.text;
typeValueExpr = {
local: false,
name,
moduleName,
valueDeclaration: decl.valueDeclaration,
};
} else if (
firstDecl && ts.isNamespaceImport(firstDecl) && symbols.importName !== null) {
// The symbol was imported via a namespace import. In this case, the name to use when
// importing it was extracted by resolveTypeSymbols.
const name = symbols.importName;
const moduleSpecifier = firstDecl.parent.parent.moduleSpecifier;
if (!ts.isStringLiteral(moduleSpecifier)) {
throw new Error('not a module specifier');
}
const moduleName = moduleSpecifier.text;
typeValueExpr = {
local: false,
name,
moduleName,
valueDeclaration: decl.valueDeclaration,
};
} else {
typeValueExpr = typeNodeToValueExpr(typeNode);
const expression = typeNodeToValueExpr(typeNode);
if (expression !== null) {
typeValueExpr = {
local: true,
expression,
};
}
}
}
}
@ -98,7 +128,7 @@ export class TypeScriptReflectionHost implements ReflectionHost {
return {
name,
nameNode: node.name,
typeExpression: typeValueExpr,
typeValueReference: typeValueExpr,
typeNode: originalTypeNode, decorators,
};
});
@ -486,3 +516,48 @@ function propertyNameToString(node: ts.PropertyName): string|null {
return null;
}
}
/**
* Resolve a `TypeReference` node to the `ts.Symbol`s for both its declaration and its local source.
*
* In the event that the `TypeReference` refers to a locally declared symbol, these will be the
* same. If the `TypeReference` refers to an imported symbol, then `decl` will be the fully resolved
* `ts.Symbol` of the referenced symbol. `local` will be the `ts.Symbol` of the `ts.Identifer` which
* points to the import statement by which the symbol was imported.
*/
function resolveTypeSymbols(typeRef: ts.TypeReferenceNode, checker: ts.TypeChecker):
{local: ts.Symbol, decl: ts.Symbol, importName: string | null}|null {
const typeName = typeRef.typeName;
// typeRefSymbol is the ts.Symbol of the entire type reference.
const typeRefSymbol: ts.Symbol|undefined = checker.getSymbolAtLocation(typeName);
if (typeRefSymbol === undefined) {
return null;
}
// local is the ts.Symbol for the local ts.Identifier for the type.
// If the type is actually locally declared or is imported by name, for example:
// import {Foo} from './foo';
// then it'll be the same as top. If the type is imported via a namespace import, for example:
// import * as foo from './foo';
// and then referenced as:
// constructor(f: foo.Foo)
// then local will be the ts.Symbol of `foo`, whereas top will be the ts.Symbol of `foo.Foo`.
// This allows tracking of the import behind whatever type reference exists.
let local = typeRefSymbol;
let importName: string|null = null;
if (ts.isQualifiedName(typeName) && ts.isIdentifier(typeName.left) &&
ts.isIdentifier(typeName.right)) {
const localTmp = checker.getSymbolAtLocation(typeName.left);
if (localTmp !== undefined) {
local = localTmp;
importName = typeName.right.text;
}
}
// De-alias the top-level type reference symbol to get the symbol of the actual declaration.
let decl = typeRefSymbol;
if (typeRefSymbol.flags & ts.SymbolFlags.Alias) {
decl = checker.getAliasedSymbol(typeRefSymbol);
}
return {local, decl, importName};
}

54
packages/compiler-cli/src/ngtsc/reflection/test/ts_host_spec.ts
View File

@ -116,12 +116,38 @@ describe('reflector', () => {
const host = new TypeScriptReflectionHost(checker);
const args = host.getConstructorParameters(clazz) !;
expect(args.length).toBe(2);
expectParameter(args[0], 'bar', 'Bar');
expectParameter(args[1], 'otherBar', 'star.Bar');
expectParameter(args[0], 'bar', {moduleName: './bar', name: 'Bar'});
expectParameter(args[1], 'otherBar', {moduleName: './bar', name: 'Bar'});
});
it('should reflect an argument from an aliased import', () => {
const {program} = makeProgram([
{
name: 'bar.ts',
contents: `
export class Bar {}
`
},
{
name: 'entry.ts',
contents: `
import {Bar as LocalBar} from './bar';
it('should reflect an nullable argument', () => {
class Foo {
constructor(bar: LocalBar) {}
}
`
}
]);
const clazz = getDeclaration(program, 'entry.ts', 'Foo', ts.isClassDeclaration);
const checker = program.getTypeChecker();
const host = new TypeScriptReflectionHost(checker);
const args = host.getConstructorParameters(clazz) !;
expect(args.length).toBe(1);
expectParameter(args[0], 'bar', {moduleName: './bar', name: 'Bar'});
});
it('should reflect a nullable argument', () => {
const {program} = makeProgram([
{
name: 'bar.ts',
@ -145,7 +171,7 @@ describe('reflector', () => {
const host = new TypeScriptReflectionHost(checker);
const args = host.getConstructorParameters(clazz) !;
expect(args.length).toBe(1);
expectParameter(args[0], 'bar', 'Bar');
expectParameter(args[0], 'bar', {moduleName: './bar', name: 'Bar'});
});
});
@ -193,14 +219,24 @@ describe('reflector', () => {
});
function expectParameter(
param: CtorParameter, name: string, type?: string, decorator?: string,
decoratorFrom?: string): void {
param: CtorParameter, name: string, type?: string | {name: string, moduleName: string},
decorator?: string, decoratorFrom?: string): void {
expect(param.name !).toEqual(name);
if (type === undefined) {
expect(param.typeExpression).toBeNull();
expect(param.typeValueReference).toBeNull();
} else {
expect(param.typeExpression).not.toBeNull();
expect(argExpressionToString(param.typeExpression !)).toEqual(type);
if (param.typeValueReference === null) {
return fail(`Expected parameter ${name} to have a typeValueReference`);
}
if (param.typeValueReference.local && typeof type === 'string') {
expect(argExpressionToString(param.typeValueReference.expression)).toEqual(type);
} else if (!param.typeValueReference.local && typeof type !== 'string') {
expect(param.typeValueReference.moduleName).toEqual(type.moduleName);
expect(param.typeValueReference.name).toEqual(type.name);
} else {
return fail(
`Mismatch between typeValueReference and expected type: ${param.name} / ${param.typeValueReference.local}`);
}
}
if (decorator !== undefined) {
expect(param.decorators).not.toBeNull();