Fix 2 "Illegal State" bugs in the Angular Indexer by atscott · Pull Request #44785 · angular/angular

atscott · 2022-01-21T22:40:07Z

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

* ng-template elements with a structural directive: <ng-template *ngIf="condition"> (This cause some test failures for some reason so I'll have to do this in another commit)

Expressions that contain a comment and preceding whitespace: {{ foo // comment. }}
SVG and other namespaced elements with a structural directive: <svg *ngIf="condition"></svg>

Each of these templates would previously crash the indexer.

The original fix for svg elements in angular@92b23f4 did not account for svg elements when they also had a structural directive on them, making the node a template. This resulted in the logic added in fix above not being applied.

The previous fix for correcting spans with comments in angular@59eef29 had the unfortunate side effect of _breaking_ the spans with comments when there was leading whitespace. This happened because the previous fix was testing one without a comment, identifying that the offset shouldn't have anything added to it, and then removing that offset adjustment (`offsets[i] + (expressionText.length - sourceToLex.length)`). Upon further investigation, this offset adjustment _was actually necessary_ for when the input had comments, but this was only because the `stripComments` function used `trim` to remove whitespace for these cases. This is the real problem -- not only does it create a ton of confusion but also it means that the behavior of the lexer and resulting spans is different between inputs with comments and inputs without comments. After reviewing how the `inputLength` of `_ParseAST` was used, it appears that the correct behavior would be to _not_ trim the input. The `inputLength` is used to advance the current index beyond points which have been processed. This _should_ include any whitespace. Additionally, `inputLength` doesn't appear to be needed at all. When there was no comment in the input, it was always equal to the `input.length` anyways. When there _is_ a comment, it should include that comment anyways to advance the index beyond the comment.

atscott · 2022-01-22T00:35:40Z

presubmit

AndrewKushnir · 2022-01-24T18:41:57Z

This PR was merged into the repository by commit ec2e6f6.

) The previous fix for correcting spans with comments in 59eef29 had the unfortunate side effect of _breaking_ the spans with comments when there was leading whitespace. This happened because the previous fix was testing one without a comment, identifying that the offset shouldn't have anything added to it, and then removing that offset adjustment (`offsets[i] + (expressionText.length - sourceToLex.length)`). Upon further investigation, this offset adjustment _was actually necessary_ for when the input had comments, but this was only because the `stripComments` function used `trim` to remove whitespace for these cases. This is the real problem -- not only does it create a ton of confusion but also it means that the behavior of the lexer and resulting spans is different between inputs with comments and inputs without comments. After reviewing how the `inputLength` of `_ParseAST` was used, it appears that the correct behavior would be to _not_ trim the input. The `inputLength` is used to advance the current index beyond points which have been processed. This _should_ include any whitespace. Additionally, `inputLength` doesn't appear to be needed at all. When there was no comment in the input, it was always equal to the `input.length` anyways. When there _is_ a comment, it should include that comment anyways to advance the index beyond the comment. PR Close #44785

…44785) The original fix for svg elements in 92b23f4 did not account for svg elements when they also had a structural directive on them, making the node a template. This resulted in the logic added in fix above not being applied. PR Close #44785

) The previous fix for correcting spans with comments in 59eef29 had the unfortunate side effect of _breaking_ the spans with comments when there was leading whitespace. This happened because the previous fix was testing one without a comment, identifying that the offset shouldn't have anything added to it, and then removing that offset adjustment (`offsets[i] + (expressionText.length - sourceToLex.length)`). Upon further investigation, this offset adjustment _was actually necessary_ for when the input had comments, but this was only because the `stripComments` function used `trim` to remove whitespace for these cases. This is the real problem -- not only does it create a ton of confusion but also it means that the behavior of the lexer and resulting spans is different between inputs with comments and inputs without comments. After reviewing how the `inputLength` of `_ParseAST` was used, it appears that the correct behavior would be to _not_ trim the input. The `inputLength` is used to advance the current index beyond points which have been processed. This _should_ include any whitespace. Additionally, `inputLength` doesn't appear to be needed at all. When there was no comment in the input, it was always equal to the `input.length` anyways. When there _is_ a comment, it should include that comment anyways to advance the index beyond the comment. PR Close #44785

…44785) The original fix for svg elements in 92b23f4 did not account for svg elements when they also had a structural directive on them, making the node a template. This resulted in the logic added in fix above not being applied. PR Close #44785

) The previous fix for correcting spans with comments in 59eef29 had the unfortunate side effect of _breaking_ the spans with comments when there was leading whitespace. This happened because the previous fix was testing one without a comment, identifying that the offset shouldn't have anything added to it, and then removing that offset adjustment (`offsets[i] + (expressionText.length - sourceToLex.length)`). Upon further investigation, this offset adjustment _was actually necessary_ for when the input had comments, but this was only because the `stripComments` function used `trim` to remove whitespace for these cases. This is the real problem -- not only does it create a ton of confusion but also it means that the behavior of the lexer and resulting spans is different between inputs with comments and inputs without comments. After reviewing how the `inputLength` of `_ParseAST` was used, it appears that the correct behavior would be to _not_ trim the input. The `inputLength` is used to advance the current index beyond points which have been processed. This _should_ include any whitespace. Additionally, `inputLength` doesn't appear to be needed at all. When there was no comment in the input, it was always equal to the `input.length` anyways. When there _is_ a comment, it should include that comment anyways to advance the index beyond the comment. PR Close #44785

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/animations](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fanimations/13.1.2/13.2.1) | | [@angular/common](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fcommon/13.1.2/13.2.1) | | [@angular/compiler](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/13.1.2/13.2.1) | | [@angular/compiler-cli](https://github.com/angular/angular) | devDependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/13.1.2/13.2.1) | | [@angular/core](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fcore/13.1.2/13.2.1) | | [@angular/forms](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fforms/13.1.2/13.2.1) | | [@angular/platform-browser](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/13.1.2/13.2.1) | | [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/13.1.2/13.2.1) | | [@angular/router](https://github.com/angular/angular) | dependencies | minor | [`13.1.2` -> `13.2.1`](https://renovatebot.com/diffs/npm/@angular%2frouter/13.1.2/13.2.1) | --- ### Release Notes <details> <summary>angular/angular</summary> ### [`v13.2.1`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1321-2022-02-02) [Compare Source](angular/angular@13.2.0...13.2.1) ##### animations | Commit | Type | Description | | -- | -- | -- | | [4644886aaf](angular/angular@4644886) | perf | remove no longer needed CssKeyframes classes ([#44903](angular/angular#44903)) ([#44919](angular/angular#44919)) | ##### common | Commit | Type | Description | | -- | -- | -- | | [b4e4617807](angular/angular@b4e4617) | fix | include query parameters for open HTTP requests in `verify` ([#44917](angular/angular#44917)) | ##### compiler-cli | Commit | Type | Description | | -- | -- | -- | | [0778e6f7d7](angular/angular@0778e6f) | fix | accept nullish coalescing operator for any and unknown types ([#44862](angular/angular#44862)) | | [07185f4ed1](angular/angular@07185f4) | fix | enable nullish coalescing check only with `strictNullChecks` ([#44862](angular/angular#44862)) | | [4a5ad1793f](angular/angular@4a5ad17) | fix | ensure casing of logical paths is preserved ([#44798](angular/angular#44798)) | ##### core | Commit | Type | Description | | -- | -- | -- | | [7ec482d9c2](angular/angular@7ec482d) | fix | Add back support for namespace URIs in createElement of dom renderer ([#44914](angular/angular#44914)) | | [250dc40a46](angular/angular@250dc40) | fix | flush delayed scoping queue while setting up TestBed ([#44814](angular/angular#44814)) | ##### forms | Commit | Type | Description | | -- | -- | -- | | [1aebbf8714](angular/angular@1aebbf8) | fix | ensure OnPush ancestors are marked dirty when the promise resolves ([#44886](angular/angular#44886)) | | [6b7fffcbeb](angular/angular@6b7fffc) | fix | Update the typed forms migration schematic to find all files. ([#44881](angular/angular#44881)) | #### Special Thanks Alan, Andrew Kushnir, Andrew Scott, Aristeidis Bampakos, Arjen, Daniel Díaz, David Shevitz, Doug Parker, Dylan Hunn, Esteban Gehring, George Kalpakas, Jessica Janiuk, JoostK, Juri Strumpflohner, Lee Robinson, Maarten Tibau, Paul Gschwendtner, Theodore Brown, arturovt, dario-piotrowicz, fru2, markostanimirovic and mgechev  ### [`v13.2.0`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1320-2022-01-26) [Compare Source](angular/angular@13.1.3...13.2.0) #### Deprecations ##### - The `CachedResourceLoader` and `RESOURCE_CACHE_PROVIDER` symbols were previously necessary in some cases to test AOT-compiled components with View Engine, but they are no longer needed since Ivy. - The `ComponentFactory` and `ComponentFactoryResolver` classes are deprecated. Since Ivy, there is no need to resolve Component factories. Please use other APIs where you Component classes can be used directly (without resolving their factories). - Since Ivy, the `CompilerOptions.useJit` and `CompilerOptions.missingTranslation` config options are unused, passing them has no effect. ##### | Commit | Type | Description | | -- | -- | -- | | [9c11183e74](angular/angular@9c11183) | docs | deprecate `CachedResourceLoader` and `RESOURCE_CACHE_PROVIDER` symbols ([#44749](angular/angular#44749)) | | [9f12e7fea4](angular/angular@9f12e7f) | docs | deprecate `ComponentFactory` and `ComponentFactoryResolver` symbols ([#44749](angular/angular#44749)) | | [4e95a316ce](angular/angular@4e95a31) | docs | deprecate unused config options from the `CompilerOptions` interface ([#44749](angular/angular#44749)) | ##### compiler | Commit | Type | Description | | -- | -- | -- | | [a4ab6d6b72](angular/angular@a4ab6d6) | feat | add support for safe calls in templates ([#44580](angular/angular#44580)) | | [abd1bc8039](angular/angular@abd1bc8) | fix | correct spans when parsing bindings with comments ([#44785](angular/angular#44785)) | | [ed67a074ce](angular/angular@ed67a07) | fix | properly compile DI factories when coverage reporting is enabled ([#44732](angular/angular#44732)) | ##### compiler-cli | Commit | Type | Description | | -- | -- | -- | | [fa835b5a29](angular/angular@fa835b5) | feat | enable extended diagnostics by default ([#44712](angular/angular#44712)) | | [73424def13](angular/angular@73424de) | feat | provide the animations for `DirectiveMeta` ([#44630](angular/angular#44630)) | | [fe3e4d6865](angular/angular@fe3e4d6) | fix | Handle `ng-template` with structural directive in indexer ([#44788](angular/angular#44788)) | | [7316e72ec5](angular/angular@7316e72) | fix | properly index <svg> elements when on a template ([#44785](angular/angular#44785)) | | [100091ebf0](angular/angular@100091e) | fix | remove leftover `_extendedTemplateDiagnostics` requirements ([#44777](angular/angular#44777)) | | [d2ae96f742](angular/angular@d2ae96f) | fix | skip `ExtendedTemplateCheckerImpl` construction if there were configuration errors ([#44778](angular/angular#44778)) | ##### core | Commit | Type | Description | | -- | -- | -- | | [5626b34264](angular/angular@5626b34) | fix | consistently use namespace short name rather than URI ([#44766](angular/angular#44766)) | | [94bfcdd9de](angular/angular@94bfcdd) | fix | error if NgZone.isInAngularZone is called with a noop zone ([#44800](angular/angular#44800)) | ##### forms | Commit | Type | Description | | -- | -- | -- | | [72092ebd26](angular/angular@72092eb) | feat | Allow a FormControl to use initial value as default. ([#44434](angular/angular#44434)) | | [f7aa937cac](angular/angular@f7aa937) | fix | Make some minor fixups for forward-compatibility with typed forms. ([#44540](angular/angular#44540)) | ##### router | Commit | Type | Description | | -- | -- | -- | | [5a4ddfd4f5](angular/angular@5a4ddfd) | feat | Allow symbol keys for `Route` `data` and `resolve` properties ([#44519](angular/angular#44519)) | #### Special Thanks Alex Rickabaugh, Andrew Kushnir, Andrew Scott, Dario Piotrowicz, Derek Cormier, Doug Parker, Douglas Parker, Dylan Hunn, George Kalpakas, Jessica Janiuk, JoostK, Kristiyan Kostadinov, Martin Probst, Oleg Postoev, Stephanie Tuerk, Tim Bowersox, Wiley Marques, Yousaf Nawaz, dario-piotrowicz, iRealNirmal, ivanwonder and shejialuo  ### [`v13.1.3`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1313-2022-01-19) [Compare Source](angular/angular@13.1.2...13.1.3) ##### animations | Commit | Type | Description | | -- | -- | -- | | [af0a152a2c](angular/angular@af0a152) | fix | apply setStyles to only rootTimelines ([#44515](angular/angular#44515)) | ##### compiler-cli | Commit | Type | Description | | -- | -- | -- | | [626f3f230b](angular/angular@626f3f2) | perf | reduce analysis work during incremental rebuilds ([#44731](angular/angular#44731)) | ##### ngcc | Commit | Type | Description | | -- | -- | -- | | [f9ca4d8499](angular/angular@f9ca4d8) | fix | support element accesses for export declarations ([#44669](angular/angular#44669)) | #### Special Thanks Alan Agius, Andrew Kushnir, AnkitSharma-007, Daniel Díaz, Dmytro Mezhenskyi, Jessica Janiuk, Joey Perrott, JoostK, Ramesh Thiruchelvam, dario-piotrowicz, iRealNirmal and Łukasz Holeczek  </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Co-authored-by: crapStone <crapstone@noreply.codeberg.org> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1148 Reviewed-by: crapStone <crapstone@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

angular-automatic-lock-bot · 2022-02-24T16:11:32Z

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

atscott added the target: patch This PR is targeted for the next patch release label Jan 21, 2022

atscott requested a review from alxhub January 21, 2022 22:40

atscott force-pushed the indexerbugs branch 2 times, most recently from dee05da to d367b1d Compare January 21, 2022 22:53

atscott force-pushed the indexerbugs branch from d367b1d to c71b3ab Compare January 22, 2022 00:32

atscott changed the title ~~Fix 3 "Illegal State" bugs in the Angular Indexer~~ Fix 2 "Illegal State" bugs in the Angular Indexer Jan 22, 2022

atscott added the area: compiler Issues related to `ngc`, Angular's template compiler label Jan 24, 2022

ngbot bot added this to the Backlog milestone Jan 24, 2022

alxhub approved these changes Jan 24, 2022

View reviewed changes

atscott added the action: merge The PR is ready for merge by the caretaker label Jan 24, 2022

AndrewKushnir closed this in f1e84a3 Jan 24, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 2 "Illegal State" bugs in the Angular Indexer#44785

Fix 2 "Illegal State" bugs in the Angular Indexer#44785
atscott wants to merge 2 commits intoangular:masterfrom
atscott:indexerbugs

atscott commented Jan 21, 2022 •

edited

Loading

Uh oh!

atscott commented Jan 22, 2022

Uh oh!

AndrewKushnir commented Jan 24, 2022

Uh oh!

angular-automatic-lock-bot bot commented Feb 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

atscott commented Jan 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott commented Jan 22, 2022

Uh oh!

AndrewKushnir commented Jan 24, 2022

Uh oh!

angular-automatic-lock-bot bot commented Feb 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

atscott commented Jan 21, 2022 •

edited

Loading