Allow two-way bindings to signal-based template variables by crisbeto · Pull Request #54714 · angular/angular

crisbeto · 2024-03-06T11:00:31Z

Includes the following changes whose goal is to properly support two-way bindings to signal-based local variables:

refactor(compiler-cli): introduce template semantics checker

Introduces a new TemplateSemanticsChecker that will be used to flag semantic errors in the user's template. Currently we do some of this in the type check block, but the problem is that it doesn't have access to the template type checker which prevents us from properly checking cases like #54670. This pass is also distinct from the extended template checks, because we don't want users to be able to turn the checks off and we want them to run even if strictTemplates are disabled.

refactor(compiler-cli): move signal identification function

Moves the function that identifies signals into a separate file so that it can be reused outside of extended diagnostics.

refactor(compiler-cli): move illegal template assignment check into template semantics checker

Moves the check which ensures that there are no writes to template variables into the TemplateSemanticsChecker to prepare for the upcoming changes.

fix(compiler-cli): flag two-way bindings to non-signal values in templates

We have a diagnostic that reports writes to template variables which worked both for regular event bindings and two-way bindings, however the latter was broken by #54154 because two-way bindings no longer had a PropertyWrite AST.

These changes fix the diagnostic and expand it to allow two-way bindings to template variables that are signals.

fix(compiler): handle two-way bindings to signal-based template variables in instruction generation

Updates the instruction generation for two-way bindings to only emit the twoWayBindingSet call when writing to template variables. Since template variables are constants, it's only allowed to write to them when they're signals. Non-signal values are flagged during template type checking.

Fixes #54670.

devversion

LGTM overall, but a couple of comments

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts

packages/compiler-cli/src/ngtsc/typecheck/template_semantics/src/template_semantics_checker.ts

devversion · 2024-03-07T16:42:43Z

packages/compiler-cli/src/ngtsc/typecheck/template_semantics/src/template_semantics_checker.ts

Should we always error if symbol === null and we don't know, except that it's a template variable?

I was a bit concerned that there may be cases where we don't have full type information and we would raise the error unnecessarily.

Interesting. maybe worth trying in g3?

Sure, will run a TGP now.

Okay so even without the TGP it appears to be problematic. It was flagging one of our own TestBed-based tests, because it can't resolve the symbol there. I'll revert it for now and we can always revisit it later.

Sounds good

devversion · 2024-03-07T16:46:02Z

packages/compiler/src/compiler_util/expression_converter.ts

Is ReadVarExpr always a constant? I see it as a variable, not a constant? This seems like a large assumption from the resolve_names phase?

o.variable() creates such AST object

It can be either a let or const. Either way we need to not re-assign it (which is what this change is doing), but just pass it to the twoWayBindingSet instruction.

But isn't this a huge assumption? A variable is technically assignable right?

We are just assuming it to be a constant template variable because the compiler converts the LexicalReadExpr into a ReadVarExpr

I might miss something

Template variables have always been read-only, we even have a diagnostic for it. That's why I think it's safe to assume that it would be a constant here as well.

Oh yeah, I think that is true. I guess my concern is that we are assuming that ReadVarExpr indicates a template variable?

Even if it's not a template variable, I think we'd want to cover it with this check. E.g. I believe that $event would also be a ReadVarExpr which shouldn't be writable either.

Right, but ReadVarExpr could technically also be some arbitrary var/let variable (I know we don't generate this right now— but it could be)

Based on our discussion over DM: I think that this is a safe assumption to make at the moment. I've added added a comment stating that it's an assumption. Will continue as is for now.

pkozlowski-opensource · 2024-03-07T16:55:21Z

packages/core/test/acceptance/authoring/model_inputs_spec.ts

I would love to see a test illustrating how it behaves with non-signal items in a collection.

This is also an interesting consideration of how do we see items produced by a loop - are those assignable or not (signals are, obviously)

So a non-signal value would basically be a no-op at runtime and we have a compiler diagnostic to flag it at compile time.

crisbeto · 2024-03-08T12:26:48Z

Passing TGP

Introduces a new `TemplateSemanticsChecker` that will be used to flag semantic errors in the user's template. Currently we do some of this in the type check block, but the problem is that it doesn't have access to the template type checker which prevents us from properly checking cases like angular#54670. This pass is also distinct from the extended template checks, because we don't want users to be able to turn the checks off and we want them to run even if `strictTemplates` are disabled.

Moves the function that identifies signals into a separate file so that it can be reused outside of extended diagnostics.

…emplate semantics checker Moves the check which ensures that there are no writes to template variables into the `TemplateSemanticsChecker` to prepare for the upcoming changes.

…lates We have a diagnostic that reports writes to template variables which worked both for regular event bindings and two-way bindings, however the latter was broken by angular#54154 because two-way bindings no longer had a `PropertyWrite` AST. These changes fix the diagnostic and expand it to allow two-way bindings to template variables that are signals.

…bles in instruction generation Updates the instruction generation for two-way bindings to only emit the `twoWayBindingSet` call when writing to template variables. Since template variables are constants, it's only allowed to write to them when they're signals. Non-signal values are flagged during template type checking. Fixes angular#54670.

atscott · 2024-03-11T18:01:48Z

This PR was merged into the repository by commit 5ae2bf4.

Introduces a new `TemplateSemanticsChecker` that will be used to flag semantic errors in the user's template. Currently we do some of this in the type check block, but the problem is that it doesn't have access to the template type checker which prevents us from properly checking cases like #54670. This pass is also distinct from the extended template checks, because we don't want users to be able to turn the checks off and we want them to run even if `strictTemplates` are disabled. PR Close #54714

Moves the function that identifies signals into a separate file so that it can be reused outside of extended diagnostics. PR Close #54714

…emplate semantics checker (#54714) Moves the check which ensures that there are no writes to template variables into the `TemplateSemanticsChecker` to prepare for the upcoming changes. PR Close #54714

…lates (#54714) We have a diagnostic that reports writes to template variables which worked both for regular event bindings and two-way bindings, however the latter was broken by #54154 because two-way bindings no longer had a `PropertyWrite` AST. These changes fix the diagnostic and expand it to allow two-way bindings to template variables that are signals. PR Close #54714

…bles in instruction generation (#54714) Updates the instruction generation for two-way bindings to only emit the `twoWayBindingSet` call when writing to template variables. Since template variables are constants, it's only allowed to write to them when they're signals. Non-signal values are flagged during template type checking. Fixes #54670. PR Close #54714

Moves the function that identifies signals into a separate file so that it can be reused outside of extended diagnostics. PR Close #54714

…emplate semantics checker (#54714) Moves the check which ensures that there are no writes to template variables into the `TemplateSemanticsChecker` to prepare for the upcoming changes. PR Close #54714

…lates (#54714) We have a diagnostic that reports writes to template variables which worked both for regular event bindings and two-way bindings, however the latter was broken by #54154 because two-way bindings no longer had a `PropertyWrite` AST. These changes fix the diagnostic and expand it to allow two-way bindings to template variables that are signals. PR Close #54714

…bles in instruction generation (#54714) Updates the instruction generation for two-way bindings to only emit the `twoWayBindingSet` call when writing to template variables. Since template variables are constants, it's only allowed to write to them when they're signals. Non-signal values are flagged during template type checking. Fixes #54670. PR Close #54714

angular-automatic-lock-bot · 2024-04-11T00:09:31Z

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

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

ngbot bot added this to the Backlog milestone Mar 6, 2024

crisbeto force-pushed the two-way-signal-in-loop branch 3 times, most recently from 536b36a to 945567a Compare March 6, 2024 12:18

crisbeto requested a review from devversion March 6, 2024 12:55

crisbeto marked this pull request as ready for review March 6, 2024 12:55

devversion approved these changes Mar 7, 2024

View reviewed changes

pkozlowski-opensource reviewed Mar 7, 2024

View reviewed changes

crisbeto force-pushed the two-way-signal-in-loop branch 2 times, most recently from dcb134c to 1626677 Compare March 8, 2024 11:58

crisbeto force-pushed the two-way-signal-in-loop branch from 1626677 to 434f463 Compare March 8, 2024 12:40

devversion approved these changes Mar 8, 2024

View reviewed changes

crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 8, 2024

crisbeto force-pushed the two-way-signal-in-loop branch from 434f463 to c148219 Compare March 9, 2024 08:27

crisbeto added 5 commits March 11, 2024 15:52

refactor(compiler-cli): move signal identification function

04588bc

Moves the function that identifies signals into a separate file so that it can be reused outside of extended diagnostics.

refactor(compiler-cli): move illegal template assignment check into t…

37e7b58

…emplate semantics checker Moves the check which ensures that there are no writes to template variables into the `TemplateSemanticsChecker` to prepare for the upcoming changes.

crisbeto force-pushed the two-way-signal-in-loop branch from c148219 to 9c82ba9 Compare March 11, 2024 14:52

crisbeto added target: rc This PR is targeted for the next release-candidate and removed target: patch This PR is targeted for the next patch release labels Mar 11, 2024

atscott pushed a commit that referenced this pull request Mar 11, 2024

refactor(compiler-cli): move signal identification function (#54714)

5d23e60

Moves the function that identifies signals into a separate file so that it can be reused outside of extended diagnostics. PR Close #54714

atscott closed this in f86088f Mar 11, 2024

atscott pushed a commit that referenced this pull request Mar 11, 2024

refactor(compiler-cli): move signal identification function (#54714)

6235095

Moves the function that identifies signals into a separate file so that it can be reused outside of extended diagnostics. PR Close #54714

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 11, 2024

Conversation

crisbeto commented Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

refactor(compiler-cli): introduce template semantics checker

refactor(compiler-cli): move signal identification function

refactor(compiler-cli): move illegal template assignment check into template semantics checker

fix(compiler-cli): flag two-way bindings to non-signal values in templates

fix(compiler): handle two-way bindings to signal-based template variables in instruction generation

Uh oh!

devversion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Mar 8, 2024

Uh oh!

atscott commented Mar 11, 2024

Uh oh!

angular-automatic-lock-bot bot commented Apr 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

crisbeto commented Mar 6, 2024 •

edited

Loading

crisbeto Mar 8, 2024 •

edited

Loading

devversion Mar 8, 2024 •

edited

Loading