-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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.
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
Allow two-way bindings to signal-based template variables #54714
Conversation
536b36a
to
945567a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, but a couple of comments
packages/compiler-cli/src/ngtsc/typecheck/template_semantics/src/template_semantics_checker.ts
Show resolved
Hide resolved
|
||
// Two-way bindings to template variables are only allowed if the variables are signals. | ||
const symbol = this.templateTypeChecker.getSymbolOfNode(target, this.component); | ||
if (symbol !== null && !isSignalReference(symbol)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we always error if symbol === null
and we don't know, except that it's a template variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. maybe worth trying in g3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will run a TGP now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
// If the expression is targeting a variable read, we only emit the `twoWayBindingSet` since | ||
// the fallback would be attempting to write into a constant. Invalid usages will be flagged | ||
// during template type checking. | ||
if (ast instanceof o.ReadVarExpr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, I think that is true. I guess my concern is that we are assuming that ReadVarExpr
indicates a template variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -589,4 +589,44 @@ describe('model inputs', () => { | |||
fixture.detectChanges(); | |||
expect(() => fixture.destroy()).not.toThrow(); | |||
}); | |||
|
|||
it('should support two-way binding to a signal @for loop variable', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
dcb134c
to
1626677
Compare
1626677
to
434f463
Compare
434f463
to
c148219
Compare
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.
c148219
to
9c82ba9
Compare
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
…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
…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
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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 ifstrictTemplates
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.