fix(compiler): nested for loops incorrectly calculating computed variables#52931
Closed
crisbeto wants to merge 1 commit intoangular:mainfrom
Closed
fix(compiler): nested for loops incorrectly calculating computed variables#52931crisbeto wants to merge 1 commit intoangular:mainfrom
crisbeto wants to merge 1 commit intoangular:mainfrom
Conversation
crisbeto
commented
Nov 15, 2023
Member
Author
There was a problem hiding this comment.
Note: my initial thinking here was that we could just skip over the current scope and look up $index in the parent scope. This doesn't work, because the variable expression is cached which will cause all other usages of $index in the template to refer to the implicit $index of the parent template, rather than the local one.
b9a8711 to
b20040e
Compare
crisbeto
commented
Nov 15, 2023
Member
Author
There was a problem hiding this comment.
@dylhunn I didn't do this for the template pipeline, because you mentioned that you're moving some code around there. Also I think this is severe enough that we should get a fix out for users as quickly as we can.
Contributor
There was a problem hiding this comment.
We probably should pursue a different solution in the template pipeline. Perhaps making everything a proper named variable, instead of inlining aliases ($first -> $index === 0) at the site of usage.
Member
Author
There was a problem hiding this comment.
Agreed that it would be optimal to just have these on the context, but I think we did it this way for perf reasons (cc @pkozlowski-opensource).
…ables The `$first`, `$last`, `$even` and `$odd` variables in `@for` loops aren't defined on the template context of the loop, but are computed based on `$index` and `$count` (e.g. `$first` is defined as `$index === 0`). We do this calculation by looking up `$index` and `$count` when one of the variables is used. The problem is that all `@for` loop variables are available implicitly which means that when a nested loop tries to rewrite a reference to an outer loop computed variable, it finds its own `$index` and `$count` first and it doesn't look up the ones on the parent at all. This means that the calculated values will be incorrect at runtime. These changes work around the issue by defining nested-level-specific variable names that can be used for lookups (e.g. `$index` at level `2` will also be available as `ɵ$index_2`). This isn't the most elegant solution, however the `TemplatDefitinionBuilder` wasn't set up to handle shadowed variables like this and it doesn't make sense to refactor it given the upcoming template pipeline. Fixes angular#52917.
b20040e to
38e727a
Compare
Member
Author
|
Pushed a change to align the test output with #52935. |
dylhunn
approved these changes
Nov 16, 2023
Contributor
dylhunn
left a comment
There was a problem hiding this comment.
Makes sense as a temporary workaround.
Contributor
|
This PR was merged into the repository by commit d9d566d. |
AndrewKushnir
pushed a commit
that referenced
this pull request
Nov 16, 2023
…ables (#52931) The `$first`, `$last`, `$even` and `$odd` variables in `@for` loops aren't defined on the template context of the loop, but are computed based on `$index` and `$count` (e.g. `$first` is defined as `$index === 0`). We do this calculation by looking up `$index` and `$count` when one of the variables is used. The problem is that all `@for` loop variables are available implicitly which means that when a nested loop tries to rewrite a reference to an outer loop computed variable, it finds its own `$index` and `$count` first and it doesn't look up the ones on the parent at all. This means that the calculated values will be incorrect at runtime. These changes work around the issue by defining nested-level-specific variable names that can be used for lookups (e.g. `$index` at level `2` will also be available as `ɵ$index_2`). This isn't the most elegant solution, however the `TemplatDefitinionBuilder` wasn't set up to handle shadowed variables like this and it doesn't make sense to refactor it given the upcoming template pipeline. Fixes #52917. PR Close #52931
Closed
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
ChellappanRajan
pushed a commit
to ChellappanRajan/angular
that referenced
this pull request
Jan 23, 2024
…ables (angular#52931) The `$first`, `$last`, `$even` and `$odd` variables in `@for` loops aren't defined on the template context of the loop, but are computed based on `$index` and `$count` (e.g. `$first` is defined as `$index === 0`). We do this calculation by looking up `$index` and `$count` when one of the variables is used. The problem is that all `@for` loop variables are available implicitly which means that when a nested loop tries to rewrite a reference to an outer loop computed variable, it finds its own `$index` and `$count` first and it doesn't look up the ones on the parent at all. This means that the calculated values will be incorrect at runtime. These changes work around the issue by defining nested-level-specific variable names that can be used for lookups (e.g. `$index` at level `2` will also be available as `ɵ$index_2`). This isn't the most elegant solution, however the `TemplatDefitinionBuilder` wasn't set up to handle shadowed variables like this and it doesn't make sense to refactor it given the upcoming template pipeline. Fixes angular#52917. PR Close angular#52931
rlmestre
pushed a commit
to rlmestre/angular
that referenced
this pull request
Jan 26, 2024
…ables (angular#52931) The `$first`, `$last`, `$even` and `$odd` variables in `@for` loops aren't defined on the template context of the loop, but are computed based on `$index` and `$count` (e.g. `$first` is defined as `$index === 0`). We do this calculation by looking up `$index` and `$count` when one of the variables is used. The problem is that all `@for` loop variables are available implicitly which means that when a nested loop tries to rewrite a reference to an outer loop computed variable, it finds its own `$index` and `$count` first and it doesn't look up the ones on the parent at all. This means that the calculated values will be incorrect at runtime. These changes work around the issue by defining nested-level-specific variable names that can be used for lookups (e.g. `$index` at level `2` will also be available as `ɵ$index_2`). This isn't the most elegant solution, however the `TemplatDefitinionBuilder` wasn't set up to handle shadowed variables like this and it doesn't make sense to refactor it given the upcoming template pipeline. Fixes angular#52917. PR Close angular#52931
amilamen
pushed a commit
to amilamen/angular
that referenced
this pull request
Jan 26, 2024
…ables (angular#52931) The `$first`, `$last`, `$even` and `$odd` variables in `@for` loops aren't defined on the template context of the loop, but are computed based on `$index` and `$count` (e.g. `$first` is defined as `$index === 0`). We do this calculation by looking up `$index` and `$count` when one of the variables is used. The problem is that all `@for` loop variables are available implicitly which means that when a nested loop tries to rewrite a reference to an outer loop computed variable, it finds its own `$index` and `$count` first and it doesn't look up the ones on the parent at all. This means that the calculated values will be incorrect at runtime. These changes work around the issue by defining nested-level-specific variable names that can be used for lookups (e.g. `$index` at level `2` will also be available as `ɵ$index_2`). This isn't the most elegant solution, however the `TemplatDefitinionBuilder` wasn't set up to handle shadowed variables like this and it doesn't make sense to refactor it given the upcoming template pipeline. Fixes angular#52917. PR Close angular#52931
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
$first,$last,$evenand$oddvariables in@forloops aren't defined on the template context of the loop, but are computed based on$indexand$count(e.g.$firstis defined as$index === 0). We do this calculation by looking up$indexand$countwhen one of the variables is used.The problem is that all
@forloop variables are available implicitly which means that when a nested loop tries to rewrite a reference to an outer loop computed variable, it finds its own$indexand$countfirst and it doesn't look up the ones on the parent at all. This means that the calculated values will be incorrect at runtime.These changes work around the issue by defining nested-level-specific variable names that can be used for lookups (e.g.
$indexat level2will also be available asɵ$index_2). This isn't the most elegant solution, however theTemplatDefitinionBuilderwasn't set up to handle shadowed variables like this and it doesn't make sense to refactor it given the upcoming template pipeline.Fixes #52917.