Skip to content

fix(compiler): do not allow unterminated interpolation to leak into later tokens - #42605

Closed
petebacondarwin wants to merge 2 commits into
angular:masterfrom
petebacondarwin:compiler-fix-lexer-interpolation-bug
Closed

petebacondarwin wants to merge 2 commits into
angular:masterfrom
petebacondarwin:compiler-fix-lexer-interpolation-bug

Conversation

@petebacondarwin

@petebacondarwin petebacondarwin commented Jun 19, 2021 •

Copy link
Copy Markdown
Contributor

The lexer tracks whether it is reading characters from inside an interpolation
when consuming a text token, so that it can identify invalid ICU expressions.
Inside an interpolation there will be no ICU expression so it is safe to
have unmatched { characters, but outside this is an error.

Previously, if an interpolation was started, by an opening marker (e.g. {{)
in the text, but the text came to an end before the closing marker (e.g. }})
then the lexer was not clearing its internal state that tracked that it was
inside an interpolation. So if when a new text token was being consumed,
the lexer, incorrectly thought it was already within an interpolation.
This resulted in invalid ICU expression errors not being reported.

There are instances of this error being hidden inside G3, and so these will
need to be fixed in order to land this change.

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release action: presubmit The PR is in need of a google3 presubmit area: compiler Issues related to `ngc`, Angular's template compiler compiler: parser labels Jun 19, 2021
@ngbot ngbot Bot modified the milestone: Backlog Jun 19, 2021
@google-cla google-cla Bot added the cla: yes label Jun 19, 2021
@petebacondarwin petebacondarwin changed the title fix(compiler): do not allow unterminated interpolation to leak into l… fix(compiler): do not allow unterminated interpolation to leak into later tokens Jun 19, 2021
@AndrewKushnir

Copy link
Copy Markdown
Contributor

Ivy Presubmit + Global Presubmit.

@petebacondarwin
petebacondarwin force-pushed the compiler-fix-lexer-interpolation-bug branch 2 times, most recently from 98c21fa to 4cee71b Compare June 21, 2021 20:42
@petebacondarwin

Copy link
Copy Markdown
Contributor Author

@AndrewKushnir can you check this new code against the failing targets in G3? Thanks!

@AndrewKushnir

Copy link
Copy Markdown
Contributor

Presubmit #2.

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

@AndrewKushnir AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 22, 2021
@AndrewKushnir

Copy link
Copy Markdown
Contributor

Global Presubmit #2.

@AndrewKushnir

Copy link
Copy Markdown
Contributor

@petebacondarwin FYI, presubmits (including TGP) are now "green" 🎉

@AndrewKushnir AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jun 22, 2021
…ater tokens

When consuming a text token, the lexer tracks whether it is reading characters
from inside an interpolation so that it can identify invalid ICU expressions.
Inside an interpolation there will be no ICU expression so it is safe to
have unmatched `{` characters, but outside an interpolation this is an error.

Previously, if an interpolation was started, by an opening marker (e.g. `{{`)
in a text token but the text came to an end before the closing marker (e.g. `}}`)
then the lexer was not clearing its internal state that tracked that it was
inside an interpolation. When the next text token was being consumed,
the lexer, incorrectly thought it was already within an interpolation.
This resulted in invalid ICU expression errors not being reported.

For example, in the following snippet, the first text block has a prematurely
ended interpolation, and the second text block contains an invalid `{` character.

```
<div>{{</div>
<div>{</div>
```

Previously, the lexer would not have identified this as an error. Now there
will be an EOF error that looks like:

```
TS-995002: Unexpected character "EOF"
(Do you have an unescaped "{" in your template? Use "{{ '{' }}") to escape it.)
```
Previously the lexer would break out of consuming a text token if it contains
a `<` character. Then if the next characters did not indicate an HTML syntax
item, such as a tag or comment, then it would start a new text token. These
consecutive text tokens are then merged into each other in a post tokenization
step.

In the commit before this, interpolation no longer leaks across text tokens.
The approach given above to handling `<` characters that appear in text is
no longer adequate. This change ensures that the lexer only breaks out of
a text token if the next characters indicate a valid HTML tag, comment,
CDATA etc.
@petebacondarwin
petebacondarwin force-pushed the compiler-fix-lexer-interpolation-bug branch from 4cee71b to 1ad8070 Compare June 22, 2021 07:14
@petebacondarwin

Copy link
Copy Markdown
Contributor Author

Caretaker: since the last presubmit, I have only changed the wording of the commit messages, so there should be no need to re-run the presubmit, unless you want to check for backsliding.

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jun 22, 2021
@dylhunn

dylhunn commented Jun 22, 2021

Copy link
Copy Markdown
Contributor

@petebacondarwin Sounds good, is that the only reason you have merge-assistance on the PR?

@AndrewKushnir AndrewKushnir 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 Jun 22, 2021
@AndrewKushnir

Copy link
Copy Markdown
Contributor

@dylhunn I've forced g3 status and removed the action: merge-assistance label.

@dylhunn dylhunn closed this in c873440 Jun 22, 2021
dylhunn pushed a commit that referenced this pull request Jun 22, 2021
…#42605)

Previously the lexer would break out of consuming a text token if it contains
a `<` character. Then if the next characters did not indicate an HTML syntax
item, such as a tag or comment, then it would start a new text token. These
consecutive text tokens are then merged into each other in a post tokenization
step.

In the commit before this, interpolation no longer leaks across text tokens.
The approach given above to handling `<` characters that appear in text is
no longer adequate. This change ensures that the lexer only breaks out of
a text token if the next characters indicate a valid HTML tag, comment,
CDATA etc.

PR Close #42605
dylhunn pushed a commit that referenced this pull request Jun 22, 2021
…ater tokens (#42605)

When consuming a text token, the lexer tracks whether it is reading characters
from inside an interpolation so that it can identify invalid ICU expressions.
Inside an interpolation there will be no ICU expression so it is safe to
have unmatched `{` characters, but outside an interpolation this is an error.

Previously, if an interpolation was started, by an opening marker (e.g. `{{`)
in a text token but the text came to an end before the closing marker (e.g. `}}`)
then the lexer was not clearing its internal state that tracked that it was
inside an interpolation. When the next text token was being consumed,
the lexer, incorrectly thought it was already within an interpolation.
This resulted in invalid ICU expression errors not being reported.

For example, in the following snippet, the first text block has a prematurely
ended interpolation, and the second text block contains an invalid `{` character.

```
<div>{{</div>
<div>{</div>
```

Previously, the lexer would not have identified this as an error. Now there
will be an EOF error that looks like:

```
TS-995002: Unexpected character "EOF"
(Do you have an unescaped "{" in your template? Use "{{ '{' }}") to escape it.)
```

PR Close #42605
dylhunn pushed a commit that referenced this pull request Jun 22, 2021
…#42605)

Previously the lexer would break out of consuming a text token if it contains
a `<` character. Then if the next characters did not indicate an HTML syntax
item, such as a tag or comment, then it would start a new text token. These
consecutive text tokens are then merged into each other in a post tokenization
step.

In the commit before this, interpolation no longer leaks across text tokens.
The approach given above to handling `<` characters that appear in text is
no longer adequate. This change ensures that the lexer only breaks out of
a text token if the next characters indicate a valid HTML tag, comment,
CDATA etc.

PR Close #42605
@angular-automatic-lock-bot

Copy link
Copy Markdown

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 Jul 23, 2021
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 compiler: parser target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants