fix(compiler-cli): report diagnostic instead of crashing on malformed…#69441
fix(compiler-cli): report diagnostic instead of crashing on malformed…#69441aparzi wants to merge 1 commit into
Conversation
… host binding `parseHostBindings` throws plain `Error`s for malformed host bindings (e.g. a property binding with a non-static value, as can happen while editing in the language service). These were uncaught during directive analysis, crashing the compiler and the Angular Language Service. Wrap the call and surface the error as a `FatalDiagnosticError` so it becomes a diagnostic and analysis can complete normally. Fixes angular#69106
| @@ -337,8 +337,11 @@ export class IncrementalCompilation implements IncrementalBuild<ClassRecord, Fil | |||
| } | |||
|
|
|||
| priorTypeCheckingResultsFor(sf: ts.SourceFile): FileTypeCheckingData | null { | |||
| // In some environments (e.g., language service during edits), type-check queries can occur | |||
There was a problem hiding this comment.
I'm not sure about the implications of this on the language service. cc @atscott
There was a problem hiding this comment.
Hmm well certainly errors are never good for language service if not caught but this seems like a suspicious place to fix it. Probably needs something higher up?
There was a problem hiding this comment.
Thanks for the feedback!
Looking "higher up" in the stack, analyzeTrait in packages/compiler-cli/src/ngtsc/transform/src/compilation.ts already wraps trait.handler.analyze() in a try/catch, but it only handles FatalDiagnosticError and re-throws everything else. That re-throw is what actually propagates up and crashes the Language Service.
Would the right direction be to generalize that catch so any unexpected error during directive analysis becomes a diagnostic instead of being re-thrown? That would protect the LS from this and similar future bugs in any handler, not just host-bindings. Happy to update the PR to take that approach if it sounds right to you.
| try { | ||
| bindings = parseHostBindings(hostMetadata); | ||
| } catch (e) { | ||
| // `parseHostBindings` throws plain `Error`s for malformed bindings (e.g. `'[class.has-]'` |
There was a problem hiding this comment.
This comment isn’t needed, please remove.
There was a problem hiding this comment.
This also seems a bit of an odd choice. Why not change the parsing function to throw fatal diagnostics instead of plain errors if it’s not allowed to throw errors
parseHostBindingsthrows plainErrors for malformed host bindings (e.g. a property binding with a non-static value, as can happen while editing in the language service). These were uncaught during directive analysis, crashing the compiler and the Angular Language Service.Wrap the call and surface the error as a
FatalDiagnosticErrorso it becomes a diagnostic and analysis can complete normally.Fixes #69106
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Issue Number: #69106
Does this PR introduce a breaking change?