From ce879fc4160e9b12db5d58fd20caf30fc05e8832 Mon Sep 17 00:00:00 2001 From: JoostK Date: Mon, 15 Jun 2020 17:55:04 +0200 Subject: [PATCH] refactor(compiler-cli): more accurate reporting of complex function call (#37587) This commit introduces a dedicated `DynamicValue` kind to indicate that a value cannot be evaluated statically as the function body is not just a single return statement. This allows more accurate reporting of why a function call failed to be evaluated, i.e. we now include a reference to the function declaration and have a tailor-made diagnostic message. PR Close #37587 --- .../partial_evaluator/src/diagnostics.ts | 11 +++++++++++ .../ngtsc/partial_evaluator/src/dynamic.ts | 19 +++++++++++++++++++ .../partial_evaluator/src/interpreter.ts | 4 +++- .../test/diagnostics_spec.ts | 10 ++++++++-- .../partial_evaluator/test/evaluator_spec.ts | 9 ++++++--- 5 files changed, 47 insertions(+), 6 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/diagnostics.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/diagnostics.ts index e6e2b48e9b..e270cb213c 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/diagnostics.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/diagnostics.ts @@ -10,6 +10,7 @@ import * as ts from 'typescript'; import {makeRelatedInformation} from '../../diagnostics'; import {Reference} from '../../imports'; +import {FunctionDefinition} from '../../reflection'; import {DynamicValue, DynamicValueVisitor} from './dynamic'; import {EnumValue, KnownFn, ResolvedModule, ResolvedValue} from './result'; @@ -104,6 +105,16 @@ class TraceDynamicValueVisitor implements DynamicValueVisitor): + ts.DiagnosticRelatedInformation[] { + return [ + makeRelatedInformation( + value.node, + 'Unable to evaluate function call of complex function. A function must have exactly one return statement.'), + makeRelatedInformation(value.reason.node, 'Function is declared here.') + ]; + } + visitInvalidExpressionType(value: DynamicValue): ts.DiagnosticRelatedInformation[] { return [makeRelatedInformation(value.node, 'Unable to evaluate an invalid expression.')]; } 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 d5b0bb82cc..c19bfa3e3d 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/dynamic.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/dynamic.ts @@ -9,6 +9,7 @@ import * as ts from 'typescript'; import {Reference} from '../../imports'; +import {FunctionDefinition} from '../../reflection'; /** * The reason why a value cannot be determined statically. @@ -55,6 +56,11 @@ export const enum DynamicValueReason { */ INVALID_EXPRESSION_TYPE, + /** + * A function call could not be evaluated as the function's body is not a single return statement. + */ + COMPLEX_FUNCTION_CALL, + /** * A value could not be determined statically for any reason other the above. */ @@ -93,6 +99,11 @@ export class DynamicValue { return new DynamicValue(node, value, DynamicValueReason.INVALID_EXPRESSION_TYPE); } + static fromComplexFunctionCall(node: ts.Node, fn: FunctionDefinition): + DynamicValue { + return new DynamicValue(node, fn, DynamicValueReason.COMPLEX_FUNCTION_CALL); + } + static fromUnknown(node: ts.Node): DynamicValue { return new DynamicValue(node, undefined, DynamicValueReason.UNKNOWN); } @@ -121,6 +132,10 @@ export class DynamicValue { return this.code === DynamicValueReason.INVALID_EXPRESSION_TYPE; } + isFromComplexFunctionCall(this: DynamicValue): this is DynamicValue { + return this.code === DynamicValueReason.COMPLEX_FUNCTION_CALL; + } + isFromUnknown(this: DynamicValue): this is DynamicValue { return this.code === DynamicValueReason.UNKNOWN; } @@ -140,6 +155,9 @@ export class DynamicValue { return visitor.visitUnknownIdentifier(this); case DynamicValueReason.INVALID_EXPRESSION_TYPE: return visitor.visitInvalidExpressionType(this); + case DynamicValueReason.COMPLEX_FUNCTION_CALL: + return visitor.visitComplexFunctionCall( + this as unknown as DynamicValue); case DynamicValueReason.UNKNOWN: return visitor.visitUnknown(this); } @@ -153,5 +171,6 @@ export interface DynamicValueVisitor { visitUnsupportedSyntax(value: DynamicValue): R; visitUnknownIdentifier(value: DynamicValue): R; visitInvalidExpressionType(value: DynamicValue): R; + visitComplexFunctionCall(value: DynamicValue): R; visitUnknown(value: DynamicValue): R; } 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 76b27e7a87..af20342297 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/src/interpreter.ts @@ -490,8 +490,10 @@ export class StaticInterpreter { private visitFunctionBody(node: ts.CallExpression, fn: FunctionDefinition, context: Context): ResolvedValue { - if (fn.body === null || fn.body.length !== 1 || !ts.isReturnStatement(fn.body[0])) { + if (fn.body === null) { return DynamicValue.fromUnknown(node); + } else if (fn.body.length !== 1 || !ts.isReturnStatement(fn.body[0])) { + return DynamicValue.fromComplexFunctionCall(node, fn); } const ret = fn.body[0] as ts.ReturnStatement; diff --git a/packages/compiler-cli/src/ngtsc/partial_evaluator/test/diagnostics_spec.ts b/packages/compiler-cli/src/ngtsc/partial_evaluator/test/diagnostics_spec.ts index 25cf1cbefe..e8bf6de5a0 100644 --- a/packages/compiler-cli/src/ngtsc/partial_evaluator/test/diagnostics_spec.ts +++ b/packages/compiler-cli/src/ngtsc/partial_evaluator/test/diagnostics_spec.ts @@ -199,10 +199,16 @@ runInEachFileSystem(() => { }`, 'complex()'); - expect(trace.length).toBe(1); - expect(trace[0].messageText).toBe('Unable to evaluate statically.'); + expect(trace.length).toBe(2); + expect(trace[0].messageText) + .toBe( + 'Unable to evaluate function call of complex function. A function must have exactly one return statement.'); expect(trace[0].file!.fileName).toBe(_('/entry.ts')); expect(getSourceCode(trace[0])).toBe('complex()'); + + expect(trace[1].messageText).toBe('Function is declared here.'); + expect(trace[1].file!.fileName).toBe(_('/entry.ts')); + expect(getSourceCode(trace[1])).toContain(`console.log('test');`); }); it('should trace object destructuring of external reference', () => { 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 72695d9ad9..7c8d53e925 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 @@ -622,15 +622,18 @@ runInEachFileSystem(() => { expect(id.text).toEqual('Target'); }); - it('should resolve functions with more than one statement to an unknown value', () => { + 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")'); if (!(value instanceof DynamicValue)) { return fail(`Should have resolved to a DynamicValue`); } - - expect(value.isFromUnknown()).toBe(true); + if (!value.isFromComplexFunctionCall()) { + return fail('Expected DynamicValue to be from complex function call'); + } expect((value.node as ts.CallExpression).expression.getText()).toBe('foo'); + expect((value.reason.node as ts.FunctionDeclaration).getText()) + .toContain('const b = bar; return b;'); }); describe('(with imported TypeScript helpers)', () => {