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 committed Sep 17, 2024
1 parent 4231e8f commit e053cd7
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 38 deletions.
20 changes: 5 additions & 15 deletions packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts
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
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
22 changes: 1 addition & 21 deletions packages/compiler/src/template/pipeline/src/ingest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1003,30 +1003,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

0 comments on commit e053cd7

Please sign in to comment.