From fb1fa8b0fc04c9cfac6551ca27bee89dcd7c72ac Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Mon, 25 Nov 2024 11:54:54 +0100 Subject: [PATCH] fix(compiler-cli): more accurate diagnostics for host binding parser errors (#58870) Currently host bindings are in a bit of a weird state, because their source spans all point to the root object literal, rather than the individual expression. This is tricky to handle at the moment, because the object is being passed around as a `Record` since the compiler needs to support both JIT and non-JIT environments, and because the AOT compiler evaluates the entire literal rather than doing it expression-by-expression. As a result, when we report errors in one of the host bindings, we end up highlighting the entire expression which can be very noisy in an IDE. These changes aim to report a more accurate error for the most common case where the `host` object is initialized to a `string -> string` object literal by matching the failing expression to one of the property initializers. Note that this isn't 100% reliable, because we can't map cases like `host: SOME_CONST`, but it's still better than the current setup. PR Close #58870 --- .../ngtsc/annotations/directive/src/shared.ts | 30 +++++++++++++++++-- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 10 +++---- packages/compiler/src/parse_util.ts | 14 +++++++-- .../src/template_parser/binding_parser.ts | 5 ++-- 4 files changed, 46 insertions(+), 13 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts b/packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts index ef194b378d2..93d26886f74 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts @@ -17,6 +17,7 @@ import { ParsedHostBindings, ParseError, parseHostBindings, + ParserError, R3DirectiveMetadata, R3HostDirectiveMetadata, R3InputMetadata, @@ -1629,10 +1630,8 @@ function evaluateHostExpressionBindings( const errors = verifyHostBindings(bindings, createSourceSpan(hostExpr)); if (errors.length > 0) { throw new FatalDiagnosticError( - // TODO: provide more granular diagnostic and output specific host expression that - // triggered an error instead of the whole host object. ErrorCode.HOST_BINDING_PARSE_ERROR, - hostExpr, + getHostBindingErrorNode(errors[0], hostExpr), errors.map((error: ParseError) => error.msg).join('\n'), ); } @@ -1640,6 +1639,31 @@ function evaluateHostExpressionBindings( return bindings; } +/** + * Attempts to match a parser error to the host binding expression that caused it. + * @param error Error to match. + * @param hostExpr Expression declaring the host bindings. + */ +function getHostBindingErrorNode(error: ParseError, hostExpr: ts.Expression): ts.Node { + // In the most common case the `host` object is an object literal with string values. We can + // confidently match the error to its expression by looking at the string value that the parser + // failed to parse and the initializers for each of the properties. If we fail to match, we fall + // back to the old behavior where the error is reported on the entire `host` object. + if (ts.isObjectLiteralExpression(hostExpr) && error.relatedError instanceof ParserError) { + for (const prop of hostExpr.properties) { + if ( + ts.isPropertyAssignment(prop) && + ts.isStringLiteralLike(prop.initializer) && + prop.initializer.text === error.relatedError.input + ) { + return prop.initializer; + } + } + } + + return hostExpr; +} + /** * Extracts and prepares the host directives metadata from an array literal expression. * @param rawHostDirectives Expression that defined the `hostDirectives`. diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index fca81c9bac5..54891ecbd6e 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -5114,9 +5114,7 @@ runInEachFileSystem((os: string) => { ); const errors = env.driveDiagnostics(); - expect(getDiagnosticSourceCode(errors[0])).toBe(`{ - '(click)': 'act() | pipe', - }`); + expect(getDiagnosticSourceCode(errors[0])).toBe(`'act() | pipe'`); expect(errors[0].messageText).toContain('/test.ts@7:17'); }); @@ -5158,10 +5156,12 @@ runInEachFileSystem((os: string) => { class FooCmp {} `, ); - const errors = env.driveDiagnostics(); - expect(trim(errors[0].messageText as string)).toContain( + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(trim(diags[0].messageText as string)).toContain( 'Host binding expression cannot contain pipes', ); + expect(getDiagnosticSourceCode(diags[0])).toBe(`'id | myPipe'`); }); it('should generate host bindings for directives', () => { diff --git a/packages/compiler/src/parse_util.ts b/packages/compiler/src/parse_util.ts index 034a4135ff5..ad89ebad811 100644 --- a/packages/compiler/src/parse_util.ts +++ b/packages/compiler/src/parse_util.ts @@ -150,9 +150,17 @@ export enum ParseErrorLevel { export class ParseError { constructor( - public span: ParseSourceSpan, - public msg: string, - public level: ParseErrorLevel = ParseErrorLevel.ERROR, + /** Location of the error. */ + readonly span: ParseSourceSpan, + /** Error message. */ + readonly msg: string, + /** Severity level of the error. */ + readonly level: ParseErrorLevel = ParseErrorLevel.ERROR, + /** + * Error that caused the error to be surfaced. For example, an error in a sub-expression that + * couldn't be parsed. Not guaranteed to be defined, but can be used to provide more context. + */ + readonly relatedError?: unknown, ) {} contextualMessage(): string { diff --git a/packages/compiler/src/template_parser/binding_parser.ts b/packages/compiler/src/template_parser/binding_parser.ts index 55a3253752b..b099dfcdc64 100644 --- a/packages/compiler/src/template_parser/binding_parser.ts +++ b/packages/compiler/src/template_parser/binding_parser.ts @@ -770,13 +770,14 @@ export class BindingParser { message: string, sourceSpan: ParseSourceSpan, level: ParseErrorLevel = ParseErrorLevel.ERROR, + relatedError?: ParserError, ) { - this.errors.push(new ParseError(sourceSpan, message, level)); + this.errors.push(new ParseError(sourceSpan, message, level, relatedError)); } private _reportExpressionParserErrors(errors: ParserError[], sourceSpan: ParseSourceSpan) { for (const error of errors) { - this._reportError(error.message, sourceSpan); + this._reportError(error.message, sourceSpan, undefined, error); } }