fix(compiler): incorrectly interpreting some HostBinding names #40233

crisbeto · 2020-12-22T11:15:01Z

Currently when analyzing the metadata of a directive, we bundle together the bindings from host and the HostBinding and HostListener together. This can become a problem later on in the compilation pipeline, because we try to evaluate the value of the binding, causing something like @HostBinding('class.foo') public true = 1; to be treated the same as host: {'[class.foo]': 'true'}.

While looking into the issue, I noticed another one that is closely related: we weren't treating quoted property names correctly. E.g. @HostBinding('class.foo') public "foo-bar" = 1; was being interpreted as classProp('foo', ctx.foo - ctx.bar) due to the same issue where property names were being evaluated.

These changes resolve both of the issues by treating all HostBinding instances as if they're reading the property from this. E.g. the @HostBinding('class.foo') public true = 1; from above is now being treated as host: {'[class.foo]': 'this.true'} which further down the pipeline becomes classProp('foo', ctx.true). This doesn't have any payload size implications for existing code, because we've always been prefixing implicit property reads with ctx.. If the property doesn't have an identifier that can be read using dotted access, we convert it to a quoted one (e.g. classProp('foo', ctx['is-foo'])).

Fixes #40220.
Fixes #40230.
Fixes #18698.

crisbeto · 2020-12-22T11:58:38Z

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

+          // through `this` so that further down the line it can't be confused for a literal value
+          // (e.g. if there's a property called `true`). There is no size penalty, because all
+          // values (except literals) are converted to `ctx.propName` eventually.
+          bindings.properties[hostPropertyName] = getSafePropertyAccessString('this', member.name);


Note for reviewer: I was conflicted between doing this (converting all accesses to go through this) versus changing the extracted information from Record<string, string> to Record<string, {name: string; canBePrimitive: boolean}> and then changing the places where we evaluate the expression to account for the canBePrimitive flag. I decided against it, because it seemed more error-prone. I'm open towards changing it, if you think that the this access is problematic.

I personally feel that we should avoid this. I personally would prefer something more akin to what you mentioned with Record, but I can be convinced otherwise.

I would also prefer if we could avoid this this here. It would probably be better if we were to build an output AST expression instead of synthesizing an expression string, so that we forego the dependency on the binding parser. But I recognize that doing so looks to be easier said than done, as the binding parser implements additional logic that is still relevant for such bindings.

Yeah, my concern is that it'll involve adding a bunch of random IFs everywhere just to handle this one edge case, whereas prefixing it with this. reflects what actually happens at runtime. I was looking to change the logic in BindingParser.parsePropertyBinding and Parser.parseSimpleBinding so that they always return a PropertyRead, but getting these classes to know when this should happen would involve adding that extra flag to the extracted information and the data format appears to be used by VE to generate metadata.json files as well.

It's worth noting this isn't the first place where we're constructing the expression as a string like this. If you look a bit further down in the same file, the way we handle @HostListener is by creating an expression like ${member.name}(${args.join(',')}) and then evaluating it.

As for building an output AST: I considered doing it, but then I'd also have to bring in an AbstractEmitterVisitor which seemed like it would be wasteful, considering that we'd be doing it for each @HostBinding and it eventually hits the same escapeIdentifier function that I'm using inside getSafePropertyAccessString.

I actually think that using this here is very reasonable.

@JoostK and @jessicajaniuk - you say you prefer not to use it, but did not give the reason for why it is bad.

The only concern I can think of is that we are manually constructing a string expression, but as @crisbeto points out there is already a precedent for this. I feel that this. actually states quite clearly what we are trying to achieve here, whereas a more complex approach avoid this string expression would be harder to comprehend and maintain IMO.

It's not that I don't like the this, it's the creation of an expression string that I'd rather avoid. But I do recognize the difficulty of avoiding it at the moment, so I'd be okay with keeping it as is.

For me, this just adds a lot of confusion to follow which this scope I may be in at any given time. I'm fine with leaving it as is here given that it seems like any alternative would be way more complicated.

packages/compiler/src/render3/util.ts

jessicajaniuk · 2020-12-22T21:25:06Z

.../compliance/test_cases/r3_view_compiler_bindings/host_bindings/host_bindings_quoted_names.ts

