Skip to content

Commit

Permalink
fix(compiler): incorrectly interpreting some HostBinding names (#40233)
Browse files Browse the repository at this point in the history
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 #40220.
Fixes #40230.
Fixes #18698.

PR Close #40233
  • Loading branch information
crisbeto authored and atscott committed Jan 7, 2021
1 parent 266cc9b commit 1045465
Show file tree
Hide file tree
Showing 14 changed files with 272 additions and 17 deletions.
8 changes: 6 additions & 2 deletions packages/compiler-cli/src/ngtsc/annotations/src/directive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* found in the LICENSE file at https://angular.io/license
*/

import {compileDeclareDirectiveFromMetadata, compileDirectiveFromMetadata, ConstantPool, Expression, ExternalExpr, Identifiers, makeBindingParser, ParsedHostBindings, ParseError, parseHostBindings, R3DependencyMetadata, R3DirectiveDef, R3DirectiveMetadata, R3FactoryTarget, R3QueryMetadata, R3ResolvedDependencyType, Statement, verifyHostBindings, WrappedNodeExpr} from '@angular/compiler';
import {compileDeclareDirectiveFromMetadata, compileDirectiveFromMetadata, ConstantPool, Expression, ExternalExpr, getSafePropertyAccessString, Identifiers, makeBindingParser, ParsedHostBindings, ParseError, parseHostBindings, R3DependencyMetadata, R3DirectiveDef, R3DirectiveMetadata, R3FactoryTarget, R3QueryMetadata, R3ResolvedDependencyType, Statement, verifyHostBindings, WrappedNodeExpr} from '@angular/compiler';
import * as ts from 'typescript';

import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
Expand Down Expand Up @@ -738,7 +738,11 @@ export function extractHostBindings(
hostPropertyName = resolved;
}

bindings.properties[hostPropertyName] = member.name;
// Since this is a decorator, we know that the value is a class member. Always access it
// 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);
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export class HostBindingDir {
}
}
HostBindingDir.ɵfac = function HostBindingDir_Factory(t) { return new (t || HostBindingDir)(); };
HostBindingDir.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: HostBindingDir, selector: "[hostBindingDir]", host: { properties: { "id": "dirId" } }, ngImport: i0 });
HostBindingDir.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: HostBindingDir, selector: "[hostBindingDir]", host: { properties: { "id": "this.dirId" } }, ngImport: i0 });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(HostBindingDir, [{
type: Directive,
args: [{ selector: '[hostBindingDir]' }]
Expand Down Expand Up @@ -298,7 +298,7 @@ export class MyDirective {
}
}
MyDirective.ɵfac = function MyDirective_Factory(t) { return new (t || MyDirective)(); };
MyDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: MyDirective, selector: "[my-dir]", host: { properties: { "tabindex": "1", "title": "myTitle", "id": "myId" } }, ngImport: i0 });
MyDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: MyDirective, selector: "[my-dir]", host: { properties: { "tabindex": "1", "title": "this.myTitle", "id": "this.myId" } }, ngImport: i0 });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(MyDirective, [{
type: Directive,
args: [{ selector: '[my-dir]', host: { '[tabindex]': '1' } }]
Expand Down Expand Up @@ -423,7 +423,7 @@ export class MyDirective {
}
}
MyDirective.ɵfac = function MyDirective_Factory(t) { return new (t || MyDirective)(); };
MyDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: MyDirective, selector: "[my-dir]", host: { properties: { "attr.tabindex": "1", "attr.title": "myTitle", "attr.id": "myId" } }, ngImport: i0 });
MyDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: MyDirective, selector: "[my-dir]", host: { properties: { "attr.tabindex": "1", "attr.title": "this.myTitle", "attr.id": "this.myId" } }, ngImport: i0 });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(MyDirective, [{
type: Directive,
args: [{ selector: '[my-dir]', host: { '[attr.tabindex]': '1' } }]
Expand Down Expand Up @@ -589,3 +589,105 @@ export declare class MyComponent {
static ɵcmp: i0.ɵɵComponentDefWithMeta<MyComponent, "my-comp", never, {}, {}, never, never>;
}

/****************************************************************************************************
* PARTIAL FILE: host_bindings_primitive_names.js
****************************************************************************************************/
import { Directive, HostBinding, NgModule } from '@angular/core';
import * as i0 from "@angular/core";
export class HostBindingDir {
}
HostBindingDir.ɵfac = function HostBindingDir_Factory(t) { return new (t || HostBindingDir)(); };
HostBindingDir.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: HostBindingDir, selector: "[hostBindingDir]", host: { properties: { "class.a": "true", "class.b": "false", "class.c": "this.true", "class.d": "this.false", "class.e": "this.other" } }, ngImport: i0 });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(HostBindingDir, [{
type: Directive,
args: [{
selector: '[hostBindingDir]',
host: {
'[class.a]': 'true',
'[class.b]': 'false',
}
}]
}], null, { true: [{
type: HostBinding,
args: ['class.c']
}], false: [{
type: HostBinding,
args: ['class.d']
}], other: [{
type: HostBinding,
args: ['class.e']
}] }); })();
export class MyModule {
}
MyModule.ɵmod = i0.ɵɵdefineNgModule({ type: MyModule });
MyModule.ɵinj = i0.ɵɵdefineInjector({ factory: function MyModule_Factory(t) { return new (t || MyModule)(); } });
(function () { (typeof ngJitMode === "undefined" || ngJitMode) && i0.ɵɵsetNgModuleScope(MyModule, { declarations: [HostBindingDir] }); })();
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(MyModule, [{
type: NgModule,
args: [{ declarations: [HostBindingDir] }]
}], null, null); })();

/****************************************************************************************************
* PARTIAL FILE: host_bindings_primitive_names.d.ts
****************************************************************************************************/
import * as i0 from "@angular/core";
export declare class HostBindingDir {
true: any;
false: any;
other: any;
static ɵfac: i0.ɵɵFactoryDef<HostBindingDir, never>;
static ɵdir: i0.ɵɵDirectiveDefWithMeta<HostBindingDir, "[hostBindingDir]", never, {}, {}, never>;
}
export declare class MyModule {
static ɵmod: i0.ɵɵNgModuleDefWithMeta<MyModule, [typeof HostBindingDir], never, never>;
static ɵinj: i0.ɵɵInjectorDef<MyModule>;
}

/****************************************************************************************************
* PARTIAL FILE: host_bindings_quoted_names.js
****************************************************************************************************/
import { Directive, HostBinding, NgModule } from '@angular/core';
import * as i0 from "@angular/core";
export class HostBindingDir {
}
HostBindingDir.ɵfac = function HostBindingDir_Factory(t) { return new (t || HostBindingDir)(); };
HostBindingDir.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: HostBindingDir, selector: "[hostBindingDir]", host: { properties: { "class.a": "this['is-a']", "class.b": "this['is-\"b\"']", "class.c": "this['\"is-c\"']" } }, ngImport: i0 });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(HostBindingDir, [{
type: Directive,
args: [{ selector: '[hostBindingDir]' }]
}], null, { 'is-a': [{
type: HostBinding,
args: ['class.a']
}], 'is-"b"': [{
type: HostBinding,
args: ['class.b']
}], '"is-c"': [{
type: HostBinding,
args: ['class.c']
}] }); })();
export class MyModule {
}
MyModule.ɵmod = i0.ɵɵdefineNgModule({ type: MyModule });
MyModule.ɵinj = i0.ɵɵdefineInjector({ factory: function MyModule_Factory(t) { return new (t || MyModule)(); } });
(function () { (typeof ngJitMode === "undefined" || ngJitMode) && i0.ɵɵsetNgModuleScope(MyModule, { declarations: [HostBindingDir] }); })();
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(MyModule, [{
type: NgModule,
args: [{ declarations: [HostBindingDir] }]
}], null, null); })();

/****************************************************************************************************
* PARTIAL FILE: host_bindings_quoted_names.d.ts
****************************************************************************************************/
import * as i0 from "@angular/core";
export declare class HostBindingDir {
'is-a': any;
'is-"b"': any;
'"is-c"': any;
static ɵfac: i0.ɵɵFactoryDef<HostBindingDir, never>;
static ɵdir: i0.ɵɵDirectiveDefWithMeta<HostBindingDir, "[hostBindingDir]", never, {}, {}, never>;
}
export declare class MyModule {
static ɵmod: i0.ɵɵNgModuleDefWithMeta<MyModule, [typeof HostBindingDir], never, never>;
static ɵinj: i0.ɵɵInjectorDef<MyModule>;
}

Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,34 @@
]
}
]
},
{
"description": "should handle host bindings with the same name as a primitive value",
"inputFiles": [
"host_bindings_primitive_names.ts"
],
"expectations": [
{
"failureMessage": "Invalid host binding code",
"files": [
"host_bindings_primitive_names.js"
]
}
]
},
{
"description": "should handle host bindings with quoted names",
"inputFiles": [
"host_bindings_quoted_names.ts"
],
"expectations": [
{
"failureMessage": "Invalid host binding code",
"files": [
"host_bindings_quoted_names.js"
]
}
]
}
]
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

HostBindingDir.ɵdir = $r3$.ɵɵdefineDirective({
type: HostBindingDir,
selectors: [["", "hostBindingDir", ""]],
hostVars: 10,
hostBindings: function HostBindingDir_HostBindings(rf, ctx) {
if (rf & 2) {
$r3$.ɵɵclassProp("a", true)("b", false)("c", ctx.true)("d", ctx.false)("e", ctx.other);
}
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import {Directive, HostBinding, NgModule} from '@angular/core';

@Directive({
selector: '[hostBindingDir]',
host: {
'[class.a]': 'true',
'[class.b]': 'false',
}
})
export class HostBindingDir {
@HostBinding('class.c') true: any;
@HostBinding('class.d') false: any;
@HostBinding('class.e') other: any;
}

@NgModule({declarations: [HostBindingDir]})
export class MyModule {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

HostBindingDir.ɵdir = $r3$.ɵɵdefineDirective({
type: HostBindingDir,
selectors: [["", "hostBindingDir", ""]],
hostVars: 6,
hostBindings: function HostBindingDir_HostBindings(rf, ctx) {
if (rf & 2) {
$r3$.ɵɵclassProp("a", ctx["is-a"])("b", ctx["is-\"b\""])("c", ctx["\"is-c\""]);
}
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import {Directive, HostBinding, NgModule} from '@angular/core';

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

@NgModule({declarations: [HostBindingDir]})
export class MyModule {
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export class MyComponent {
}
}
MyComponent.ɵfac = function MyComponent_Factory(t) { return new (t || MyComponent)(); };
MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", inputs: { name: "name" }, host: { attributes: { "title": "foo title" }, properties: { "style": "myStyle", "class": "myClass", "id": "id", "title": "title" }, styleAttribute: "width:200px; height:500px", classAttribute: "foo baz" }, ngImport: i0, template: { source: '', isInline: true } });
MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", inputs: { name: "name" }, host: { attributes: { "title": "foo title" }, properties: { "style": "this.myStyle", "class": "this.myClass", "id": "this.id", "title": "this.title" }, styleAttribute: "width:200px; height:500px", classAttribute: "foo baz" }, ngImport: i0, template: { source: '', isInline: true } });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(MyComponent, [{
type: Component,
args: [{
Expand Down Expand Up @@ -83,7 +83,7 @@ export class WidthDirective {
}
}
WidthDirective.ɵfac = function WidthDirective_Factory(t) { return new (t || WidthDirective)(); };
WidthDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: WidthDirective, selector: "[myWidthDir]", host: { properties: { "style.width": "myWidth", "class.foo": "myFooClass", "id": "id", "title": "title" } }, ngImport: i0 });
WidthDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: WidthDirective, selector: "[myWidthDir]", host: { properties: { "style.width": "this.myWidth", "class.foo": "this.myFooClass", "id": "this.id", "title": "this.title" } }, ngImport: i0 });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(WidthDirective, [{
type: Directive,
args: [{ selector: '[myWidthDir]' }]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ export class MyComponent {
}
}
MyComponent.ɵfac = function MyComponent_Factory(t) { return new (t || MyComponent)(); };
MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "ng-component", host: { properties: { "class.apple": "yesToApple", "style.color": "color", "class.tomato": "yesToTomato", "style.transition": "transition", "style.border": "border", "class.orange": "yesToOrange" } }, ngImport: i0, template: { source: '', isInline: true } });
MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "ng-component", host: { properties: { "class.apple": "yesToApple", "style.color": "color", "class.tomato": "yesToTomato", "style.transition": "transition", "style.border": "this.border", "class.orange": "this.yesToOrange" } }, ngImport: i0, template: { source: '', isInline: true } });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(MyComponent, [{
type: Component,
args: [{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export class MyComponent {
}
}
MyComponent.ɵfac = function MyComponent_Factory(t) { return new (t || MyComponent)(); };
MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", host: { properties: { "style": "myStyle", "class": "myClass", "style.color": "myColorProp", "class.foo": "myFooClass" }, styleAttribute: "width:200px; height:500px", classAttribute: "foo baz" }, ngImport: i0, template: { source: '', isInline: true } });
MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", host: { properties: { "style": "this.myStyle", "class": "this.myClass", "style.color": "this.myColorProp", "class.foo": "this.myFooClass" }, styleAttribute: "width:200px; height:500px", classAttribute: "foo baz" }, ngImport: i0, template: { source: '', isInline: true } });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(MyComponent, [{
type: Component,
args: [{
Expand Down Expand Up @@ -80,7 +80,7 @@ export class MyComponent {
}
}
MyComponent.ɵfac = function MyComponent_Factory(t) { return new (t || MyComponent)(); };
MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", host: { properties: { "style.height.pt": "myHeightProp", "class.bar": "myBarClass", "style": "myStyle", "style.width": "myWidthProp", "class.foo": "myFooClass", "class": "myClasses" } }, ngImport: i0, template: { source: '', isInline: true } });
MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", host: { properties: { "style.height.pt": "myHeightProp", "class.bar": "myBarClass", "style": "this.myStyle", "style.width": "this.myWidthProp", "class.foo": "this.myFooClass", "class": "this.myClasses" } }, ngImport: i0, template: { source: '', isInline: true } });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(MyComponent, [{
type: Component,
args: [{
Expand Down Expand Up @@ -149,7 +149,7 @@ export class MyComponent {
}
}
MyComponent.ɵfac = function MyComponent_Factory(t) { return new (t || MyComponent)(); };
MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", host: { properties: { "style!important": "myStyleExp", "class!important": "myClassExp", "class.foo!important": "myFooClassExp", "style.width!important": "myWidthExp" } }, ngImport: i0, template: { source: `
MyComponent.ɵcmp = i0.ɵɵngDeclareComponent({ version: "0.0.0-PLACEHOLDER", type: MyComponent, selector: "my-component", host: { properties: { "style!important": "myStyleExp", "class!important": "myClassExp", "class.foo!important": "this.myFooClassExp", "style.width!important": "this.myWidthExp" } }, ngImport: i0, template: { source: `
<div [style.height!important]="myHeightExp"
[class.bar!important]="myBarClassExp"></div>
`, isInline: true } });
Expand Down Expand Up @@ -368,7 +368,7 @@ export class ClassDirective {
}
}
ClassDirective.ɵfac = function ClassDirective_Factory(t) { return new (t || ClassDirective)(); };
ClassDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: ClassDirective, selector: "[myClassDir]", host: { properties: { "class": "myClassMap" } }, ngImport: i0 });
ClassDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: ClassDirective, selector: "[myClassDir]", host: { properties: { "class": "this.myClassMap" } }, ngImport: i0 });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(ClassDirective, [{
type: Directive,
args: [{ selector: '[myClassDir]' }]
Expand All @@ -383,7 +383,7 @@ export class WidthDirective {
}
}
WidthDirective.ɵfac = function WidthDirective_Factory(t) { return new (t || WidthDirective)(); };
WidthDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: WidthDirective, selector: "[myWidthDir]", host: { properties: { "style.width": "myWidth", "class.foo": "myFooClass" } }, ngImport: i0 });
WidthDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: WidthDirective, selector: "[myWidthDir]", host: { properties: { "style.width": "this.myWidth", "class.foo": "this.myFooClass" } }, ngImport: i0 });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(WidthDirective, [{
type: Directive,
args: [{ selector: '[myWidthDir]' }]
Expand All @@ -401,7 +401,7 @@ export class HeightDirective {
}
}
HeightDirective.ɵfac = function HeightDirective_Factory(t) { return new (t || HeightDirective)(); };
HeightDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: HeightDirective, selector: "[myHeightDir]", host: { properties: { "style.height": "myHeight", "class.bar": "myBarClass" } }, ngImport: i0 });
HeightDirective.ɵdir = i0.ɵɵngDeclareDirective({ version: "0.0.0-PLACEHOLDER", type: HeightDirective, selector: "[myHeightDir]", host: { properties: { "style.height": "this.myHeight", "class.bar": "this.myBarClass" } }, ngImport: i0 });
(function () { (typeof ngDevMode === "undefined" || ngDevMode) && i0.ɵsetClassMetadata(HeightDirective, [{
type: Directive,
args: [{ selector: '[myHeightDir]' }]
Expand Down

0 comments on commit 1045465

Please sign in to comment.