Skip to content

feat(compiler): Recover on malformed keyed reads and keyed writes#39004

Closed
ayazhafiz wants to merge 5 commits intoangular:masterfrom
ayazhafiz:i/38596-keyed
Closed

feat(compiler): Recover on malformed keyed reads and keyed writes#39004
ayazhafiz wants to merge 5 commits intoangular:masterfrom
ayazhafiz:i/38596-keyed

Conversation

@ayazhafiz
Copy link
Contributor

This patch adds support for recovering well-formed (and near-complete)
ASTs for semantically malformed keyed reads and keyed writes. See the
added tests for details on the types of semantics we can now recover;
in particular, notice that some assumptions are made about the form of
a keyed read/write intended by a user. For example, in the malformed
expression a[1 + = 2, we assume that the user meant to write a binary
expression for the key of a, and assign that key the value 2. In
particular, we now parse this as a[1 + <empty expression>] = 2. There
are some different interpretations that can be made here, but I think
this is reasonable.

The actual changes in the parser code are fairly minimal (a nice
surprise!); the biggest addition is a writeContext that marks whether
the = operator can serve as a recovery point after error detection.

Part of #38596

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Feature

@pullapprove pullapprove bot requested a review from JoostK September 26, 2020 00:02
Comment on lines 214 to 221
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is okay to keep both errors here, but an argument could be made that they are overlapping. Let me know what you think.

Copy link
Member

Choose a reason for hiding this comment

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

I think having both is fine.

@ayazhafiz ayazhafiz requested a review from atscott September 26, 2020 01:02
@ayazhafiz ayazhafiz added area: compiler Issues related to `ngc`, Angular's template compiler target: patch This PR is targeted for the next patch release feature Label used to distinguish feature request from other issues labels Sep 26, 2020
@ngbot ngbot bot added this to the needsTriage milestone Sep 26, 2020
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it would almost be worth adding these to a separate test(compiler) commit before the changes to the code to make it clear that we were already able to recover from errors in keyed reads and that the changes only affect keyed writes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I will do this.

Comment on lines 214 to 221
Copy link
Member

Choose a reason for hiding this comment

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

I think having both is fine.

This patch adds support for recovering well-formed (and near-complete)
ASTs for semantically malformed keyed reads and keyed writes. See the
added tests for details on the types of semantics we can now recover;
in particular, notice that some assumptions are made about the form of
a keyed read/write intended by a user. For example, in the malformed
expression `a[1 + = 2`, we assume that the user meant to write a binary
expression for the key of `a`, and assign that key the value `2`. In
particular, we now parse this as `a[1 + <empty expression>] = 2`. There
are some different interpretations that can be made here, but I think
this is reasonable.

The actual changes in the parser code are fairly minimal (a nice
surprise!); the biggest addition is a `writeContext` that marks whether
the `=` operator can serve as a recovery point after error detection.

Part of angular#38596
@ayazhafiz ayazhafiz added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: presubmit The PR is in need of a google3 presubmit labels Oct 2, 2020
@ngbot ngbot bot added the action: merge The PR is ready for merge by the caretaker label Oct 2, 2020
@ayazhafiz
Copy link
Contributor Author

Caretaker: can you run presubmit?

@atscott
Copy link
Contributor

atscott commented Oct 2, 2020

presubmit

@atscott atscott removed merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note action: presubmit The PR is in need of a google3 presubmit labels Oct 2, 2020
josephperrott pushed a commit that referenced this pull request Oct 2, 2020
…9004)

This patch adds support for recovering well-formed (and near-complete)
ASTs for semantically malformed keyed reads and keyed writes. See the
added tests for details on the types of semantics we can now recover;
in particular, notice that some assumptions are made about the form of
a keyed read/write intended by a user. For example, in the malformed
expression `a[1 + = 2`, we assume that the user meant to write a binary
expression for the key of `a`, and assign that key the value `2`. In
particular, we now parse this as `a[1 + <empty expression>] = 2`. There
are some different interpretations that can be made here, but I think
this is reasonable.

The actual changes in the parser code are fairly minimal (a nice
surprise!); the biggest addition is a `writeContext` that marks whether
the `=` operator can serve as a recovery point after error detection.

Part of #38596

PR Close #39004
@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 2, 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 feature Label used to distinguish feature request from other issues target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants