Skip to content

Commit

Permalink
fix(compiler-cli): enable nullish coalescing check only with `strictN…
Browse files Browse the repository at this point in the history 
…ullChecks` (#44862)

TypeScript configures `strictNullChecks` to be disabled by default, so the nullish
coalescing check should follow the same default. The rule actively depends on
`strictNullChecks`, as TypeScript doesn't include `null`/`undefined` in its types
otherwise so the check wouldn't have a way to differentiate between them.

This commit also takes the `strict` flag into account when `strictNullChecks` itself
is not configured.

PR Close #44862
  • Loading branch information
@JoostK @thePunderWoman
JoostK authored and thePunderWoman committed Jan 31, 2022
1 parent 73c97ae commit 366e424
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 10 deletions.
4 changes: 3 additions & 1 deletion ...compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ export const factory: TemplateCheckFactory<
name: ExtendedTemplateDiagnosticName.NULLISH_COALESCING_NOT_NULLABLE,
create: (options: NgCompilerOptions) => {
// Require `strictNullChecks` to be enabled.
if (options.strictNullChecks === false) {
const strictNullChecks =
options.strictNullChecks === undefined ? !!options.strict : !!options.strictNullChecks;
if (!strictNullChecks) {
return null;
}

Expand Down
30 changes: 21 additions & 9 deletions ...ended/test/checks/nullish_coalescing_not_nullable/nullish_coalescing_not_nullable_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,24 @@ runInEachFileSystem(() => {

it('should return a check if `strictNullChecks` is enabled', () => {
expect(nullishCoalescingNotNullableFactory.create({strictNullChecks: true})).toBeDefined();
expect(nullishCoalescingNotNullableFactory.create({})).not.toBeNull(); // Defaults enabled.
});

it('should return a check if `strictNullChecks` is not configured but `strict` is enabled',
() => {
expect(nullishCoalescingNotNullableFactory.create({strict: true})).toBeDefined();
});

it('should not return a check if `strictNullChecks` is disabled', () => {
expect(nullishCoalescingNotNullableFactory.create({strictNullChecks: false})).toBeNull();
expect(nullishCoalescingNotNullableFactory.create({})).toBeNull(); // Defaults disabled.
});

it('should not return a check if `strict` is enabled but `strictNullChecks` is disabled',
() => {
expect(nullishCoalescingNotNullableFactory.create({strict: true, strictNullChecks: false}))
.toBeNull();
});

it('should produce nullish coalescing warning', () => {
const fileName = absoluteFrom('/main.ts');
const {program, templateTypeChecker} = setup([{
Expand All @@ -48,7 +59,7 @@ runInEachFileSystem(() => {
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory],
{} /* options */);
{strictNullChecks: true} /* options */);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(1);
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
Expand All @@ -69,7 +80,7 @@ runInEachFileSystem(() => {
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory],
{} /* options */);
{strictNullChecks: true} /* options */);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(0);
});
Expand All @@ -87,7 +98,7 @@ runInEachFileSystem(() => {
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory],
{} /* options */);
{strictNullChecks: true} /* options */);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(0);
});
Expand All @@ -105,7 +116,7 @@ runInEachFileSystem(() => {
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory],
{} /* options */);
{strictNullChecks: true} /* options */);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(0);
});
Expand All @@ -123,7 +134,7 @@ runInEachFileSystem(() => {
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory],
{} /* options */);
{strictNullChecks: true} /* options */);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(0);
});
Expand Down Expand Up @@ -152,7 +163,7 @@ runInEachFileSystem(() => {
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory],
{} /* options */);
{strictNullChecks: true} /* options */);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(1);
expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning);
Expand Down Expand Up @@ -185,7 +196,7 @@ runInEachFileSystem(() => {
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory],
{} /* options */);
{strictNullChecks: true} /* options */);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(0);
});
Expand All @@ -210,7 +221,7 @@ runInEachFileSystem(() => {
const component = getClass(sf, 'TestCmp');
const extendedTemplateChecker = new ExtendedTemplateCheckerImpl(
templateTypeChecker, program.getTypeChecker(), [nullishCoalescingNotNullableFactory],
{} /* options */);
{strictNullChecks: true} /* options */);
const diags = extendedTemplateChecker.getDiagnosticsForComponent(component);
expect(diags.length).toBe(0);
});
Expand All @@ -232,6 +243,7 @@ runInEachFileSystem(() => {
program.getTypeChecker(),
[nullishCoalescingNotNullableFactory],
{
strictNullChecks: true,
extendedDiagnostics: {
checks: {
nullishCoalescingNotNullable: DiagnosticCategoryLabel.Error,
Expand Down

0 comments on commit 366e424

Please sign in to comment.