From ad5760a8913a053ef4fe26524c9d14f61f930155 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Wed, 4 Dec 2024 23:13:19 -0700 Subject: [PATCH 1/3] better boolean literal compare message --- .../src/rules/no-unnecessary-condition.ts | 28 +++- .../rules/no-unnecessary-condition.test.ts | 128 +++++++++++++++--- 2 files changed, 138 insertions(+), 18 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index c4bed147eb88..0bfe46d28efc 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -212,7 +212,7 @@ export default createRule({ alwaysTruthyFunc: 'This callback should return a conditional, but return is always truthy.', literalBooleanExpression: - 'Unnecessary conditional, comparison is always {{trueOrFalse}}. Both sides of the comparison always have a literal type.', + 'Unnecessary conditional, comparison is always {{trueOrFalse}}, since `{{left}} {{operator}} {{right}}` is {{trueOrFalse}}.', never: 'Unnecessary conditional, value is `never`.', neverNullish: 'Unnecessary conditional, expected left-hand side of `??` operator to be possibly null or undefined.', @@ -491,6 +491,9 @@ export default createRule({ node, messageId: 'literalBooleanExpression', data: { + left: valueOfLiteralTypeToExpression(leftStaticValue.value), + operator, + right: valueOfLiteralTypeToExpression(rightStaticValue.value), trueOrFalse: conditionIsTrue ? 'true' : 'false', }, }); @@ -892,3 +895,26 @@ export default createRule({ }; }, }); + +/** + * Converts a value to its source code representation as a literal. It's + * assumed that the input is a primitive that has such a representation. + * + * @example + * null => 'null' + * undefined => 'undefined' + * true => 'true' + * 42n => '42n' + * 'foo' => '"foo"' + * 23 => '23' + */ +function valueOfLiteralTypeToExpression(value: unknown): string { + if (typeof value === 'bigint') { + return `${value}n`; + } + if (value === undefined) { + return 'undefined'; + } + // covers null, boolean, number, string + return JSON.stringify(value); +} 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 7aaed5539e21..fc79a35985e0 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -1020,7 +1020,12 @@ switch (b1) { { column: 18, line: 16, messageId: 'alwaysTruthy' }, { column: 8, - data: { trueOrFalse: 'true' }, + data: { + left: 'true', + operator: '===', + right: 'true', + trueOrFalse: 'true', + }, line: 18, messageId: 'literalBooleanExpression', }, @@ -1116,6 +1121,9 @@ function test(a: 'a') { { column: 10, data: { + left: '"a"', + operator: '===', + right: '"a"', trueOrFalse: 'true', }, line: 3, @@ -1132,6 +1140,9 @@ a > b; errors: [ { data: { + left: '"34"', + operator: '>', + right: '"56"', trueOrFalse: 'false', }, line: 4, @@ -1147,7 +1158,12 @@ if (y === 0) { `, errors: [ { - data: { trueOrFalse: 'false' }, + data: { + left: '1', + operator: '===', + right: '0', + trueOrFalse: 'false', + }, line: 3, messageId: 'literalBooleanExpression', }, @@ -1161,7 +1177,12 @@ if (1 == '1') { `, errors: [ { - data: { trueOrFalse: 'true' }, + data: { + left: '1', + operator: '==', + right: '"1"', + trueOrFalse: 'true', + }, line: 3, messageId: 'literalBooleanExpression', }, @@ -1173,7 +1194,12 @@ if (1 == '1') { `, errors: [ { - data: { trueOrFalse: 'false' }, + data: { + left: '2.3', + operator: '>', + right: '2.3', + trueOrFalse: 'false', + }, line: 2, messageId: 'literalBooleanExpression', }, @@ -1185,7 +1211,12 @@ if (1 == '1') { `, errors: [ { - data: { trueOrFalse: 'true' }, + data: { + left: '2.3', + operator: '>=', + right: '2.3', + trueOrFalse: 'true', + }, line: 2, messageId: 'literalBooleanExpression', }, @@ -1197,7 +1228,12 @@ if (1 == '1') { `, errors: [ { - data: { trueOrFalse: 'false' }, + data: { + left: '2n', + operator: '<', + right: '2n', + trueOrFalse: 'false', + }, line: 2, messageId: 'literalBooleanExpression', }, @@ -1209,7 +1245,12 @@ if (1 == '1') { `, errors: [ { - data: { trueOrFalse: 'true' }, + data: { + left: '2n', + operator: '<=', + right: '2n', + trueOrFalse: 'true', + }, line: 2, messageId: 'literalBooleanExpression', }, @@ -1221,7 +1262,12 @@ if (1 == '1') { `, errors: [ { - data: { trueOrFalse: 'true' }, + data: { + left: '-2n', + operator: '!==', + right: '2n', + trueOrFalse: 'true', + }, line: 2, messageId: 'literalBooleanExpression', }, @@ -1235,7 +1281,12 @@ if (1 == '2') { `, errors: [ { - data: { trueOrFalse: 'false' }, + data: { + left: '1', + operator: '==', + right: '"2"', + trueOrFalse: 'false', + }, line: 3, messageId: 'literalBooleanExpression', }, @@ -1249,7 +1300,12 @@ if (1 != '2') { `, errors: [ { - data: { trueOrFalse: 'true' }, + data: { + left: '1', + operator: '!=', + right: '"2"', + trueOrFalse: 'true', + }, line: 3, messageId: 'literalBooleanExpression', }, @@ -1270,6 +1326,9 @@ if (x === Foo.a) { { column: 5, data: { + left: 1, + operator: '===', + right: 1, trueOrFalse: 'true', }, line: 8, @@ -1289,7 +1348,12 @@ function takesMaybeValue(a: null | object) { errors: [ { column: 14, - data: { trueOrFalse: 'true' }, + data: { + left: 'null', + operator: '==', + right: 'undefined', + trueOrFalse: 'true', + }, endColumn: 28, endLine: 4, line: 4, @@ -1309,7 +1373,12 @@ function takesMaybeValue(a: null | object) { errors: [ { column: 14, - data: { trueOrFalse: 'false' }, + data: { + left: 'null', + operator: '===', + right: 'undefined', + trueOrFalse: 'false', + }, endColumn: 29, endLine: 4, line: 4, @@ -1329,7 +1398,12 @@ function takesMaybeValue(a: null | object) { errors: [ { column: 14, - data: { trueOrFalse: 'false' }, + data: { + left: 'null', + operator: '!=', + right: 'undefined', + trueOrFalse: 'false', + }, endColumn: 28, endLine: 4, line: 4, @@ -1349,7 +1423,12 @@ function takesMaybeValue(a: null | object) { errors: [ { column: 14, - data: { trueOrFalse: 'true' }, + data: { + left: 'null', + operator: '!==', + right: 'undefined', + trueOrFalse: 'true', + }, endColumn: 29, endLine: 4, line: 4, @@ -1363,7 +1442,12 @@ true === false; `, errors: [ { - data: { trueOrFalse: 'false' }, + data: { + left: 'true', + operator: '===', + right: 'false', + trueOrFalse: 'false', + }, messageId: 'literalBooleanExpression', }, ], @@ -1374,7 +1458,12 @@ true === true; `, errors: [ { - data: { trueOrFalse: 'true' }, + data: { + left: 'true', + operator: '===', + right: 'true', + trueOrFalse: 'true', + }, messageId: 'literalBooleanExpression', }, ], @@ -1385,7 +1474,12 @@ true === undefined; `, errors: [ { - data: { trueOrFalse: 'false' }, + data: { + left: 'true', + operator: '===', + right: 'undefined', + trueOrFalse: 'false', + }, messageId: 'literalBooleanExpression', }, ], From ef65a13a76c52ab5abc3b0d7c8d8dd4a69c4b92c Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Fri, 6 Dec 2024 10:30:47 -0700 Subject: [PATCH 2/3] rename --- .../src/rules/no-unnecessary-condition.ts | 6 +-- .../rules/no-unnecessary-condition.test.ts | 40 +++++++++---------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index 0bfe46d28efc..4a014e92f806 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -183,7 +183,7 @@ export type MessageId = | 'alwaysNullish' | 'alwaysTruthy' | 'alwaysTruthyFunc' - | 'literalBooleanExpression' + | 'comparisonBetweenLiteralTypes' | 'never' | 'neverNullish' | 'neverOptionalChain' @@ -211,7 +211,7 @@ export default createRule({ alwaysTruthy: 'Unnecessary conditional, value is always truthy.', alwaysTruthyFunc: 'This callback should return a conditional, but return is always truthy.', - literalBooleanExpression: + comparisonBetweenLiteralTypes: 'Unnecessary conditional, comparison is always {{trueOrFalse}}, since `{{left}} {{operator}} {{right}}` is {{trueOrFalse}}.', never: 'Unnecessary conditional, value is `never`.', neverNullish: @@ -489,7 +489,7 @@ export default createRule({ context.report({ node, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', data: { left: valueOfLiteralTypeToExpression(leftStaticValue.value), operator, 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 fc79a35985e0..0e9a955b4f20 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -1027,7 +1027,7 @@ switch (b1) { trueOrFalse: 'true', }, line: 18, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1127,7 +1127,7 @@ function test(a: 'a') { trueOrFalse: 'true', }, line: 3, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1146,7 +1146,7 @@ a > b; trueOrFalse: 'false', }, line: 4, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1165,7 +1165,7 @@ if (y === 0) { trueOrFalse: 'false', }, line: 3, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1184,7 +1184,7 @@ if (1 == '1') { trueOrFalse: 'true', }, line: 3, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1201,7 +1201,7 @@ if (1 == '1') { trueOrFalse: 'false', }, line: 2, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1218,7 +1218,7 @@ if (1 == '1') { trueOrFalse: 'true', }, line: 2, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1235,7 +1235,7 @@ if (1 == '1') { trueOrFalse: 'false', }, line: 2, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1252,7 +1252,7 @@ if (1 == '1') { trueOrFalse: 'true', }, line: 2, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1269,7 +1269,7 @@ if (1 == '1') { trueOrFalse: 'true', }, line: 2, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1288,7 +1288,7 @@ if (1 == '2') { trueOrFalse: 'false', }, line: 3, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1307,7 +1307,7 @@ if (1 != '2') { trueOrFalse: 'true', }, line: 3, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1332,7 +1332,7 @@ if (x === Foo.a) { trueOrFalse: 'true', }, line: 8, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1357,7 +1357,7 @@ function takesMaybeValue(a: null | object) { endColumn: 28, endLine: 4, line: 4, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1382,7 +1382,7 @@ function takesMaybeValue(a: null | object) { endColumn: 29, endLine: 4, line: 4, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1407,7 +1407,7 @@ function takesMaybeValue(a: null | object) { endColumn: 28, endLine: 4, line: 4, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1432,7 +1432,7 @@ function takesMaybeValue(a: null | object) { endColumn: 29, endLine: 4, line: 4, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1448,7 +1448,7 @@ true === false; right: 'false', trueOrFalse: 'false', }, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1464,7 +1464,7 @@ true === true; right: 'true', trueOrFalse: 'true', }, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, @@ -1480,7 +1480,7 @@ true === undefined; right: 'undefined', trueOrFalse: 'false', }, - messageId: 'literalBooleanExpression', + messageId: 'comparisonBetweenLiteralTypes', }, ], }, From 3d1186ac30e45f7cf1f3a4b449363ba29a0ffb16 Mon Sep 17 00:00:00 2001 From: Kirk Waiblinger Date: Sat, 14 Dec 2024 10:53:25 -0700 Subject: [PATCH 3/3] use typeToString --- .../src/rules/no-unnecessary-condition.ts | 27 ++----------------- .../rules/no-unnecessary-condition.test.ts | 27 ++++++++++++++++++- 2 files changed, 28 insertions(+), 26 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts index f50a5368dbda..c9660416a81b 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-condition.ts @@ -491,9 +491,9 @@ export default createRule({ node, messageId: 'comparisonBetweenLiteralTypes', data: { - left: valueOfLiteralTypeToExpression(leftStaticValue.value), + left: checker.typeToString(leftType), operator, - right: valueOfLiteralTypeToExpression(rightStaticValue.value), + right: checker.typeToString(rightType), trueOrFalse: conditionIsTrue ? 'true' : 'false', }, }); @@ -900,26 +900,3 @@ export default createRule({ }; }, }); - -/** - * Converts a value to its source code representation as a literal. It's - * assumed that the input is a primitive that has such a representation. - * - * @example - * null => 'null' - * undefined => 'undefined' - * true => 'true' - * 42n => '42n' - * 'foo' => '"foo"' - * 23 => '23' - */ -function valueOfLiteralTypeToExpression(value: unknown): string { - if (typeof value === 'bigint') { - return `${value}n`; - } - if (value === undefined) { - return 'undefined'; - } - // covers null, boolean, number, string - return JSON.stringify(value); -} 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 fb78e399954e..de69b94afac3 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts @@ -1354,7 +1354,32 @@ if (x === Foo.a) { { column: 5, data: { - left: 1, + left: 'Foo.a', + operator: '===', + right: 'Foo.a', + trueOrFalse: 'true', + }, + line: 8, + messageId: 'comparisonBetweenLiteralTypes', + }, + ], + }, + { + code: ` +enum Foo { + a = 1, + b = 2, +} + +const x = Foo.a; +if (x === 1) { +} + `, + errors: [ + { + column: 5, + data: { + left: 'Foo.a', operator: '===', right: 1, trueOrFalse: 'true',