feat(compiler): support safe keyed read expressions by crisbeto · Pull Request #41911 · angular/angular

crisbeto · 2021-05-01T12:21:33Z

Currently we support safe property (a?.b) and method (a?.b()) accesses, but we don't handle safe keyed reads (a?.[0]) which is inconsistent. These changes expand the compiler in order to support safe key read expressions as well.

AndrewKushnir

@crisbeto thanks for the changes 👍 I've just added a couple minor comments related to extra test cases.

Reviewed-for: fw-core

packages/compiler/test/expression_parser/parser_spec.ts

packages/core/test/acceptance/integration_spec.ts

JoostK

This looks pretty good. I left some comments and a request to add tests for the Ivy language service in packages/language-service/ivy/test/legacy/template_target_spec.ts.

packages/compiler/src/expression_parser/parser.ts

packages/compiler/test/expression_parser/parser_spec.ts

packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts

packages/compiler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/GOLDEN_PARTIAL.js

crisbeto · 2021-05-05T20:58:01Z

All of the feedback has been addressed.

...iler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/safe_keyed_read_template.js

packages/compiler/test/expression_parser/parser_spec.ts

AndrewKushnir · 2021-05-13T20:44:35Z

@crisbeto once again, thanks a lot for these changes, this is a great thing to support in expressions! 👍

I'm curious if it might be straightforward to also add optional chaining with function calls (as a followup PR, following the changes from this one), like someObj.method?.(). If we need to bump a compiler version and/or do any other checks for libraries, we can just do that for both changes at the same time. If it's not an easy change - no worries, we can do that later!

crisbeto · 2021-05-13T20:46:43Z

From what I could tell, we already support safe function calls.

JoostK · 2021-05-13T20:50:00Z

From what I could tell, we already support safe function calls.

