Skip to content

fix: [no-unnecessary-condition] use assignability APIs in no-unnecessary-condition (POC) #10378

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

fix: [no-unnecessary-condition] use assignability APIs in no-unnecessary-condition (POC) #10378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b6139da
basics in place
kirkwaiblinger Nov 15, 2024
cc887e5
simplify
kirkwaiblinger Nov 16, 2024
da1887c
add back comment
kirkwaiblinger Nov 23, 2024
c2b72da
assignability
kirkwaiblinger Nov 23, 2024
d8b30bc
more tests
kirkwaiblinger Nov 23, 2024
0b8d6b8
comments
kirkwaiblinger Nov 23, 2024
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
2 changes: 1 addition & 1 deletion packages/eslint-plugin/docs/rules/no-unnecessary-condition.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ function assert(condition: unknown): asserts condition {

assert(false); // Unnecessary; condition is always falsy.

const neverNull = {};
const neverNull = { someProperty: 'someValue' };
Copy link
Member

Choose a reason for hiding this comment

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

That's due to TS's (questionable?) decision to give neverNull type {} instead of type object, even when initializing a const variable.

😩 this again... is there a TypeScript issue filed that we can reference? It feels like a bug to me.

I also don't mind it very much. It's not often that folks use {}. This docs change kind of makes it more realistic to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

😩 this again... is there a TypeScript issue filed that we can reference? It feels like a bug to me.

That's a good question. I'll take an action item to investigate this.

I also don't mind it very much. It's not often that folks use {}. This docs change kind of makes it more realistic to me.

Agreed that the docs change is probably an improvement. But, the false negative here makes the rule a little less understandable to me, so I'd lean on the side of wanting it to be correct if we can get it to be.

Copy link
Member Author

@kirkwaiblinger kirkwaiblinger Nov 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, dear, the moment I start playing around with this, I immediately find infuriating behavior around {}....

function takesObject(x: object) {
  if (x) {
    console.log('should always be true')
  } else {
    console.error('this should be unreachable');
  }
}
const o: {} = 0;
// no error here!?!?!?!?
takesObject(o);

I hate the {} type so much.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regarding const o = {}; // has type {}, started discord conversation at https://discord.com/channels/508357248330760243/508357638677856287/1310419206260654081.

Regarding {} -> object assignments, filed microsoft/TypeScript#60582... and immediately learned this is a duplicate of microsoft/TypeScript#56205, in which this behavior is deemed intentional 🫤.

Reminder to self: I'm pretty sure we'll have some good info to add to the motivation for https://typescript-eslint.io/rules/no-empty-object-type/ as an outcome of these discussions 😆

assert(neverNull); // Unnecessary; condition is always truthy.

function isString(value: unknown): value is string {
Expand Down
102 changes: 73 additions & 29 deletions packages/eslint-plugin/src/rules/no-unnecessary-condition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,41 +41,36 @@ const getValueOfLiteralType = (
return type.value;
};

const isFalsyBigInt = (type: ts.Type): boolean => {
return (
tsutils.isLiteralType(type) &&
valueIsPseudoBigInt(type.value) &&
!getValueOfLiteralType(type)
);
};
const isTruthyLiteral = (type: ts.Type): boolean =>
tsutils.isTrueLiteralType(type) ||
(type.isLiteral() && !!getValueOfLiteralType(type));

const isPossiblyFalsy = (type: ts.Type): boolean =>
const isPossiblyFalsy = (
checker: ts.TypeChecker,
type: ts.Type,
falsyTypes: ts.Type[],
): boolean =>
tsutils
.unionTypeParts(type)
// Intersections like `string & {}` can also be possibly falsy,
// requiring us to look into the intersection.
.flatMap(type => tsutils.intersectionTypeParts(type))
// PossiblyFalsy flag includes literal values, so exclude ones that
// are definitely truthy
.filter(t => !isTruthyLiteral(t))
.some(type => isTypeFlagSet(type, ts.TypeFlags.PossiblyFalsy));
.some(type =>
falsyTypes.some(falsyType => checker.isTypeAssignableTo(falsyType, type)),
);

const isPossiblyTruthy = (type: ts.Type): boolean =>
const isPossiblyTruthy = (
checker: ts.TypeChecker,
type: ts.Type,
falsyTypes: ts.Type[],
): boolean =>
tsutils
.unionTypeParts(type)
.map(type => tsutils.intersectionTypeParts(type))
.map(unionPart => tsutils.intersectionTypeParts(unionPart))
.some(intersectionParts =>
// It is possible to define intersections that are always falsy,
// like `"" & { __brand: string }`.
intersectionParts.every(
type =>
!tsutils.isFalsyType(type) &&
// below is a workaround for ts-api-utils bug
// see https://github.com/JoshuaKGoldberg/ts-api-utils/issues/544
!isFalsyBigInt(type),
// It is possible to define intersections that are always falsy,
// like `"" & { __brand: string }`.
intersectionPart =>
!falsyTypes.some(falsyType =>
checker.isTypeAssignableTo(intersectionPart, falsyType),
),
),
);

Expand Down Expand Up @@ -167,6 +162,49 @@ function booleanComparison(
}
}

function getFalsyTypes(checker: ts.TypeChecker) {
return [
{
type: checker.getNullType(),
value: null,
},
{
type: checker.getUndefinedType(),
value: undefined,
},
{
type: checker.getFalseType(),
value: false,
},
{
type: checker.getStringLiteralType(''),
value: '',
},
{
type: checker.getNumberLiteralType(0),
value: 0,
},
// i honestly don't think this matters, but idk :shrug:
{
type: checker.getNumberLiteralType(-0),
value: -0,
},
// this doesn't seem to have any effect. I don't think you can create a NaN literal type in TS.
{
type: checker.getNumberLiteralType(NaN),
value: NaN,
},
// not available in all supported versions; see https://github.com/microsoft/TypeScript/issues/58563
{
type: checker.getBigIntLiteralType({ base10Value: '0', negative: false }),
value: 0n,
},
] as const satisfies {
type: ts.Type;
value: unknown;
}[];
}

// #endregion

export type Options = [
Expand Down Expand Up @@ -287,6 +325,8 @@ export default createRule<Options, MessageId>({
});
}

const falsyTypes = getFalsyTypes(checker).map(({ type }) => type);

function nodeIsArrayType(node: TSESTree.Expression): boolean {
const nodeType = getConstrainedTypeAtLocation(services, node);
return tsutils
Expand Down Expand Up @@ -399,9 +439,9 @@ export default createRule<Options, MessageId>({

if (isTypeFlagSet(type, ts.TypeFlags.Never)) {
messageId = 'never';
} else if (!isPossiblyTruthy(type)) {
} else if (!isPossiblyTruthy(checker, type, falsyTypes)) {
messageId = !isUnaryNotArgument ? 'alwaysFalsy' : 'alwaysTruthy';
} else if (!isPossiblyFalsy(type)) {
} else if (!isPossiblyFalsy(checker, type, falsyTypes)) {
messageId = !isUnaryNotArgument ? 'alwaysTruthy' : 'alwaysFalsy';
}

Expand Down Expand Up @@ -655,13 +695,17 @@ export default createRule<Options, MessageId>({
if (returnTypes.some(t => isTypeAnyType(t) || isTypeUnknownType(t))) {
return;
}
if (!returnTypes.some(isPossiblyFalsy)) {
if (
!returnTypes.some(type => isPossiblyFalsy(checker, type, falsyTypes))
) {
return context.report({
node: callback,
messageId: 'alwaysTruthyFunc',
});
}
if (!returnTypes.some(isPossiblyTruthy)) {
if (
!returnTypes.some(type => isPossiblyTruthy(checker, type, falsyTypes))
) {
return context.report({
node: callback,
messageId: 'alwaysFalsyFunc',
Expand Down
2 changes: 1 addition & 1 deletion packages/eslint-plugin/tests/docs-eslint-output-snapshots/no-unnecessary-condition.shot
View file
Open in desktop

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

66 changes: 60 additions & 6 deletions packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,6 +735,7 @@ if (x) {
allowRuleToRunWithoutStrictNullChecksIKnowWhatIAmDoing: true,
},
],
skip: true,
},
`
interface Foo {
Expand Down Expand Up @@ -982,6 +983,44 @@ isString('falafel');
`,
options: [{ checkTypePredicates: true }],
},
{
// {} can be falsy, so this is fine
code: `
declare let foo: {};
foo &&= 1;
`,
},
{
// { toFixed(): string } can be falsy since 0 is assignable, so this is fine
code: `
declare let foo: { toFixed(): string };
foo &&= 1;
`,
},
{
code: `
function usesEmptyObject(x: {}) {
if (x) {
}
}
`,
},
{
code: `
function usesEmptyObject(x: { toFixed(): string }) {
if (x) {
}
}
`,
},
{
code: `
function usesEmptyObject(x: { toString(): string }) {
if (x) {
}
}
`,
},
],

invalid: [
Expand Down Expand Up @@ -1033,7 +1072,8 @@ switch (b1) {
unnecessaryConditionTest('"always truthy"', 'alwaysTruthy'),
unnecessaryConditionTest(`undefined`, 'alwaysFalsy'),
unnecessaryConditionTest('null', 'alwaysFalsy'),
unnecessaryConditionTest('void', 'alwaysFalsy'),
// generated code is a TS error (void cannot be tested for truthiness)
// unnecessaryConditionTest('void', 'alwaysFalsy'),
unnecessaryConditionTest('never', 'never'),
unnecessaryConditionTest('string & number', 'never'),
// More complex logical expressions
Expand Down Expand Up @@ -1645,7 +1685,7 @@ if (arr.filter) {
function truthy() {
return [];
}
function falsy() {}
function falsy(): undefined {}
[1, 3, 5].filter(truthy);
[1, 2, 3].find(falsy);
[1, 2, 3].findLastIndex(falsy);
Expand Down Expand Up @@ -2318,6 +2358,7 @@ if (x) {
tsconfigRootDir: path.join(rootPath, 'unstrict'),
},
},
skip: true,
},
{
code: `
Expand Down Expand Up @@ -2442,8 +2483,8 @@ foo ??= null;
},
{
code: `
declare let foo: {};
foo ||= 1;
declare let foo: { bar: null };
foo ||= { bar: null };
`,
errors: [
{
Expand Down Expand Up @@ -2472,8 +2513,8 @@ foo ||= null;
},
{
code: `
declare let foo: {};
foo &&= 1;
declare let foo: { bar: string };
foo &&= { bar: 'none' };
`,
errors: [
{
Expand Down Expand Up @@ -2766,5 +2807,18 @@ isString('fa' + 'lafel');
'((string & { __brandA: string }) | (number & { __brandB: string })) & ("foo" | 123)',
'alwaysTruthy',
),
{
code: `
const neverNull: object = {};
if (neverNull) {
}
`,
errors: [
{
line: 3,
messageId: 'alwaysTruthy',
},
],
},
],
});
Loading