Skip to content

Commit

Permalink
fix(compiler): this.a should always refer to class property a
Browse files Browse the repository at this point in the history 
Consider a template with a context variable `a`:
```
<ng-template let-a>{{this.a}}</ng-template>
```

t push -fAn interpolation inside that template to `this.a` should intuitively read the class variable `a`. However, today, it refers to the context variable `a`, both in the TCB and the generated code.

In this commit, the above interpolation now refers to the class field `a`.

BREAKING CHANGE: `this.foo` property reads no longer refer to template context variables. If you intended to read the template variable, do not use `this.`.
Fixes angular#55115
  • Loading branch information
@dylhunn
dylhunn committed Sep 17, 2024
1 parent 4231e8f commit 49dbfc0
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 54 deletions.
20 changes: 5 additions & 15 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2709,21 +2709,11 @@ class TcbExpressionTranslator {
* context). This method assists in resolving those.
*/
protected resolve(ast: AST): ts.Expression | null {
// TODO: this is actually a bug, because `ImplicitReceiver` extends `ThisReceiver`. Consider a
// case when the explicit `this` read is inside a template with a context that also provides the
// variable name being read:
// ```
// <ng-template let-a>{{this.a}}</ng-template>
// ```
// Clearly, `this.a` should refer to the class property `a`. However, because of this code,
// `this.a` will refer to `let-a` on the template context.
//
// Note that the generated code is actually consistent with this bug. To fix it, we have to:
// - Check `!(ast.receiver instanceof ThisReceiver)` in this condition
// - Update `ingest.ts` in the Template Pipeline (see the corresponding comment)
// - Turn off legacy TemplateDefinitionBuilder
// - Fix g3, and release in a major version
if (ast instanceof PropertyRead && ast.receiver instanceof ImplicitReceiver) {
if (
ast instanceof PropertyRead &&
ast.receiver instanceof ImplicitReceiver &&
!(ast.receiver instanceof ThisReceiver)
) {
// Try to resolve a bound target for this expression. If no such target is available, then
// the expression is referencing the top-level component context. In that case, `null` is
// returned here to let it fall through resolution so it will be caught when the
Expand Down
3 changes: 1 addition & 2 deletions ...compiler-cli/test/compliance/test_cases/r3_view_compiler_template/ng_template_implicit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ MyComponent_ng_template_0_Template(rf, ctx) {
if (rf & 1) {
i0.ɵɵtext(0);
} if (rf & 2) {
// NOTE: The fact that `this.` still refers to template context is a TDB and TCB bug; we should fix this eventually.
const $a_r1$ = ctx.$implicit;
const $a_r1$ = i0.ɵɵnextContext();
i0.ɵɵtextInterpolate($a_r1$);
}
}
Expand Down
151 changes: 114 additions & 37 deletions packages/compiler/src/template/pipeline/src/ingest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,18 @@
* found in the LICENSE file at https://angular.io/license
*/

import {ConstantPool} from '../../../constant_pool';
import {SecurityContext} from '../../../core';
import { ConstantPool } from '../../../constant_pool';
import { SecurityContext } from '../../../core';
import * as e from '../../../expression_parser/ast';
import * as i18n from '../../../i18n/i18n_ast';
import {splitNsName} from '../../../ml_parser/tags';
import { splitNsName } from '../../../ml_parser/tags';
import * as o from '../../../output/output_ast';
import {ParseSourceSpan} from '../../../parse_util';
import { ParseSourceSpan } from '../../../parse_util';
import * as t from '../../../render3/r3_ast';
import {DeferBlockDepsEmitMode, R3ComponentDeferMetadata} from '../../../render3/view/api';
import {icuFromI18nMessage} from '../../../render3/view/i18n/util';
import {DomElementSchemaRegistry} from '../../../schema/dom_element_schema_registry';
import {BindingParser} from '../../../template_parser/binding_parser';
import { DeferBlockDepsEmitMode, R3ComponentDeferMetadata } from '../../../render3/view/api';
import { icuFromI18nMessage } from '../../../render3/view/i18n/util';
import { DomElementSchemaRegistry } from '../../../schema/dom_element_schema_registry';
import { BindingParser } from '../../../template_parser/binding_parser';
import * as ir from '../ir';

import {
Expand All @@ -27,7 +27,7 @@ import {
type CompilationJob,
type ViewCompilationUnit,
} from './compilation';
import {BINARY_OPERATORS, namespaceForKey, prefixWithNamespace} from './conversion';
import { BINARY_OPERATORS, namespaceForKey, prefixWithNamespace } from './conversion';

const compatibilityMode = ir.CompatibilityMode.TemplateDefinitionBuilder;

Expand All @@ -41,7 +41,7 @@ export function isI18nRootNode(meta?: i18n.I18nMeta): meta is i18n.Message {
return meta instanceof i18n.Message;
}

export function isSingleI18nIcu(meta?: i18n.I18nMeta): meta is i18n.I18nMeta & {nodes: [i18n.Icu]} {
export function isSingleI18nIcu(meta?: i18n.I18nMeta): meta is i18n.I18nMeta & { nodes: [i18n.Icu] } {
return isI18nRootNode(meta) && meta.nodes.length === 1 && meta.nodes[0] instanceof i18n.Icu;
}

Expand Down Expand Up @@ -76,7 +76,7 @@ export interface HostBindingInput {
componentName: string;
componentSelector: string;
properties: e.ParsedProperty[] | null;
attributes: {[key: string]: o.Expression};
attributes: { [key: string]: o.Expression };
events: e.ParsedEvent[] | null;
}

Expand Down Expand Up @@ -331,7 +331,7 @@ function ingestTemplate(unit: ViewCompilationUnit, tmpl: t.Template): void {
ingestReferences(templateOp, tmpl);
ingestNodes(childView, tmpl.children);

for (const {name, value} of tmpl.variables) {
for (const { name, value } of tmpl.variables) {
childView.contextVariables.set(name, value !== '' ? value : '$implicit');
}

Expand Down Expand Up @@ -439,8 +439,8 @@ function ingestBoundText(
const i18nPlaceholders =
text.i18n instanceof i18n.Container
? text.i18n.children
.filter((node): node is i18n.Placeholder => node instanceof i18n.Placeholder)
.map((placeholder) => placeholder.name)
.filter((node): node is i18n.Placeholder => node instanceof i18n.Placeholder)
.map((placeholder) => placeholder.name)
: [];
if (i18nPlaceholders.length > 0 && i18nPlaceholders.length !== value.expressions.length) {
throw Error(
Expand Down Expand Up @@ -710,11 +710,108 @@ function ingestDeferBlock(unit: ViewCompilationUnit, deferBlock: t.DeferredBlock
deferOnOps.push(
ir.createDeferOnOp(
deferXref,
<<<<<<< HEAD
{kind: ir.DeferTriggerKind.Idle},
ir.DeferOpModifierKind.NONE,
null!,
),
);
=======
{ kind: ir.DeferTriggerKind.Idle },
prefetch,
triggers.idle.sourceSpan,
);
deferOnOps.push(deferOnOp);
}
if (triggers.immediate !== undefined) {
const deferOnOp = ir.createDeferOnOp(
deferXref,
{ kind: ir.DeferTriggerKind.Immediate },
prefetch,
triggers.immediate.sourceSpan,
);
deferOnOps.push(deferOnOp);
}
if (triggers.timer !== undefined) {
const deferOnOp = ir.createDeferOnOp(
deferXref,
{ kind: ir.DeferTriggerKind.Timer, delay: triggers.timer.delay },
prefetch,
triggers.timer.sourceSpan,
);
deferOnOps.push(deferOnOp);
}
if (triggers.hover !== undefined) {
const deferOnOp = ir.createDeferOnOp(
deferXref,
{
kind: ir.DeferTriggerKind.Hover,
targetName: triggers.hover.reference,
targetXref: null,
targetSlot: null,
targetView: null,
targetSlotViewSteps: null,
},
prefetch,
triggers.hover.sourceSpan,
);
deferOnOps.push(deferOnOp);
}
if (triggers.interaction !== undefined) {
const deferOnOp = ir.createDeferOnOp(
deferXref,
{
kind: ir.DeferTriggerKind.Interaction,
targetName: triggers.interaction.reference,
targetXref: null,
targetSlot: null,
targetView: null,
targetSlotViewSteps: null,
},
prefetch,
triggers.interaction.sourceSpan,
);
deferOnOps.push(deferOnOp);
}
if (triggers.viewport !== undefined) {
const deferOnOp = ir.createDeferOnOp(
deferXref,
{
kind: ir.DeferTriggerKind.Viewport,
targetName: triggers.viewport.reference,
targetXref: null,
targetSlot: null,
targetView: null,
targetSlotViewSteps: null,
},
prefetch,
triggers.viewport.sourceSpan,
);
deferOnOps.push(deferOnOp);
}
if (triggers.when !== undefined) {
if (triggers.when.value instanceof e.Interpolation) {
// TemplateDefinitionBuilder supports this case, but it's very strange to me. What would it
// even mean?
throw new Error(`Unexpected interpolation in defer block when trigger`);
}
const deferOnOp = ir.createDeferWhenOp(
deferXref,
convertAst(triggers.when.value, unit.job, triggers.when.sourceSpan),
prefetch,
triggers.when.sourceSpan,
);
deferWhenOps.push(deferOnOp);
}

// If no (non-prefetching) defer triggers were provided, default to `idle`.
if (deferOnOps.length === 0 && deferWhenOps.length === 0) {
deferOnOps.push(
ir.createDeferOnOp(deferXref, { kind: ir.DeferTriggerKind.Idle }, false, null!),
);
}
prefetch = true;
>>>>>>> 79da6c00d1 (fix(compiler): `this.a` should always refer to class property `a`)
}
unit.create.push(deferOnOps);
Expand Down Expand Up @@ -833,7 +930,7 @@ function ingestIcu(unit: ViewCompilationUnit, icu: t.Icu) {
if (icu.i18n instanceof i18n.Message && isSingleI18nIcu(icu.i18n)) {
const xref = unit.job.allocateXrefId();
unit.create.push(ir.createIcuStartOp(xref, icu.i18n, icuFromI18nMessage(icu.i18n).name, null!));
for (const [placeholder, text] of Object.entries({...icu.vars, ...icu.placeholders})) {
for (const [placeholder, text] of Object.entries({ ...icu.vars, ...icu.placeholders })) {
if (text instanceof t.BoundText) {
ingestBoundText(unit, text, placeholder);
} else {
Expand Down Expand Up @@ -1003,30 +1100,10 @@ function convertAst(
if (ast instanceof e.ASTWithSource) {
return convertAst(ast.ast, job, baseSourceSpan);
} else if (ast instanceof e.PropertyRead) {
const isThisReceiver = ast.receiver instanceof e.ThisReceiver;
// Whether this is an implicit receiver, *excluding* explicit reads of `this`.
const isImplicitReceiver =
ast.receiver instanceof e.ImplicitReceiver && !(ast.receiver instanceof e.ThisReceiver);
// Whether the name of the read is a node that should be never retain its explicit this
// receiver.
const isSpecialNode = ast.name === '$any' || ast.name === '$event';
// TODO: The most sensible condition here would be simply `isImplicitReceiver`, to convert only
// actual implicit `this` reads, and not explicit ones. However, TemplateDefinitionBuilder (and
// the Typecheck block!) both have the same bug, in which they also consider explicit `this`
// reads to be implicit. This causes problems when the explicit `this` read is inside a
// template with a context that also provides the variable name being read:
// ```
// <ng-template let-a>{{this.a}}</ng-template>
// ```
// The whole point of the explicit `this` was to access the class property, but TDB and the
// current TCB treat the read as implicit, and give you the context property instead!
//
// For now, we emulate this old behavior by aggressively converting explicit reads to to
// implicit reads, except for the special cases that TDB and the current TCB protect. However,
// it would be an improvement to fix this.
//
// See also the corresponding comment for the TCB, in `type_check_block.ts`.
if (isImplicitReceiver || (isThisReceiver && !isSpecialNode)) {
if (isImplicitReceiver) {
return new ir.LexicalReadExpr(ast.name);
} else {
return new o.ReadPropExpr(
Expand Down Expand Up @@ -1692,7 +1769,7 @@ function astOf(ast: e.AST | e.ASTWithSource): e.AST {
*/
function ingestReferences(op: ir.ElementOpBase, element: t.Element | t.Template): void {
assertIsArray<ir.LocalRef>(op.localRefs);
for (const {name, value} of element.references) {
for (const { name, value } of element.references) {
op.localRefs.push({
name,
target: value,
Expand Down

0 comments on commit 49dbfc0

Please sign in to comment.