From 70c3461be38765efedac1e4c5dd1156023a29690 Mon Sep 17 00:00:00 2001 From: JoostK Date: Sat, 17 Jul 2021 22:50:03 +0200 Subject: [PATCH] fix(compiler-cli): use correct module import for types behind a `forwardRef` (#42887) The static interpreter assumed that a foreign function expression would have to be imported from the absolute module specifier that was used for the foreign function itself. This assumption does not hold for the `forwardRef` foreign function resolver, as that extracts the resolved expression from the function's argument, which is not behind the absolute module import of the `forwardRef` function. The prior behavior has worked for the typical usage of `forwardRef`, when it is contained within the same source file as where the static evaluation started. In that case, the resulting reference would incorrectly have an absolute module guess of `@angular/core`, but the local identifier emit strategy was capable of emitting the reference without generating an import using the absolute module guess. In the scenario where the static interpreter would first have to follow a reference to a different source that contained the `forwardRef` would the compilation fail. In that case, there is no local identifier available such that the absolute module emitter would try to locate the imported symbol from `@angular/core`. which fails as the symbol is not exported from there. This commit fixes the issue by checking whether a foreign expression occurs in the same source file as the call expression. If it does, then the absolute module specifier that was used to resolve the call expression is ignored. Fixes #42865 PR Close #42887 --- .../partial_evaluator/src/interpreter.ts | 9 +- .../partial_evaluator/test/evaluator_spec.ts | 94 ++++++++++++++++++- .../src/ngtsc/partial_evaluator/test/utils.ts | 11 +++ .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 54 +++++++++++ 4 files changed, 162 insertions(+), 6 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts index 44868a6b52..7102efbcdd 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts @@ -463,13 +463,14 @@ export class StaticInterpreter { node, DynamicValue.fromExternalReference(node.expression, lhs)); } - // If the function is declared in a different file, resolve the foreign function expression - // using the absolute module name of that file (if any). - if (lhs.bestGuessOwningModule !== null) { + // If the foreign expression occurs in a different file, then assume that the owning module + // of the call expression should also be used for the resolved foreign expression. + if (expr.getSourceFile() !== node.expression.getSourceFile() && + lhs.bestGuessOwningModule !== null) { context = { ...context, absoluteModuleName: lhs.bestGuessOwningModule.specifier, - resolutionContext: node.getSourceFile().fileName, + resolutionContext: lhs.bestGuessOwningModule.resolutionContext, }; } diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts index 710143d799..452b7dd412 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/test/evaluator_spec.ts @@ -14,10 +14,10 @@ import {DependencyTracker} from '../../incremental/api'; import {Declaration, DeclarationKind, isConcreteDeclaration, KnownDeclaration, SpecialDeclarationKind, TypeScriptReflectionHost} from '../../reflection'; import {getDeclaration, makeProgram} from '../../testing'; import {DynamicValue} from '../src/dynamic'; -import {PartialEvaluator} from '../src/interface'; +import {ForeignFunctionResolver, PartialEvaluator} from '../src/interface'; import {EnumValue, ResolvedValue} from '../src/result'; -import {evaluate, firstArgFfr, makeEvaluator, makeExpression, owningModuleOf} from './utils'; +import {arrowReturnValueFfr, evaluate, firstArgFfr, makeEvaluator, makeExpression, owningModuleOf, returnTypeFfr} from './utils'; runInEachFileSystem(() => { describe('ngtsc metadata', () => { @@ -646,6 +646,96 @@ runInEachFileSystem(() => { expect(id.text).toEqual('Target'); }); + it('should not associate an owning module when a FFR-resolved expression is within the originating source file', + () => { + const resolved = evaluate( + `import {forwardRef} from 'forward'; + class Foo {}`, + 'forwardRef(() => Foo)', [{ + name: _('/node_modules/forward/index.d.ts'), + contents: `export declare function forwardRef(fn: () => T): T;`, + }], + arrowReturnValueFfr); + if (!(resolved instanceof Reference)) { + return fail('Expected expression to resolve to a reference'); + } + expect((resolved.node as ts.ClassDeclaration).name!.text).toBe('Foo'); + expect(resolved.bestGuessOwningModule).toBeNull(); + }); + + it('should not associate an owning module when a FFR-resolved expression is imported using a relative import', + () => { + const resolved = evaluate( + `import {forwardRef} from 'forward'; + import {Foo} from './foo';`, + 'forwardRef(() => Foo)', + [ + { + name: _('/node_modules/forward/index.d.ts'), + contents: `export declare function forwardRef(fn: () => T): T;`, + }, + { + name: _('/foo.ts'), + contents: `export class Foo {}`, + } + ], + arrowReturnValueFfr); + if (!(resolved instanceof Reference)) { + return fail('Expected expression to resolve to a reference'); + } + expect((resolved.node as ts.ClassDeclaration).name!.text).toBe('Foo'); + expect(resolved.bestGuessOwningModule).toBeNull(); + }); + + it('should associate an owning module when a FFR-resolved expression is imported using an absolute import', + () => { + const {expression, checker} = makeExpression( + `import {forwardRef} from 'forward'; + import {Foo} from 'external';`, + `forwardRef(() => Foo)`, [ + { + name: _('/node_modules/forward/index.d.ts'), + contents: `export declare function forwardRef(fn: () => T): T;`, + }, + { + name: _('/node_modules/external/index.d.ts'), + contents: `export declare class Foo {}`, + } + ]); + const evaluator = makeEvaluator(checker); + const resolved = evaluator.evaluate(expression, arrowReturnValueFfr); + if (!(resolved instanceof Reference)) { + return fail('Expected expression to resolve to a reference'); + } + expect((resolved.node as ts.ClassDeclaration).name!.text).toBe('Foo'); + expect(resolved.bestGuessOwningModule).toEqual({ + specifier: 'external', + resolutionContext: expression.getSourceFile().fileName, + }); + }); + + it('should associate an owning module when a FFR-resolved expression is within the foreign file', + () => { + const {expression, checker} = + makeExpression(`import {external} from 'external';`, `external()`, [{ + name: _('/node_modules/external/index.d.ts'), + contents: ` + export declare class Foo {} + export declare function external(): Foo; + ` + }]); + const evaluator = makeEvaluator(checker); + const resolved = evaluator.evaluate(expression, returnTypeFfr); + if (!(resolved instanceof Reference)) { + return fail('Expected expression to resolve to a reference'); + } + expect((resolved.node as ts.ClassDeclaration).name!.text).toBe('Foo'); + expect(resolved.bestGuessOwningModule).toEqual({ + specifier: 'external', + resolutionContext: expression.getSourceFile().fileName, + }); + }); + it('should resolve functions with more than one statement to a complex function call', () => { const value = evaluate(`function foo(bar) { const b = bar; return b; }`, 'foo("test")'); diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/test/utils.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/test/utils.ts index 38daf368e1..1bb6c46d83 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/test/utils.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/test/utils.ts @@ -62,3 +62,14 @@ export function firstArgFfr( args: ReadonlyArray): ts.Expression { return args[0]; } + +export const arrowReturnValueFfr: ForeignFunctionResolver = (_ref, args) => { + // Extracts the `Foo` from `() => Foo`. + return (args[0] as ts.ArrowFunction).body as ts.Expression; +}; + +export const returnTypeFfr: ForeignFunctionResolver = (ref) => { + // Extract the `Foo` from the return type of the `external` function declaration. + return ((ref.node as ts.FunctionDeclaration).type as ts.TypeReferenceNode).typeName as + ts.Identifier; +}; diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 7b865e80aa..64cd47c6a3 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -1558,6 +1558,60 @@ function allTests(os: string) { expect(jsContents).toContain('exports: function () { return [BarModule]; }'); }); + it('should use relative import for forward references that were resolved from a relative file', + () => { + env.write('dir.ts', ` + import {Directive, forwardRef} from '@angular/core'; + + export const useFoo = forwardRef(() => Foo); + + @Directive({selector: 'foo'}) + export class Foo {} + `); + env.write('test.ts', ` + import {NgModule} from '@angular/core'; + import {useFoo} from './dir'; + + @NgModule({ + declarations: [useFoo], + }) + export class FooModule {} + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('import * as i1 from "./dir";'); + expect(jsContents).toContain('declarations: [i1.Foo]'); + }); + + it('should use absolute import for forward references that were resolved from an absolute file', + () => { + env.write('dir.ts', ` + import {Directive, forwardRef} from '@angular/core'; + + export const useFoo = forwardRef(() => Foo); + + @Directive({selector: 'foo'}) + export class Foo {} + `); + env.write('test.ts', ` + import {forwardRef, NgModule} from '@angular/core'; + import {useFoo} from 'dir'; + + @NgModule({ + declarations: [useFoo], + }) + export class FooModule {} + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('import * as i1 from "dir";'); + expect(jsContents).toContain('declarations: [i1.Foo]'); + }); + it('should compile Pipes without errors', () => { env.write('test.ts', ` import {Pipe} from '@angular/core';