Skip to content
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.

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

feat(eslint-plugin): [prefer-nullish-coalescing] add ignorePrimitives option #6487

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
b427e6b
issue-4906 - prefer-nullish-coalescing enhancment
Feb 18, 2023
8e016d3
remove comment
Feb 18, 2023
09f207a
add doc
Feb 18, 2023
24fb9c5
CR: sort keys
Feb 24, 2023
7aec9fa
CR: import once
Feb 24, 2023
ed7d525
CR: remove unncessary "as const"
Feb 24, 2023
6954d62
Merge remote-tracking branch 'upstream/main' into issue-4906-prefer-n…
May 27, 2023
6be0360
CR: Fix markdown lint rule
May 27, 2023
a576095
CR: Use binary OR to simplify loop
May 27, 2023
8dd233f
CR: More positive test cases
May 27, 2023
25a0852
CR: mixed
May 27, 2023
e66ee9b
CR: remove implied line
May 27, 2023
5c4ab38
CR: Move the note up
May 27, 2023
3b1e012
prettier fixz
May 27, 2023
2e579a6
Fix coverage, ts didn't know it has a default so it's not optional
May 27, 2023
e38a2a9
Merge branch 'main' into issue-4906-prefer-nullish-enhancment
Jun 3, 2023
7d30fe5
CR1: Remove redundant check
Jun 3, 2023
7907cf5
CR2: Remove redundant check for the type-system
Jun 3, 2023
40c1839
Add bigint option
Jun 3, 2023
d085f34
markdown too
Jun 3, 2023
2dff212
Fixed union test and JSON.stringify(ignorePrimitives)
Jun 3, 2023
bea946b
Merge branch 'main' into issue-4906-prefer-nullish-enhancment
Jun 6, 2023
6d4aa01
Merge branch 'main' into issue-4906-prefer-nullish-enhancment
Jun 12, 2023
357fd59
Merge branch 'main' into issue-4906-prefer-nullish-enhancment
Jun 16, 2023
d3f6ee0
Merge branch 'main' into issue-4906-prefer-nullish-enhancment
Jun 17, 2023
e34fb0a
Merge branch 'main' into issue-4906-prefer-nullish-enhancment
Jun 20, 2023
419a383
Merge branch 'main' into issue-4906-prefer-nullish-enhancment
Jun 24, 2023
81ada98
Merge branch 'main' into issue-4906-prefer-nullish-enhancment
Jun 26, 2023
53bbfba
Merge branch 'main' into issue-4906-prefer-nullish-enhancment
Jun 27, 2023
d1b0347
Merge branch 'main' into issue-4906-prefer-nullish-enhancment
Jun 28, 2023
ef29982
Merge branch 'main' into issue-4906-prefer-nullish-enhancment
Jul 6, 2023
3747796
flattening map
Jul 8, 2023
4b6c03f
flattening map 2
Jul 8, 2023
7207e05
add examples to docs
Jul 8, 2023
5a18ee3
Merge remote-tracking branch 'upstream/main' into issue-4906-prefer-n…
Jul 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 23 additions & 0 deletions packages/eslint-plugin/docs/rules/prefer-nullish-coalescing.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,29 @@ a ?? (b && c && d);

**_NOTE:_** Errors for this specific case will be presented as suggestions (see below), instead of fixes. This is because it is not always safe to automatically convert `||` to `??` within a mixed logical expression, as we cannot tell the intended precedence of the operator. Note that by design, `??` requires parentheses when used with `&&` or `||` in the same expression.

### `ignorePrimitives`
omril1 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Docs] Could you add some examples please? We generally try to -at least for newly added rule options- so users can see exactly how the code looks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to look at the style for examples with options, this rule seems unique in how the examples are presented but decided to keep it the same since I've noticed bradzacher did the same

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh yeah we'll eventually clean all these up... eventually. Thanks!