Those may actually only be someObj?.method(), which is a SafeMethodCall in the AST (we're not treating the safe property read as the receiver of a call expression, but rather the whole call expression is specialised for the ?. syntax). So it could be that Andrew's point is valid.

crisbeto · 2021-05-13T20:54:10Z

Edit: I re-read the tests again and you're right, this isn't something we handle at the moment.

For reference, I'm talking about these tests: https://github.com/angular/angular/blob/master/packages/compiler/test/expression_parser/parser_spec.ts#L371

From an AST standpoint, wouldn't it be weird to classify someObj?.method() as a safe method call? I believe that under the TS AST that would be a safe property access expression.

JoostK · 2021-05-13T21:02:45Z

From an AST standpoint, wouldn't it be weird to classify someObj?.method() as a safe method call? I believe that under the TS AST that would be a safe property access expression.

Yes, our AST is weird. I don't actually know why we're treating it as a special AST node. We have the same weirdness around FunctionCall vs MethodCall, where the latter has a property access receiver.

tehpsalmist · 2021-05-20T15:22:40Z

Just as a suggestion, it might be a good idea to include [a]?.[b] as a test case for the functionality, too. Thanks to everyone involved!

crisbeto · 2021-05-21T16:32:41Z

Marking this as ready for presubmit and merge, based on our discussion.

crisbeto · 2021-05-21T17:44:19Z

@petebacondarwin just to double-check: should I keep the changes to the minVersion here? https://github.com/angular/angular/pull/41911/files#diff-cf1b6ab0cb8378b511b1930cd9def496e3f238504d3f28168ae7a97a457f8047L110

petebacondarwin · 2021-05-21T18:47:32Z

I'd be cool either leaving it as it currently is.
But perhaps changing it back to 12.0.0 is better. This would avoid churning that bit of code, and the application developer would still get an error if they tried to use a library which had this feature in a 12.0.0 app.

crisbeto · 2021-05-21T18:54:10Z

The minVersion changes have been reverted.

alxhub · 2021-05-21T20:52:04Z

packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts

I'm not sure this is necessary/desirable. In 2 of the 3 cases, a sub-expression is already marked with the source span of the SafeKeyedRead. It's odd to have an outer expression that's also annotated.

I think this could be refactored:

don't have the catch-all addParseSpanInfo, and mark the element access expression with the span inside the veWillInferAnyFor case.

that'll let you get rid of let node and return from inside each case instead.

I don't have the full context here, but I was following the pattern from visitSafeMethodCall and visitSafePropertyRead right above it. Should they be refactored as well?

Possibly, but I think that's a bigger change that should be in a different PR. For now, I think following the pattern from visitSafeMethodCall and visitSafePropertyRead is correct.

packages/compiler/test/expression_parser/parser_spec.ts

packages/compiler/src/expression_parser/ast.ts

Currently we support safe property (`a?.b`) and method (`a?.b()`) accesses, but we don't handle safe keyed reads (`a?.[0]`) which is inconsistent. These changes expand the compiler in order to support safe key read expressions as well.

crisbeto

I've addressed the latest set of feedback.

crisbeto · 2021-05-23T07:22:59Z

packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts

I don't have the full context here, but I was following the pattern from visitSafeMethodCall and visitSafePropertyRead right above it. Should they be refactored as well?

packages/compiler/src/expression_parser/ast.ts

packages/compiler/test/expression_parser/parser_spec.ts

AndrewKushnir · 2021-05-26T21:11:22Z

@crisbeto FYI I've started a presubmit, will keep this thread updated. I've also removed the "action: merge" label for now since @alxhub requested changes (which you've addressed), so he may want to have a final look. Thank you.

alxhub · 2021-06-01T23:13:04Z

packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts

Possibly, but I think that's a bigger change that should be in a different PR. For now, I think following the pattern from visitSafeMethodCall and visitSafePropertyRead is correct.

AndrewKushnir · 2021-06-02T05:30:40Z

Global Presubmit.

AndrewKushnir · 2021-06-02T20:31:31Z

FYI presubmits (including a global one) went well, waiting for the language-service group approval before merging this PR.

atscott

LGTM for language service. Looks like no updates are needed there. I might still recommend adding a simple quick info or go to definition test for a property read on a?.[0].pr|op just to have an automated test for it (rather than the manual testing I did).

angular-automatic-lock-bot · 2021-07-04T16:05:58Z

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

google-cla bot added the cla: yes label May 1, 2021

crisbeto force-pushed the safe-array-access branch 3 times, most recently from 4ca10d0 to 8e89b50 Compare May 1, 2021 16:46

crisbeto marked this pull request as ready for review May 1, 2021 17:08

pullapprove bot requested review from JoostK, kyliau and mhevery May 1, 2021 17:08

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler target: minor This PR is targeted for the next minor release labels May 1, 2021

ngbot bot modified the milestone: Backlog May 1, 2021

AndrewKushnir approved these changes May 3, 2021

View reviewed changes

packages/compiler/test/expression_parser/parser_spec.ts Outdated Show resolved Hide resolved

packages/core/test/acceptance/integration_spec.ts Outdated Show resolved Hide resolved

JoostK reviewed May 4, 2021

View reviewed changes

JoostK requested a review from alxhub May 4, 2021 18:24

crisbeto force-pushed the safe-array-access branch 2 times, most recently from ab8697a to 402f75b Compare May 5, 2021 20:51

JoostK reviewed May 7, 2021

View reviewed changes

...iler-cli/test/compliance/test_cases/r3_view_compiler/safe_access/safe_keyed_read_template.js Outdated Show resolved Hide resolved

packages/compiler/test/expression_parser/parser_spec.ts Outdated Show resolved Hide resolved

JoostK approved these changes May 7, 2021

View reviewed changes

crisbeto force-pushed the safe-array-access branch from 402f75b to 49bc65d Compare May 16, 2021 10:50

crisbeto force-pushed the safe-array-access branch from 49bc65d to 0bc64da Compare May 21, 2021 18:53

crisbeto force-pushed the safe-array-access branch from 0bc64da to e98ad89 Compare May 21, 2021 19:47

alxhub reviewed May 21, 2021

View reviewed changes

alxhub requested changes May 21, 2021

View reviewed changes

packages/compiler/test/expression_parser/parser_spec.ts Outdated Show resolved Hide resolved

packages/compiler/src/expression_parser/ast.ts Outdated Show resolved Hide resolved

crisbeto added 4 commits May 23, 2021 09:20

fixup! feat(compiler): support safe keyed read expressions

413bb6e

fixup! feat(compiler): support safe keyed read expressions

a9ae5fb

fixup! feat(compiler): support safe keyed read expressions

89bb80e

crisbeto commented May 23, 2021

View reviewed changes

crisbeto force-pushed the safe-array-access branch from e98ad89 to 89bb80e Compare May 23, 2021 07:48

AndrewKushnir mentioned this pull request May 25, 2021

Support safe function calls in templates #42298

Closed

AndrewKushnir requested a review from alxhub May 26, 2021 21:10

AndrewKushnir removed the action: merge The PR is ready for merge by the caretaker label May 26, 2021

crisbeto mentioned this pull request Jun 1, 2021

feat(compiler): add support for shorthand property declarations in templates #42421

Closed

alxhub approved these changes Jun 1, 2021

View reviewed changes

crisbeto added the action: merge The PR is ready for merge by the caretaker label Jun 2, 2021

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jun 2, 2021

AndrewKushnir requested review from atscott and removed request for mhevery June 2, 2021 20:29

atscott approved these changes Jun 2, 2021

View reviewed changes

AndrewKushnir removed the request for review from kyliau June 3, 2021 02:19

jessicajaniuk closed this in ba08485 Jun 3, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 4, 2021

Conversation

crisbeto commented May 1, 2021

Uh oh!

AndrewKushnir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JoostK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

crisbeto commented May 5, 2021

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir commented May 13, 2021

Uh oh!

crisbeto commented May 13, 2021

Uh oh!

JoostK commented May 13, 2021

Uh oh!

crisbeto commented May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoostK commented May 13, 2021

Uh oh!

tehpsalmist commented May 20, 2021

Uh oh!

crisbeto commented May 21, 2021

Uh oh!

crisbeto commented May 21, 2021

Uh oh!

petebacondarwin commented May 21, 2021

Uh oh!

crisbeto commented May 21, 2021

Uh oh!

alxhub May 21, 2021

Choose a reason for hiding this comment

Uh oh!

crisbeto May 23, 2021

Choose a reason for hiding this comment

Uh oh!

alxhub Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

crisbeto May 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir commented May 26, 2021

Uh oh!

alxhub Jun 1, 2021

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir commented Jun 2, 2021

Uh oh!

AndrewKushnir commented Jun 2, 2021

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Jul 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

crisbeto commented May 13, 2021 •

edited

Loading