Navigation Menu

Skip to content

Commit

Permalink
fix(compiler-cli): use correct module import for types behind a `forw…
Browse files Browse the repository at this point in the history
…ardRef` (#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
  • Loading branch information
JoostK authored and alxhub committed Jul 20, 2021
1 parent ca7d4c3 commit 70c3461
Show file tree
Hide file tree
Showing 4 changed files with 162 additions and 6 deletions.
Expand Up @@ -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,
};
}

Expand Down
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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<T>(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<T>(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<T>(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")');

Expand Down
11 changes: 11 additions & 0 deletions packages/compiler-cli/src/ngtsc/partial_evaluator/test/utils.ts
Expand Up @@ -62,3 +62,14 @@ export function firstArgFfr(
args: ReadonlyArray<ts.Expression>): 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;
};
54 changes: 54 additions & 0 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Expand Up @@ -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';
Expand Down

0 comments on commit 70c3461

Please sign in to comment.