fix(compiler-cli): dom property binding check in signal extended diagnostic#54324
fix(compiler-cli): dom property binding check in signal extended diagnostic#54324cexbrayat wants to merge 1 commit intoangular:mainfrom
Conversation
e3fdd9a to
3b828db
Compare
There was a problem hiding this comment.
Can we have a test to ensure this doesn't throw an error is id is a model() ?
There was a problem hiding this comment.
For now, it does throw if id is a model, in addition to the compiler error that will complain about the incompatible type between WritableSignal<number> and number, see authoring_spec.ts below.
This is a draft PR that we'll adjust, it's just to start a discussion after what we discussed with @devversion
We're not yet sure if this is a good idea
There was a problem hiding this comment.
Sounds like this is fine? I do see the problem where an input accepts a Signal reference. We are breaking these cases, while arguably one should never pass around a signal, "aside" from using two-way binding syntax.
There was a problem hiding this comment.
I remember that we didn't want to do it initially (see #49660) as there a legitimate of passing signals. model() inputs are one of them.
There was a problem hiding this comment.
With recent changes of the AST, we should be able to skip two way data bindings I think. With this approach we are just trying to solve this in the intermediary until we can rely on proper DOM type checking (or a combination of both checks)
3b828db to
97801dd
Compare
97801dd to
68d831e
Compare
There was a problem hiding this comment.
Sounds like this is fine? I do see the problem where an input accepts a Signal reference. We are breaking these cases, while arguably one should never pass around a signal, "aside" from using two-way binding syntax.
There was a problem hiding this comment.
I believe the migration could check the target of the expression, so that we can skip property bindings to inputs?
There was a problem hiding this comment.
Do you know what should we check to see that? The binding type doesn't allow us to know as far as I can see.
There was a problem hiding this comment.
IIRC there is a getExpressionTarget method
There was a problem hiding this comment.
I think I've got something working using the symbol of the node and checking if it is an input. I amended the PR if you want to take a look.
34c641f to
f870e46
Compare
devversion
left a comment
There was a problem hiding this comment.
Awesome work! Thanks @cexbrayat
This comment was marked as outdated.
This comment was marked as outdated.
|
FYI: This seems to break some internal targets: |
|
@devversion Is this something you're looking into? |
|
@cexbrayat I won't have time currently, but can help providing as much details as possible. My guess is that the symbol retrieval is triggered on some unexpected AST node. Will have a look at the templates |
|
@devversion Did you have time to take a look at the templates which broke the internal targets? |
|
@cexbrayat The template looks pretty simple and fine. All inputs are targeting signal inputs— so that's good. At least the error is somewhat "reasonable". Can you print what type of node we get in https://github.com/angular/angular/blob/main/packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts#L796-L803? Maybe using |
f870e46 to
febf14a
Compare
|
@devversion I pushed a new commit with a log, let me know if that helps |
|
@cexbrayat Thanks. Hmm. This didn't help a lot: It sounds like some input in the type check block code is directly accessed. in form of. e.g. I should probably debug this further, but sharing some early information here, as much as I can. |
|
I think the assumption of the receiver being e.g. a property access is simply not correct. There are cases, e.g. with |
|
@cexbrayat sent a fix. Note that I think that relying on |
… input with restricted access Currently when attempting to retrieve a TCB symbol for an input binding that refers to a signal input with e.g. `protected`, while the `honorAccessModifiersForInputBindings` flag is `false`, Angular will throw a runtime exception because the symbol retrieval code always expects a proper field access in the TCB. This is not the case with `honorAccessModifiersForInputBindings = false`, as TCB will allocate a temporary variable when ignoring the field access. This will then trigger the runtime exception (which we added to flag such "unexpected" cases). This commit handles it gracefully, as it's valid TCB, but we simply cannot generate a proper TCB symbol (yet). This is similar to `@Input` decorator inputs. In the future we may implement logic to build up TCB symbols for non-property access bindings, for both signal inputs or `@Input` inputs. This commit just avoids a build exception. Related to: angular#54324.
… input with restricted access (#55774) Currently when attempting to retrieve a TCB symbol for an input binding that refers to a signal input with e.g. `protected`, while the `honorAccessModifiersForInputBindings` flag is `false`, Angular will throw a runtime exception because the symbol retrieval code always expects a proper field access in the TCB. This is not the case with `honorAccessModifiersForInputBindings = false`, as TCB will allocate a temporary variable when ignoring the field access. This will then trigger the runtime exception (which we added to flag such "unexpected" cases). This commit handles it gracefully, as it's valid TCB, but we simply cannot generate a proper TCB symbol (yet). This is similar to `@Input` decorator inputs. In the future we may implement logic to build up TCB symbols for non-property access bindings, for both signal inputs or `@Input` inputs. This commit just avoids a build exception. Related to: #54324. PR Close #55774
… input with restricted access (#55774) Currently when attempting to retrieve a TCB symbol for an input binding that refers to a signal input with e.g. `protected`, while the `honorAccessModifiersForInputBindings` flag is `false`, Angular will throw a runtime exception because the symbol retrieval code always expects a proper field access in the TCB. This is not the case with `honorAccessModifiersForInputBindings = false`, as TCB will allocate a temporary variable when ignoring the field access. This will then trigger the runtime exception (which we added to flag such "unexpected" cases). This commit handles it gracefully, as it's valid TCB, but we simply cannot generate a proper TCB symbol (yet). This is similar to `@Input` decorator inputs. In the future we may implement logic to build up TCB symbols for non-property access bindings, for both signal inputs or `@Input` inputs. This commit just avoids a build exception. Related to: #54324. PR Close #55774
febf14a to
7b66fad
Compare
…nostic The compiler now checks if a signal is properly called on dom property bindings. The ideal solution would be for the compiler to check if dom property bindings in general are properly typed, but this is currently not the case, and it is a bigger task to land this change. In the meantime, the signal diagnostic is augmented to catch cases like the following: ``` <div [id]="mySignal"></div> ```
7b66fad to
d93ae67
Compare
|
@devversion I rebased on main now that your PR has been merged |
|
@cexbrayat Thx. I'm running a presubmit and TGP now. |
|
This PR was merged into the repository by commit 237eaca. |
…nostic (#54324) The compiler now checks if a signal is properly called on dom property bindings. The ideal solution would be for the compiler to check if dom property bindings in general are properly typed, but this is currently not the case, and it is a bigger task to land this change. In the meantime, the signal diagnostic is augmented to catch cases like the following: ``` <div [id]="mySignal"></div> ``` PR Close #54324
|
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. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, we have no warning when forgetting to call a signal in dom property binding
What is the new behavior?
The compiler now checks if a signal is properly called on dom property bindings. The ideal solution would be for the compiler to check if dom property bindings in general are properly typed, but this is currently not the case, and it is a bigger task to land this change (see #54098).
In the meantime, we can maybe use the extended diagnostic for interpolated signals to catch these issues.
Obviously the name of the diagnostic is no longer accurate, but this PR is to start a discussion.
Does this PR introduce a breaking change?
Other information