From 14695caf493e359da0182ad427357bb29989b280 Mon Sep 17 00:00:00 2001 From: Nikita Date: Sat, 20 Aug 2022 22:23:49 +0200 Subject: [PATCH 1/3] fix(eslint-plugin): [strict-bool-expr] fix logical chain --- .../src/rules/strict-boolean-expressions.ts | 5 +- .../rules/strict-boolean-expressions.test.ts | 61 +++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index 5493e84d271..d3f7751e8d3 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -215,11 +215,12 @@ export default util.createRule({ node.type === AST_NODE_TYPES.LogicalExpression && node.operator !== '??' ) { - checkNode(node.left, isTestExpr); + checkNode(node.left, true); // we ignore the right operand when not in a context of a test expression + // if the right operand is itself a logical expression, it will be checked separately if (isTestExpr) { - checkNode(node.right, isTestExpr); + checkNode(node.right, true); } return; } diff --git a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts index b6da08b5965..d80ad566449 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -304,6 +304,67 @@ if (y) { ], }, + // a chain of logical operators should check every operand except the last one in a chain + { + options: [{ allowString: false, allowNumber: false }], + code: "'asd' && 123 && [] && null;", + errors: [ + { messageId: 'conditionErrorString', line: 1, column: 1 }, + { messageId: 'conditionErrorNumber', line: 1, column: 10 }, + { messageId: 'conditionErrorObject', line: 1, column: 17 }, + ], + }, + { + options: [{ allowString: false, allowNumber: false }], + code: "'asd' || 123 || [] || null;", + errors: [ + { messageId: 'conditionErrorString', line: 1, column: 1 }, + { messageId: 'conditionErrorNumber', line: 1, column: 10 }, + { messageId: 'conditionErrorObject', line: 1, column: 17 }, + ], + }, + { + options: [{ allowString: false, allowNumber: false }], + code: "(1 && 'a' && null) || 0 || '' || {};", + errors: [ + { messageId: 'conditionErrorNumber', line: 1, column: 2 }, + { messageId: 'conditionErrorString', line: 1, column: 7 }, + { messageId: 'conditionErrorNullish', line: 1, column: 14 }, + { messageId: 'conditionErrorNumber', line: 1, column: 23 }, + { messageId: 'conditionErrorString', line: 1, column: 28 }, + ], + }, + { + options: [{ allowString: false, allowNumber: false }], + code: "(1 || 'a' || null) && 0 && '' && {};", + errors: [ + { messageId: 'conditionErrorNumber', line: 1, column: 2 }, + { messageId: 'conditionErrorString', line: 1, column: 7 }, + { messageId: 'conditionErrorNullish', line: 1, column: 14 }, + { messageId: 'conditionErrorNumber', line: 1, column: 23 }, + { messageId: 'conditionErrorString', line: 1, column: 28 }, + ], + }, + { + options: [{ allowString: false, allowNumber: false }], + code: "(1 && []) || ('a' && {});", + errors: [ + { messageId: 'conditionErrorNumber', line: 1, column: 2 }, + { messageId: 'conditionErrorObject', line: 1, column: 7 }, + { messageId: 'conditionErrorString', line: 1, column: 15 }, + ], + }, + { + options: [{ allowString: false, allowNumber: false }], + code: "if ((1 && []) || ('a' && {})) void 0;", + errors: [ + { messageId: 'conditionErrorNumber', line: 1, column: 6 }, + { messageId: 'conditionErrorObject', line: 1, column: 11 }, + { messageId: 'conditionErrorString', line: 1, column: 19 }, + { messageId: 'conditionErrorObject', line: 1, column: 26 }, + ], + }, + // nullish in boolean context ...batchedSingleLineTests({ code: noFormat` From fd8ffbb404adb3b52ec120a20c2b1384fa8b2a58 Mon Sep 17 00:00:00 2001 From: Nikita Date: Wed, 24 Aug 2022 21:20:44 +0200 Subject: [PATCH 2/3] make code more logical hopefully --- .../src/rules/strict-boolean-expressions.ts | 87 +++++++++++++------ 1 file changed, 61 insertions(+), 26 deletions(-) diff --git a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts index d3f7751e8d3..b84df197bbb 100644 --- a/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts +++ b/packages/eslint-plugin/src/rules/strict-boolean-expressions.ts @@ -166,16 +166,16 @@ export default util.createRule({ }); } - const checkedNodes = new Set(); + const traversedNodes = new Set(); return { - ConditionalExpression: checkTestExpression, - DoWhileStatement: checkTestExpression, - ForStatement: checkTestExpression, - IfStatement: checkTestExpression, - WhileStatement: checkTestExpression, - 'LogicalExpression[operator!="??"]': checkNode, - 'UnaryExpression[operator="!"]': checkUnaryLogicalExpression, + ConditionalExpression: traverseTestExpression, + DoWhileStatement: traverseTestExpression, + ForStatement: traverseTestExpression, + IfStatement: traverseTestExpression, + WhileStatement: traverseTestExpression, + 'LogicalExpression[operator!="??"]': traverseLogicalExpression, + 'UnaryExpression[operator="!"]': traverseUnaryLogicalExpression, }; type TestExpression = @@ -185,46 +185,81 @@ export default util.createRule({ | TSESTree.IfStatement | TSESTree.WhileStatement; - function checkTestExpression(node: TestExpression): void { + /** + * Inspects condition of a test expression. (`if`, `while`, `for`, etc.) + */ + function traverseTestExpression(node: TestExpression): void { if (node.test == null) { return; } - checkNode(node.test, true); + traverseNode(node.test, true); + } + + /** + * Inspects the argument of a unary logical expression (`!`). + */ + function traverseUnaryLogicalExpression( + node: TSESTree.UnaryExpression, + ): void { + traverseNode(node.argument, true); } - function checkUnaryLogicalExpression(node: TSESTree.UnaryExpression): void { - checkNode(node.argument, true); + /** + * Inspects the arguments of a logical expression (`&&`, `||`). + * + * If the logical expression is a descendant of a test expression, + * the `isCondition` flag should be set to true. + * Otherwise, if the logical expression is there on it's own, + * it's used for control flow and is not a condition itself. + */ + function traverseLogicalExpression( + node: TSESTree.LogicalExpression, + isCondition = false, + ): void { + // left argument is always treated as a condition + traverseNode(node.left, true); + // if the logical expression is used for control flow, + // then it's right argument is used for it's side effects only + traverseNode(node.right, isCondition); } /** - * This function analyzes the type of a node and checks if it is allowed in a boolean context. - * It can recurse when checking nested logical operators, so that only the outermost operands are reported. - * The right operand of a logical expression is ignored unless it's a part of a test expression (if/while/ternary/etc). - * @param node The AST node to check. - * @param isTestExpr Whether the node is a descendant of a test expression. + * Inspects any node. + * + * If it's a logical expression then it recursively traverses its arguments. + * If it's any other kind of node then it's type is finally checked against the rule, + * unless `isCondition` flag is set to false, in which case + * it's assumed to be used for side effects only and is skipped. */ - function checkNode(node: TSESTree.Node, isTestExpr = false): void { + function traverseNode(node: TSESTree.Node, isCondition: boolean): void { // prevent checking the same node multiple times - if (checkedNodes.has(node)) { + if (traversedNodes.has(node)) { return; } - checkedNodes.add(node); + traversedNodes.add(node); // for logical operator, we check its operands if ( node.type === AST_NODE_TYPES.LogicalExpression && node.operator !== '??' ) { - checkNode(node.left, true); + traverseLogicalExpression(node, isCondition); + return; + } - // we ignore the right operand when not in a context of a test expression - // if the right operand is itself a logical expression, it will be checked separately - if (isTestExpr) { - checkNode(node.right, true); - } + // skip if node is not a condition + if (!isCondition) { return; } + checkNode(node); + } + + /** + * This function does the actual type check on a node. + * It analyzes the type of a node and checks if it is allowed in a boolean context. + */ + function checkNode(node: TSESTree.Node): void { const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); const type = util.getConstrainedTypeAtLocation(typeChecker, tsNode); const types = inspectVariantTypes(tsutils.unionTypeParts(type)); From 5ee55febf223dbb37c1f0e8ef534285da1d2a945 Mon Sep 17 00:00:00 2001 From: Nikita Date: Thu, 8 Sep 2022 15:47:41 +0200 Subject: [PATCH 3/3] more tests --- .../rules/strict-boolean-expressions.test.ts | 70 ++++++++++++++----- 1 file changed, 53 insertions(+), 17 deletions(-) diff --git a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts index d80ad566449..42b57e89ebf 100644 --- a/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts +++ b/packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts @@ -117,6 +117,20 @@ ruleTester.run('strict-boolean-expressions', rule, { (x: T) => x ? 1 : 0; `, }), + + // logical operator + ...batchedSingleLineTests({ + options: [{ allowString: true, allowNumber: true }], + code: ` + 1 && true && 'x' && {}; + let x = 0 || false || '' || null; + if (1 && true && 'x') void 0; + if (0 || false || '') void 0; + 1 && true && 'x' ? {} : null; + 0 || false || '' ? null : {}; + `, + }), + { code: ` declare const x: string[] | null; @@ -304,7 +318,7 @@ if (y) { ], }, - // a chain of logical operators should check every operand except the last one in a chain + // shouldn't check last logical operand when used for control flow { options: [{ allowString: false, allowNumber: false }], code: "'asd' && 123 && [] && null;", @@ -325,35 +339,37 @@ if (y) { }, { options: [{ allowString: false, allowNumber: false }], - code: "(1 && 'a' && null) || 0 || '' || {};", + code: "let x = (1 && 'a' && null) || 0 || '' || {};", errors: [ - { messageId: 'conditionErrorNumber', line: 1, column: 2 }, - { messageId: 'conditionErrorString', line: 1, column: 7 }, - { messageId: 'conditionErrorNullish', line: 1, column: 14 }, - { messageId: 'conditionErrorNumber', line: 1, column: 23 }, - { messageId: 'conditionErrorString', line: 1, column: 28 }, + { messageId: 'conditionErrorNumber', line: 1, column: 10 }, + { messageId: 'conditionErrorString', line: 1, column: 15 }, + { messageId: 'conditionErrorNullish', line: 1, column: 22 }, + { messageId: 'conditionErrorNumber', line: 1, column: 31 }, + { messageId: 'conditionErrorString', line: 1, column: 36 }, ], }, { options: [{ allowString: false, allowNumber: false }], - code: "(1 || 'a' || null) && 0 && '' && {};", + code: "return (1 || 'a' || null) && 0 && '' && {};", errors: [ - { messageId: 'conditionErrorNumber', line: 1, column: 2 }, - { messageId: 'conditionErrorString', line: 1, column: 7 }, - { messageId: 'conditionErrorNullish', line: 1, column: 14 }, - { messageId: 'conditionErrorNumber', line: 1, column: 23 }, - { messageId: 'conditionErrorString', line: 1, column: 28 }, + { messageId: 'conditionErrorNumber', line: 1, column: 9 }, + { messageId: 'conditionErrorString', line: 1, column: 14 }, + { messageId: 'conditionErrorNullish', line: 1, column: 21 }, + { messageId: 'conditionErrorNumber', line: 1, column: 30 }, + { messageId: 'conditionErrorString', line: 1, column: 35 }, ], }, { options: [{ allowString: false, allowNumber: false }], - code: "(1 && []) || ('a' && {});", + code: "console.log((1 && []) || ('a' && {}));", errors: [ - { messageId: 'conditionErrorNumber', line: 1, column: 2 }, - { messageId: 'conditionErrorObject', line: 1, column: 7 }, - { messageId: 'conditionErrorString', line: 1, column: 15 }, + { messageId: 'conditionErrorNumber', line: 1, column: 14 }, + { messageId: 'conditionErrorObject', line: 1, column: 19 }, + { messageId: 'conditionErrorString', line: 1, column: 27 }, ], }, + + // should check all logical operands when used in a condition { options: [{ allowString: false, allowNumber: false }], code: "if ((1 && []) || ('a' && {})) void 0;", @@ -364,6 +380,26 @@ if (y) { { messageId: 'conditionErrorObject', line: 1, column: 26 }, ], }, + { + options: [{ allowString: false, allowNumber: false }], + code: "let x = null || 0 || 'a' || [] ? {} : undefined;", + errors: [ + { messageId: 'conditionErrorNullish', line: 1, column: 9 }, + { messageId: 'conditionErrorNumber', line: 1, column: 17 }, + { messageId: 'conditionErrorString', line: 1, column: 22 }, + { messageId: 'conditionErrorObject', line: 1, column: 29 }, + ], + }, + { + options: [{ allowString: false, allowNumber: false }], + code: "return !(null || 0 || 'a' || []);", + errors: [ + { messageId: 'conditionErrorNullish', line: 1, column: 10 }, + { messageId: 'conditionErrorNumber', line: 1, column: 18 }, + { messageId: 'conditionErrorString', line: 1, column: 23 }, + { messageId: 'conditionErrorObject', line: 1, column: 30 }, + ], + }, // nullish in boolean context ...batchedSingleLineTests({