From ce30888a265ab8f0615139f9a22332a0712b167e Mon Sep 17 00:00:00 2001 From: JoostK Date: Mon, 28 Oct 2019 21:11:35 +0100 Subject: [PATCH] feat(ivy): graceful evaluation of unknown or invalid expressions (#33453) During static evaluation of expressions within ngtsc, it may occur that certain expressions or just parts thereof cannot be statically interpreted for some reason. The static interpreter keeps track of the failure reason and the code path that was evaluated by means of `DynamicValue`, which will allow descriptive errors. In some situations however, the static interpreter would throw an exception instead, resulting in a crash of the compilation. Not only does this cause non-descriptive errors, more importantly does it prevent the evaluated result from being partial, i.e. parts of the result can be dynamic if their value does not have to be statically available to the compiler. This commit refactors the static interpreter to never throw errors for certain expressions that it cannot evaluate. Resolves FW-1582 PR Close #33453 --- .../ngtsc/partial_evaluator/src/dynamic.ts | 13 +-- .../partial_evaluator/src/interpreter.ts | 32 +++---- .../partial_evaluator/test/evaluator_spec.ts | 96 +++++++++++++++++++ 3 files changed, 116 insertions(+), 25 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/dynamic.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/dynamic.ts index 85880d8e74..5c7a04a2ca 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/dynamic.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/dynamic.ts @@ -38,9 +38,10 @@ export const enum DynamicValueReason { EXTERNAL_REFERENCE, /** - * A type of `ts.Expression` that `StaticInterpreter` doesn't know how to evaluate. + * Syntax that `StaticInterpreter` doesn't know how to evaluate, for example a type of + * `ts.Expression` that is not supported. */ - UNKNOWN_EXPRESSION_TYPE, + UNSUPPORTED_SYNTAX, /** * A declaration of a `ts.Identifier` could not be found. @@ -80,8 +81,8 @@ export class DynamicValue { return new DynamicValue(node, ref, DynamicValueReason.EXTERNAL_REFERENCE); } - static fromUnknownExpressionType(node: ts.Node): DynamicValue { - return new DynamicValue(node, undefined, DynamicValueReason.UNKNOWN_EXPRESSION_TYPE); + static fromUnsupportedSyntax(node: ts.Node): DynamicValue { + return new DynamicValue(node, undefined, DynamicValueReason.UNSUPPORTED_SYNTAX); } static fromUnknownIdentifier(node: ts.Identifier): DynamicValue { @@ -108,8 +109,8 @@ export class DynamicValue { return this.code === DynamicValueReason.EXTERNAL_REFERENCE; } - isFromUnknownExpressionType(this: DynamicValue): this is DynamicValue { - return this.code === DynamicValueReason.UNKNOWN_EXPRESSION_TYPE; + isFromUnsupportedSyntax(this: DynamicValue): this is DynamicValue { + return this.code === DynamicValueReason.UNSUPPORTED_SYNTAX; } isFromUnknownIdentifier(this: DynamicValue): this is DynamicValue { 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 87544a61be..61c4653984 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts @@ -138,7 +138,7 @@ export class StaticInterpreter { } else if (this.host.isClass(node)) { result = this.visitDeclaration(node, context); } else { - return DynamicValue.fromUnknownExpressionType(node); + return DynamicValue.fromUnsupportedSyntax(node); } if (result instanceof DynamicValue && result.node !== node) { return DynamicValue.fromDynamicInput(node, result); @@ -184,7 +184,8 @@ export class StaticInterpreter { if (spread instanceof DynamicValue) { return DynamicValue.fromDynamicInput(node, spread); } else if (!(spread instanceof Map)) { - throw new Error(`Unexpected value in spread assignment: ${spread}`); + return DynamicValue.fromDynamicInput( + node, DynamicValue.fromInvalidExpressionType(property, spread)); } spread.forEach((value, key) => map.set(key, value)); } else { @@ -292,9 +293,6 @@ export class StaticInterpreter { private visitElementAccessExpression(node: ts.ElementAccessExpression, context: Context): ResolvedValue { const lhs = this.visitExpression(node.expression, context); - if (node.argumentExpression === undefined) { - throw new Error(`Expected argument in ElementAccessExpression`); - } if (lhs instanceof DynamicValue) { return DynamicValue.fromDynamicInput(node, lhs); } @@ -303,8 +301,7 @@ export class StaticInterpreter { return DynamicValue.fromDynamicInput(node, rhs); } if (typeof rhs !== 'string' && typeof rhs !== 'number') { - throw new Error( - `ElementAccessExpression index should be string or number, got ${typeof rhs}: ${rhs}`); + return DynamicValue.fromInvalidExpressionType(node, rhs); } return this.accessHelper(node, lhs, rhs, context); @@ -360,10 +357,7 @@ export class StaticInterpreter { return new ArrayConcatBuiltinFn(node, lhs); } if (typeof rhs !== 'number' || !Number.isInteger(rhs)) { - return DynamicValue.fromUnknown(node); - } - if (rhs < 0 || rhs >= lhs.length) { - throw new Error(`Index out of bounds: ${rhs} vs ${lhs.length}`); + return DynamicValue.fromInvalidExpressionType(node, rhs); } return lhs[rhs]; } else if (lhs instanceof Reference) { @@ -498,7 +492,7 @@ export class StaticInterpreter { ResolvedValue { const operatorKind = node.operator; if (!UNARY_OPERATORS.has(operatorKind)) { - throw new Error(`Unsupported prefix unary operator: ${ts.SyntaxKind[operatorKind]}`); + return DynamicValue.fromUnsupportedSyntax(node); } const op = UNARY_OPERATORS.get(operatorKind) !; @@ -513,14 +507,14 @@ export class StaticInterpreter { private visitBinaryExpression(node: ts.BinaryExpression, context: Context): ResolvedValue { const tokenKind = node.operatorToken.kind; if (!BINARY_OPERATORS.has(tokenKind)) { - throw new Error(`Unsupported binary operator: ${ts.SyntaxKind[tokenKind]}`); + return DynamicValue.fromUnsupportedSyntax(node); } const opRecord = BINARY_OPERATORS.get(tokenKind) !; let lhs: ResolvedValue, rhs: ResolvedValue; if (opRecord.literal) { - lhs = literal(this.visitExpression(node.left, context)); - rhs = literal(this.visitExpression(node.right, context)); + lhs = literal(this.visitExpression(node.left, context), node.left); + rhs = literal(this.visitExpression(node.right, context), node.right); } else { lhs = this.visitExpression(node.left, context); rhs = this.visitExpression(node.right, context); @@ -554,9 +548,9 @@ export class StaticInterpreter { private visitSpreadElement(node: ts.SpreadElement, context: Context): ResolvedValueArray { const spread = this.visitExpression(node.expression, context); if (spread instanceof DynamicValue) { - return [DynamicValue.fromDynamicInput(node.expression, spread)]; + return [DynamicValue.fromDynamicInput(node, spread)]; } else if (!Array.isArray(spread)) { - throw new Error(`Unexpected value in spread expression: ${spread}`); + return [DynamicValue.fromInvalidExpressionType(node, spread)]; } else { return spread; } @@ -582,12 +576,12 @@ function isFunctionOrMethodReference(ref: Reference): ts.isFunctionExpression(ref.node); } -function literal(value: ResolvedValue): any { +function literal(value: ResolvedValue, node: ts.Node): any { if (value instanceof DynamicValue || value === null || value === undefined || typeof value === 'string' || typeof value === 'number' || typeof value === 'boolean') { return value; } - throw new Error(`Value ${value} is not literal and cannot be used in this context.`); + return DynamicValue.fromInvalidExpressionType(node, value); } function isVariableDeclarationDeclared(node: ts.VariableDeclaration): boolean { 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 022b345648..a071ad349b 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 @@ -151,6 +151,11 @@ runInEachFileSystem(() => { it('array access works', () => { expect(evaluate(`const a = [1, 2, 3];`, 'a[1] + a[0]')).toEqual(3); }); + it('array access out of bounds is `undefined`', () => { + expect(evaluate(`const a = [1, 2, 3];`, 'a[-1]')).toEqual(undefined); + expect(evaluate(`const a = [1, 2, 3];`, 'a[3]')).toEqual(undefined); + }); + it('array `length` property access works', () => { expect(evaluate(`const a = [1, 2, 3];`, 'a[\'length\'] + 1')).toEqual(4); }); @@ -182,6 +187,97 @@ runInEachFileSystem(() => { it('supports null', () => { expect(evaluate('const a = null;', 'a')).toEqual(null); }); + it('resolves unknown binary operators as dynamic value', () => { + const value = evaluate('declare const window: any;', '"location" in window'); + if (!(value instanceof DynamicValue)) { + return fail(`Should have resolved to a DynamicValue`); + } + expect(value.node.getText()).toEqual('"location" in window'); + expect(value.isFromUnsupportedSyntax()).toBe(true); + }); + + it('resolves unknown unary operators as dynamic value', () => { + const value = evaluate('let index = 0;', '++index'); + if (!(value instanceof DynamicValue)) { + return fail(`Should have resolved to a DynamicValue`); + } + expect(value.node.getText()).toEqual('++index'); + expect(value.isFromUnsupportedSyntax()).toBe(true); + }); + + it('resolves invalid element accesses as dynamic value', () => { + const value = evaluate('const a = {}; const index: any = true;', 'a[index]'); + if (!(value instanceof DynamicValue)) { + return fail(`Should have resolved to a DynamicValue`); + } + expect(value.node.getText()).toEqual('a[index]'); + if (!value.isFromInvalidExpressionType()) { + return fail('Should have an invalid expression type as reason'); + } + expect(value.reason).toEqual(true); + }); + + it('resolves invalid array accesses as dynamic value', () => { + const value = evaluate('const a = []; const index = 1.5;', 'a[index]'); + if (!(value instanceof DynamicValue)) { + return fail(`Should have resolved to a DynamicValue`); + } + expect(value.node.getText()).toEqual('a[index]'); + if (!value.isFromInvalidExpressionType()) { + return fail('Should have an invalid expression type as reason'); + } + expect(value.reason).toEqual(1.5); + }); + + it('resolves binary operator on non-literals as dynamic value', () => { + const value = evaluate('const a: any = []; const b: any = [];', 'a + b'); + if (!(value instanceof DynamicValue)) { + return fail(`Should have resolved to a DynamicValue`); + } + expect(value.node.getText()).toEqual('a + b'); + if (!(value.reason instanceof DynamicValue)) { + return fail(`Should have a DynamicValue as reason`); + } + if (!value.reason.isFromInvalidExpressionType()) { + return fail('Should have an invalid expression type as reason'); + } + expect(value.reason.node.getText()).toEqual('a'); + expect(value.reason.reason).toEqual([]); + }); + + it('resolves invalid spreads in array literals as dynamic value', () => { + const array = evaluate('const a: any = true;', '[1, ...a]'); + if (!Array.isArray(array)) { + return fail(`Should have resolved to an array`); + } + expect(array[0]).toBe(1); + const value = array[1]; + if (!(value instanceof DynamicValue)) { + return fail(`Should have resolved to a DynamicValue`); + } + expect(value.node.getText()).toEqual('...a'); + if (!value.isFromInvalidExpressionType()) { + return fail('Should have an invalid spread element as reason'); + } + expect(value.reason).toEqual(true); + }); + + it('resolves invalid spreads in object literals as dynamic value', () => { + const value = evaluate('const a: any = true;', '{b: true, ...a}'); + if (!(value instanceof DynamicValue)) { + return fail(`Should have resolved to a DynamicValue`); + } + expect(value.node.getText()).toEqual('{b: true, ...a}'); + if (!value.isFromDynamicInput()) { + return fail('Should have a dynamic input as reason'); + } + expect(value.reason.node.getText()).toEqual('...a'); + if (!value.reason.isFromInvalidExpressionType()) { + return fail('Should have an invalid spread element as reason'); + } + expect(value.reason.reason).toEqual(true); + }); + it('resolves access from external variable declarations as dynamic value', () => { const value = evaluate('declare const window: any;', 'window.location'); if (!(value instanceof DynamicValue)) {