Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(compiler): record end of expression Token #33549

Closed
wants to merge 1 commit into from

Conversation

@ayazhafiz
Copy link
Member

ayazhafiz commented Nov 2, 2019

In the past, only the starting index of an expression Token has been
recorded, so a parser could demarkate the span of a token only by the
start locations of two tokens. This may lead to trailing whitespace
being included in the token span:

{{ token1   + token2 }}
   ^^^^^^^^^             recorded span of `token1`

It's also not enough for a parser to determine the end of a token by
adding the length of the token value to the token's start location,
because lexed expression values may not exactly reflect the source code.
For example, "d\\"e" is lexed as a string token whose value is d"e.

Instead, this commit adds a end field to expression tokens. end
is one past the last index of the token source code. This will enable a
parser to determine the span of a token just by looking at that token.

This is a breaking change because the contructor interface of Token
has changed.

Part of #33477.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Feature

Does this PR introduce a breaking change?

  • Yes

Token construction has changed, which could affect any external project using the compiler API.

@ayazhafiz ayazhafiz requested a review from alxhub Nov 2, 2019
@ayazhafiz ayazhafiz requested a review from angular/fw-compiler as a code owner Nov 2, 2019
@ayazhafiz ayazhafiz self-assigned this Nov 2, 2019
@ngbot ngbot bot modified the milestone: needsTriage Nov 2, 2019
@googlebot googlebot added the cla: yes label Nov 2, 2019
@JamesHenry

This comment has been minimized.

Copy link

JamesHenry commented Nov 9, 2019

Thank you for continuing to drive these improvements, @ayazhafiz! It's going to really help https://github.com/angular-eslint/angular-eslint

@kyliau
kyliau approved these changes Nov 26, 2019
@JoostK
JoostK approved these changes Nov 26, 2019
In the past, only the starting index of an expression Token has been
recorded, so a parser could demarkate the span of a token only by the
start locations of two tokens. This may lead to trailing whitespace
being included in the token span:

```html
{{ token1   + token2 }}
   ^^^^^^^^^             recorded span of `token1`
```

It's also not enough for a parser to determine the end of a token by
adding the length of the token value to the token's start location,
because lexed expression values may not exactly reflect the source code.
For example, `"d\\"e"` is lexed as a string token whose value is `d"e`.

Instead, this commit adds a `end` field to expression tokens. `end`
is one past the last index of the token source code. This will enable a
parser to determine the span of a token just by looking at that token.

This is a breaking change because the contructor interface of `Token`
has changed.

Part of #33477.
@ayazhafiz ayazhafiz force-pushed the ayazhafiz:fix/expression-end-span branch from 32e9a5a to 1788b87 Dec 4, 2019
@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Dec 13, 2019

@alxhub @kyliau can you run g3sync?

@kyliau

This comment has been minimized.

Copy link
Member

kyliau commented Dec 13, 2019

@alxhub
alxhub approved these changes Dec 17, 2019
@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Dec 18, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure status "google3" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Dec 18, 2019

Caretaker: could you re-run g3sync?

@ayazhafiz ayazhafiz force-pushed the ayazhafiz:fix/expression-end-span branch from 9570d73 to 1788b87 Dec 19, 2019
@kyliau

This comment has been minimized.

Copy link
Member

kyliau commented Dec 19, 2019

@kyliau

This comment has been minimized.

Copy link
Member

kyliau commented Dec 19, 2019

@ayazhafiz should this target master only, or both master and patch? 9.0 is currently on patch branch.

@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Dec 20, 2019

@ayazhafiz should this target master only, or both master and patch? 9.0 is currently on patch branch.

I think master and patch is good.

alxhub added a commit that referenced this pull request Jan 6, 2020
In the past, only the starting index of an expression Token has been
recorded, so a parser could demarkate the span of a token only by the
start locations of two tokens. This may lead to trailing whitespace
being included in the token span:

```html
{{ token1   + token2 }}
   ^^^^^^^^^             recorded span of `token1`
```

It's also not enough for a parser to determine the end of a token by
adding the length of the token value to the token's start location,
because lexed expression values may not exactly reflect the source code.
For example, `"d\\"e"` is lexed as a string token whose value is `d"e`.

Instead, this commit adds a `end` field to expression tokens. `end`
is one past the last index of the token source code. This will enable a
parser to determine the span of a token just by looking at that token.

This is a breaking change because the contructor interface of `Token`
has changed.

Part of #33477.

PR Close #33549
@alxhub alxhub closed this in 19944c2 Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.