Skip to content

Commit fb1fa8b

Browse files
crisbetothePunderWoman
authored andcommitted
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<string, string>` 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
1 parent ea0bf74 commit fb1fa8b

File tree

4 files changed

+46
-13
lines changed

4 files changed

+46
-13
lines changed

packages/compiler-cli/src/ngtsc/annotations/directive/src/shared.ts

+27-3
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
ParsedHostBindings,
1818
ParseError,
1919
parseHostBindings,
20+
ParserError,
2021
R3DirectiveMetadata,
2122
R3HostDirectiveMetadata,
2223
R3InputMetadata,
@@ -1629,17 +1630,40 @@ function evaluateHostExpressionBindings(
16291630
const errors = verifyHostBindings(bindings, createSourceSpan(hostExpr));
16301631
if (errors.length > 0) {
16311632
throw new FatalDiagnosticError(
1632-
// TODO: provide more granular diagnostic and output specific host expression that
1633-
// triggered an error instead of the whole host object.
16341633
ErrorCode.HOST_BINDING_PARSE_ERROR,
1635-
hostExpr,
1634+
getHostBindingErrorNode(errors[0], hostExpr),
16361635
errors.map((error: ParseError) => error.msg).join('\n'),
16371636
);
16381637
}
16391638

16401639
return bindings;
16411640
}
16421641

1642+
/**
1643+
* Attempts to match a parser error to the host binding expression that caused it.
1644+
* @param error Error to match.
1645+
* @param hostExpr Expression declaring the host bindings.
1646+
*/
1647+
function getHostBindingErrorNode(error: ParseError, hostExpr: ts.Expression): ts.Node {
1648+
// In the most common case the `host` object is an object literal with string values. We can
1649+
// confidently match the error to its expression by looking at the string value that the parser
1650+
// failed to parse and the initializers for each of the properties. If we fail to match, we fall
1651+
// back to the old behavior where the error is reported on the entire `host` object.
1652+
if (ts.isObjectLiteralExpression(hostExpr) && error.relatedError instanceof ParserError) {
1653+
for (const prop of hostExpr.properties) {
1654+
if (
1655+
ts.isPropertyAssignment(prop) &&
1656+
ts.isStringLiteralLike(prop.initializer) &&
1657+
prop.initializer.text === error.relatedError.input
1658+
) {
1659+
return prop.initializer;
1660+
}
1661+
}
1662+
}
1663+
1664+
return hostExpr;
1665+
}
1666+
16431667
/**
16441668
* Extracts and prepares the host directives metadata from an array literal expression.
16451669
* @param rawHostDirectives Expression that defined the `hostDirectives`.

packages/compiler-cli/test/ngtsc/ngtsc_spec.ts

+5-5
Original file line numberDiff line numberDiff line change
@@ -5114,9 +5114,7 @@ runInEachFileSystem((os: string) => {
51145114
);
51155115

51165116
const errors = env.driveDiagnostics();
5117-
expect(getDiagnosticSourceCode(errors[0])).toBe(`{
5118-
'(click)': 'act() | pipe',
5119-
}`);
5117+
expect(getDiagnosticSourceCode(errors[0])).toBe(`'act() | pipe'`);
51205118
expect(errors[0].messageText).toContain('/test.ts@7:17');
51215119
});
51225120

@@ -5158,10 +5156,12 @@ runInEachFileSystem((os: string) => {
51585156
class FooCmp {}
51595157
`,
51605158
);
5161-
const errors = env.driveDiagnostics();
5162-
expect(trim(errors[0].messageText as string)).toContain(
5159+
const diags = env.driveDiagnostics();
5160+
expect(diags.length).toBe(1);
5161+
expect(trim(diags[0].messageText as string)).toContain(
51635162
'Host binding expression cannot contain pipes',
51645163
);
5164+
expect(getDiagnosticSourceCode(diags[0])).toBe(`'id | myPipe'`);
51655165
});
51665166

51675167
it('should generate host bindings for directives', () => {

packages/compiler/src/parse_util.ts

+11-3
Original file line numberDiff line numberDiff line change
@@ -150,9 +150,17 @@ export enum ParseErrorLevel {
150150

151151
export class ParseError {
152152
constructor(
153-
public span: ParseSourceSpan,
154-
public msg: string,
155-
public level: ParseErrorLevel = ParseErrorLevel.ERROR,
153+
/** Location of the error. */
154+
readonly span: ParseSourceSpan,
155+
/** Error message. */
156+
readonly msg: string,
157+
/** Severity level of the error. */
158+
readonly level: ParseErrorLevel = ParseErrorLevel.ERROR,
159+
/**
160+
* Error that caused the error to be surfaced. For example, an error in a sub-expression that
161+
* couldn't be parsed. Not guaranteed to be defined, but can be used to provide more context.
162+
*/
163+
readonly relatedError?: unknown,
156164
) {}
157165

158166
contextualMessage(): string {

packages/compiler/src/template_parser/binding_parser.ts

+3-2
Original file line numberDiff line numberDiff line change
@@ -770,13 +770,14 @@ export class BindingParser {
770770
message: string,
771771
sourceSpan: ParseSourceSpan,
772772
level: ParseErrorLevel = ParseErrorLevel.ERROR,
773+
relatedError?: ParserError,
773774
) {
774-
this.errors.push(new ParseError(sourceSpan, message, level));
775+
this.errors.push(new ParseError(sourceSpan, message, level, relatedError));
775776
}
776777

777778
private _reportExpressionParserErrors(errors: ParserError[], sourceSpan: ParseSourceSpan) {
778779
for (const error of errors) {
779-
this._reportError(error.message, sourceSpan);
780+
this._reportError(error.message, sourceSpan, undefined, error);
780781
}
781782
}
782783

0 commit comments

Comments
 (0)