Skip to content

feat(compiler-cli): add extended diagnostic for invalid nullish coalescing #60279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mmalerba wants to merge 1 commit into from
Closed

feat(compiler-cli): add extended diagnostic for invalid nullish coalescing #60279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions adev/src/content/reference/extended-diagnostics/NG8114.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.

<docs-code language="typescript">

import {Component, signal, Signal} from '@angular/core';

@Component({
template: `
<button [disabled]="hasPermission() && task()?.disabled ?? true">
Run
</button>
`,
})
class MyComponent {
hasPermission = input(false);
task = input<Task|undefined>(undefined);
}

</docs-code>

## 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.

<docs-code language="typescript">

import {Component, signal, Signal} from '@angular/core';

@Component({
template: `
<button [disabled]="hasPermission() && (task()?.disabled ?? true)">
Run
</button>
`,
})
class MyComponent {
hasPermission = input(false);
task = input<Task|undefined>(undefined);
}

</docs-code>
46 changes: 24 additions & 22 deletions adev/src/content/reference/extended-diagnostics/overview.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,31 @@ 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

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):

Expand All @@ -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"
}
}

}
}
</docs-code>

Expand Down Expand Up @@ -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).
1 change: 1 addition & 0 deletions goldens/public-api/compiler-cli/error_code.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 2 additions & 0 deletions goldens/public-api/compiler-cli/extended_template_diagnostic_name.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
5 changes: 5 additions & 0 deletions packages/compiler-cli/src/ngtsc/diagnostics/src/error_code.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/diagnostics/src/extended_template_diagnostic_name.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
}
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/typecheck/extended/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand Down
14 changes: 14 additions & 0 deletions ...er-cli/src/ngtsc/typecheck/extended/checks/unparenthesized_nullish_coalescing/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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",
],
)
62 changes: 62 additions & 0 deletions ...piler-cli/src/ngtsc/typecheck/extended/checks/unparenthesized_nullish_coalescing/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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<ErrorCode.UNPARENTHESIZED_NULLISH_COALESCING> {
override code = ErrorCode.UNPARENTHESIZED_NULLISH_COALESCING as const;

override visitNode(
ctx: TemplateContext<ErrorCode.UNPARENTHESIZED_NULLISH_COALESCING>,
component: ts.ClassDeclaration,
node: TmplAstNode | AST,
): NgTemplateDiagnostic<ErrorCode.UNPARENTHESIZED_NULLISH_COALESCING>[] {
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(),
};
4 changes: 3 additions & 1 deletion packages/compiler-cli/src/ngtsc/typecheck/extended/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -38,6 +39,7 @@ export const ALL_DIAGNOSTIC_FACTORIES: readonly TemplateCheckFactory<
uninvokedFunctionInEventBindingFactory,
unusedLetDeclarationFactory,
skipHydrationNotStaticFactory,
unparenthesizedNullishCoalescingFactory,
];

export const SUPPORTED_DIAGNOSTIC_NAMES = new Set<string>([
Expand Down
30 changes: 30 additions & 0 deletions ...i/src/ngtsc/typecheck/extended/test/checks/unparenthesized_nullish_coalescing/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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",
],
)
Loading