diff --git a/goldens/public-api/compiler-cli/error_code.md b/goldens/public-api/compiler-cli/error_code.md index a98edb2a7f8cfd..82f55e621025c2 100644 --- a/goldens/public-api/compiler-cli/error_code.md +++ b/goldens/public-api/compiler-cli/error_code.md @@ -59,6 +59,7 @@ export enum ErrorCode { NGMODULE_REEXPORT_NAME_COLLISION = 6006, NGMODULE_VE_DEPENDENCY_ON_IVY_LIB = 6999, NULLISH_COALESCING_NOT_NULLABLE = 8102, + OPTIONAL_CHAIN_NOT_NULLABLE = 8105, // (undocumented) PARAM_MISSING_TOKEN = 2003, // (undocumented) diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts index 9b112352967961..328a6ed1c4df34 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -256,6 +256,18 @@ export enum ErrorCode { */ TEXT_ATTRIBUTE_NOT_BINDING = 8104, + /** + * The left side of an optional chain operation is not nullable. + * + * ``` + * {{ foo?.bar }} + * {{ foo?.['bar'] }} + * {{ foo?.() }} + * ``` + * When the type of foo doesn't include `null` or `undefined`. + */ + OPTIONAL_CHAIN_NOT_NULLABLE = 8105, + /** * The template type-checking engine would need to generate an inline type check block for a * component, but the current type-checking environment doesn't support it. diff --git a/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts b/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts index 8002e7a9ffbf76..dded5d7fe621f9 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts @@ -18,6 +18,7 @@ export enum ExtendedTemplateDiagnosticName { INVALID_BANANA_IN_BOX = 'invalidBananaInBox', NULLISH_COALESCING_NOT_NULLABLE = 'nullishCoalescingNotNullable', + OPTIONAL_CHAIN_NOT_NULLABLE = 'optionalChainNotNullable', MISSING_CONTROL_FLOW_DIRECTIVE = 'missingControlFlowDirective', TEXT_ATTRIBUTE_NOT_BINDING = 'textAttributeNotBinding', } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel index 9109bdd8b6a803..4cdaaf3f02ba45 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel @@ -15,6 +15,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/invalid_banana_in_box", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/missing_control_flow_directive", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/nullish_coalescing_not_nullable", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/optional_chain_not_nullable", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding", "@npm//typescript", ], diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/optional_chain_not_nullable/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/optional_chain_not_nullable/BUILD.bazel new file mode 100644 index 00000000000000..12ef6aa30bbcc6 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/optional_chain_not_nullable/BUILD.bazel @@ -0,0 +1,15 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "optional_chain_not_nullable", + srcs = ["index.ts"], + visibility = ["//packages/compiler-cli/src/ngtsc:__subpackages__"], + deps = [ + "//packages/compiler", + "//packages/compiler-cli/src/ngtsc/core:api", + "//packages/compiler-cli/src/ngtsc/diagnostics", + "//packages/compiler-cli/src/ngtsc/typecheck/api", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/api", + "@npm//typescript", + ], +) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/optional_chain_not_nullable/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/optional_chain_not_nullable/index.ts new file mode 100644 index 00000000000000..520145834d275b --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/optional_chain_not_nullable/index.ts @@ -0,0 +1,85 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {AST, SafeCall, SafeKeyedRead, SafePropertyRead, TmplAstNode} from '@angular/compiler'; +import ts from 'typescript'; + +import {NgCompilerOptions} from '../../../../core/api'; +import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics'; +import {NgTemplateDiagnostic, SymbolKind} from '../../../api'; +import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api'; + +/** + * Ensures the left side of an optional chain operation is nullable. + * Returns diagnostics for the cases where the operator is useless. + * This check should only be use if `strictNullChecks` is enabled, + * otherwise it would produce inaccurate results. + */ +class OptionalChainNotNullableCheck extends + TemplateCheckWithVisitor { + override code = ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE as const; + + override visitNode( + ctx: TemplateContext, component: ts.ClassDeclaration, + node: TmplAstNode|AST): NgTemplateDiagnostic[] { + if (!(node instanceof SafeCall) && !(node instanceof SafePropertyRead) && + !(node instanceof SafeKeyedRead)) + return []; + + const symbolLeft = ctx.templateTypeChecker.getSymbolOfNode(node.receiver, component); + if (symbolLeft === null || symbolLeft.kind !== SymbolKind.Expression) { + return []; + } + const typeLeft = symbolLeft.tsType; + if (typeLeft.flags & (ts.TypeFlags.Any | ts.TypeFlags.Unknown)) { + // We should not make assumptions about the any and unknown types; using a nullish coalescing + // operator is acceptable for those. + return []; + } + + // If the left operand's type is different from its non-nullable self, then it must + // contain a null or undefined so this nullish coalescing operator is useful. No diagnostic to + // report. + if (typeLeft.getNonNullableType() !== typeLeft) return []; + + const symbol = ctx.templateTypeChecker.getSymbolOfNode(node, component)!; + if (symbol.kind !== SymbolKind.Expression) { + return []; + } + const templateMapping = + ctx.templateTypeChecker.getTemplateMappingAtTcbLocation(symbol.tcbLocation); + if (templateMapping === null) { + return []; + } + const advice = node instanceof SafePropertyRead ? + `the '?.' operator can be replaced with the '.' operator` : + `the '?.' operator can be safely removed`; + const diagnostic = ctx.makeTemplateDiagnostic( + templateMapping.span, + `The left side of this optional chain operation does not include 'null' or 'undefined' in its type, therefore ${ + advice}.`); + return [diagnostic]; + } +} + +export const factory: TemplateCheckFactory< + ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE, + ExtendedTemplateDiagnosticName.OPTIONAL_CHAIN_NOT_NULLABLE> = { + code: ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE, + name: ExtendedTemplateDiagnosticName.OPTIONAL_CHAIN_NOT_NULLABLE, + create: (options: NgCompilerOptions) => { + // Require `strictNullChecks` to be enabled. + const strictNullChecks = + options.strictNullChecks === undefined ? !!options.strict : !!options.strictNullChecks; + if (!strictNullChecks) { + return null; + } + + return new OptionalChainNotNullableCheck(); + }, +}; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts index 5823b60bcfeef6..3e15ebb1091cba 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts @@ -12,6 +12,7 @@ import {TemplateCheckFactory} from './api'; import {factory as invalidBananaInBoxFactory} from './checks/invalid_banana_in_box'; import {factory as missingControlFlowDirectiveFactory} from './checks/missing_control_flow_directive'; import {factory as nullishCoalescingNotNullableFactory} from './checks/nullish_coalescing_not_nullable'; +import {factory as optionalChainNotNullableFactory} from './checks/optional_chain_not_nullable'; import {factory as textAttributeNotBindingFactory} from './checks/text_attribute_not_binding'; export {ExtendedTemplateCheckerImpl} from './src/extended_template_checker'; @@ -20,6 +21,7 @@ export const ALL_DIAGNOSTIC_FACTORIES: readonly TemplateCheckFactory[] = [ invalidBananaInBoxFactory, nullishCoalescingNotNullableFactory, + optionalChainNotNullableFactory, missingControlFlowDirectiveFactory, textAttributeNotBindingFactory, ]; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/optional_chain_not_nullable/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/optional_chain_not_nullable/BUILD.bazel new file mode 100644 index 00000000000000..8d4994813e9363 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/optional_chain_not_nullable/BUILD.bazel @@ -0,0 +1,27 @@ +load("//tools:defaults.bzl", "jasmine_node_test", "ts_library") + +ts_library( + name = "test_lib", + testonly = True, + srcs = ["optional_chain_not_nullable_spec.ts"], + deps = [ + "//packages/compiler", + "//packages/compiler-cli/src/ngtsc/core:api", + "//packages/compiler-cli/src/ngtsc/diagnostics", + "//packages/compiler-cli/src/ngtsc/file_system", + "//packages/compiler-cli/src/ngtsc/file_system/testing", + "//packages/compiler-cli/src/ngtsc/testing", + "//packages/compiler-cli/src/ngtsc/typecheck/extended", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/optional_chain_not_nullable", + "//packages/compiler-cli/src/ngtsc/typecheck/testing", + "@npm//typescript", + ], +) + +jasmine_node_test( + name = "test", + bootstrap = ["//tools/testing:node_no_angular_es2015"], + deps = [ + ":test_lib", + ], +) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/optional_chain_not_nullable/optional_chain_not_nullable_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/optional_chain_not_nullable/optional_chain_not_nullable_spec.ts new file mode 100644 index 00000000000000..b6bf8a1aee94a9 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/optional_chain_not_nullable/optional_chain_not_nullable_spec.ts @@ -0,0 +1,267 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {DiagnosticCategoryLabel} from '@angular/compiler-cli/src/ngtsc/core/api'; +import ts from 'typescript'; + +import {ErrorCode, ExtendedTemplateDiagnosticName, ngErrorCode} from '../../../../../diagnostics'; +import {absoluteFrom, getSourceFileOrError} from '../../../../../file_system'; +import {runInEachFileSystem} from '../../../../../file_system/testing'; +import {getSourceCodeForDiagnostic} from '../../../../../testing'; +import {getClass, setup} from '../../../../testing'; +import {factory as optionalChainNotNullableFactory} from '../../../checks/optional_chain_not_nullable'; +import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker'; + +runInEachFileSystem(() => { + describe('OptionalChainNotNullableCheck', () => { + it('binds the error code to its extended template diagnostic name', () => { + expect(optionalChainNotNullableFactory.code).toBe(ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE); + expect(optionalChainNotNullableFactory.name) + .toBe(ExtendedTemplateDiagnosticName.OPTIONAL_CHAIN_NOT_NULLABLE); + }); + + it('should return a check if `strictNullChecks` is enabled', () => { + expect(optionalChainNotNullableFactory.create({strictNullChecks: true})).toBeDefined(); + }); + + it('should return a check if `strictNullChecks` is not configured but `strict` is enabled', + () => { + expect(optionalChainNotNullableFactory.create({strict: true})).toBeDefined(); + }); + + it('should not return a check if `strictNullChecks` is disabled', () => { + expect(optionalChainNotNullableFactory.create({strictNullChecks: false})).toBeNull(); + expect(optionalChainNotNullableFactory.create({})).toBeNull(); // Defaults disabled. + }); + + it('should not return a check if `strict` is enabled but `strictNullChecks` is disabled', + () => { + expect(optionalChainNotNullableFactory.create({strict: true, strictNullChecks: false})) + .toBeNull(); + }); + + it('should produce optional chain warning for property access', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `{{ var1?.bar }}`, + }, + source: 'export class TestCmp { var1: { foo: string } = { foo: "bar" }; }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [optionalChainNotNullableFactory], + {strictNullChecks: true} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE)); + expect(diags[0].messageText) + .toContain(`the '?.' operator can be replaced with the '.' operator`); + expect(getSourceCodeForDiagnostic(diags[0])).toBe(`bar`); + }); + + it('should produce optional chain warning for indexed access', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `{{ var1?.['bar'] }}`, + }, + source: 'export class TestCmp { var1: { foo: string } = { foo: "bar" }; }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [optionalChainNotNullableFactory], + {strictNullChecks: true} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE)); + expect(diags[0].messageText).toContain(`the '?.' operator can be safely removed`); + expect(getSourceCodeForDiagnostic(diags[0])).toBe(`var1?.['bar']`); + }); + + it('should produce optional chain warning for method call', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `{{ foo?.() }}`, + }, + source: 'export class TestCmp { foo: () => string }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [optionalChainNotNullableFactory], + {strictNullChecks: true} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE)); + expect(diags[0].messageText).toContain(`the '?.' operator can be safely removed`); + expect(getSourceCodeForDiagnostic(diags[0])).toBe(`foo?.()`); + }); + + it('should produce optional chain warning for classes with inline TCBs', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup( + [{ + fileName, + templates: { + 'TestCmp': `{{ var1?.bar }}`, + }, + source: 'class TestCmp { var1: { foo: string } = { foo: "bar" }; }' + }], + {inlining: true}); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [optionalChainNotNullableFactory], + {strictNullChecks: true} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Warning); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE)); + expect(getSourceCodeForDiagnostic(diags[0])).toBe(`bar`); + }); + + it('should not produce optional chain warning for a nullable type', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `{{ var1?.bar }}`, + }, + source: 'export class TestCmp { var1: string | null = "text"; }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [optionalChainNotNullableFactory], + {strictNullChecks: true} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(0); + }); + + it('should not produce optional chain warning for the any type', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `{{ var1?.bar }}`, + }, + source: 'export class TestCmp { var1: any; }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [optionalChainNotNullableFactory], + {strictNullChecks: true} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(0); + }); + + it('should not produce optional chain warning for the unknown type', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `{{ var1?.bar }}`, + }, + source: 'export class TestCmp { var1: unknown; }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [optionalChainNotNullableFactory], + {strictNullChecks: true} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(0); + }); + + it('should not produce optional chain warning for a type that includes undefined', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `{{ var1?.bar }}`, + }, + source: 'export class TestCmp { var1: string | undefined = "text"; }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [optionalChainNotNullableFactory], + {strictNullChecks: true} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(0); + }); + + it('should not produce optional chain warning when the left side is a nullable expression', + () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([ + { + fileName, + templates: { + 'TestCmp': `{{ func()?.foo }}`, + }, + source: ` + export class TestCmp { + func = (): { foo: string } | null => null; + } + `, + }, + ]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, program.getTypeChecker(), [optionalChainNotNullableFactory], + {strictNullChecks: true} /* options */); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(0); + }); + + it('should respect configured diagnostic category', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([{ + fileName, + templates: { + 'TestCmp': `{{ var1?.bar }}`, + }, + source: 'export class TestCmp { var1: { foo: string } = { foo: "bar" }; }' + }]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, + program.getTypeChecker(), + [optionalChainNotNullableFactory], + { + strictNullChecks: true, + extendedDiagnostics: { + checks: { + optionalChainNotNullable: DiagnosticCategoryLabel.Error, + }, + }, + }, + ); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + + expect(diags.length).toBe(1); + expect(diags[0].category).toBe(ts.DiagnosticCategory.Error); + expect(diags[0].code).toBe(ngErrorCode(ErrorCode.OPTIONAL_CHAIN_NOT_NULLABLE)); + }); + }); +});