+
+@Directive({selector: '[hostBindingDir]'})
+export class HostBindingDir {
+  @HostBinding('class.a') 'is-a': any;


Is there any way around the any usage here. Can we make it unknown instead.

I figured that it doesn't really matter since it's a test to verify the compiler output. I'll type it better when addressing the rest of the feedback.

jessicajaniuk · 2020-12-22T21:25:38Z

.../compliance/test_cases/r3_view_compiler_bindings/host_bindings/host_bindings_quoted_names.js

@@ -0,0 +1,11 @@
+


(nit) There's a leading space here.

jessicajaniuk · 2020-12-22T21:26:12Z

...mpliance/test_cases/r3_view_compiler_bindings/host_bindings/host_bindings_primitive_names.ts

+  }
+})
+export class HostBindingDir {
+  @HostBinding('class.c') true: any;


It would be great if we can avoid adding additional any types. Can unknown work here, instead?

jessicajaniuk · 2020-12-22T21:28:19Z

packages/compiler-cli/src/ngtsc/annotations/src/directive.ts

+          // through `this` so that further down the line it can't be confused for a literal value
+          // (e.g. if there's a property called `true`). There is no size penalty, because all
+          // values (except literals) are converted to `ctx.propName` eventually.
+          bindings.properties[hostPropertyName] = getSafePropertyAccessString('this', member.name);


I personally feel that we should avoid this. I personally would prefer something more akin to what you mentioned with Record, but I can be convinced otherwise.

petebacondarwin · 2020-12-31T14:51:05Z

@crisbeto - can you mark that this PR will fix #18698 too?

Currently when analyzing the metadata of a directive, we bundle together the bindings from `host` and the `HostBinding` and `HostListener` together. This can become a problem later on in the compilation pipeline, because we try to evaluate the value of the binding, causing something like `@HostBinding('class.foo') public true = 1;` to be treated the same as `host: {'[class.foo]': 'true'}`. While looking into the issue, I noticed another one that is closely related: we weren't treating quoted property names correctly. E.g. `@HostBinding('class.foo') public "foo-bar" = 1;` was being interpreted as `classProp('foo', ctx.foo - ctx.bar)` due to the same issue where property names were being evaluated. These changes resolve both of the issues by treating all `HostBinding` instance as if they're reading the property from `this`. E.g. the `@HostBinding('class.foo') public true = 1;` from above is now being treated as `host: {'[class.foo]': 'this.true'}` which further down the pipeline becomes `classProp('foo', ctx.true)`. This doesn't have any payload size implications for existing code, because we've always been prefixing implicit property reads with `ctx.`. If the property doesn't have an identifier that can be read using dotted access, we convert it to a quoted one (e.g. `classProp('foo', ctx['is-foo']))`. Fixes angular#40220. Fixes angular#40230. Fixes angular#18698.

AndrewKushnir · 2021-01-07T02:23:05Z

Presubmit.

(note: fw-core approval is still needed, re-requested review from Jessica)

atscott · 2021-01-07T21:15:19Z

Hi @crisbeto, I changed this to minor only because there is a merge conflict with the 11.0.x branch.

angular-automatic-lock-bot · 2021-02-07T16:34:01Z

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 Dec 22, 2020

crisbeto force-pushed the 40220-compiler-host-binding-names branch 2 times, most recently from c0b0c1f to 2b1812a Compare December 22, 2020 11:54

crisbeto commented Dec 22, 2020

View reviewed changes

crisbeto force-pushed the 40220-compiler-host-binding-names branch from 2b1812a to ff5e46f Compare December 22, 2020 12:05

crisbeto marked this pull request as ready for review December 22, 2020 13:01

pullapprove bot requested review from jessicajaniuk and JoostK December 22, 2020 13:01

pullapprove bot added area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime labels Dec 22, 2020

ngbot bot modified the milestone: Backlog Dec 22, 2020

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 type: bug/fix labels Dec 22, 2020

jessicajaniuk reviewed Dec 22, 2020

View reviewed changes

crisbeto force-pushed the 40220-compiler-host-binding-names branch from ff5e46f to b4ebec6 Compare December 29, 2020 12:43

JoostK mentioned this pull request Dec 31, 2020

fix(compiler): support host binding when property name being keyword #18801

Closed

3 tasks

crisbeto force-pushed the 40220-compiler-host-binding-names branch from b4ebec6 to f7bdfce Compare December 31, 2020 16:32

JoostK approved these changes Jan 6, 2021

View reviewed changes

crisbeto force-pushed the 40220-compiler-host-binding-names branch from f7bdfce to a49b361 Compare January 6, 2021 20:36

crisbeto added action: merge The PR is ready for merge by the caretaker action: presubmit The PR is in need of a google3 presubmit and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 6, 2021

AndrewKushnir requested a review from jessicajaniuk January 7, 2021 02:24

jessicajaniuk approved these changes Jan 7, 2021

View reviewed changes

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

atscott added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jan 7, 2021

atscott closed this in 1045465 Jan 7, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler): incorrectly interpreting some HostBinding names #40233

fix(compiler): incorrectly interpreting some HostBinding names #40233

crisbeto commented Dec 22, 2020 •

edited

crisbeto Dec 22, 2020 •

edited

jessicajaniuk Dec 22, 2020

JoostK Dec 22, 2020

crisbeto Dec 23, 2020

petebacondarwin Jan 6, 2021

JoostK Jan 6, 2021

jessicajaniuk Jan 7, 2021

jessicajaniuk Dec 22, 2020

crisbeto Dec 23, 2020

jessicajaniuk Dec 22, 2020

jessicajaniuk Dec 22, 2020

jessicajaniuk Dec 22, 2020

petebacondarwin commented Dec 31, 2020

AndrewKushnir commented Jan 7, 2021 •

edited

atscott commented Jan 7, 2021

angular-automatic-lock-bot bot commented Feb 7, 2021

fix(compiler): incorrectly interpreting some HostBinding names #40233

fix(compiler): incorrectly interpreting some HostBinding names #40233

Conversation

crisbeto commented Dec 22, 2020 • edited

crisbeto Dec 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Dec 31, 2020

AndrewKushnir commented Jan 7, 2021 • edited

atscott commented Jan 7, 2021

angular-automatic-lock-bot bot commented Feb 7, 2021

crisbeto commented Dec 22, 2020 •

edited

crisbeto Dec 22, 2020 •

edited

AndrewKushnir commented Jan 7, 2021 •

edited