Skip to content

ICU source spans and type checking #39072

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

ICU source spans and type checking #39072

wants to merge 3 commits into from
+238 −29

Conversation

JoostK
Copy link
Member

@JoostK JoostK commented Sep 30, 2020

See individual commits.

@JoostK JoostK added type: bug/fix severity2: inconvenient area: compiler Issues related to `ngc`, Angular's template compiler labels Sep 30, 2020
@ngbot ngbot bot modified the milestone: needsTriage Sep 30, 2020
@JoostK 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 JoostK force-pushed the i18n/icu-spans branch 3 times, most recently from fb3dc09 to 55d481b Compare October 1, 2020 15:55
@JoostK JoostK marked this pull request as ready for review October 1, 2020 16:51
@JoostK JoostK requested review from kyliau and AndrewKushnir October 1, 2020 16:52
@pullapprove pullapprove bot requested a review from petebacondarwin October 1, 2020 16:52
@JoostK JoostK added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 1, 2020
petebacondarwin
petebacondarwin approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tidy PR!

packages/compiler/test/render3/r3_ast_spans_spec.ts Outdated
Comment on lines 458 to 461
['Icu:Var', 'nestedVar'],
['Icu:Var', 'item.var'],
['Icu:Placeholder', '{{nestedPlaceholder}}'],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
AndrewKushnir approved these changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

kyliau
kyliau approved these changes Oct 7, 2020
View reviewed changes
Copy link
Contributor

@kyliau kyliau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for language service

@kyliau
Copy link
Contributor

kyliau commented Oct 7, 2020

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

@JoostK 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
@ngbot
Copy link

ngbot bot commented Oct 8, 2020

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

JoostK added 2 commits October 8, 2020 16:37
@JoostK
refactor(compiler): associate accurate source spans with ICU expressions
01f153f 
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.
@JoostK
test(language-service): test ICU expressions in hybrid visitor
c154dd3 
This commit adds tests for locating expressions within ICU expressions.
@JoostK
fix(compiler-cli): type checking of expressions within ICUs
f9eb202 
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
Copy link
Contributor

kyliau commented Oct 8, 2020

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

@atscott atscott closed this in 4b06ef7 Oct 8, 2020
atscott pushed a commit that referenced this pull request Oct 8, 2020
@JoostK @atscott
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 pushed a commit that referenced this pull request Oct 8, 2020
@JoostK @atscott
fix(compiler-cli): type checking of expressions within ICUs (#39072)
0a16e60 
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
Copy link

matheo commented Oct 17, 2020

@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 :-?
image

I'm wondering what's happening :(

@JoostK
Copy link
Member Author

JoostK commented Oct 17, 2020

@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
Copy link

matheo commented Oct 17, 2020 •
edited
Loading

@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!)

@atscott atscott mentioned this pull request Oct 30, 2020
Closed
@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 Nov 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir

@petebacondarwin petebacondarwin

@kyliau kyliau

@alxhub alxhub

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler breaking changes cla: yes target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

Code used inside ICU expressions is not fully type safe/checked.
8 participants
@JoostK @kyliau @matheo @petebacondarwin @alxhub @AndrewKushnir @jelbourn @googlebot