fix(compiler): fix handling of escaped characters in the lexer by petebacondarwin · Pull Request #28978 · angular/angular

petebacondarwin · 2019-02-26T13:45:32Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Issue Number: #28843

Previously the start of a character indicated by an escape sequence
was being incorrectly computed by the lexer, which caused tokens
to include the start of the escaped character sequence in the
preceding token. In particular this affected the name extracted
from opening tags if the name was terminated by an escape sequence.
For example, <t\n> would have the name t\ rather than t.

What is the new behavior?

This fix refactors the lexer to use a "cursor" object to iterate over
the characters in the template source. There are two cursor implementations,
one expects a simple string, the other expects a string that contains
JavaScript escape sequences that need to be unescaped.

Does this PR introduce a breaking change?

Yes
No

Other information

Alternative to #28844

cexbrayat

As I spent a few hours lately trying to fix this bug, I can only agree that the cursor abstraction makes the whole section much easier to understand, and feels less hacky than our initial ideas. 👍

cexbrayat · 2019-02-26T14:00:42Z

(unrelated) nit: the param is not correct anymore, should be options.

Nor the _tokenizeIcu param either!

cexbrayat · 2019-02-26T14:09:21Z

in case this is unvolontary: it used to be forced to lowercase (the parameter of _consumeRawTextWithTagClose is still named lowercaseTagName).

I shall add a test to check :-)

Actually the original code was a bit weird. It used the raw tag name in the opening tag token but then a lower-cased version in the closing tag token. For matching the opening tag to the closing tag it is using the _attemptStrCaseInsensitive() function anyway so it doesn't matter.

As the code is now, the closing tag token is using the same casing as the opening tag token - which I think is more consistent and correct.

I changed the other occurrence of lowercaseTagName instead.

alxhub

Very nice work, I love the new CharacterCursor abstraction. Just a couple typings nits and a question about performance.

alxhub · 2019-02-27T19:21:05Z

Suggested change

clone(): CharacterCursor;

clone(): this;

I tried that too :-)

Unfortunately if you make the interface use this then the return value actually does have to be this (and not new SomeClass()). I.E. the following code is node invalid:

clone(): this { return new PlainCharacterCursor(this); }

You get the error:

Type 'PlainCharacterCursor' is not assignable to type 'this'.

alxhub · 2019-02-27T19:23:16Z

This seems expensive (allocating on every advance). Can we avoid having two copies of the same state, or the need to copy between them?

I wondered about this but without testing it we don't really know if it is a performance hit. These objects are small and always have the same shape, so I wouldn't be surprised if the memory could be used efficiently.

I could definitely move the file, input and end properties out of the CursorState object, which would halve the number of properties that need copying.

Also I could move the copy of the object into the processEscapeSequence so that we only make a copy if we hit an escape. Otherwise the two object stay the same.

alxhub · 2019-02-27T21:43:36Z

Presubmit

The parts of a token are supposed to be an array of not-null strings, but we were using `null` for tags that had no prefix. This has been fixed to use the empty string in such cases, which allows the `null !` hack to be removed.

Previously the start of a character indicated by an escape sequence was being incorrectly computed by the lexer, which caused tokens to include the start of the escaped character sequence in the preceding token. In particular this affected the name extracted from opening tags if the name was terminated by an escape sequence. For example, `<t\n>` would have the name `t\` rather than `t`. This fix refactors the lexer to use a "cursor" object to iterate over the characters in the template source. There are two cursor implementations, one expects a simple string, the other expects a string that contains JavaScript escape sequences that need to be unescaped.

petebacondarwin · 2019-02-28T10:07:53Z

+      super(fileOrCursor);
      this.internalState = {...fileOrCursor.internalState};
+    } else {
+      super(fileOrCursor, range !);


TS was not clever enough to know that we must have come via the second signature.

IgorMinar · 2019-02-28T10:09:17Z

merge-assistance: g3 is good http://test/OCL:236077460:BASE:236077487:1551346219091:61b6ddf8

Previously the start of a character indicated by an escape sequence was being incorrectly computed by the lexer, which caused tokens to include the start of the escaped character sequence in the preceding token. In particular this affected the name extracted from opening tags if the name was terminated by an escape sequence. For example, `<t\n>` would have the name `t\` rather than `t`. This fix refactors the lexer to use a "cursor" object to iterate over the characters in the template source. There are two cursor implementations, one expects a simple string, the other expects a string that contains JavaScript escape sequences that need to be unescaped. PR Close #28978

PR Close #28978

angular-automatic-lock-bot · 2019-09-14T18:23:36Z

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.}

petebacondarwin requested a review from a team February 26, 2019 13:45

googlebot added the cla: yes label Feb 26, 2019

ngbot Bot added this to the Backlog milestone Feb 26, 2019

petebacondarwin force-pushed the ngtsc-lexer-fix branch from 28db305 to 0321bcf Compare February 26, 2019 13:48

cexbrayat approved these changes Feb 26, 2019

View reviewed changes

cexbrayat mentioned this pull request Feb 26, 2019

fix(ivy): tokenize escaped new lines in inline templates #28844

Closed

14 tasks

petebacondarwin force-pushed the ngtsc-lexer-fix branch 2 times, most recently from c1bcf89 to 1c00170 Compare February 27, 2019 08:39

alxhub approved these changes Feb 27, 2019

View reviewed changes

petebacondarwin force-pushed the ngtsc-lexer-fix branch from 1c00170 to 960381d Compare February 27, 2019 20:19

petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Feb 27, 2019

petebacondarwin added 5 commits February 28, 2019 10:04

fixup! fix(ivy): correctly tokenize escaped characters in templates

56f77ea

fixup! fix(ivy): correctly tokenize escaped characters in templates

b84bac1

docs(compiler): correct lexer argument descriptions

a094903

petebacondarwin force-pushed the ngtsc-lexer-fix branch from 960381d to a094903 Compare February 28, 2019 10:04

IgorMinar 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 Feb 28, 2019

petebacondarwin commented Feb 28, 2019

View reviewed changes

IgorMinar closed this in 76979e1 Feb 28, 2019

IgorMinar pushed a commit that referenced this pull request Feb 28, 2019

docs(compiler): correct lexer argument descriptions (#28978)

cb20b3b

PR Close #28978

petebacondarwin deleted the ngtsc-lexer-fix branch February 28, 2019 10:53

cexbrayat mentioned this pull request Feb 28, 2019

ivy: ngcc fails to compile inline templates with escaped new lines #28843

Closed

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 14, 2019

Uh oh!

Conversation

petebacondarwin commented Feb 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

cexbrayat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alxhub commented Feb 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Feb 28, 2019

Uh oh!

angular-automatic-lock-bot Bot commented Sep 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

petebacondarwin commented Feb 26, 2019 •

edited

Loading

petebacondarwin Feb 27, 2019 •

edited

Loading