Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(compiler): do not throw away render3 AST on errors #39413

Closed
wants to merge 2 commits into from

Conversation

ayazhafiz
Copy link
Member

Currently render3's parseTemplate throws away the parsed AST and
returns an empty list of HTML nodes if HTML->R3 translation failed. This
is not preferrable in some contexts like that of a language service,
where we would like a well-formed AST even if it is has errors.

Currently render3's `parseTemplate` throws away the parsed AST and
returns an empty list of HTML nodes if HTML->R3 translation failed. This
is not preferrable in some contexts like that of a language service,
where we would like a well-formed AST even if it is has errors.
@google-cla google-cla bot added the cla: yes label Oct 24, 2020
@ayazhafiz ayazhafiz requested review from kyliau and alxhub and removed request for AndrewKushnir October 24, 2020 17:53
@ayazhafiz ayazhafiz added area: compiler Issues related to `ngc`, Angular's template compiler type: bug/fix labels Oct 24, 2020
@ngbot ngbot bot added this to the needsTriage milestone Oct 24, 2020
Comment on lines 2039 to +2041
if (parseResult.errors && parseResult.errors.length > 0) {
// TODO(ayazhafiz): we may not always want to bail out at this point (e.g. in
// the context of a language service).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to do some more investigation to understand the consequences of trying to translate an ML AST with errors to R3. The language service will need this translation to happen even in the presence of errors, but my prediction is that the translation will emit even more, possibly overlapping or incorrect errors. Here is where @alxhub's idea of consolidating parsers becomes very attractive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's good to be careful here - we might hit assertion failures in the translator that throw, not just additional errors.

@ayazhafiz ayazhafiz added this to PRs In Review in Ivy Language Service Oct 24, 2020
@ayazhafiz ayazhafiz added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 26, 2020
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label Oct 26, 2020
@ayazhafiz
Copy link
Member Author

Caretaker: can you run g3sync?

@ayazhafiz ayazhafiz added the target: patch This PR is targeted for the next patch release label Oct 26, 2020
@kyliau
Copy link
Contributor

kyliau commented Oct 26, 2020

@ayazhafiz ayazhafiz removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Oct 27, 2020
@alxhub alxhub added target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release labels Oct 27, 2020
alxhub pushed a commit that referenced this pull request Oct 27, 2020
Currently render3's `parseTemplate` throws away the parsed AST and
returns an empty list of HTML nodes if HTML->R3 translation failed. This
is not preferrable in some contexts like that of a language service,
where we would like a well-formed AST even if it is has errors.

PR Close #39413
@alxhub alxhub closed this in 19b88ce Oct 27, 2020
@ayazhafiz ayazhafiz moved this from PRs In Review to Done in Ivy Language Service Nov 3, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: rc This PR is targeted for the next release-candidate type: bug/fix
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants