ICU source spans and type checking by JoostK · Pull Request #39072 · angular/angular

JoostK · 2020-09-30T22:25:11Z

See individual commits.

petebacondarwin

Nice tidy PR!

packages/compiler/src/expression_parser/parser.ts

packages/compiler/src/i18n/i18n_ast.ts

packages/compiler/test/i18n/translation_bundle_spec.ts

packages/compiler/test/render3/r3_ast_absolute_span_spec.ts

packages/compiler/test/render3/r3_ast_spans_spec.ts

petebacondarwin · 2020-10-01T18:56:10Z

packages/compiler/test/render3/r3_ast_spans_spec.ts

This seems a strange ordering. What is the algorithm for this?

I was surprised too! I didn't deep dive into it, but the nested ICU had been collapsed into the outer ICU datastructure where nestedVar comes before item.var in the list of vars, surprisingly.

AndrewKushnir

LGTM, just one proposal to add tests for nested ICUs. Thanks @JoostK 👍

packages/compiler/test/render3/r3_ast_spans_spec.ts

kyliau

LGTM for language service

kyliau · 2020-10-07T23:11:24Z

presubmit: http://test/OCL:335973414:BASE:335973435:1602112262578:6626a3d3

ngbot · 2020-10-08T10:32:53Z

I see that you just added the action: merge label, but the following checks are still failing:
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.

Prior to this change, expressions within ICUs would have a source span corresponding with the whole ICU. This commit narrows down the source spans of these expressions to the exact location in the source file, as a prerequisite for reporting type check errors within these expressions.

This commit adds tests for locating expressions within ICU expressions.

Expressions within ICU expressions in templates were not previously type-checked, as they were skipped while traversing the elements within a template. This commit enables type checking of these expressions by actually visiting the expressions. BREAKING CHANGE: Expressions within ICUs are now type-checked again, fixing a regression in Ivy. This may cause compilation failures if errors are found in expressions that appear within an ICU. Please correct these expressions to resolve the type-check errors. Fixes angular#39064

kyliau · 2020-10-08T16:54:39Z

presubmit: http://test/OCL:335973414:BASE:336106137:1602176042884:c6670c25

This commit adds tests for locating expressions within ICU expressions. PR Close #39072

Expressions within ICU expressions in templates were not previously type-checked, as they were skipped while traversing the elements within a template. This commit enables type checking of these expressions by actually visiting the expressions. BREAKING CHANGE: Expressions within ICUs are now type-checked again, fixing a regression in Ivy. This may cause compilation failures if errors are found in expressions that appear within an ICU. Please correct these expressions to resolve the type-check errors. Fixes #39064 PR Close #39072

matheo · 2020-10-17T23:12:02Z

@JoostK in the company we're using pug with ng-add-pug-loader to patch the webpack config,
but with the latest @angular-devkit/build-angular (0.1001.7) and compiler (10.1.6)
we started to get weird errors while ng serveing and changing files.

Changing one template throws a lot of errors from nowhere
and from my point of view it seems that pug is no longer recognized :-?

I'm wondering what's happening :(

JoostK · 2020-10-17T23:36:33Z

@matheo that is unrelated to this change, as this only landed on master (for v11) and only affects type-checking, not parsing. The issue you're seeing is angular/angular-cli#18290, where the forked type-checker process does not use Webpack, so any custom loader is disregarded. Disabling the forked type checker should do the trick, although I'm not sure why you only started seeing problems now as this has been the case for a while.

matheo · 2020-10-17T23:47:04Z

@JoostK I was aware of the not so pertinent comment,
but anywhere else to get this answer with the solution from you? :)
Thank you so much
(I need to become a GDE soon!)

angular-automatic-lock-bot · 2020-11-18T16:28:36Z

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

JoostK added type: bug/fix severity2: inconvenient area: compiler Issues related to `ngc`, Angular's template compiler labels Sep 30, 2020

ngbot bot modified the milestone: needsTriage Sep 30, 2020

googlebot added the cla: yes label Sep 30, 2020

JoostK linked an issue Sep 30, 2020 that may be closed by this pull request

Code used inside ICU expressions is not fully type safe/checked. #39064

Closed

JoostK force-pushed the i18n/icu-spans branch 3 times, most recently from fb3dc09 to 55d481b Compare October 1, 2020 15:55

JoostK marked this pull request as ready for review October 1, 2020 16:51

JoostK requested review from AndrewKushnir and kyliau October 1, 2020 16:52

pullapprove bot requested a review from petebacondarwin October 1, 2020 16:52

JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 1, 2020

jelbourn removed the severity2: inconvenient label Oct 1, 2020

petebacondarwin approved these changes Oct 1, 2020

View reviewed changes

JoostK force-pushed the i18n/icu-spans branch from 55d481b to 504fca0 Compare October 1, 2020 19:23

AndrewKushnir approved these changes Oct 1, 2020

View reviewed changes

packages/compiler/test/render3/r3_ast_spans_spec.ts Outdated Show resolved Hide resolved

JoostK force-pushed the i18n/icu-spans branch from aa4f845 to 75922e6 Compare October 1, 2020 19:54

alxhub approved these changes Oct 7, 2020

View reviewed changes

kyliau approved these changes Oct 7, 2020

View reviewed changes

JoostK added action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 8, 2020

JoostK added 2 commits October 8, 2020 16:37

test(language-service): test ICU expressions in hybrid visitor

c154dd3

This commit adds tests for locating expressions within ICU expressions.

JoostK force-pushed the i18n/icu-spans branch from a0521cf to f9eb202 Compare October 8, 2020 14:38

JoostK added the breaking changes label Oct 8, 2020

atscott closed this in 4b06ef7 Oct 8, 2020

atscott pushed a commit that referenced this pull request Oct 8, 2020

test(language-service): test ICU expressions in hybrid visitor (#39072)

f84b378

This commit adds tests for locating expressions within ICU expressions. PR Close #39072

atscott mentioned this pull request Oct 30, 2020

fix(compiler): invalid extraction text #39486

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 18, 2020

Conversation

JoostK commented Sep 30, 2020

Uh oh!

petebacondarwin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

petebacondarwin Oct 1, 2020

Choose a reason for hiding this comment

Uh oh!

JoostK Oct 1, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kyliau left a comment

Choose a reason for hiding this comment

Uh oh!

kyliau commented Oct 7, 2020

Uh oh!

ngbot bot commented Oct 8, 2020

Uh oh!

kyliau commented Oct 8, 2020

Uh oh!

matheo commented Oct 17, 2020

Uh oh!

JoostK commented Oct 17, 2020

Uh oh!

matheo commented Oct 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

angular-automatic-lock-bot bot commented Nov 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

matheo commented Oct 17, 2020 •

edited

Loading