fix(compiler): allow banana-in-a-box bindings to end with non-null assertion #37809
Conversation
Nice fix!
| @@ -150,7 +169,7 @@ export class Parser { | |||
| const tokens = this._lexer.tokenize(templateValue); | |||
| const parser = new _ParseAST( | |||
| templateValue, templateUrl, absoluteValueOffset, tokens, templateValue.length, | |||
| false /* parseAction */, this.errors, 0 /* relative offset */); | |||
| ParseFlags.None /* parseFlags */, this.errors, 0 /* relative offset */); | |||
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 */); |
| expect(() => parse('<div (prop)="v! = $event"></div>', [])) | ||
| .toThrowError(/Unexpected token '=' at column 4/); |
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)
There was a problem hiding this comment.
yep, makes sense
|
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? |
|
Picking this one up again. This was rebased on latest master and I tested the build artefacts in |
|
Blocked on a g3 sync issue as this change is incompatible with Codelyzer. |
| @@ -399,7 +418,7 @@ export class _ParseAST { | |||
|
|
|||
| constructor( | |||
| public input: string, public location: string, public absoluteOffset: number, | |||
| public tokens: Token[], public inputLength: number, public parseAction: boolean, | |||
| public tokens: Token[], public inputLength: number, public parseFlags: number, | |||
There was a problem hiding this comment.
| public tokens: Token[], public inputLength: number, public parseFlags: number, | |
| public tokens: Token[], public inputLength: number, public parseFlags: ParseFlags, |
??
There was a problem hiding this comment.
Thanks, good catch!
| None = 0, | ||
|
|
||
| /** | ||
| * Whether an output binding is parsed. |
There was a problem hiding this comment.
| * Whether an output binding is parsed. | |
| * Whether an output binding is being parsed. |
| Action = 1 << 0, | ||
|
|
||
| /** | ||
| * Whether an assignment event is parsed, i.e. an expression originating from two-way-binding |
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 |
| @@ -29,6 +29,21 @@ export class TemplateBindingParseResult { | |||
| public errors: ParserError[]) {} | |||
| } | |||
|
|
|||
| export const enum ParseFlags { | |||
There was a problem hiding this comment.
Could you please add a quick comment above this enum on what it represents?
| if ((this.parseFlags & ParseFlags.AssignmentEvent) && this.next.isOperator('!') && | ||
| this.peek(1).isOperator('=')) { | ||
| this.advance(); | ||
| this.advance(); |
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? |
…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
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.
The text was updated successfully, but these errors were encountered: