diff --git a/adev/src/content/reference/extended-diagnostics/NG8114.md b/adev/src/content/reference/extended-diagnostics/NG8114.md new file mode 100644 index 000000000000..b6a594b169db --- /dev/null +++ b/adev/src/content/reference/extended-diagnostics/NG8114.md @@ -0,0 +1,52 @@ +# Unparenthesized Nullish Coalescing + +This diagnostic detects cases where the nullish coalescing operator (`??`) is mixed with the logical +or (`||`) or logical and (`&&`) operators without parentheses to disambiguate precedence. + + + +import {Component, signal, Signal} from '@angular/core'; + +@Component({ + template: ` + + `, +}) +class MyComponent { + hasPermission = input(false); + task = input(undefined); +} + + + +## What's wrong with that? + +Without disambiguating parentheses, its difficult to understand whether the `??` or `||`/`&&` is +evaluated first. This is considered an error in TypeScript and JavaScript, but has historically been +allowed in Angular templates. + +## What should I do instead? + +Always use parentheses to disambiguate in theses situations. If you're unsure what the original +intent of the code was but want to keep the behavior the same, place the parentheses around the `??` +operator. + + + +import {Component, signal, Signal} from '@angular/core'; + +@Component({ + template: ` + + `, +}) +class MyComponent { + hasPermission = input(false); + task = input(undefined); +} + + \ No newline at end of file diff --git a/adev/src/content/reference/extended-diagnostics/overview.md b/adev/src/content/reference/extended-diagnostics/overview.md index 94714804e9d9..a4d8f8bcfc44 100644 --- a/adev/src/content/reference/extended-diagnostics/overview.md +++ b/adev/src/content/reference/extended-diagnostics/overview.md @@ -8,19 +8,20 @@ The Angular compiler includes "extended diagnostics" which identify many of thes Currently, Angular supports the following extended diagnostics: -| Code | Name | -|:---------|:-----------------------------------------------------------------| -| `NG8101` | [`invalidBananaInBox`](extended-diagnostics/NG8101) | -| `NG8102` | [`nullishCoalescingNotNullable`](extended-diagnostics/NG8102) | -| `NG8103` | [`missingControlFlowDirective`](extended-diagnostics/NG8103) | -| `NG8104` | [`textAttributeNotBinding`](extended-diagnostics/NG8104) | -| `NG8105` | [`missingNgForOfLet`](extended-diagnostics/NG8105) | -| `NG8106` | [`suffixNotSupported`](extended-diagnostics/NG8106) | -| `NG8107` | [`optionalChainNotNullable`](extended-diagnostics/NG8107) | -| `NG8108` | [`skipHydrationNotStatic`](extended-diagnostics/NG8108) | -| `NG8109` | [`interpolatedSignalNotInvoked`](extended-diagnostics/NG8109) | -| `NG8111` | [`uninvokedFunctionInEventBinding`](extended-diagnostics/NG8111) | -| `NG8113` | [`unusedStandaloneImports`](extended-diagnostics/NG8113) | +| Code | Name | +| :------- | :---------------------------------------------------------------- | +| `NG8101` | [`invalidBananaInBox`](extended-diagnostics/NG8101) | +| `NG8102` | [`nullishCoalescingNotNullable`](extended-diagnostics/NG8102) | +| `NG8103` | [`missingControlFlowDirective`](extended-diagnostics/NG8103) | +| `NG8104` | [`textAttributeNotBinding`](extended-diagnostics/NG8104) | +| `NG8105` | [`missingNgForOfLet`](extended-diagnostics/NG8105) | +| `NG8106` | [`suffixNotSupported`](extended-diagnostics/NG8106) | +| `NG8107` | [`optionalChainNotNullable`](extended-diagnostics/NG8107) | +| `NG8108` | [`skipHydrationNotStatic`](extended-diagnostics/NG8108) | +| `NG8109` | [`interpolatedSignalNotInvoked`](extended-diagnostics/NG8109) | +| `NG8111` | [`uninvokedFunctionInEventBinding`](extended-diagnostics/NG8111) | +| `NG8113` | [`unusedStandaloneImports`](extended-diagnostics/NG8113) | +| `NG8114` | [`unparenthesizedNullishCoalescing`](extended-diagnostics/NG8114) | ## Configuration @@ -28,10 +29,10 @@ Extended diagnostics are warnings by default and do not block compilation. Each diagnostic can be configured as either: | Error category | Effect | -|:--- | :--- | +| :------------- | :----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `warning` | Default - The compiler emits the diagnostic as a warning but does not block compilation. The compiler will still exist with status code 0, even if warnings are emitted. | | `error` | The compiler emits the diagnostic as an error and fails the compilation. The compiler will exit with a non-zero status code if one or more errors are emitted. | -| `suppress` | The compiler does *not* emit the diagnostic at all. | +| `suppress` | The compiler does _not_ emit the diagnostic at all. | Check severity can be configured as an [Angular compiler option](reference/configs/angular-compiler-options): @@ -48,7 +49,8 @@ Check severity can be configured as an [Angular compiler option](reference/confi // The category to use for any diagnostics not listed in `checks` above. "defaultCategory": "error" } - } + +} } @@ -78,11 +80,11 @@ Defaulting to error is a very powerful tool; just be aware of this semver caveat The Angular team is always open to suggestions about new diagnostics that could be added. Extended diagnostics should generally: -* Detect a common, non-obvious developer mistake with Angular templates -* Clearly articulate why this pattern can lead to bugs or unintended behavior -* Suggest one or more clear solutions -* Have a low, preferably zero, false-positive rate -* Apply to the vast majority of Angular applications (not specific to an unofficial library) -* Improve program correctness or performance (not style, that responsibility falls to a linter) +- Detect a common, non-obvious developer mistake with Angular templates +- Clearly articulate why this pattern can lead to bugs or unintended behavior +- Suggest one or more clear solutions +- Have a low, preferably zero, false-positive rate +- Apply to the vast majority of Angular applications (not specific to an unofficial library) +- Improve program correctness or performance (not style, that responsibility falls to a linter) If you have an idea for an extended diagnostic which fits these criteria, consider filing a [feature request](https://github.com/angular/angular/issues/new?template=2-feature-request.yaml). diff --git a/goldens/public-api/compiler-cli/error_code.api.md b/goldens/public-api/compiler-cli/error_code.api.md index b8412619d50d..cf93df36ca65 100644 --- a/goldens/public-api/compiler-cli/error_code.api.md +++ b/goldens/public-api/compiler-cli/error_code.api.md @@ -105,6 +105,7 @@ export enum ErrorCode { UNDECORATED_CLASS_USING_ANGULAR_FEATURES = 2007, UNDECORATED_PROVIDER = 2005, UNINVOKED_FUNCTION_IN_EVENT_BINDING = 8111, + UNPARENTHESIZED_NULLISH_COALESCING = 8114, UNSUPPORTED_INITIALIZER_API_USAGE = 8110, UNUSED_LET_DECLARATION = 8112, UNUSED_STANDALONE_IMPORTS = 8113, diff --git a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md index ee99d7d58475..a6d1181880b9 100644 --- a/goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md +++ b/goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md @@ -29,6 +29,8 @@ export enum ExtendedTemplateDiagnosticName { // (undocumented) UNINVOKED_FUNCTION_IN_EVENT_BINDING = "uninvokedFunctionInEventBinding", // (undocumented) + UNPARENTHESIZED_NULLISH_COALESCING = "unparenthesizedNullishCoalescing", + // (undocumented) UNUSED_LET_DECLARATION = "unusedLetDeclaration", // (undocumented) UNUSED_STANDALONE_IMPORTS = "unusedStandaloneImports" 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 a5013d246f95..27d2f64bd7c1 100644 --- a/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts +++ b/packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts @@ -518,6 +518,11 @@ export enum ErrorCode { */ UNUSED_STANDALONE_IMPORTS = 8113, + /** + * An expression mixes nullish coalescing and logical and/or without parentheses. + */ + UNPARENTHESIZED_NULLISH_COALESCING = 8114, + /** * 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 c79de4ce6af1..cd0b57011fab 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 @@ -29,4 +29,5 @@ export enum ExtendedTemplateDiagnosticName { CONTROL_FLOW_PREVENTING_CONTENT_PROJECTION = 'controlFlowPreventingContentProjection', UNUSED_LET_DECLARATION = 'unusedLetDeclaration', UNUSED_STANDALONE_IMPORTS = 'unusedStandaloneImports', + UNPARENTHESIZED_NULLISH_COALESCING = 'unparenthesizedNullishCoalescing', } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel index f4527da5c181..8041420af0b7 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel @@ -22,6 +22,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/suffix_not_supported", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/text_attribute_not_binding", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/uninvoked_function_in_event_binding", + "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unparenthesized_nullish_coalescing", "//packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unused_let_declaration", "@npm//typescript", ], diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unparenthesized_nullish_coalescing/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unparenthesized_nullish_coalescing/BUILD.bazel new file mode 100644 index 000000000000..73014d1fbc2f --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unparenthesized_nullish_coalescing/BUILD.bazel @@ -0,0 +1,14 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "unparenthesized_nullish_coalescing", + srcs = ["index.ts"], + visibility = ["//packages/compiler-cli/src/ngtsc:__subpackages__"], + deps = [ + "//packages/compiler", + "//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/unparenthesized_nullish_coalescing/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unparenthesized_nullish_coalescing/index.ts new file mode 100644 index 000000000000..154a99c36092 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/checks/unparenthesized_nullish_coalescing/index.ts @@ -0,0 +1,62 @@ +/** + * @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.dev/license + */ + +import {AST, Binary, TmplAstNode} from '@angular/compiler'; +import ts from 'typescript'; +import {ErrorCode, ExtendedTemplateDiagnosticName} from '../../../../diagnostics'; +import {NgTemplateDiagnostic, SymbolKind} from '../../../api'; +import {TemplateCheckFactory, TemplateCheckWithVisitor, TemplateContext} from '../../api'; + +/** + * Ensures that parentheses are used to disambiguate precedence when nullish coalescing is mixed + * with logical and/or. Returns diagnostics for the cases where parentheses are needed. + */ +class UnparenthesizedNullishCoalescing extends TemplateCheckWithVisitor { + override code = ErrorCode.UNPARENTHESIZED_NULLISH_COALESCING as const; + + override visitNode( + ctx: TemplateContext, + component: ts.ClassDeclaration, + node: TmplAstNode | AST, + ): NgTemplateDiagnostic[] { + if (node instanceof Binary) { + if (node.operation === '&&' || node.operation === '||') { + if ( + (node.left instanceof Binary && node.left.operation === '??') || + (node.right instanceof Binary && node.right.operation === '??') + ) { + const symbol = ctx.templateTypeChecker.getSymbolOfNode(node, component); + if (symbol?.kind !== SymbolKind.Expression) { + return []; + } + const sourceMapping = ctx.templateTypeChecker.getSourceMappingAtTcbLocation( + symbol.tcbLocation, + ); + if (sourceMapping === null) { + return []; + } + const diagnostic = ctx.makeTemplateDiagnostic( + sourceMapping.span, + `Parentheses are required to disambiguate precedence when mixing '??' with '&&' and '||'.`, + ); + return [diagnostic]; + } + } + } + return []; + } +} + +export const factory: TemplateCheckFactory< + ErrorCode.UNPARENTHESIZED_NULLISH_COALESCING, + ExtendedTemplateDiagnosticName.UNPARENTHESIZED_NULLISH_COALESCING +> = { + code: ErrorCode.UNPARENTHESIZED_NULLISH_COALESCING, + name: ExtendedTemplateDiagnosticName.UNPARENTHESIZED_NULLISH_COALESCING, + create: () => new UnparenthesizedNullishCoalescing(), +}; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts index 51cf10c9a38f..f47f5f9f8e35 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts @@ -15,11 +15,12 @@ import {factory as missingControlFlowDirectiveFactory} from './checks/missing_co import {factory as missingNgForOfLetFactory} from './checks/missing_ngforof_let'; import {factory as nullishCoalescingNotNullableFactory} from './checks/nullish_coalescing_not_nullable'; import {factory as optionalChainNotNullableFactory} from './checks/optional_chain_not_nullable'; +import {factory as skipHydrationNotStaticFactory} from './checks/skip_hydration_not_static'; import {factory as suffixNotSupportedFactory} from './checks/suffix_not_supported'; import {factory as textAttributeNotBindingFactory} from './checks/text_attribute_not_binding'; import {factory as uninvokedFunctionInEventBindingFactory} from './checks/uninvoked_function_in_event_binding'; +import {factory as unparenthesizedNullishCoalescingFactory} from './checks/unparenthesized_nullish_coalescing'; import {factory as unusedLetDeclarationFactory} from './checks/unused_let_declaration'; -import {factory as skipHydrationNotStaticFactory} from './checks/skip_hydration_not_static'; export {ExtendedTemplateCheckerImpl} from './src/extended_template_checker'; @@ -38,6 +39,7 @@ export const ALL_DIAGNOSTIC_FACTORIES: readonly TemplateCheckFactory< uninvokedFunctionInEventBindingFactory, unusedLetDeclarationFactory, skipHydrationNotStaticFactory, + unparenthesizedNullishCoalescingFactory, ]; export const SUPPORTED_DIAGNOSTIC_NAMES = new Set([ diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unparenthesized_nullish_coalescing/BUILD.bazel b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unparenthesized_nullish_coalescing/BUILD.bazel new file mode 100644 index 000000000000..06938cfee062 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unparenthesized_nullish_coalescing/BUILD.bazel @@ -0,0 +1,30 @@ +load("//tools:defaults.bzl", "jasmine_node_test", "ts_library") + +ts_library( + name = "test_lib", + testonly = True, + srcs = ["unparenthesized_nullish_coalescing_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/unparenthesized_nullish_coalescing", + "//packages/compiler-cli/src/ngtsc/typecheck/testing", + "@npm//typescript", + ], +) + +jasmine_node_test( + name = "test", + bootstrap = ["//tools/testing:node_no_angular"], + data = [ + "//packages/core:npm_package", + ], + deps = [ + ":test_lib", + ], +) diff --git a/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unparenthesized_nullish_coalescing/unparenthesized_nullish_coalescing_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unparenthesized_nullish_coalescing/unparenthesized_nullish_coalescing_spec.ts new file mode 100644 index 000000000000..b9f1425b99ec --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/typecheck/extended/test/checks/unparenthesized_nullish_coalescing/unparenthesized_nullish_coalescing_spec.ts @@ -0,0 +1,161 @@ +/** + * @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.dev/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 unparenthesizedNullishCoalescingFactory} from '../../../checks/unparenthesized_nullish_coalescing'; +import {ExtendedTemplateCheckerImpl} from '../../../src/extended_template_checker'; + +runInEachFileSystem(() => { + describe('UnparenthesizedNullishCoalescingCheck', () => { + it('binds the error code to its extended template diagnostic name', () => { + expect(unparenthesizedNullishCoalescingFactory.code).toBe( + ErrorCode.UNPARENTHESIZED_NULLISH_COALESCING, + ); + expect(unparenthesizedNullishCoalescingFactory.name).toBe( + ExtendedTemplateDiagnosticName.UNPARENTHESIZED_NULLISH_COALESCING, + ); + }); + + it('should produce warning when mixing nullish coalescing with logical and', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([ + { + fileName, + templates: { + 'TestCmp': `{{ a && b ?? c }}`, + }, + source: 'export class TestCmp { }', + }, + ]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, + program.getTypeChecker(), + [unparenthesizedNullishCoalescingFactory], + {} /* 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.UNPARENTHESIZED_NULLISH_COALESCING)); + expect(getSourceCodeForDiagnostic(diags[0])).toBe(`a && b ?? c`); + }); + + it('should produce warning when mixing nullish coalescing with logical or', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([ + { + fileName, + templates: { + 'TestCmp': `{{ a ?? b || c }}`, + }, + source: 'export class TestCmp { }', + }, + ]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, + program.getTypeChecker(), + [unparenthesizedNullishCoalescingFactory], + {} /* 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.UNPARENTHESIZED_NULLISH_COALESCING)); + expect(getSourceCodeForDiagnostic(diags[0])).toBe(`a ?? b || c`); + }); + + it('should not produce warning when mixing nullish coalescing with logical and with parens', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([ + { + fileName, + templates: { + 'TestCmp': `{{ a && (b ?? c) }}`, + }, + source: 'export class TestCmp { }', + }, + ]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, + program.getTypeChecker(), + [unparenthesizedNullishCoalescingFactory], + {} /* options */, + ); + const diags = extendedTemplateChecker.getDiagnosticsForComponent(component); + expect(diags.length).toBe(0); + }); + + it('should produce warning when mixing nullish coalescing with logical or', () => { + const fileName = absoluteFrom('/main.ts'); + const {program, templateTypeChecker} = setup([ + { + fileName, + templates: { + 'TestCmp': `{{ a ?? (b || c) }}`, + }, + source: 'export class TestCmp { }', + }, + ]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, + program.getTypeChecker(), + [unparenthesizedNullishCoalescingFactory], + {} /* 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': `{{ a && b ?? c }}`, + }, + source: 'export class TestCmp { }', + }, + ]); + const sf = getSourceFileOrError(program, fileName); + const component = getClass(sf, 'TestCmp'); + + const extendedTemplateChecker = new ExtendedTemplateCheckerImpl( + templateTypeChecker, + program.getTypeChecker(), + [unparenthesizedNullishCoalescingFactory], + { + extendedDiagnostics: { + checks: { + unparenthesizedNullishCoalescing: 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.UNPARENTHESIZED_NULLISH_COALESCING)); + }); + }); +});