From b07b6f1d4078feca5f321bf5d90427e4b2bb08ee Mon Sep 17 00:00:00 2001 From: JoostK Date: Tue, 12 Nov 2019 21:37:44 +0100 Subject: [PATCH] fix(ivy): avoid infinite recursion when evaluation source files (#33772) When ngtsc comes across a source file during partial evaluation, it would determine all exported symbols from that module and evaluate their values greedily. This greedy evaluation strategy introduces unnecessary work and can fall into infinite recursion when the evaluation result of an exported expression would circularly depend on the source file. This would primarily occur in CommonJS code, where the `exports` variable can be used to refer to an exported variable. This variable would be resolved to the source file itself, thereby greedily evaluating all exported symbols and thus ending up evaluating the `exports` variable again. This variable would be resolved to the source file itself, thereby greedily evaluating all exported symbols and thus ending u evaluating the `exports` variable again. This variable would be resolved to the source file itself, thereby greedily evaluating all exported symbols and thus ending up evaluating the `exports` variable again. This variable would be resolved to the source file itself, thereby greedily evaluating all exported symbols and thus ending up evaluating the `exports` variable again. This went on for some time until all stack frames were exhausted. This commit introduces a `ResolvedModule` that delays the evaluation of its exports until they are actually requested. This avoids the circular dependency when evaluating `exports`, thereby fixing the issue. Fix #33734 PR Close #33772 --- .../partial_evaluator/src/interpreter.ts | 20 ++++++---- .../src/ngtsc/partial_evaluator/src/result.ts | 37 ++++++++++++++++--- .../partial_evaluator/test/evaluator_spec.ts | 32 ++++++++++++++++ 3 files changed, 75 insertions(+), 14 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 73e960df56..7d7456b6aa 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts @@ -16,7 +16,7 @@ import {isDeclaration} from '../../util/src/typescript'; import {ArrayConcatBuiltinFn, ArraySliceBuiltinFn} from './builtin'; import {DynamicValue} from './dynamic'; import {DependencyTracker, ForeignFunctionResolver} from './interface'; -import {BuiltinFn, EnumValue, ResolvedValue, ResolvedValueArray, ResolvedValueMap} from './result'; +import {BuiltinFn, EnumValue, ResolvedModule, ResolvedValue, ResolvedValueArray, ResolvedValueMap} from './result'; import {evaluateTsHelperInline} from './ts_helpers'; @@ -183,11 +183,14 @@ export class StaticInterpreter { const spread = this.visitExpression(property.expression, context); if (spread instanceof DynamicValue) { return DynamicValue.fromDynamicInput(node, spread); - } else if (!(spread instanceof Map)) { + } else if (spread instanceof Map) { + spread.forEach((value, key) => map.set(key, value)); + } else if (spread instanceof ResolvedModule) { + spread.getExports().forEach((value, key) => map.set(key, value)); + } else { return DynamicValue.fromDynamicInput( node, DynamicValue.fromInvalidExpressionType(property, spread)); } - spread.forEach((value, key) => map.set(key, value)); } else { return DynamicValue.fromUnknown(node); } @@ -323,19 +326,18 @@ export class StaticInterpreter { if (declarations === null) { return DynamicValue.fromUnknown(node); } - const map = new Map(); - declarations.forEach((decl, name) => { + + return new ResolvedModule(declarations, decl => { const declContext = { ...context, ...joinModuleContext(context, node, decl), }; + // Visit both concrete and inline declarations. // TODO(alxhub): remove cast once TS is upgraded in g3. - const value = decl.node !== null ? + return decl.node !== null ? this.visitDeclaration(decl.node, declContext) : this.visitExpression((decl as InlineDeclaration).expression, declContext); - map.set(name, value); }); - return map; } private accessHelper( @@ -348,6 +350,8 @@ export class StaticInterpreter { } else { return undefined; } + } else if (lhs instanceof ResolvedModule) { + return lhs.getExport(strIndex); } else if (Array.isArray(lhs)) { if (rhs === 'length') { return lhs.length; diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/result.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/result.ts index bfcc2cd007..0cacefbc74 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/result.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/result.ts @@ -9,6 +9,7 @@ import * as ts from 'typescript'; import {Reference} from '../../imports'; +import {Declaration} from '../../reflection'; import {DynamicValue} from './dynamic'; @@ -21,23 +22,47 @@ import {DynamicValue} from './dynamic'; * available statically. */ export type ResolvedValue = number | boolean | string | null | undefined | Reference | EnumValue | - ResolvedValueArray | ResolvedValueMap | BuiltinFn | DynamicValue; + ResolvedValueArray | ResolvedValueMap | ResolvedModule | BuiltinFn | DynamicValue; /** * An array of `ResolvedValue`s. * * This is a reified type to allow the circular reference of `ResolvedValue` -> `ResolvedValueArray` - * -> - * `ResolvedValue`. + * -> `ResolvedValue`. */ export interface ResolvedValueArray extends Array {} /** * A map of strings to `ResolvedValue`s. * - * This is a reified type to allow the circular reference of `ResolvedValue` -> `ResolvedValueMap` -> - * `ResolvedValue`. - */ export interface ResolvedValueMap extends Map {} + * This is a reified type to allow the circular reference of `ResolvedValue` -> `ResolvedValueMap` + * -> `ResolvedValue`. + */ +export interface ResolvedValueMap extends Map {} + +/** + * A collection of publicly exported declarations from a module. Each declaration is evaluated + * lazily upon request. + */ +export class ResolvedModule { + constructor( + private exports: Map, + private evaluate: (decl: Declaration) => ResolvedValue) {} + + getExport(name: string): ResolvedValue { + if (!this.exports.has(name)) { + return undefined; + } + + return this.evaluate(this.exports.get(name) !); + } + + getExports(): ResolvedValueMap { + const map = new Map(); + this.exports.forEach((decl, name) => { map.set(name, this.evaluate(decl)); }); + return map; + } +} /** * A value member of an enumeration. 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 d34755f792..fbb1e5e9b9 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 @@ -389,6 +389,38 @@ runInEachFileSystem(() => { }); }); + it('module spread works', () => { + const map = evaluate>( + `import * as mod from './module'; const c = {...mod, c: 3};`, 'c', [ + {name: _('/module.ts'), contents: `export const a = 1; export const b = 2;`}, + ]); + + const obj: {[key: string]: number} = {}; + map.forEach((value, key) => obj[key] = value); + expect(obj).toEqual({ + a: 1, + b: 2, + c: 3, + }); + }); + + it('evaluates module exports lazily to avoid infinite recursion', () => { + const value = evaluate(`import * as mod1 from './mod1';`, 'mod1.primary', [ + { + name: _('/mod1.ts'), + contents: ` + import * as mod2 from './mod2'; + export const primary = mod2.indirection; + export const secondary = 2;` + }, + { + name: _('/mod2.ts'), + contents: `import * as mod1 from './mod1'; export const indirection = mod1.secondary;` + }, + ]); + expect(value).toEqual(2); + }); + it('indirected-via-object function call works', () => { expect(evaluate( `