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

fix(ivy): record correct absolute source span for template binding expressions #31813

Closed
wants to merge 5 commits into from

Conversation

@ayazhafiz
Copy link
Member

ayazhafiz commented Jul 23, 2019

Expressions in an inline template binding are improperly recorded as
spaning an offset calculated from the start of the template binding
attribute key, whereas they should be calculated from the start of the
attribute value, which contains the actual binding AST.

PR Checklist

Please check if your PR fulfills the following requirements:

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: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@ayazhafiz ayazhafiz requested a review from alxhub Jul 23, 2019
@ayazhafiz ayazhafiz requested a review from angular/fw-compiler as a code owner Jul 23, 2019
@ayazhafiz ayazhafiz self-assigned this Jul 23, 2019
@ngbot ngbot bot added this to the needsTriage milestone Jul 23, 2019
@googlebot googlebot added the cla: yes label Jul 23, 2019
ayazhafiz added a commit to ayazhafiz/angular that referenced this pull request Jul 26, 2019
`TemplateVisitor#visitBoundAttribute` currently has to invoke visiting
expressions manually (this is fixed in angular#31813). Previously, it did not
bind `targetToIdentifier` to the visitor before deferring to the
expression visitor, which breaks the `targetToIdentifier` code. This
fixes that and adds a test to ensure the closure processed correctly.

This change is urgent; without it, many indexing targets in g3 are
broken.
AndrewKushnir added a commit that referenced this pull request Jul 26, 2019
…31861)

`TemplateVisitor#visitBoundAttribute` currently has to invoke visiting
expressions manually (this is fixed in #31813). Previously, it did not
bind `targetToIdentifier` to the visitor before deferring to the
expression visitor, which breaks the `targetToIdentifier` code. This
fixes that and adds a test to ensure the closure processed correctly.

This change is urgent; without it, many indexing targets in g3 are
broken.

PR Close #31861
@ayazhafiz ayazhafiz changed the title fix(ivy): record correct absolute source span for ngForOf expressions fix(ivy): record correct absolute source span for template binding expressions Jul 27, 2019
@ayazhafiz ayazhafiz force-pushed the ayazhafiz:fix/ngfor branch from 7cc8c2c to d372520 Jul 27, 2019
sabeersulaiman added a commit to sabeersulaiman/angular that referenced this pull request Sep 6, 2019
…ngular#31861)

`TemplateVisitor#visitBoundAttribute` currently has to invoke visiting
expressions manually (this is fixed in angular#31813). Previously, it did not
bind `targetToIdentifier` to the visitor before deferring to the
expression visitor, which breaks the `targetToIdentifier` code. This
fixes that and adds a test to ensure the closure processed correctly.

This change is urgent; without it, many indexing targets in g3 are
broken.

PR Close angular#31861
@ayazhafiz ayazhafiz force-pushed the ayazhafiz:fix/ngfor branch from 796271d to 9a5521a Dec 8, 2019
@ayazhafiz ayazhafiz requested review from petebacondarwin and JoostK Dec 8, 2019
@JoostK
JoostK approved these changes Dec 8, 2019
Copy link
Member

JoostK left a comment

LGTM, with one comment.

const absoluteValueOffset = attribute.valueSpan ?
attribute.valueSpan.start.offset :
// If there is no value span the attribute does not have a value, like `attr` in
//`<div attr></div>`. In this case, point to one beyond the attribute name.

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 8, 2019

Member

Shouldn't this be more like this?

Suggested change
//`<div attr></div>`. In this case, point to one beyond the attribute name.
//`<div attr></div>`. In this case, point to the end of the attribute name.

This comment has been minimized.

Copy link
@ayazhafiz

ayazhafiz Dec 8, 2019

Author Member

I mean to say that the offset of the attribute value is one character past the last character of the attribute name. I have clarified that comment. It shouldn't point to the last character of the attribute name.

This comment has been minimized.

Copy link
@JoostK

JoostK Dec 8, 2019

Member

Yeah, but I don't see the one character offset in the code. Adding the attribute name's length ends up at the end of the attribute name, not one character beyond it? Am I missing something here?

This comment has been minimized.

Copy link
@ayazhafiz

ayazhafiz Dec 8, 2019

Author Member

I believe it does:

word
^^^^^
01234

so adding 0 + len(word) = 4 which is one beyond the last character of the word. Let me know if I'm misinterpreting something.

@petebacondarwin

This comment has been minimized.

Copy link
Member

petebacondarwin commented Dec 9, 2019

I think this should target "master & patch" right?

@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Dec 13, 2019

Caretaker: can you run g3sync?

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Dec 13, 2019

Hi @ayazhafiz, I've started g3 presubmit. Could you please also rebase this PR on top of the latest master (PR was updated from master 5 days ago), so we re-run CI with the most up-to-date version? Thank you.

@AndrewKushnir

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Dec 14, 2019

FYI, VE and Ivy presubmits are successful.

ayazhafiz added 5 commits Jul 23, 2019
Expressions in an inline template binding are improperly recorded as
spaning an offset calculated from the start of the template binding
attribute key, whereas they should be calculated from the start of the
attribute value, which contains the actual binding AST.
…essions
…essions
…essions
…essions
@ayazhafiz ayazhafiz force-pushed the ayazhafiz:fix/ngfor branch from fdd7f0f to 799cb3e Dec 14, 2019
@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Dec 14, 2019

@AndrewKushnir I've rebased the branch on master (f79110c).

@kara

This comment has been minimized.

Copy link
Contributor

kara commented Dec 16, 2019

@ayazhafiz Are you still waiting on a review from @alxhub? If not, please remove him as a pending reviewer and I'll merge this

@ayazhafiz ayazhafiz removed the request for review from alxhub Dec 16, 2019
@ayazhafiz

This comment has been minimized.

Copy link
Member Author

ayazhafiz commented Dec 16, 2019

@kara done!

@kara kara removed the PR action: review label Dec 16, 2019
@kara kara closed this in ddc229b Dec 16, 2019
kara added a commit that referenced this pull request Dec 16, 2019
…#31813)

Expressions in an inline template binding are improperly recorded as
spaning an offset calculated from the start of the template binding
attribute key, whereas they should be calculated from the start of the
attribute value, which contains the actual binding AST.

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

This comment has been minimized.

Copy link

angular-automatic-lock-bot bot commented Jan 16, 2020

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 Jan 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.