From 139f5b367234443d12a68c1042b92352cb48fcd7 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Thu, 12 Jul 2018 15:04:07 -0700 Subject: [PATCH] fix(ivy): references track the identifier they were discovered under (#24862) Previously, references had the concept of an identifier, but would not properly detect whether the identifier should be used or not when generating an expression. This change fixes that logic. Additionally, now whenever an identifier resolves to a reference (even one imported from another module) as part of resolving an expression, the reference is updated to use that identifier. This ensures that for a class Foo declared in foo.ts, but referenced in an expression in bar.ts, the Reference returned includes the identifier from bar.ts, meaning that writing an expression in bar.ts for the Reference will not generate an import. PR Close #24862 --- .../src/ngtsc/metadata/src/resolver.ts | 23 +++++++++++++-- .../src/ngtsc/metadata/test/resolver_spec.ts | 29 +++++++++++-------- 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts b/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts index 530406392c..98a4e7cb21 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/resolver.ts @@ -100,6 +100,8 @@ export abstract class Reference { * import if needed. */ abstract toExpression(context: ts.SourceFile): Expression|null; + + abstract withIdentifier(identifier: ts.Identifier): Reference; } /** @@ -112,6 +114,8 @@ export class NodeReference extends Reference { constructor(node: ts.Node, readonly moduleName: string|null) { super(node); } toExpression(context: ts.SourceFile): null { return null; } + + withIdentifier(identifier: ts.Identifier): NodeReference { return this; } } /** @@ -125,7 +129,7 @@ export class ResolvedReference extends Reference { readonly expressable = true; toExpression(context: ts.SourceFile): Expression { - if (ts.getOriginalNode(context) === ts.getOriginalNode(this.node).getSourceFile()) { + if (ts.getOriginalNode(context) === ts.getOriginalNode(this.identifier).getSourceFile()) { return new WrappedNodeExpr(this.identifier); } else { // Relative import from context -> this.node.getSourceFile(). @@ -150,6 +154,10 @@ export class ResolvedReference extends Reference { } } } + + withIdentifier(identifier: ts.Identifier): ResolvedReference { + return new ResolvedReference(this.node, identifier); + } } /** @@ -168,12 +176,16 @@ export class AbsoluteReference extends Reference { readonly expressable = true; toExpression(context: ts.SourceFile): Expression { - if (ts.getOriginalNode(context) === ts.getOriginalNode(this.node.getSourceFile())) { + if (ts.getOriginalNode(context) === ts.getOriginalNode(this.identifier).getSourceFile()) { return new WrappedNodeExpr(this.identifier); } else { return new ExternalExpr(new ExternalReference(this.moduleName, this.symbolName)); } } + + withIdentifier(identifier: ts.Identifier): AbsoluteReference { + return new AbsoluteReference(this.node, identifier, this.moduleName, this.symbolName); + } } /** @@ -379,8 +391,13 @@ class StaticInterpreter { if (decl === null) { return DYNAMIC_VALUE; } - return this.visitDeclaration( + const result = this.visitDeclaration( decl.node, {...context, absoluteModuleName: decl.viaModule || context.absoluteModuleName}); + if (result instanceof Reference) { + return result.withIdentifier(node); + } else { + return result; + } } private visitDeclaration(node: ts.Declaration, context: Context): ResolvedValue { diff --git a/packages/compiler-cli/src/ngtsc/metadata/test/resolver_spec.ts b/packages/compiler-cli/src/ngtsc/metadata/test/resolver_spec.ts index c4d1cc27a8..ee34ca4501 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/test/resolver_spec.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/test/resolver_spec.ts @@ -6,12 +6,12 @@ * found in the LICENSE file at https://angular.io/license */ -import {ExternalExpr} from '@angular/compiler'; +import {WrappedNodeExpr} from '@angular/compiler'; import * as ts from 'typescript'; import {TypeScriptReflectionHost} from '..'; import {getDeclaration, makeProgram} from '../../testing/in_memory_typescript'; -import {Reference, ResolvedValue, staticallyResolve} from '../src/resolver'; +import {AbsoluteReference, Reference, ResolvedValue, staticallyResolve} from '../src/resolver'; function makeSimpleProgram(contents: string): ts.Program { return makeProgram([{name: 'entry.ts', contents}]).program; @@ -145,11 +145,13 @@ describe('ngtsc metadata', () => { expect(ts.isFunctionDeclaration(resolved.node)).toBe(true); expect(resolved.expressable).toBe(true); const reference = resolved.toExpression(program.getSourceFile('entry.ts') !); - if (!(reference instanceof ExternalExpr)) { - return fail('Expected expression reference to be an external (import) expression'); + if (!(reference instanceof WrappedNodeExpr)) { + return fail('Expected expression reference to be a wrapped node'); } - expect(reference.value.moduleName).toBe('./second'); - expect(reference.value.name).toBe('foo'); + if (!ts.isIdentifier(reference.node)) { + return fail('Expected expression to be an Identifier'); + } + expect(reference.node.getSourceFile()).toEqual(program.getSourceFile('entry.ts') !); }); it('absolute imports work', () => { @@ -168,17 +170,20 @@ describe('ngtsc metadata', () => { const result = getDeclaration(program, 'entry.ts', 'target$', ts.isVariableDeclaration); const expr = result.initializer !; const resolved = staticallyResolve(expr, host, checker); - if (!(resolved instanceof Reference)) { - return fail('Expected expression to resolve to a reference'); + if (!(resolved instanceof AbsoluteReference)) { + return fail('Expected expression to resolve to an absolute reference'); } + expect(resolved.moduleName).toBe('some_library'); expect(ts.isFunctionDeclaration(resolved.node)).toBe(true); expect(resolved.expressable).toBe(true); const reference = resolved.toExpression(program.getSourceFile('entry.ts') !); - if (!(reference instanceof ExternalExpr)) { - return fail('Expected expression reference to be an external (import) expression'); + if (!(reference instanceof WrappedNodeExpr)) { + return fail('Expected expression reference to be a wrapped node'); } - expect(reference.value.moduleName).toBe('some_library'); - expect(reference.value.name).toBe('foo'); + if (!ts.isIdentifier(reference.node)) { + return fail('Expected expression to be an Identifier'); + } + expect(reference.node.getSourceFile()).toEqual(program.getSourceFile('entry.ts') !); }); it('reads values from default exports', () => {