If you would like to ignore certain primitive types that can be falsy then you may pass an object containing a boolean value for each primitive:

- `string: true`, ignores `null` or `undefined` unions with `string` (default: false).
- `number: true`, ignores `null` or `undefined` unions with `number` (default: false).
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
- `bigint: true`, ignores `null` or `undefined` unions with `bigint` (default: false).
- `boolean: true`, ignores `null` or `undefined` unions with `boolean` (default: false).
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved

Incorrect code for `ignorePrimitives: { string: true }`, and correct code for `ignorePrimitives: { string: false }`:

```ts
const foo: string | undefined = 'bar';
foo || 'a string';
```

Correct code for `ignorePrimitives: { string: true }`:

```ts
const foo: string | undefined = 'bar';
foo ?? 'a string';
```

## When Not To Use It

If you are not using TypeScript 3.7 (or greater), then you will not be able to use this rule, as the operator is not supported.
Expand Down
54 changes: 46 additions & 8 deletions packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,16 @@ import * as util from '../util';

export type Options = [
{
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing?: boolean;
ignoreConditionalTests?: boolean;
ignoreTernaryTests?: boolean;
ignoreMixedLogicalExpressions?: boolean;
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing?: boolean;
ignorePrimitives?: {
bigint?: boolean;
boolean?: boolean;
number?: boolean;
string?: boolean;
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Non-actionable] In theory we could allow this to be a boolean to configure all of the primitives at once. I think it's fine to leave that as a follow-up to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, leaving it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need to open an issue for it or can I reference the same one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a new issue for tracking. It gets confusing having multiple PRs target the same issue.

If you don't have the time to file a new issue, no worries (I do hate asking folks to fill out paperwork). I'll set a reminder for a couple days from now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#7180

ignoreTernaryTests?: boolean;
},
];

Expand Down Expand Up @@ -44,16 +50,25 @@ export default util.createRule<Options, MessageIds>({
{
type: 'object',
properties: {
ignoreConditionalTests: {
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: {
type: 'boolean',
},
ignoreTernaryTests: {
ignoreConditionalTests: {
type: 'boolean',
},
ignoreMixedLogicalExpressions: {
type: 'boolean',
},
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: {
ignorePrimitives: {
type: 'object',
properties: {
bigint: { type: 'boolean' },
boolean: { type: 'boolean' },
number: { type: 'boolean' },
string: { type: 'boolean' },
},
},
ignoreTernaryTests: {
type: 'boolean',
},
},
Expand All @@ -63,20 +78,27 @@ export default util.createRule<Options, MessageIds>({
},
defaultOptions: [
{
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: false,
ignoreConditionalTests: true,
ignoreTernaryTests: true,
ignoreMixedLogicalExpressions: true,
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: false,
ignorePrimitives: {
bigint: false,
boolean: false,
number: false,
string: false,
},
},
],
create(
context,
[
{
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing,
ignoreConditionalTests,
ignoreTernaryTests,
ignoreMixedLogicalExpressions,
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing,
ignorePrimitives,
ignoreTernaryTests,
},
],
) {
Expand Down Expand Up @@ -279,6 +301,22 @@ export default util.createRule<Options, MessageIds>({
return;
}

const ignorableFlags = [
ignorePrimitives!.bigint && ts.TypeFlags.BigInt,
ignorePrimitives!.boolean && ts.TypeFlags.BooleanLiteral,
ignorePrimitives!.number && ts.TypeFlags.Number,
ignorePrimitives!.string && ts.TypeFlags.String,
]
.filter((flag): flag is number => flag !== undefined)
.reduce((previous, flag) => previous | flag, 0);
if (
(type as ts.UnionOrIntersectionType).types.some(t =>
tsutils.isTypeFlagSet(t, ignorableFlags),
)
) {
return;
}

const barBarOperator = util.nullThrows(
sourceCode.getTokenAfter(
node.left,
Expand Down