Skip to content
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

Jump to bottom

fix(eslint-plugin): [strict-boolean-expressions] check all conditions in a logical operator chain #5539

Merged
merged 4 commits into from Sep 8, 2022
Merged

fix(eslint-plugin): [strict-boolean-expressions] check all conditions in a logical operator chain #5539

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14695ca
fix(eslint-plugin): [strict-bool-expr] fix logical chain
phaux Aug 20, 2022
fd8ffbb
make code more logical hopefully
phaux Aug 24, 2022
a0019ca
Merge branch 'typescript-eslint:main' into strict-bool-expr-fix-logic…
phaux Sep 8, 2022
5ee55fe
more tests
phaux Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 61 additions & 25 deletions packages/eslint-plugin/src/rules/strict-boolean-expressions.ts
View file
Open in desktop
Expand Up @@ -166,16 +166,16 @@ export default util.createRule<Options, MessageId>({
});
}

const checkedNodes = new Set<TSESTree.Node>();
const traversedNodes = new Set<TSESTree.Node>();

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 =
Expand All @@ -185,45 +185,81 @@ export default util.createRule<Options, MessageId>({
| 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, isTestExpr);
traverseLogicalExpression(node, isCondition);
return;
}

// we ignore the right operand when not in a context of a test expression
if (isTestExpr) {
checkNode(node.right, isTestExpr);
}
// 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));
Expand Down
61 changes: 61 additions & 0 deletions packages/eslint-plugin/tests/rules/strict-boolean-expressions.test.ts
View file
Open in desktop
Expand Up @@ -304,6 +304,67 @@ if (y) {
],
},

// a chain of logical operators should check every operand except the last one in a chain
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some passing (valid) test cases too, please?

{
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<MessageId, Options>({
code: noFormat`
Expand Down