Skip to content

Commit 9090a75

Browse files
JeanMecheatscott
JeanMeche
authored and
atscott
committed
refactor(compiler-cli): relax nullish coalescing non nullable diagnostics on indexed access
When noUncheckedIndexedAccess is not enabled, indexed accesses do not include undefined in the type. This relaxes the check for nullish coalescing similarly to optional chaining. Fixes #70655 fixes #70655
1 parent 9e5ae4b commit 9090a75

5 files changed

Lines changed: 224 additions & 8 deletions

File tree

‎packages/compiler-cli/src/ngtsc/typecheck/extended/api/api.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,12 @@ import {
1313
ParseSourceSpan,
1414
TmplAstNode,
1515
TmplAstTemplate,
16+
KeyedRead,
17+
SafePropertyRead,
18+
SafeKeyedRead,
19+
SafeCall,
20+
ParenthesizedExpression,
21+
NonNullAssert,
1622
} from '@angular/compiler';
1723
import ts from 'typescript';
1824

@@ -161,3 +167,23 @@ class TemplateVisitor<Code extends ErrorCode> extends CombinedRecursiveAstVisito
161167
return this.diagnostics;
162168
}
163169
}
170+
171+
/**
172+
* Checks if the given AST node originates from a KeyedRead (indexed access)
173+
* by unwrapping parentheses, non-null assertions, and traversing the receivers
174+
* of safe navigation operations (?. property access, ?.[] keyed access, ?.() calls).
175+
*/
176+
export function isAccessFromUncheckedIndex(node: AST): boolean {
177+
if (node instanceof KeyedRead) {
178+
return true;
179+
} else if (
180+
node instanceof SafePropertyRead ||
181+
node instanceof SafeKeyedRead ||
182+
node instanceof SafeCall
183+
) {
184+
return isAccessFromUncheckedIndex(node.receiver);
185+
} else if (node instanceof ParenthesizedExpression || node instanceof NonNullAssert) {
186+
return isAccessFromUncheckedIndex(node.expression);
187+
}
188+
return false;
189+
}

‎packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.dev/license
77
*/
88

9-
import {AST, Binary, TmplAstNode} from '@angular/compiler';
9+
import {AST, Binary, KeyedRead, TmplAstNode} from '@angular/compiler';
1010
import ts from 'typescript';
1111

1212
import {NgCompilerOptions} from '../../../../core/api';
@@ -17,6 +17,7 @@ import {
1717
TemplateCheckWithVisitor,
1818
TemplateContext,
1919
formatExtendedError,
20+
isAccessFromUncheckedIndex,
2021
} from '../../api';
2122

2223
/**
@@ -28,13 +29,23 @@ import {
2829
class NullishCoalescingNotNullableCheck extends TemplateCheckWithVisitor<ErrorCode.NULLISH_COALESCING_NOT_NULLABLE> {
2930
override code = ErrorCode.NULLISH_COALESCING_NOT_NULLABLE as const;
3031

32+
constructor(private readonly noUncheckedIndexedAccess: boolean) {
33+
super();
34+
}
35+
3136
override visitNode(
3237
ctx: TemplateContext<ErrorCode.NULLISH_COALESCING_NOT_NULLABLE>,
3338
component: ts.ClassDeclaration,
3439
node: TmplAstNode | AST,
3540
): NgTemplateDiagnostic<ErrorCode.NULLISH_COALESCING_NOT_NULLABLE>[] {
3641
if (!(node instanceof Binary) || node.operation !== '??') return [];
3742

43+
// When `noUncheckedIndexedAccess` is disabled, an indexed access is not checked
44+
// and may result in `undefined`.
45+
if (!this.noUncheckedIndexedAccess && isAccessFromUncheckedIndex(node.left)) {
46+
return [];
47+
}
48+
3849
const symbolLeft = ctx.templateTypeChecker.getSymbolOfNode(node.left, component);
3950
if (symbolLeft === null || symbolLeft.kind !== SymbolKind.Expression) {
4051
return [];
@@ -86,6 +97,8 @@ export const factory: TemplateCheckFactory<
8697
return null;
8798
}
8899

89-
return new NullishCoalescingNotNullableCheck();
100+
const noUncheckedIndexedAccess = !!options.noUncheckedIndexedAccess;
101+
102+
return new NullishCoalescingNotNullableCheck(noUncheckedIndexedAccess);
90103
},
91104
};

‎packages/compiler-cli/src/ngtsc/typecheck/extended/checks/optional_chain_not_nullable/index.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import {
2424
TemplateCheckWithVisitor,
2525
TemplateContext,
2626
formatExtendedError,
27+
isAccessFromUncheckedIndex,
2728
} from '../../api';
2829

2930
/**
@@ -54,7 +55,7 @@ class OptionalChainNotNullableCheck extends TemplateCheckWithVisitor<ErrorCode.O
5455

5556
// When `noUncheckedIndexedAccess` is disabled, an indexed access is not checked
5657
// and may result in `undefined`.
57-
if (node.receiver instanceof KeyedRead && !this.noUncheckedIndexedAccess) {
58+
if (!this.noUncheckedIndexedAccess && isAccessFromUncheckedIndex(node.receiver)) {
5859
return [];
5960
}
6061

‎packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts‎

Lines changed: 151 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,155 @@ runInEachFileSystem(() => {
217217
expect(diags.length).toBe(0);
218218
});
219219

220+
it('should not produce nullish coalescing warning for an indexed access if noUncheckedIndexedAccess is false', () => {
221+
const fileName = absoluteFrom('/main.ts');
222+
const {program, templateTypeChecker} = setup(
223+
[
224+
{
225+
fileName,
226+
templates: {
227+
'TestCmp': `{{ arr[0] ?? 'foo' }}`,
228+
},
229+
source: `
230+
export class TestCmp {
231+
arr: Array<string> = [];
232+
}
233+
`,
234+
},
235+
],
236+
{options: {noUncheckedIndexedAccess: false}},
237+
);
238+
const sf = getSourceFileOrError(program, fileName);
239+
const component = getClass(sf, 'TestCmp');
240+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
241+
templateTypeChecker,
242+
program.getTypeChecker(),
243+
[nullishCoalescingNotNullableFactory],
244+
{strictNullChecks: true} /* options */,
245+
);
246+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
247+
expect(diags.length).toBe(0);
248+
});
249+
250+
it('should not produce nullish coalescing warning for an indexed access if noUncheckedIndexedAccess is true', () => {
251+
const fileName = absoluteFrom('/main.ts');
252+
const {program, templateTypeChecker} = setup(
253+
[
254+
{
255+
fileName,
256+
templates: {
257+
'TestCmp': `{{ arr[0] ?? 'foo' }}`,
258+
},
259+
source: `
260+
export class TestCmp {
261+
arr: Array<string> = [];
262+
}
263+
`,
264+
},
265+
],
266+
{options: {noUncheckedIndexedAccess: true}},
267+
);
268+
const sf = getSourceFileOrError(program, fileName);
269+
const component = getClass(sf, 'TestCmp');
270+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
271+
templateTypeChecker,
272+
program.getTypeChecker(),
273+
[nullishCoalescingNotNullableFactory],
274+
{strictNullChecks: true} /* options */,
275+
);
276+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
277+
expect(diags.length).toBe(0);
278+
});
279+
280+
it('should not produce nullish coalescing warning for a SafePropertyRead off an indexed access if noUncheckedIndexedAccess is false', () => {
281+
const fileName = absoluteFrom('/main.ts');
282+
const {program, templateTypeChecker} = setup(
283+
[
284+
{
285+
fileName,
286+
templates: {
287+
'TestCmp': `{{ foos[0]?.name ?? 'foo' }}`,
288+
},
289+
source: `
290+
export class TestCmp {
291+
foos: Array<{name: string}> = [];
292+
}
293+
`,
294+
},
295+
],
296+
{options: {noUncheckedIndexedAccess: false}},
297+
);
298+
const sf = getSourceFileOrError(program, fileName);
299+
const component = getClass(sf, 'TestCmp');
300+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
301+
templateTypeChecker,
302+
program.getTypeChecker(),
303+
[nullishCoalescingNotNullableFactory],
304+
{strictNullChecks: true} /* options */,
305+
);
306+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
307+
expect(diags.length).toBe(0);
308+
});
309+
310+
it('should not produce nullish coalescing warning for a SafeKeyedRead if noUncheckedIndexedAccess is false', () => {
311+
const fileName = absoluteFrom('/main.ts');
312+
const {program, templateTypeChecker} = setup(
313+
[
314+
{
315+
fileName,
316+
templates: {
317+
'TestCmp': `{{ foos?.[0] ?? 'foo' }}`,
318+
},
319+
source: `
320+
export class TestCmp {
321+
foos: Array<string> | null = null;
322+
}
323+
`,
324+
},
325+
],
326+
{options: {noUncheckedIndexedAccess: false}},
327+
);
328+
const sf = getSourceFileOrError(program, fileName);
329+
const component = getClass(sf, 'TestCmp');
330+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
331+
templateTypeChecker,
332+
program.getTypeChecker(),
333+
[nullishCoalescingNotNullableFactory],
334+
{strictNullChecks: true} /* options */,
335+
);
336+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
337+
expect(diags.length).toBe(0);
338+
});
339+
340+
it('should not produce nullish coalescing warning for a parenthesized indexed access if noUncheckedIndexedAccess is false', () => {
341+
const fileName = absoluteFrom('/main.ts');
342+
const {program, templateTypeChecker} = setup(
343+
[
344+
{
345+
fileName,
346+
templates: {
347+
'TestCmp': `{{ (foos[0]) ?? 'foo' }}`,
348+
},
349+
source: `
350+
export class TestCmp {
351+
foos: Array<string> = [];
352+
}
353+
`,
354+
},
355+
],
356+
{options: {noUncheckedIndexedAccess: false}},
357+
);
358+
const sf = getSourceFileOrError(program, fileName);
359+
const component = getClass(sf, 'TestCmp');
360+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
361+
templateTypeChecker,
362+
program.getTypeChecker(),
363+
[nullishCoalescingNotNullableFactory],
364+
{strictNullChecks: true} /* options */,
365+
);
366+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
367+
expect(diags.length).toBe(0);
368+
});
220369
it('warns for pipe arguments which are likely configured incorrectly (?? operates on "format" here)', () => {
221370
const fileName = absoluteFrom('/main.ts');
222371
const {program, templateTypeChecker} = setup([
@@ -341,7 +490,7 @@ runInEachFileSystem(() => {
341490
expect(diags.length).toBe(0);
342491
});
343492

344-
it('should produce nullish coalescing warning for a non-nullable ElementAccessExpression', () => {
493+
it('should not produce nullish coalescing warning for a non-nullable ElementAccessExpression when noUncheckedIndexedAccess is false', () => {
345494
const fileName = absoluteFrom('/main.ts');
346495
const {program, templateTypeChecker} = setup([
347496
{
@@ -362,10 +511,7 @@ runInEachFileSystem(() => {
362511
{strictNullChecks: true} /* options */,
363512
);
364513
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
365-
expect(diags.length).toBe(1);
366-
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
367-
expect(diags[0].code).toBe(ngErrorCode(ErrorCode.NULLISH_COALESCING_NOT_NULLABLE));
368-
expect(getSourceCodeForDiagnostic(diags[0])).toBe(`myDict[key] ?? 'foo'`);
514+
expect(diags.length).toBe(0);
369515
});
370516

371517
it('should respect configured diagnostic category', () => {

‎packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/optional_chain_not_nullable/optional_chain_not_nullable_spec.ts‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -256,6 +256,36 @@ runInEachFileSystem(() => {
256256
expect(diags.length).toBe(0);
257257
});
258258

259+
it('should not produce optional chain warning for a chained safe property read off an indexed access if noUncheckedIndexedAccess is false', () => {
260+
const fileName = absoluteFrom('/main.ts');
261+
const {program, templateTypeChecker} = setup(
262+
[
263+
{
264+
fileName,
265+
templates: {
266+
'TestCmp': `{{ arr[0]?.bar?.length }}`,
267+
},
268+
source: `
269+
export class TestCmp {
270+
arr: Array<{ bar: string }> = [];
271+
}
272+
`,
273+
},
274+
],
275+
{options: {noUncheckedIndexedAccess: false}},
276+
);
277+
const sf = getSourceFileOrError(program, fileName);
278+
const component = getClass(sf, 'TestCmp');
279+
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
280+
templateTypeChecker,
281+
program.getTypeChecker(),
282+
[optionalChainNotNullableFactory],
283+
{strictNullChecks: true} /* options */,
284+
);
285+
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
286+
expect(diags.length).toBe(0);
287+
});
288+
259289
it('should produce optional chain warning for an indexed access if noUncheckedIndexedAccess is true', () => {
260290
const fileName = absoluteFrom('/main.ts');
261291
const {program, templateTypeChecker} = setup(

0 commit comments

Comments
 (0)