From 59ed6056e6fb8bb3338fd1ca4db98059398d3d95 Mon Sep 17 00:00:00 2001 From: MBuchalik Date: Sun, 13 Aug 2023 13:15:04 +0200 Subject: [PATCH 1/2] fix(eslint-plugin): [no-unnecessary-condition] false positives with branded types (#7293) --- packages/eslint-plugin/src/rules/no-unnecessary-condition.ts | 3 +++ .../tests/rules/no-unnecessary-condition.test.ts | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 007a8a07a6d6..7ff2bcf2eec4 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -28,6 +28,9 @@ const isTruthyLiteral = (type: ts.Type): boolean => const isPossiblyFalsy = (type: 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)) diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts index c68c735ae11e..7d6369b5ff1b 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -83,6 +83,11 @@ const result2 = foo() == null; necessaryConditionTest('null | object'), necessaryConditionTest('undefined | true'), necessaryConditionTest('void | true'), + // "branded" type + necessaryConditionTest('string & {}'), + necessaryConditionTest('string & { __brand: string }'), + necessaryConditionTest('number & {}'), + necessaryConditionTest('boolean & {}'), necessaryConditionTest('any'), // any necessaryConditionTest('unknown'), // unknown From aef1214e339646d9241fc6b958079aadcd65c81b Mon Sep 17 00:00:00 2001 From: MBuchalik Date: Sat, 19 Aug 2023 11:26:40 +0200 Subject: [PATCH 2/2] Improve test coverage and handle branded types in 'isPossiblyTruthy' --- .../src/rules/no-unnecessary-condition.ts | 9 ++- .../rules/no-unnecessary-condition.test.ts | 60 ++++++++++++++++++- 2 files changed, 66 insertions(+), 3 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 7ff2bcf2eec4..c5de895264ed 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -37,7 +37,14 @@ const isPossiblyFalsy = (type: ts.Type): boolean => .some(type => isTypeFlagSet(type, ts.TypeFlags.PossiblyFalsy)); const isPossiblyTruthy = (type: ts.Type): boolean => - tsutils.unionTypeParts(type).some(type => !tsutils.isFalsyType(type)); + tsutils + .unionTypeParts(type) + .map(type => tsutils.intersectionTypeParts(type)) + .some(intersectionParts => + // It is possible to define intersections that are always falsy, + // like `"" & { __brand: string }`. + intersectionParts.every(type => !tsutils.isFalsyType(type)), + ); // Nullish utilities const nullishFlag = ts.TypeFlags.Undefined | ts.TypeFlags.Null; diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts index 7d6369b5ff1b..b72eb4f13c19 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -86,8 +86,31 @@ const result2 = foo() == null; // "branded" type necessaryConditionTest('string & {}'), necessaryConditionTest('string & { __brand: string }'), - necessaryConditionTest('number & {}'), - necessaryConditionTest('boolean & {}'), + necessaryConditionTest('number & { __brand: string }'), + necessaryConditionTest('boolean & { __brand: string }'), + necessaryConditionTest('bigint & { __brand: string }'), + necessaryConditionTest('string & {} & { __brand: string }'), + necessaryConditionTest( + 'string & { __brandA: string } & { __brandB: string }', + ), + necessaryConditionTest('string & { __brand: string } | number'), + necessaryConditionTest('(string | number) & { __brand: string }'), + necessaryConditionTest('string & ({ __brand: string } | number)'), + necessaryConditionTest('("" | "foo") & { __brand: string }'), + necessaryConditionTest( + '(string & { __brandA: string }) | (number & { __brandB: string })', + ), + necessaryConditionTest( + '((string & { __brandA: string }) | (number & { __brandB: string }) & ("" | "foo"))', + ), + necessaryConditionTest( + '{ __brandA: string} & (({ __brandB: string } & string) | ({ __brandC: string } & number))', + ), + necessaryConditionTest( + '(string | number) & ("foo" | 123 | { __brandA: string })', + ), + + necessaryConditionTest('string & string'), necessaryConditionTest('any'), // any necessaryConditionTest('unknown'), // unknown @@ -650,6 +673,7 @@ const t1 = b2 && b1 ? 'yes' : 'no'; unnecessaryConditionTest('null', 'alwaysFalsy'), unnecessaryConditionTest('void', 'alwaysFalsy'), unnecessaryConditionTest('never', 'never'), + unnecessaryConditionTest('string & number', 'never'), // More complex logical expressions { @@ -1826,5 +1850,37 @@ foo &&= null; }, ], }, + + // "branded" types + unnecessaryConditionTest('"" & {}', 'alwaysFalsy'), + unnecessaryConditionTest('"" & { __brand: string }', 'alwaysFalsy'), + unnecessaryConditionTest( + '("" | false) & { __brand: string }', + 'alwaysFalsy', + ), + unnecessaryConditionTest( + '((string & { __brandA: string }) | (number & { __brandB: string })) & ""', + 'alwaysFalsy', + ), + unnecessaryConditionTest( + '("foo" | "bar") & { __brand: string }', + 'alwaysTruthy', + ), + unnecessaryConditionTest( + '(123 | true) & { __brand: string }', + 'alwaysTruthy', + ), + unnecessaryConditionTest( + '(string | number) & ("foo" | 123) & { __brand: string }', + 'alwaysTruthy', + ), + unnecessaryConditionTest( + '((string & { __brandA: string }) | (number & { __brandB: string })) & "foo"', + 'alwaysTruthy', + ), + unnecessaryConditionTest( + '((string & { __brandA: string }) | (number & { __brandB: string })) & ("foo" | 123)', + 'alwaysTruthy', + ), ], });