Skip to content

Commit 9db5b15

Browse files
abrahamguoabrahamguo
authored
fix: unsafe report for no-lonely-if (#19087)
* fix unsafe report * move functions around
1 parent 68fa497 commit 9db5b15

4 files changed

Lines changed: 152 additions & 142 deletions

File tree

‎lib/rules/curly.js‎

Lines changed: 1 addition & 139 deletions
Original file line numberDiff line numberDiff line change
@@ -108,40 +108,6 @@ module.exports = {
108108
return first.loc.start.line === lastExcludingSemicolon.loc.end.line;
109109
}
110110

111-
/**
112-
* Determines if the given node is a lexical declaration (let, const, function, or class)
113-
* @param {ASTNode} node The node to check
114-
* @returns {boolean} True if the node is a lexical declaration
115-
* @private
116-
*/
117-
function isLexicalDeclaration(node) {
118-
if (node.type === "VariableDeclaration") {
119-
return node.kind === "const" || node.kind === "let";
120-
}
121-
122-
return node.type === "FunctionDeclaration" || node.type === "ClassDeclaration";
123-
}
124-
125-
/**
126-
* Checks if the given token is an `else` token or not.
127-
* @param {Token} token The token to check.
128-
* @returns {boolean} `true` if the token is an `else` token.
129-
*/
130-
function isElseKeywordToken(token) {
131-
return token.value === "else" && token.type === "Keyword";
132-
}
133-
134-
/**
135-
* Determines whether the given node has an `else` keyword token as the first token after.
136-
* @param {ASTNode} node The node to check.
137-
* @returns {boolean} `true` if the node is followed by an `else` keyword token.
138-
*/
139-
function isFollowedByElseKeyword(node) {
140-
const nextToken = sourceCode.getTokenAfter(node);
141-
142-
return Boolean(nextToken) && isElseKeywordToken(nextToken);
143-
}
144-
145111
/**
146112
* Determines if a semicolon needs to be inserted after removing a set of curly brackets, in order to avoid a SyntaxError.
147113
* @param {Token} closingBracket The } token
@@ -196,110 +162,6 @@ module.exports = {
196162
return false;
197163
}
198164

199-
/**
200-
* Determines whether the code represented by the given node contains an `if` statement
201-
* that would become associated with an `else` keyword directly appended to that code.
202-
*
203-
* Examples where it returns `true`:
204-
*
205-
* if (a)
206-
* foo();
207-
*
208-
* if (a) {
209-
* foo();
210-
* }
211-
*
212-
* if (a)
213-
* foo();
214-
* else if (b)
215-
* bar();
216-
*
217-
* while (a)
218-
* if (b)
219-
* if(c)
220-
* foo();
221-
* else
222-
* bar();
223-
*
224-
* Examples where it returns `false`:
225-
*
226-
* if (a)
227-
* foo();
228-
* else
229-
* bar();
230-
*
231-
* while (a) {
232-
* if (b)
233-
* if(c)
234-
* foo();
235-
* else
236-
* bar();
237-
* }
238-
*
239-
* while (a)
240-
* if (b) {
241-
* if(c)
242-
* foo();
243-
* }
244-
* else
245-
* bar();
246-
* @param {ASTNode} node Node representing the code to check.
247-
* @returns {boolean} `true` if an `if` statement within the code would become associated with an `else` appended to that code.
248-
*/
249-
function hasUnsafeIf(node) {
250-
switch (node.type) {
251-
case "IfStatement":
252-
if (!node.alternate) {
253-
return true;
254-
}
255-
return hasUnsafeIf(node.alternate);
256-
case "ForStatement":
257-
case "ForInStatement":
258-
case "ForOfStatement":
259-
case "LabeledStatement":
260-
case "WithStatement":
261-
case "WhileStatement":
262-
return hasUnsafeIf(node.body);
263-
default:
264-
return false;
265-
}
266-
}
267-
268-
/**
269-
* Determines whether the existing curly braces around the single statement are necessary to preserve the semantics of the code.
270-
* The braces, which make the given block body, are necessary in either of the following situations:
271-
*
272-
* 1. The statement is a lexical declaration.
273-
* 2. Without the braces, an `if` within the statement would become associated with an `else` after the closing brace:
274-
*
275-
* if (a) {
276-
* if (b)
277-
* foo();
278-
* }
279-
* else
280-
* bar();
281-
*
282-
* if (a)
283-
* while (b)
284-
* while (c) {
285-
* while (d)
286-
* if (e)
287-
* while(f)
288-
* foo();
289-
* }
290-
* else
291-
* bar();
292-
* @param {ASTNode} node `BlockStatement` body with exactly one statement directly inside. The statement can have its own nested statements.
293-
* @returns {boolean} `true` if the braces are necessary - removing them (replacing the given `BlockStatement` body with its single statement content)
294-
* would change the semantics of the code or produce a syntax error.
295-
*/
296-
function areBracesNecessary(node) {
297-
const statement = node.body[0];
298-
299-
return isLexicalDeclaration(statement) ||
300-
hasUnsafeIf(statement) && isFollowedByElseKeyword(node);
301-
}
302-
303165
/**
304166
* Prepares to check the body of a node to see if it's a block statement.
305167
* @param {ASTNode} node The node to report if there's a problem.
@@ -318,7 +180,7 @@ module.exports = {
318180
const hasBlock = (body.type === "BlockStatement");
319181
let expected = null;
320182

321-
if (hasBlock && (body.body.length !== 1 || areBracesNecessary(body))) {
183+
if (hasBlock && (body.body.length !== 1 || astUtils.areBracesNecessary(body, sourceCode))) {
322184
expected = true;
323185
} else if (multiOnly) {
324186
expected = false;

‎lib/rules/no-lonely-if.js‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,12 @@
44
*/
55
"use strict";
66

7+
//------------------------------------------------------------------------------
8+
// Requirements
9+
//------------------------------------------------------------------------------
10+
11+
const astUtils = require("./utils/ast-utils");
12+
713
//------------------------------------------------------------------------------
814
// Rule Definition
915
//------------------------------------------------------------------------------
@@ -36,8 +42,8 @@ module.exports = {
3642
grandparent = parent.parent;
3743

3844
if (parent && parent.type === "BlockStatement" &&
39-
parent.body.length === 1 && grandparent &&
40-
grandparent.type === "IfStatement" &&
45+
parent.body.length === 1 && !astUtils.areBracesNecessary(parent, sourceCode) &&
46+
grandparent && grandparent.type === "IfStatement" &&
4147
parent === grandparent.alternate) {
4248
context.report({
4349
node,

‎lib/rules/utils/ast-utils.js‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2311,6 +2311,147 @@ module.exports = {
23112311
return node.type === "TemplateLiteral" && node.expressions.length === 0;
23122312
},
23132313

2314+
/**
2315+
* Determines whether the existing curly braces around the single statement are necessary to preserve the semantics of the code.
2316+
* The braces, which make the given block body, are necessary in either of the following situations:
2317+
*
2318+
* 1. The statement is a lexical declaration.
2319+
* 2. Without the braces, an `if` within the statement would become associated with an `else` after the closing brace:
2320+
*
2321+
* if (a) {
2322+
* if (b)
2323+
* foo();
2324+
* }
2325+
* else
2326+
* bar();
2327+
*
2328+
* if (a)
2329+
* while (b)
2330+
* while (c) {
2331+
* while (d)
2332+
* if (e)
2333+
* while(f)
2334+
* foo();
2335+
* }
2336+
* else
2337+
* bar();
2338+
* @param {ASTNode} node `BlockStatement` body with exactly one statement directly inside. The statement can have its own nested statements.
2339+
* @param {SourceCode} sourceCode The source code
2340+
* @returns {boolean} `true` if the braces are necessary - removing them (replacing the given `BlockStatement` body with its single statement content)
2341+
* would change the semantics of the code or produce a syntax error.
2342+
*/
2343+
areBracesNecessary(node, sourceCode) {
2344+
2345+
/**
2346+
* Determines if the given node is a lexical declaration (let, const, function, or class)
2347+
* @param {ASTNode} nodeToCheck The node to check
2348+
* @returns {boolean} True if the node is a lexical declaration
2349+
* @private
2350+
*/
2351+
function isLexicalDeclaration(nodeToCheck) {
2352+
if (nodeToCheck.type === "VariableDeclaration") {
2353+
return nodeToCheck.kind === "const" || nodeToCheck.kind === "let";
2354+
}
2355+
2356+
return nodeToCheck.type === "FunctionDeclaration" || nodeToCheck.type === "ClassDeclaration";
2357+
}
2358+
2359+
2360+
/**
2361+
* Checks if the given token is an `else` token or not.
2362+
* @param {Token} token The token to check.
2363+
* @returns {boolean} `true` if the token is an `else` token.
2364+
*/
2365+
function isElseKeywordToken(token) {
2366+
return token.value === "else" && token.type === "Keyword";
2367+
}
2368+
2369+
/**
2370+
* Determines whether the given node has an `else` keyword token as the first token after.
2371+
* @param {ASTNode} nodeToCheck The node to check.
2372+
* @returns {boolean} `true` if the node is followed by an `else` keyword token.
2373+
*/
2374+
function isFollowedByElseKeyword(nodeToCheck) {
2375+
const nextToken = sourceCode.getTokenAfter(nodeToCheck);
2376+
2377+
return Boolean(nextToken) && isElseKeywordToken(nextToken);
2378+
}
2379+
2380+
/**
2381+
* Determines whether the code represented by the given node contains an `if` statement
2382+
* that would become associated with an `else` keyword directly appended to that code.
2383+
*
2384+
* Examples where it returns `true`:
2385+
*
2386+
* if (a)
2387+
* foo();
2388+
*
2389+
* if (a) {
2390+
* foo();
2391+
* }
2392+
*
2393+
* if (a)
2394+
* foo();
2395+
* else if (b)
2396+
* bar();
2397+
*
2398+
* while (a)
2399+
* if (b)
2400+
* if(c)
2401+
* foo();
2402+
* else
2403+
* bar();
2404+
*
2405+
* Examples where it returns `false`:
2406+
*
2407+
* if (a)
2408+
* foo();
2409+
* else
2410+
* bar();
2411+
*
2412+
* while (a) {
2413+
* if (b)
2414+
* if(c)
2415+
* foo();
2416+
* else
2417+
* bar();
2418+
* }
2419+
*
2420+
* while (a)
2421+
* if (b) {
2422+
* if(c)
2423+
* foo();
2424+
* }
2425+
* else
2426+
* bar();
2427+
* @param {ASTNode} nodeToCheck Node representing the code to check.
2428+
* @returns {boolean} `true` if an `if` statement within the code would become associated with an `else` appended to that code.
2429+
*/
2430+
function hasUnsafeIf(nodeToCheck) {
2431+
switch (nodeToCheck.type) {
2432+
case "IfStatement":
2433+
if (!nodeToCheck.alternate) {
2434+
return true;
2435+
}
2436+
return hasUnsafeIf(nodeToCheck.alternate);
2437+
case "ForStatement":
2438+
case "ForInStatement":
2439+
case "ForOfStatement":
2440+
case "LabeledStatement":
2441+
case "WithStatement":
2442+
case "WhileStatement":
2443+
return hasUnsafeIf(nodeToCheck.body);
2444+
default:
2445+
return false;
2446+
}
2447+
}
2448+
2449+
const statement = node.body[0];
2450+
2451+
return isLexicalDeclaration(statement) ||
2452+
hasUnsafeIf(statement) && isFollowedByElseKeyword(node);
2453+
},
2454+
23142455
isReferenceToGlobalVariable,
23152456
isLogicalExpression,
23162457
isCoalesceExpression,

‎tests/lib/rules/no-lonely-if.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ ruleTester.run("no-lonely-if", rule, {
2323
// Examples of code that should not trigger the rule
2424
valid: [
2525
"if (a) {;} else if (b) {;}",
26-
"if (a) {;} else { if (b) {;} ; }"
26+
"if (a) {;} else { if (b) {;} ; }",
27+
"if (a) if (a) {} else { if (b) {} } else {}"
2728
],
2829

2930
// Examples of code that should trigger the rule

0 commit comments

Comments
 (0)