Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(compiler-cli): track visited source files in PartialEvaluator #29539

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/compiler-cli/src/ngtsc/annotations/src/util.ts
Expand Up @@ -253,8 +253,8 @@ export function unwrapForwardRef(node: ts.Expression, reflector: ReflectionHost)
* @returns an unwrapped argument if `ref` pointed to forwardRef, or null otherwise
*/
export function forwardRefResolver(
ref: Reference<ts.FunctionDeclaration|ts.MethodDeclaration>,
args: ts.Expression[]): ts.Expression|null {
ref: Reference<ts.FunctionDeclaration|ts.MethodDeclaration|ts.FunctionExpression>,
args: ReadonlyArray<ts.Expression>): ts.Expression|null {
if (!isAngularCoreReference(ref, 'forwardRef') || args.length !== 1) {
return null;
}
Expand All @@ -266,8 +266,8 @@ export function forwardRefResolver(
* @param resolvers Resolvers to be combined.
*/
export function combineResolvers(resolvers: ForeignFunctionResolver[]): ForeignFunctionResolver {
return (ref: Reference<ts.FunctionDeclaration|ts.MethodDeclaration>,
args: ts.Expression[]): ts.Expression |
return (ref: Reference<ts.FunctionDeclaration|ts.MethodDeclaration|ts.FunctionExpression>,
args: ReadonlyArray<ts.Expression>): ts.Expression |
null => {
for (const resolver of resolvers) {
const resolved = resolver(ref, args);
Expand Down
Expand Up @@ -18,11 +18,18 @@ export type ForeignFunctionResolver =
(node: Reference<ts.FunctionDeclaration|ts.MethodDeclaration|ts.FunctionExpression>,
args: ReadonlyArray<ts.Expression>) => ts.Expression | null;

export type VisitedFilesCallback = (sf: ts.SourceFile) => void;

export class PartialEvaluator {
constructor(private host: ReflectionHost, private checker: ts.TypeChecker) {}

evaluate(expr: ts.Expression, foreignFunctionResolver?: ForeignFunctionResolver): ResolvedValue {
const interpreter = new StaticInterpreter(this.host, this.checker);
evaluate(
expr: ts.Expression, foreignFunctionResolver?: ForeignFunctionResolver,
visitedFilesCb?: VisitedFilesCallback): ResolvedValue {
const interpreter = new StaticInterpreter(this.host, this.checker, visitedFilesCb);
if (visitedFilesCb) {
visitedFilesCb(expr.getSourceFile());
}
return interpreter.visit(expr, {
absoluteModuleName: null,
resolutionContext: expr.getSourceFile().fileName,
Expand Down
Expand Up @@ -14,7 +14,7 @@ import {Declaration, ReflectionHost} from '../../reflection';

import {ArraySliceBuiltinFn} from './builtin';
import {DynamicValue} from './dynamic';
import {ForeignFunctionResolver} from './interface';
import {ForeignFunctionResolver, VisitedFilesCallback} from './interface';
import {BuiltinFn, EnumValue, ResolvedValue, ResolvedValueArray, ResolvedValueMap} from './result';


Expand Down Expand Up @@ -79,7 +79,9 @@ interface Context {
}

export class StaticInterpreter {
constructor(private host: ReflectionHost, private checker: ts.TypeChecker) {}
constructor(
private host: ReflectionHost, private checker: ts.TypeChecker,
private visitedFilesCb?: VisitedFilesCallback) {}

visit(node: ts.Expression, context: Context): ResolvedValue {
return this.visitExpression(node, context);
Expand Down Expand Up @@ -231,6 +233,9 @@ export class StaticInterpreter {
}

private visitDeclaration(node: ts.Declaration, context: Context): ResolvedValue {
if (this.visitedFilesCb) {
this.visitedFilesCb(node.getSourceFile());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that was easier than I expected.

I suppose this catches everything. We add the starting expression, plus the ts.SourceFile of any declaration we traverse while evaluating... I think that'll work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't think of any other scenarios (except for the annoying re-re-export situation) that we would miss.

}
if (this.host.isClass(node)) {
return this.getReference(node, context);
} else if (ts.isVariableDeclaration(node)) {
Expand Down
Expand Up @@ -15,10 +15,6 @@ import {DynamicValue} from '../src/dynamic';
import {ForeignFunctionResolver, PartialEvaluator} from '../src/interface';
import {EnumValue, ResolvedValue} from '../src/result';

function makeSimpleProgram(contents: string): ts.Program {
return makeProgram([{name: 'entry.ts', contents}]).program;
}

function makeExpression(
code: string, expr: string, supportingFiles: {name: string, contents: string}[] = []): {
expression: ts.Expression,
Expand All @@ -40,12 +36,16 @@ function makeExpression(
};
}

function makeEvaluator(checker: ts.TypeChecker): PartialEvaluator {
const reflectionHost = new TypeScriptReflectionHost(checker);
return new PartialEvaluator(reflectionHost, checker);
}

function evaluate<T extends ResolvedValue>(
code: string, expr: string, supportingFiles: {name: string, contents: string}[] = [],
foreignFunctionResolver?: ForeignFunctionResolver): T {
const {expression, checker, program, options, host} = makeExpression(code, expr, supportingFiles);
const reflectionHost = new TypeScriptReflectionHost(checker);
const evaluator = new PartialEvaluator(reflectionHost, checker);
const {expression, checker} = makeExpression(code, expr, supportingFiles);
const evaluator = makeEvaluator(checker);
return evaluator.evaluate(expression, foreignFunctionResolver) as T;
}

Expand Down Expand Up @@ -334,6 +334,56 @@ describe('ngtsc metadata', () => {
}
expect(id.text).toEqual('Target');
});

describe('(visited file tracking)', () => {
it('should track each time a source file is visited', () => {
const visitedFilesSpy = jasmine.createSpy('visitedFilesCb');
const {expression, checker} =
makeExpression(`class A { static foo = 42; } function bar() { return A.foo; }`, 'bar()');
const evaluator = makeEvaluator(checker);
evaluator.evaluate(expression, undefined, visitedFilesSpy);
expect(visitedFilesSpy)
.toHaveBeenCalledTimes(3); // The initial expression, followed by two declaration visited
expect(visitedFilesSpy.calls.allArgs().map(args => args[0].fileName)).toEqual([
'/entry.ts', '/entry.ts', '/entry.ts'
]);
});

it('should track imported source files', () => {
const visitedFilesSpy = jasmine.createSpy('visitedFilesCb');
const {expression, checker} = makeExpression(`import {Y} from './other'; const A = Y;`, 'A', [
{name: 'other.ts', contents: `export const Y = 'test';`},
{name: 'not-visited.ts', contents: `export const Z = 'nope';`}
]);
const evaluator = makeEvaluator(checker);
evaluator.evaluate(expression, undefined, visitedFilesSpy);
expect(visitedFilesSpy).toHaveBeenCalledTimes(3);
expect(visitedFilesSpy.calls.allArgs().map(args => args[0].fileName)).toEqual([
'/entry.ts', '/entry.ts', '/other.ts'
]);
});

it('should track files passed through during re-exports', () => {
const visitedFilesSpy = jasmine.createSpy('visitedFilesCb');
const {expression, checker} =
makeExpression(`import * as mod from './direct-reexport';`, 'mod.value.property', [
{name: 'const.ts', contents: 'export const value = {property: "test"};'},
{name: 'def.ts', contents: `import {value} from './const'; export default value;`},
{name: 'indirect-reexport.ts', contents: `import value from './def'; export {value};`},
{name: 'direct-reexport.ts', contents: `export {value} from './indirect-reexport';`},
]);
const evaluator = makeEvaluator(checker);
evaluator.evaluate(expression, undefined, visitedFilesSpy);
expect(visitedFilesSpy).toHaveBeenCalledTimes(3);
expect(visitedFilesSpy.calls.allArgs().map(args => args[0].fileName)).toEqual([
'/entry.ts',
'/direct-reexport.ts',
// Not '/indirect-reexport.ts' or '/def.ts'.
// TS skips through them when finding the original symbol for `value`
'/const.ts',
]);
});
});
});

function owningModuleOf(ref: Reference): string|null {
Expand Down