Skip to content

Commit 08049fa

Browse files
JoostKatscott
JoostK
authored and
atscott
committed
fix(compiler-cli): enable narrowing of using type guard methods (#44447)
The changes in 2028c39 caused method calls to be emitted using additional parenthesis into the TCB, which in turn prevented proper type narrowing when the method acts as a type guard. This commit special-cases method calls from property reads to avoid the additional parenthesis. Fixes #44353 PR Close #44447
1 parent c375e5d commit 08049fa

File tree

5 files changed

+97
-40
lines changed

5 files changed

+97
-40
lines changed

‎packages/compiler-cli/src/ngtsc/typecheck/src/expression.ts‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -313,12 +313,31 @@ class AstTranslator implements AstVisitor {
313313

314314
visitCall(ast: Call): ts.Expression {
315315
const args = ast.args.map(expr => this.translate(expr));
316-
const expr = wrapForDiagnostics(this.translate(ast.receiver));
316+
317+
let expr: ts.Expression;
318+
const receiver = ast.receiver;
319+
320+
// For calls that have a property read as receiver, we have to special-case their emit to avoid
321+
// inserting superfluous parenthesis as they prevent TypeScript from applying a narrowing effect
322+
// if the method acts as a type guard.
323+
if (receiver instanceof PropertyRead) {
324+
const resolved = this.maybeResolve(receiver);
325+
if (resolved !== null) {
326+
expr = resolved;
327+
} else {
328+
const propertyReceiver = wrapForDiagnostics(this.translate(receiver.receiver));
329+
expr = ts.createPropertyAccess(propertyReceiver, receiver.name);
330+
addParseSpanInfo(expr, receiver.nameSpan);
331+
}
332+
} else {
333+
expr = this.translate(receiver);
334+
}
335+
317336
let node: ts.Expression;
318337

319338
// Safe property/keyed reads will produce a ternary whose value is nullable.
320339
// We have to generate a similar ternary around the call.
321-
if (ast.receiver instanceof SafePropertyRead || ast.receiver instanceof SafeKeyedRead) {
340+
if (receiver instanceof SafePropertyRead || receiver instanceof SafeKeyedRead) {
322341
if (this.config.strictSafeNavigationTypes) {
323342
// "a?.method(...)" becomes (null as any ? a!.method(...) : undefined)
324343
const call = ts.createCall(ts.createNonNullExpression(expr), undefined, args);

‎packages/compiler-cli/src/ngtsc/typecheck/test/diagnostics_spec.ts‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import ts from 'typescript';
1111
import {absoluteFrom, getSourceFileOrError} from '../../file_system';
1212
import {runInEachFileSystem, TestFile} from '../../file_system/testing';
1313
import {OptimizeFor, TypeCheckingConfig} from '../api';
14-
import {ngForDeclaration, ngForDts, setup, TestDeclaration} from '../testing';
14+
import {ngForDeclaration, ngForDts, ngIfDeclaration, ngIfDts, setup, TestDeclaration} from '../testing';
1515

1616
runInEachFileSystem(() => {
1717
describe('template diagnostics', () => {
@@ -339,6 +339,17 @@ runInEachFileSystem(() => {
339339
]);
340340
});
341341

342+
it('should support type-narrowing for methods with type guards', () => {
343+
const messages = diagnose(
344+
`<div *ngIf="hasSuccess()">{{ success }}</div>`, `
345+
class TestComponent {
346+
hasSuccess(): this is { success: boolean };
347+
}`,
348+
[ngIfDeclaration()], [ngIfDts()]);
349+
350+
expect(messages).toEqual([]);
351+
});
352+
342353
describe('outputs', () => {
343354
it('should produce a diagnostic for directive outputs', () => {
344355
const messages = diagnose(

‎packages/compiler-cli/src/ngtsc/typecheck/test/span_comments_spec.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ describe('type check blocks diagnostics', () => {
3939
const TEMPLATE = '{{ m({foo: a, bar: b}) }}';
4040
expect(tcbWithSpans(TEMPLATE))
4141
.toContain(
42-
'((((ctx).m /*3,4*/) /*3,4*/)({ "foo": ((ctx).a /*11,12*/) /*11,12*/, "bar": ((ctx).b /*19,20*/) /*19,20*/ } /*5,21*/) /*3,22*/)');
42+
'(ctx).m /*3,4*/({ "foo": ((ctx).a /*11,12*/) /*11,12*/, "bar": ((ctx).b /*19,20*/) /*19,20*/ } /*5,21*/) /*3,22*/');
4343
});
4444

4545
it('should annotate literal map expressions with shorthand declarations', () => {
@@ -48,7 +48,7 @@ describe('type check blocks diagnostics', () => {
4848
const TEMPLATE = '{{ m({a, b}) }}';
4949
expect(tcbWithSpans(TEMPLATE))
5050
.toContain(
51-
'((((ctx).m /*3,4*/) /*3,4*/)({ "a": ((ctx).a /*6,7*/) /*6,7*/, "b": ((ctx).b /*9,10*/) /*9,10*/ } /*5,11*/) /*3,12*/)');
51+
'((ctx).m /*3,4*/({ "a": ((ctx).a /*6,7*/) /*6,7*/, "b": ((ctx).b /*9,10*/) /*9,10*/ } /*5,11*/) /*3,12*/)');
5252
});
5353

5454
it('should annotate literal array expressions', () => {
@@ -76,21 +76,21 @@ describe('type check blocks diagnostics', () => {
7676
const TEMPLATE = `{{ method(a, b) }}`;
7777
expect(tcbWithSpans(TEMPLATE))
7878
.toContain(
79-
'((((ctx).method /*3,9*/) /*3,9*/)(((ctx).a /*10,11*/) /*10,11*/, ((ctx).b /*13,14*/) /*13,14*/) /*3,15*/)');
79+
'(ctx).method /*3,9*/(((ctx).a /*10,11*/) /*10,11*/, ((ctx).b /*13,14*/) /*13,14*/) /*3,15*/');
8080
});
8181

8282
it('should annotate method calls of variables', () => {
8383
const TEMPLATE = `<ng-template let-method>{{ method(a, b) }}</ng-template>`;
8484
expect(tcbWithSpans(TEMPLATE))
8585
.toContain(
86-
'((_t2 /*27,33*/)(((ctx).a /*34,35*/) /*34,35*/, ((ctx).b /*37,38*/) /*37,38*/) /*27,39*/)');
86+
'_t2 /*27,33*/(((ctx).a /*34,35*/) /*34,35*/, ((ctx).b /*37,38*/) /*37,38*/) /*27,39*/');
8787
});
8888

8989
it('should annotate function calls', () => {
9090
const TEMPLATE = `{{ method(a)(b, c) }}`;
9191
expect(tcbWithSpans(TEMPLATE))
9292
.toContain(
93-
'(((((ctx).method /*3,9*/) /*3,9*/)(((ctx).a /*10,11*/) /*10,11*/) /*3,12*/)(((ctx).b /*13,14*/) /*13,14*/, ((ctx).c /*16,17*/) /*16,17*/) /*3,18*/)');
93+
'(ctx).method /*3,9*/(((ctx).a /*10,11*/) /*10,11*/) /*3,12*/(((ctx).b /*13,14*/) /*13,14*/, ((ctx).c /*16,17*/) /*16,17*/) /*3,18*/');
9494
});
9595

9696
it('should annotate property access', () => {
@@ -135,7 +135,7 @@ describe('type check blocks diagnostics', () => {
135135
const TEMPLATE = `{{ a?.method(b) }}`;
136136
expect(tcbWithSpans(TEMPLATE))
137137
.toContain(
138-
'((null as any ? ((null as any ? (((ctx).a /*3,4*/) /*3,4*/)!.method /*6,12*/ : undefined) /*3,12*/)!(((ctx).b /*13,14*/) /*13,14*/) : undefined) /*3,15*/)');
138+
'((null as any ? (null as any ? (((ctx).a /*3,4*/) /*3,4*/)!.method /*6,12*/ : undefined) /*3,12*/!(((ctx).b /*13,14*/) /*13,14*/) : undefined) /*3,15*/)');
139139
});
140140

141141
it('should annotate safe keyed reads', () => {

‎packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts‎

Lines changed: 29 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,12 @@ describe('type check blocks', () => {
2020

2121
it('should generate literal map expressions', () => {
2222
const TEMPLATE = '{{ method({foo: a, bar: b}) }}';
23-
expect(tcb(TEMPLATE)).toContain('(((ctx).method))({ "foo": ((ctx).a), "bar": ((ctx).b) })');
23+
expect(tcb(TEMPLATE)).toContain('(ctx).method({ "foo": ((ctx).a), "bar": ((ctx).b) })');
2424
});
2525

2626
it('should generate literal array expressions', () => {
2727
const TEMPLATE = '{{ method([a, b]) }}';
28-
expect(tcb(TEMPLATE)).toContain('(((ctx).method))([((ctx).a), ((ctx).b)])');
28+
expect(tcb(TEMPLATE)).toContain('(ctx).method([((ctx).a), ((ctx).b)])');
2929
});
3030

3131
it('should handle non-null assertions', () => {
@@ -115,7 +115,7 @@ describe('type check blocks', () => {
115115
it('should handle method calls of template variables', () => {
116116
const TEMPLATE = `<ng-template let-a>{{a(1)}}</ng-template>`;
117117
expect(tcb(TEMPLATE)).toContain('var _t2 = _t1.$implicit;');
118-
expect(tcb(TEMPLATE)).toContain('(_t2)(1)');
118+
expect(tcb(TEMPLATE)).toContain('_t2(1)');
119119
});
120120

121121
it('should handle implicit vars when using microsyntax', () => {
@@ -126,7 +126,7 @@ describe('type check blocks', () => {
126126
it('should handle direct calls of an implicit template variable', () => {
127127
const TEMPLATE = `<div *ngFor="let a of letters">{{a(1)}}</div>`;
128128
expect(tcb(TEMPLATE)).toContain('var _t2 = _t1.$implicit;');
129-
expect(tcb(TEMPLATE)).toContain('(_t2)(1)');
129+
expect(tcb(TEMPLATE)).toContain('_t2(1)');
130130
});
131131

132132
describe('type constructors', () => {
@@ -609,13 +609,13 @@ describe('type check blocks', () => {
609609
it('should handle $any accessed through `this`', () => {
610610
const TEMPLATE = `{{this.$any(a)}}`;
611611
const block = tcb(TEMPLATE);
612-
expect(block).toContain('((((ctx).$any))(((ctx).a)))');
612+
expect(block).toContain('((ctx).$any(((ctx).a)))');
613613
});
614614

615615
it('should handle $any accessed through a property read', () => {
616616
const TEMPLATE = `{{foo.$any(a)}}`;
617617
const block = tcb(TEMPLATE);
618-
expect(block).toContain('((((((ctx).foo)).$any))(((ctx).a)))');
618+
expect(block).toContain('((((ctx).foo)).$any(((ctx).a)))');
619619
});
620620

621621
describe('experimental DOM checking via lib.dom.d.ts', () => {
@@ -688,28 +688,27 @@ describe('type check blocks', () => {
688688
const TEMPLATE = `<div dir (dirOutput)="foo($event)"></div>`;
689689
const block = tcb(TEMPLATE, DIRECTIVES);
690690
expect(block).toContain(
691-
'_t1["outputField"].subscribe(function ($event): any { (((ctx).foo))($event); });');
691+
'_t1["outputField"].subscribe(function ($event): any { (ctx).foo($event); });');
692692
});
693693

694694
it('should emit a listener function with AnimationEvent for animation events', () => {
695695
const TEMPLATE = `<div (@animation.done)="foo($event)"></div>`;
696696
const block = tcb(TEMPLATE);
697-
expect(block).toContain(
698-
'function ($event: i1.AnimationEvent): any { (((ctx).foo))($event); }');
697+
expect(block).toContain('function ($event: i1.AnimationEvent): any { (ctx).foo($event); }');
699698
});
700699

701700
it('should emit addEventListener calls for unclaimed outputs', () => {
702701
const TEMPLATE = `<div (event)="foo($event)"></div>`;
703702
const block = tcb(TEMPLATE);
704703
expect(block).toContain(
705-
'_t1.addEventListener("event", function ($event): any { (((ctx).foo))($event); });');
704+
'_t1.addEventListener("event", function ($event): any { (ctx).foo($event); });');
706705
});
707706

708707
it('should allow to cast $event using $any', () => {
709708
const TEMPLATE = `<div (event)="foo($any($event))"></div>`;
710709
const block = tcb(TEMPLATE);
711710
expect(block).toContain(
712-
'_t1.addEventListener("event", function ($event): any { (((ctx).foo))(($event as any)); });');
711+
'_t1.addEventListener("event", function ($event): any { (ctx).foo(($event as any)); });');
713712
});
714713

715714
it('should detect writes to template variables', () => {
@@ -724,7 +723,7 @@ describe('type check blocks', () => {
724723
const block = tcb(TEMPLATE);
725724

726725
expect(block).toContain(
727-
'_t1.addEventListener("event", function ($event): any { (((ctx).foo))(((ctx).$event)); });');
726+
'_t1.addEventListener("event", function ($event): any { (ctx).foo(((ctx).$event)); });');
728727
});
729728
});
730729

@@ -852,18 +851,18 @@ describe('type check blocks', () => {
852851
it('should check types of directive outputs when enabled', () => {
853852
const block = tcb(TEMPLATE, DIRECTIVES);
854853
expect(block).toContain(
855-
'_t1["outputField"].subscribe(function ($event): any { (((ctx).foo))($event); });');
854+
'_t1["outputField"].subscribe(function ($event): any { (ctx).foo($event); });');
856855
expect(block).toContain(
857-
'_t2.addEventListener("nonDirOutput", function ($event): any { (((ctx).foo))($event); });');
856+
'_t2.addEventListener("nonDirOutput", function ($event): any { (ctx).foo($event); });');
858857
});
859858
it('should not check types of directive outputs when disabled', () => {
860859
const DISABLED_CONFIG:
861860
TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfOutputEvents: false};
862861
const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
863-
expect(block).toContain('function ($event: any): any { (((ctx).foo))($event); }');
862+
expect(block).toContain('function ($event: any): any { (ctx).foo($event); }');
864863
// Note that DOM events are still checked, that is controlled by `checkTypeOfDomEvents`
865864
expect(block).toContain(
866-
'addEventListener("nonDirOutput", function ($event): any { (((ctx).foo))($event); });');
865+
'addEventListener("nonDirOutput", function ($event): any { (ctx).foo($event); });');
867866
});
868867
});
869868

@@ -872,14 +871,13 @@ describe('type check blocks', () => {
872871

873872
it('should check types of animation events when enabled', () => {
874873
const block = tcb(TEMPLATE, DIRECTIVES);
875-
expect(block).toContain(
876-
'function ($event: i1.AnimationEvent): any { (((ctx).foo))($event); }');
874+
expect(block).toContain('function ($event: i1.AnimationEvent): any { (ctx).foo($event); }');
877875
});
878876
it('should not check types of animation events when disabled', () => {
879877
const DISABLED_CONFIG:
880878
TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfAnimationEvents: false};
881879
const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
882-
expect(block).toContain('function ($event: any): any { (((ctx).foo))($event); }');
880+
expect(block).toContain('function ($event: any): any { (ctx).foo($event); }');
883881
});
884882
});
885883

@@ -889,18 +887,18 @@ describe('type check blocks', () => {
889887
it('should check types of DOM events when enabled', () => {
890888
const block = tcb(TEMPLATE, DIRECTIVES);
891889
expect(block).toContain(
892-
'_t1["outputField"].subscribe(function ($event): any { (((ctx).foo))($event); });');
890+
'_t1["outputField"].subscribe(function ($event): any { (ctx).foo($event); });');
893891
expect(block).toContain(
894-
'_t2.addEventListener("nonDirOutput", function ($event): any { (((ctx).foo))($event); });');
892+
'_t2.addEventListener("nonDirOutput", function ($event): any { (ctx).foo($event); });');
895893
});
896894
it('should not check types of DOM events when disabled', () => {
897895
const DISABLED_CONFIG: TypeCheckingConfig = {...BASE_CONFIG, checkTypeOfDomEvents: false};
898896
const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
899897
// Note that directive outputs are still checked, that is controlled by
900898
// `checkTypeOfOutputEvents`
901899
expect(block).toContain(
902-
'_t1["outputField"].subscribe(function ($event): any { (((ctx).foo))($event); });');
903-
expect(block).toContain('function ($event: any): any { (((ctx).foo))($event); }');
900+
'_t1["outputField"].subscribe(function ($event): any { (ctx).foo($event); });');
901+
expect(block).toContain('function ($event: any): any { (ctx).foo($event); }');
904902
});
905903
});
906904

@@ -1009,15 +1007,15 @@ describe('type check blocks', () => {
10091007
it('should use undefined for safe navigation operations when enabled', () => {
10101008
const block = tcb(TEMPLATE, DIRECTIVES);
10111009
expect(block).toContain(
1012-
'(null as any ? ((null as any ? (((ctx).a))!.method : undefined))!() : undefined)');
1010+
'(null as any ? (null as any ? (((ctx).a))!.method : undefined)!() : undefined)');
10131011
expect(block).toContain('(null as any ? (((ctx).a))!.b : undefined)');
10141012
expect(block).toContain('(null as any ? (((ctx).a))![0] : undefined)');
10151013
});
10161014
it('should use an \'any\' type for safe navigation operations when disabled', () => {
10171015
const DISABLED_CONFIG:
10181016
TypeCheckingConfig = {...BASE_CONFIG, strictSafeNavigationTypes: false};
10191017
const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
1020-
expect(block).toContain('(((((((ctx).a))!.method as any)) as any)())');
1018+
expect(block).toContain('((((((ctx).a))!.method as any) as any)())');
10211019
expect(block).toContain('((((ctx).a))!.b as any)');
10221020
expect(block).toContain('(((((ctx).a))![0] as any)');
10231021
});
@@ -1027,18 +1025,18 @@ describe('type check blocks', () => {
10271025
const TEMPLATE = `{{a.method()?.b}} {{a()?.method()}} {{a.method()?.[0]}}`;
10281026
it('should check the presence of a property/method on the receiver when enabled', () => {
10291027
const block = tcb(TEMPLATE, DIRECTIVES);
1030-
expect(block).toContain('(null as any ? ((((((ctx).a)).method))())!.b : undefined)');
1028+
expect(block).toContain('(null as any ? ((((ctx).a)).method())!.b : undefined)');
10311029
expect(block).toContain(
1032-
'(null as any ? ((null as any ? ((((ctx).a))())!.method : undefined))!() : undefined)');
1033-
expect(block).toContain('(null as any ? ((((((ctx).a)).method))())![0] : undefined)');
1030+
'(null as any ? (null as any ? ((ctx).a())!.method : undefined)!() : undefined)');
1031+
expect(block).toContain('(null as any ? ((((ctx).a)).method())![0] : undefined)');
10341032
});
10351033
it('should not check the presence of a property/method on the receiver when disabled', () => {
10361034
const DISABLED_CONFIG:
10371035
TypeCheckingConfig = {...BASE_CONFIG, strictSafeNavigationTypes: false};
10381036
const block = tcb(TEMPLATE, DIRECTIVES, DISABLED_CONFIG);
1039-
expect(block).toContain('((((((((ctx).a)).method))()) as any).b)');
1040-
expect(block).toContain('((((((((ctx).a))()) as any).method) as any)())');
1041-
expect(block).toContain('((((((((ctx).a)).method))()) as any)[0])');
1037+
expect(block).toContain('(((((ctx).a)).method()) as any).b');
1038+
expect(block).toContain('(((((ctx).a()) as any).method as any)())');
1039+
expect(block).toContain('(((((ctx).a)).method()) as any)[0]');
10421040
});
10431041
});
10441042

‎packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,35 @@ export function angularAnimationsDts(): TestFile {
120120
};
121121
}
122122

123+
export function ngIfDeclaration(): TestDeclaration {
124+
return {
125+
type: 'directive',
126+
file: absoluteFrom('/ngif.d.ts'),
127+
selector: '[ngIf]',
128+
name: 'NgIf',
129+
inputs: {ngIf: 'ngIf'},
130+
ngTemplateGuards: [{type: 'binding', inputName: 'ngIf'}],
131+
hasNgTemplateContextGuard: true,
132+
isGeneric: true,
133+
};
134+
}
135+
136+
export function ngIfDts(): TestFile {
137+
return {
138+
name: absoluteFrom('/ngif.d.ts'),
139+
contents: `
140+
export declare class NgIf<T> {
141+
ngIf: T;
142+
static ngTemplateContextGuard<T>(dir: NgIf<T>, ctx: any): ctx is NgIfContext<Exclude<T, false|0|''|null|undefined>>
143+
}
144+
145+
export declare class NgIfContext<T> {
146+
$implicit: T;
147+
ngIf: T;
148+
}`,
149+
};
150+
}
151+
123152
export function ngForDeclaration(): TestDeclaration {
124153
return {
125154
type: 'directive',

0 commit comments

Comments
 (0)