Skip to content

fix(eslint-plugin): [prefer-nullish-coalescing] treat any/unknown as eligible for nullish coalescing #10865

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ Examples of code for this rule with `{ ignoreConditionalTests: false }`:
<TabItem value="❌ Incorrect">

```ts option='{ "ignoreConditionalTests": false }'
declare const a: string | null;
declare let a: string | null;
declare const b: string | null;

if (a || b) {
Expand All @@ -102,7 +102,7 @@ a || b ? true : false;
<TabItem value="✅ Correct">

```ts option='{ "ignoreConditionalTests": false }'
declare const a: string | null;
declare let a: string | null;
declare const b: string | null;

if (a ?? b) {
Expand Down Expand Up @@ -133,7 +133,7 @@ Examples of code for this rule with `{ ignoreMixedLogicalExpressions: false }`:
<TabItem value="❌ Incorrect">

```ts option='{ "ignoreMixedLogicalExpressions": false }'
declare const a: string | null;
declare let a: string | null;
declare const b: string | null;
declare const c: string | null;
declare const d: string | null;
Expand All @@ -149,7 +149,7 @@ a || (b && c && d);
<TabItem value="✅ Correct">

```ts option='{ "ignoreMixedLogicalExpressions": false }'
declare const a: string | null;
declare let a: string | null;
declare const b: string | null;
declare const c: string | null;
declare const d: string | null;
Expand Down
21 changes: 19 additions & 2 deletions packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,10 @@ import {
getTypeName,
getTypeOfPropertyOfName,
getValueOfLiteralType,
isAlwaysNullish,
isArrayMethodCallWithPredicate,
isIdentifier,
isNullableType,
isPossiblyFalsy,
isPossiblyNullish,
isPossiblyTruthy,
isTypeAnyType,
isTypeFlagSet,
Expand All @@ -31,6 +29,25 @@ import {
} from '../util/assertionFunctionUtils';

// #region

const nullishFlag = ts.TypeFlags.Undefined | ts.TypeFlags.Null;

function isNullishType(type: ts.Type): boolean {
return tsutils.isTypeFlagSet(type, nullishFlag);
}

function isAlwaysNullish(type: ts.Type): boolean {
return tsutils.unionTypeParts(type).every(isNullishType);
}

/**
* Note that this differs from {@link isNullableType} in that it doesn't consider
* `any` or `unknown` to be nullable.
*/
function isPossiblyNullish(type: ts.Type): boolean {
return tsutils.unionTypeParts(type).some(isNullishType);
}

function toStaticValue(
type: ts.Type,
):
Expand Down
37 changes: 28 additions & 9 deletions packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
isNodeEqual,
isNodeOfTypes,
isNullLiteral,
isPossiblyNullish,
isNullableType,
isUndefinedIdentifier,
nullThrows,
NullThrowsReasons,
Expand Down Expand Up @@ -193,7 +193,7 @@ export default createRule<Options, MessageIds>({
* a nullishness check, taking into account the rule's configuration.
*/
function isTypeEligibleForPreferNullish(type: ts.Type): boolean {
if (!isPossiblyNullish(type)) {
if (!isNullableType(type)) {
return false;
}

Expand All @@ -211,14 +211,33 @@ export default createRule<Options, MessageIds>({
]
.filter((flag): flag is number => typeof flag === 'number')
.reduce((previous, flag) => previous | flag, 0);

if (ignorableFlags === 0) {
// any types are eligible for conversion.
return true;
}

// if the type is `any` or `unknown` we can't make any assumptions
// about the value, so it could be any primitive, even though the flags
// won't be set.
//
// technically, this is true of `void` as well, however, it's a TS error
// to test `void` for truthiness, so we don't need to bother checking for
// it in valid code.
if (
tsutils.isTypeFlagSet(type, ts.TypeFlags.Any | ts.TypeFlags.Unknown)
) {
return false;
}

if (
type.flags !== ts.TypeFlags.Null &&
type.flags !== ts.TypeFlags.Undefined &&
(type as ts.UnionOrIntersectionType).types.some(t =>
tsutils
.intersectionTypeParts(t)
.some(t => tsutils.isTypeFlagSet(t, ignorableFlags)),
)
tsutils
.typeParts(type)
.some(t =>
tsutils
.intersectionTypeParts(t)
Copy link
Member

Choose a reason for hiding this comment

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

[Test]: Can this be simplified to use .typeParts() instead of .intersectionTypeParts()? Changing this doesn't seem to fail any test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe... I think I'd like to leave that line as is since technically it wasn't touched in this PR though. (The change that I had to make was because the type assertion type as ts.UnionOrIntersectionType was no longer valid). It's generally a bit of a smell I think if we treated unions and intersections the same shallowly, but not recursively 🤔 But I don't have a test case in mind

.some(t => tsutils.isTypeFlagSet(t, ignorableFlags)),
)
) {
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin/src/util/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export * from './getConstraintInfo';
export * from './getValueOfLiteralType';
export * from './isHigherPrecedenceThanAwait';
export * from './skipChainExpression';
export * from './truthinessAndNullishUtils';
export * from './truthinessUtils';

// this is done for convenience - saves migrating all of the old rules
export * from '@typescript-eslint/type-utils';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,3 @@ export const isPossiblyTruthy = (type: ts.Type): boolean =>
// like `"" & { __brand: string }`.
intersectionParts.every(type => !tsutils.isFalsyType(type)),
);

// Nullish utilities
const nullishFlag = ts.TypeFlags.Undefined | ts.TypeFlags.Null;
const isNullishType = (type: ts.Type): boolean =>
tsutils.isTypeFlagSet(type, nullishFlag);

export const isPossiblyNullish = (type: ts.Type): boolean =>
tsutils.unionTypeParts(type).some(isNullishType);

export const isAlwaysNullish = (type: ts.Type): boolean =>
tsutils.unionTypeParts(type).every(isNullishType);

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading