Skip to content

Commit

Permalink
fix: unsafe report for no-lonely-if (eslint#19087)
Browse files Browse the repository at this point in the history 
* fix unsafe report

* move functions around
  • Loading branch information
@abrahamguo
abrahamguo authored Nov 6, 2024
1 parent 68fa497 commit 9db5b15
Show file tree
Hide file tree
Showing 4 changed files with 152 additions and 142 deletions.
140 changes: 1 addition & 139 deletions lib/rules/curly.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,40 +108,6 @@ module.exports = {
return first.loc.start.line === lastExcludingSemicolon.loc.end.line;
}

/**
* Determines if the given node is a lexical declaration (let, const, function, or class)
* @param {ASTNode} node The node to check
* @returns {boolean} True if the node is a lexical declaration
* @private
*/
function isLexicalDeclaration(node) {
if (node.type === "VariableDeclaration") {
return node.kind === "const" || node.kind === "let";
}

return node.type === "FunctionDeclaration" || node.type === "ClassDeclaration";
}

/**
* Checks if the given token is an `else` token or not.
* @param {Token} token The token to check.
* @returns {boolean} `true` if the token is an `else` token.
*/
function isElseKeywordToken(token) {
return token.value === "else" && token.type === "Keyword";
}

/**
* Determines whether the given node has an `else` keyword token as the first token after.
* @param {ASTNode} node The node to check.
* @returns {boolean} `true` if the node is followed by an `else` keyword token.
*/
function isFollowedByElseKeyword(node) {
const nextToken = sourceCode.getTokenAfter(node);

return Boolean(nextToken) && isElseKeywordToken(nextToken);
}

/**
* Determines if a semicolon needs to be inserted after removing a set of curly brackets, in order to avoid a SyntaxError.
* @param {Token} closingBracket The } token
Expand Down Expand Up @@ -196,110 +162,6 @@ module.exports = {
return false;
}

/**
* Determines whether the code represented by the given node contains an `if` statement
* that would become associated with an `else` keyword directly appended to that code.
*
* Examples where it returns `true`:
*
* if (a)
* foo();
*
* if (a) {
* foo();
* }
*
* if (a)
* foo();
* else if (b)
* bar();
*
* while (a)
* if (b)
* if(c)
* foo();
* else
* bar();
*
* Examples where it returns `false`:
*
* if (a)
* foo();
* else
* bar();
*
* while (a) {
* if (b)
* if(c)
* foo();
* else
* bar();
* }
*
* while (a)
* if (b) {
* if(c)
* foo();
* }
* else
* bar();
* @param {ASTNode} node Node representing the code to check.
* @returns {boolean} `true` if an `if` statement within the code would become associated with an `else` appended to that code.
*/
function hasUnsafeIf(node) {
switch (node.type) {
case "IfStatement":
if (!node.alternate) {
return true;
}
return hasUnsafeIf(node.alternate);
case "ForStatement":
case "ForInStatement":
case "ForOfStatement":
case "LabeledStatement":
case "WithStatement":
case "WhileStatement":
return hasUnsafeIf(node.body);
default:
return false;
}
}

/**
* Determines whether the existing curly braces around the single statement are necessary to preserve the semantics of the code.
* The braces, which make the given block body, are necessary in either of the following situations:
*
* 1. The statement is a lexical declaration.
* 2. Without the braces, an `if` within the statement would become associated with an `else` after the closing brace:
*
* if (a) {
* if (b)
* foo();
* }
* else
* bar();
*
* if (a)
* while (b)
* while (c) {
* while (d)
* if (e)
* while(f)
* foo();
* }
* else
* bar();
* @param {ASTNode} node `BlockStatement` body with exactly one statement directly inside. The statement can have its own nested statements.
* @returns {boolean} `true` if the braces are necessary - removing them (replacing the given `BlockStatement` body with its single statement content)
* would change the semantics of the code or produce a syntax error.
*/
function areBracesNecessary(node) {
const statement = node.body[0];

return isLexicalDeclaration(statement) ||
hasUnsafeIf(statement) && isFollowedByElseKeyword(node);
}

/**
* Prepares to check the body of a node to see if it's a block statement.
* @param {ASTNode} node The node to report if there's a problem.
Expand All @@ -318,7 +180,7 @@ module.exports = {
const hasBlock = (body.type === "BlockStatement");
let expected = null;

if (hasBlock && (body.body.length !== 1 || areBracesNecessary(body))) {
if (hasBlock && (body.body.length !== 1 || astUtils.areBracesNecessary(body, sourceCode))) {
expected = true;
} else if (multiOnly) {
expected = false;
Expand Down
10 changes: 8 additions & 2 deletions lib/rules/no-lonely-if.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@
*/
"use strict";

//------------------------------------------------------------------------------
// Requirements
//------------------------------------------------------------------------------

const astUtils = require("./utils/ast-utils");

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
Expand Down Expand Up @@ -36,8 +42,8 @@ module.exports = {
grandparent = parent.parent;

if (parent && parent.type === "BlockStatement" &&
parent.body.length === 1 && grandparent &&
grandparent.type === "IfStatement" &&
parent.body.length === 1 && !astUtils.areBracesNecessary(parent, sourceCode) &&
grandparent && grandparent.type === "IfStatement" &&
parent === grandparent.alternate) {
context.report({
node,
Expand Down
141 changes: 141 additions & 0 deletions lib/rules/utils/ast-utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2311,6 +2311,147 @@ module.exports = {
return node.type === "TemplateLiteral" && node.expressions.length === 0;
},

/**
* Determines whether the existing curly braces around the single statement are necessary to preserve the semantics of the code.
* The braces, which make the given block body, are necessary in either of the following situations:
*
* 1. The statement is a lexical declaration.
* 2. Without the braces, an `if` within the statement would become associated with an `else` after the closing brace:
*
* if (a) {
* if (b)
* foo();
* }
* else
* bar();
*
* if (a)
* while (b)
* while (c) {
* while (d)
* if (e)
* while(f)
* foo();
* }
* else
* bar();
* @param {ASTNode} node `BlockStatement` body with exactly one statement directly inside. The statement can have its own nested statements.
* @param {SourceCode} sourceCode The source code
* @returns {boolean} `true` if the braces are necessary - removing them (replacing the given `BlockStatement` body with its single statement content)
* would change the semantics of the code or produce a syntax error.
*/
areBracesNecessary(node, sourceCode) {

/**
* Determines if the given node is a lexical declaration (let, const, function, or class)
* @param {ASTNode} nodeToCheck The node to check
* @returns {boolean} True if the node is a lexical declaration
* @private
*/
function isLexicalDeclaration(nodeToCheck) {
if (nodeToCheck.type === "VariableDeclaration") {
return nodeToCheck.kind === "const" || nodeToCheck.kind === "let";
}

return nodeToCheck.type === "FunctionDeclaration" || nodeToCheck.type === "ClassDeclaration";
}


/**
* Checks if the given token is an `else` token or not.
* @param {Token} token The token to check.
* @returns {boolean} `true` if the token is an `else` token.
*/
function isElseKeywordToken(token) {
return token.value === "else" && token.type === "Keyword";
}

/**
* Determines whether the given node has an `else` keyword token as the first token after.
* @param {ASTNode} nodeToCheck The node to check.
* @returns {boolean} `true` if the node is followed by an `else` keyword token.
*/
function isFollowedByElseKeyword(nodeToCheck) {
const nextToken = sourceCode.getTokenAfter(nodeToCheck);

return Boolean(nextToken) && isElseKeywordToken(nextToken);
}

/**
* Determines whether the code represented by the given node contains an `if` statement
* that would become associated with an `else` keyword directly appended to that code.
*
* Examples where it returns `true`:
*
* if (a)
* foo();
*
* if (a) {
* foo();
* }
*
* if (a)
* foo();
* else if (b)
* bar();
*
* while (a)
* if (b)
* if(c)
* foo();
* else
* bar();
*
* Examples where it returns `false`:
*
* if (a)
* foo();
* else
* bar();
*
* while (a) {
* if (b)
* if(c)
* foo();
* else
* bar();
* }
*
* while (a)
* if (b) {
* if(c)
* foo();
* }
* else
* bar();
* @param {ASTNode} nodeToCheck Node representing the code to check.
* @returns {boolean} `true` if an `if` statement within the code would become associated with an `else` appended to that code.
*/
function hasUnsafeIf(nodeToCheck) {
switch (nodeToCheck.type) {
case "IfStatement":
if (!nodeToCheck.alternate) {
return true;
}
return hasUnsafeIf(nodeToCheck.alternate);
case "ForStatement":
case "ForInStatement":
case "ForOfStatement":
case "LabeledStatement":
case "WithStatement":
case "WhileStatement":
return hasUnsafeIf(nodeToCheck.body);
default:
return false;
}
}

const statement = node.body[0];

return isLexicalDeclaration(statement) ||
hasUnsafeIf(statement) && isFollowedByElseKeyword(node);
},

isReferenceToGlobalVariable,
isLogicalExpression,
isCoalesceExpression,
Expand Down
3 changes: 2 additions & 1 deletion tests/lib/rules/no-lonely-if.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ ruleTester.run("no-lonely-if", rule, {
// Examples of code that should not trigger the rule
valid: [
"if (a) {;} else if (b) {;}",
"if (a) {;} else { if (b) {;} ; }"
"if (a) {;} else { if (b) {;} ; }",
"if (a) if (a) {} else { if (b) {} } else {}"
],

// Examples of code that should trigger the rule
Expand Down

0 comments on commit 9db5b15

Please sign in to comment.