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 3 "Illegal State" bugs in the Angular Indexer #44678

Closed
wants to merge 3 commits into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Jan 10, 2022

This PR contains 3 commits, each of which fixes a separate crash in the Angular Indexer:

  • Bindings to component properties using this as a receiver: {{this.foo}}
  • Expressions that contain a comment: {{foo // comment}}
  • SVG and other namespaced elements: <svg></svg>

Each of these templates would previously crash the indexer.

…exer

Previously, the Angular Indexer made an assumption that in any binding to
a property of an `ImplicitReceiver`, the property name begins at the start
of the binding. This is true for normal reads from `ImplicitReceiver` as
the implicit receiver has no representation in the template.

However, `ThisReceiver` inherits from `ImplicitReceiver` and _does_ have a
template representation. Such a binding looks like `{{this.foo}}`. This
commit corrects the logic of the indexer to use the `nameSpan` of the
property binding instead of its `sourceSpan` to locate the identifier.
@alxhub alxhub requested a review from atscott January 10, 2022 22:38
@alxhub alxhub added area: compiler Issues related to `ngc`, Angular's template compiler area: language-service Issues related to Angular's VS Code language service type: bug/fix labels Jan 10, 2022
@ngbot ngbot bot modified the milestone: Backlog Jan 10, 2022
@alxhub alxhub added the target: patch This PR is targeted for the next patch release label Jan 10, 2022
When parsing a binding with a comment at the end of the expression, the
parser previously had logic to offset the parsed spans by the length of the
comment. This logic seemed not to serve any useful purpose, and instead
resulted in the corruption of the spans. For example, in the expression
`{{foo // comment}}`, the parser would map the parsed `foo` `PropertyRead`
node at the location of the characters 'ent' from the comment suffix.

This commit removes that logic, correcting the parsed spans of such nodes in
the presence of comments. Removing this logic does not seem to have caused
any tests to fail.
In the `Element` node of a parsed `<svg>` element, the `name` is recorded
as `:svg:svg`. When the Angular Indexer ran over this element, it would
attempt to find this name in the template text and fail, as the namespace
portion of the name was added automatically at parse time and is of course
missing from the original template.

This commit changes the indexer to detect these namespaced elements and only
search for the tag name.
@atscott
Copy link
Contributor

atscott commented Jan 11, 2022

presubmit

@alxhub alxhub added the action: merge The PR is ready for merge by the caretaker label Jan 11, 2022
@atscott
Copy link
Contributor

atscott commented Jan 11, 2022

This PR was merged into the repository by commit 92b23f4.

@atscott atscott closed this in f83fb3a Jan 11, 2022
atscott pushed a commit that referenced this pull request Jan 11, 2022
)

When parsing a binding with a comment at the end of the expression, the
parser previously had logic to offset the parsed spans by the length of the
comment. This logic seemed not to serve any useful purpose, and instead
resulted in the corruption of the spans. For example, in the expression
`{{foo // comment}}`, the parser would map the parsed `foo` `PropertyRead`
node at the location of the characters 'ent' from the comment suffix.

This commit removes that logic, correcting the parsed spans of such nodes in
the presence of comments. Removing this logic does not seem to have caused
any tests to fail.

PR Close #44678
atscott pushed a commit that referenced this pull request Jan 11, 2022
In the `Element` node of a parsed `<svg>` element, the `name` is recorded
as `:svg:svg`. When the Angular Indexer ran over this element, it would
attempt to find this name in the template text and fail, as the namespace
portion of the name was added automatically at parse time and is of course
missing from the original template.

This commit changes the indexer to detect these namespaced elements and only
search for the tag name.

PR Close #44678
atscott pushed a commit that referenced this pull request Jan 11, 2022
…exer (#44678)

Previously, the Angular Indexer made an assumption that in any binding to
a property of an `ImplicitReceiver`, the property name begins at the start
of the binding. This is true for normal reads from `ImplicitReceiver` as
the implicit receiver has no representation in the template.

However, `ThisReceiver` inherits from `ImplicitReceiver` and _does_ have a
template representation. Such a binding looks like `{{this.foo}}`. This
commit corrects the logic of the indexer to use the `nameSpan` of the
property binding instead of its `sourceSpan` to locate the identifier.

PR Close #44678
atscott pushed a commit that referenced this pull request Jan 11, 2022
)

When parsing a binding with a comment at the end of the expression, the
parser previously had logic to offset the parsed spans by the length of the
comment. This logic seemed not to serve any useful purpose, and instead
resulted in the corruption of the spans. For example, in the expression
`{{foo // comment}}`, the parser would map the parsed `foo` `PropertyRead`
node at the location of the characters 'ent' from the comment suffix.

This commit removes that logic, correcting the parsed spans of such nodes in
the presence of comments. Removing this logic does not seem to have caused
any tests to fail.

PR Close #44678
atscott pushed a commit that referenced this pull request Jan 11, 2022
In the `Element` node of a parsed `<svg>` element, the `name` is recorded
as `:svg:svg`. When the Angular Indexer ran over this element, it would
attempt to find this name in the template text and fail, as the namespace
portion of the name was added automatically at parse time and is of course
missing from the original template.

This commit changes the indexer to detect these namespaced elements and only
search for the tag name.

PR Close #44678
@angular-automatic-lock-bot
Copy link

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 Feb 11, 2022
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 area: language-service Issues related to Angular's VS Code language service target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants