fix(compiler): allow banana-in-a-box bindings to end with non-null assertion#37809
fix(compiler): allow banana-in-a-box bindings to end with non-null assertion#37809JoostK wants to merge 1 commit intoangular:masterfrom
Conversation
ab42126 to
1abd5c6
Compare
There was a problem hiding this comment.
I think the comment is unnecessary now
| ParseFlags.None /* parseFlags */, this.errors, 0 /* relative offset */); | |
| ParseFlags.None, this.errors, 0 /* relative offset */); |
There was a problem hiding this comment.
This error message isn't incorrect, but I think I better message would mention that the non-null assertion operator cannot appear on the LHS of an assignment expression. If this sounds reasonable, could you add a todo here?
There was a problem hiding this comment.
Agreed, I added a note here. It's really not that hard to have a specialized error message, however I'd rather not touch this at the moment (especially if we do want to support this syntax)
1abd5c6 to
491678d
Compare
491678d to
4e7a212
Compare
|
The g3 failure here is legitimate, as this does break |
|
@JoostK we can fix this internally just by patching |
|
@JoostK Looks like this PR stalled. Any idea what the status is here and whether we still plan on landing it? |
4e7a212 to
d241b0b
Compare
|
Picking this one up again. This was rebased on latest master and I tested the build artefacts in |
There was a problem hiding this comment.
| * Whether an assignment event is parsed, i.e. an expression originating from two-way-binding | |
| * Whether an assignment event is being parsed, i.e. an expression originating from two-way-binding |
There was a problem hiding this comment.
Could you please add a quick comment above this enum on what it represents?
There was a problem hiding this comment.
Could you plz add a quick comment here to provide more context on why we have 2 advance calls?
|
@JoostK FYI the most recent TGP is fully "green" (internal-only link) with this change: there are no issues with Codelyzer. I don't have an access to Alex's test runs (the results are no longer available), but I believe TGP should've triggered Codelyzer-related targets. @alxhub I've also looked at the Codelyzer folder and didn't notice any updates that make it compatible with this change. So I'm wondering if we should run more tests or we can proceed with caution (merge and sync the change as a separate commit, be ready to rollback). What do you think? |
5c1335a to
8b4bf90
Compare
cdc9bc2 to
2d86518
Compare
|
@JoostK Can you rebase this to resolve the conflicts and ping me when you do? We'll run another TGP and see how it goes. Hopefully we can land this. |
…sertion For two-way-bindings that use the banana-in-a-box syntax, the compiler synthesizes an event assignment expression from the primary expression. It is valid for the primary expression to be terminated by the non-null operator, however naive string substitution is used for the synthesized expression, such that the `!` would immediately precede the `=` token, resulting in the valid `!=` operator token. The expression would still parse correctly but it doesn't implement the proper semantics, resulting in incorrect runtime behavior. Changing the expression substitution to force a space between the primary expression and the assignment avoids this mistake, but it uncovers a new issue. The grammar does not allow for the LHS of an assignment to be the non-null operator, so the synthesized expression would fail to parse. To alleviate this, the synthesized expression is parsed with a special parser flag to allow for this syntax. Fixes angular#36551
2d86518 to
9751bcb
Compare
|
Rebased and added to the merge queue 👍 |
|
TGP is "green" :) Adding this PR to the merge queue... |
|
This PR was merged into the repository by commit db6cf7e. |
…sertion (#37809) For two-way-bindings that use the banana-in-a-box syntax, the compiler synthesizes an event assignment expression from the primary expression. It is valid for the primary expression to be terminated by the non-null operator, however naive string substitution is used for the synthesized expression, such that the `!` would immediately precede the `=` token, resulting in the valid `!=` operator token. The expression would still parse correctly but it doesn't implement the proper semantics, resulting in incorrect runtime behavior. Changing the expression substitution to force a space between the primary expression and the assignment avoids this mistake, but it uncovers a new issue. The grammar does not allow for the LHS of an assignment to be the non-null operator, so the synthesized expression would fail to parse. To alleviate this, the synthesized expression is parsed with a special parser flag to allow for this syntax. Fixes #36551 PR Close #37809
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fanimations/13.2.1/13.2.2) | | [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fcommon/13.2.1/13.2.2) | | [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/13.2.1/13.2.2) | | [@angular/compiler-cli](https://github.com/angular/angular) | devDependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/13.2.1/13.2.2) | | [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fcore/13.2.1/13.2.2) | | [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fforms/13.2.1/13.2.2) | | [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/13.2.1/13.2.2) | | [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/13.2.1/13.2.2) | | [@angular/router](https://github.com/angular/angular) | dependencies | patch | [`13.2.1` -> `13.2.2`](https://renovatebot.com/diffs/npm/@angular%2frouter/13.2.1/13.2.2) | --- ### Release Notes <details> <summary>angular/angular</summary> ### [`v13.2.2`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#​1322-2022-02-08) [Compare Source](angular/angular@13.2.1...13.2.2) ##### compiler | Commit | Type | Description | | -- | -- | -- | | [37af6abb49](angular/angular@37af6ab) | fix | allow banana-in-a-box bindings to end with non-null assertion ([#​37809](angular/angular#37809)) | ##### forms | Commit | Type | Description | | -- | -- | -- | | [b75e90f809](angular/angular@b75e90f) | fix | incorrectly keeping track of ngModel with ngFor inside a form ([#​40459](angular/angular#40459)) | ##### http | Commit | Type | Description | | -- | -- | -- | | [3fae6637e7](angular/angular@3fae663) | perf | remove IE special status handling ([#​44354](angular/angular#44354)) | ##### upgrade | Commit | Type | Description | | -- | -- | -- | | [b9aab0c87b](angular/angular@b9aab0c) | fix | Do not trigger duplicate navigation events from Angular Router ([#​43441](angular/angular#43441)) | #### Special Thanks Alan Agius, Alan Cohen, Andrew Kushnir, Andrew Scott, Daniel Díaz, Dario Piotrowicz, Doug Parker, Jayson Acosta, Joey Perrott, JoostK, Kristiyan Kostadinov, Olivier Capuozzo, Ramzan, Shai Reznik, TANMAY SRIVASTAVA, dario-piotrowicz, iRealNirmal, jhonyeduardo, mgechev and zuckjet <!-- CHANGELOG SPLIT MARKER --> </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1160 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
For two-way-bindings that use the banana-in-a-box syntax, the compiler
synthesizes an event assignment expression from the primary expression.
It is valid for the primary expression to be terminated by the non-null
operator, however naive string substitution is used for the synthesized
expression, such that the
!would immediately precede the=token,resulting in the valid
!=operator token. The expression would stillparse correctly but it doesn't implement the proper semantics, resulting
in incorrect runtime behavior.
Changing the expression substitution to force a space between the
primary expression and the assignment avoids this mistake, but it
uncovers a new issue. The grammar does not allow for the LHS of an
assignment to be the non-null operator, so the synthesized expression
would fail to parse. To alleviate this, the synthesized expression is
parsed with a special parser flag to allow for this syntax.
Fixes #36551
Implementation note: it is up for debate whether extending the grammar to accept more expressions as LHS of an assignment would be desirable for regular output bindings. I choose not to change the grammar at this moment, as it seems like a change that needs to be introduced in at least a minor. Additionally, I feel like the whole expression substitution is a bit of a hack in the first place, so an alternative way of handling this case may be desirable in the future. To allow for this cleanup, I feel like it's best to wait for VE removal as the parser infrastructure is currently quite convoluted.