no-lonely-if: Keep all comments (#1047)

sindresorhus · Jan 29, 2021 · da94ca9 · da94ca9
1 parent 378a494
commit da94ca9
Show file tree

Hide file tree

Showing 5 changed files with 215 additions and 107 deletions.
diff --git a/rules/no-lonely-if.js b/rules/no-lonely-if.js
@@ -1,6 +1,8 @@
 'use strict';
+const {isParenthesized, isNotSemicolonToken} = require('eslint-utils');
 const getDocumentationUrl = require('./utils/get-documentation-url');
 const needsSemicolon = require('./utils/needs-semicolon');
+const removeSpacesAfter = require('./utils/remove-spaces-after');
 
 const MESSAGE_ID = 'no-lonely-if';
 const messages = {
@@ -35,43 +37,100 @@ const needParenthesis = node => (
 	node.type === 'SequenceExpression'
 );
 
+function getIfStatementTokens(node, sourceCode) {
+	const tokens = {};
+
+	tokens.ifToken = sourceCode.getFirstToken(node);
+	tokens.openingParenthesisToken = sourceCode.getFirstToken(node, 1);
+
+	const {consequent} = node;
+	tokens.closingParenthesisToken = sourceCode.getTokenBefore(consequent);
+
+	if (consequent.type === 'BlockStatement') {
+		tokens.openingBraceToken = sourceCode.getFirstToken(consequent);
+		tokens.closingBraceToken = sourceCode.getLastToken(consequent);
+	}
+
+	return tokens;
+}
+
+function fix(innerIfStatement, sourceCode) {
+	return function * (fixer) {
+		const outerIfStatement = (
+			innerIfStatement.parent.type === 'BlockStatement' ?
+				innerIfStatement.parent :
+				innerIfStatement
+		).parent;
+		const outer = {
+			...outerIfStatement,
+			...getIfStatementTokens(outerIfStatement, sourceCode)
+		};
+		const inner = {
+			...innerIfStatement,
+			...getIfStatementTokens(innerIfStatement, sourceCode)
+		};
+
+		// Remove inner `if` token
+		yield fixer.remove(inner.ifToken);
+		yield removeSpacesAfter(inner.ifToken, sourceCode, fixer);
+
+		// Remove outer `{}`
+		if (outer.openingBraceToken) {
+			yield fixer.remove(outer.openingBraceToken);
+			yield removeSpacesAfter(outer.openingBraceToken, sourceCode, fixer);
+			yield fixer.remove(outer.closingBraceToken);
+
+			const tokenBefore = sourceCode.getTokenBefore(outer.closingBraceToken, {includeComments: true});
+			yield removeSpacesAfter(tokenBefore, sourceCode, fixer);
+		}
+
+		// Add new `()`
+		yield fixer.insertTextBefore(outer.openingParenthesisToken, '(');
+		yield fixer.insertTextAfter(
+			inner.closingParenthesisToken,
+			`)${inner.consequent.type === 'EmptyStatement' ? '' : ' '}`
+		);
+
+		// Add ` && `
+		yield fixer.insertTextAfter(outer.closingParenthesisToken, ' && ');
+
+		// Remove `()` if `test` don't need it
+		for (const {test, openingParenthesisToken, closingParenthesisToken} of [outer, inner]) {
+			if (
+				isParenthesized(test, sourceCode) ||
+				!needParenthesis(test)
+			) {
+				yield fixer.remove(openingParenthesisToken);
+				yield fixer.remove(closingParenthesisToken);
+			}
+
+			yield removeSpacesAfter(closingParenthesisToken, sourceCode, fixer);
+		}
+
+		// If the `if` statement has no block, and is not followed by a semicolon,
+		// make sure that fixing the issue would not change semantics due to ASI.
+		// Similar logic https://github.com/eslint/eslint/blob/2124e1b5dad30a905dc26bde9da472bf622d3f50/lib/rules/no-lonely-if.js#L61-L77
+		if (inner.consequent.type !== 'BlockStatement') {
+			const lastToken = sourceCode.getLastToken(inner.consequent);
+			if (isNotSemicolonToken(lastToken)) {
+				const nextToken = sourceCode.getTokenAfter(outer);
+				if (needsSemicolon(lastToken, sourceCode, nextToken.value)) {
+					yield fixer.insertTextBefore(nextToken, ';');
+				}
+			}
+		}
+	};
+}
+
 const create = context => {
 	const sourceCode = context.getSourceCode();
-	const getText = node => sourceCode.getText(node);
-	const getTestNodeText = node => needParenthesis(node) ? `(${getText(node)})` : getText(node);
 
 	return {
-		[selector](inner) {
-			const {parent, test} = inner;
-			const outer = parent.type === 'BlockStatement' ? parent.parent : parent;
-
+		[selector](node) {
 			context.report({
-				node: inner,
+				node,
 				messageId: MESSAGE_ID,
-				* fix(fixer) {
-					// Merge `test`
-					yield fixer.replaceText(outer.test, `${getTestNodeText(outer.test)} && ${getTestNodeText(test)}`);
-
-					// Replace `consequent`
-					const {consequent} = inner;
-					let consequentText = getText(consequent);
-					// If the `if` statement has no block, and is not followed by a semicolon,
-					// make sure that fixing the issue would not change semantics due to ASI.
-					// Similar logic https://github.com/eslint/eslint/blob/2124e1b5dad30a905dc26bde9da472bf622d3f50/lib/rules/no-lonely-if.js#L61-L77
-					if (
-						consequent.type !== 'BlockStatement' &&
-						outer.consequent.type === 'BlockStatement' &&
-						!consequentText.endsWith(';')
-					) {
-						const lastToken = sourceCode.getLastToken(consequent);
-						const nextToken = sourceCode.getTokenAfter(outer);
-						if (needsSemicolon(lastToken, sourceCode, nextToken.value)) {
-							consequentText += ';';
-						}
-					}
-
-					yield fixer.replaceText(outer.consequent, consequentText);
-				}
+				fix: fix(node, sourceCode)
 			});
 		}
 	};

diff --git a/rules/utils/remove-spaces-after.js b/rules/utils/remove-spaces-after.js
@@ -0,0 +1,14 @@
+'use strict';
+
+function removeSpacesAfter(indexOrNode, sourceCode, fixer) {
+	let index = indexOrNode;
+	if (typeof indexOrNode === 'object' && Array.isArray(indexOrNode.range)) {
+		index = indexOrNode.range[1];
+	}
+
+	const textAfter = sourceCode.text.slice(index);
+	const [leadingSpaces] = textAfter.match(/^\s*/);
+	return fixer.removeRange([index, index + leadingSpaces.length]);
+}
+
+module.exports = removeSpacesAfter;
diff --git a/test/no-lonely-if.js b/test/no-lonely-if.js
@@ -92,23 +92,24 @@ test.snapshot([
 	'if (((a || b))) if (((c || d)));',
 	// Comments
 	outdent`
-		if /* will keep */
+		if // 1
 		(
-			/* will keep */
-			a /* will keep */
-				.b /* will keep */
-		) /* keep */{
-			/* will remove */
+			// 2
+			a // 3
+				.b // 4
+		) // 5
+		{
+			// 6
 			if (
-				/* will remove */
-				c /* will keep */
-					.d /* will remove */
+				// 7
+				c // 8
+					.d // 9
 			) {
-				/* will keep */
+				// 10
 				foo();
-				/* will keep */
+				// 11
 			}
-			/* will remove */
+			// 12
 		}
 	`,
 	// Semicolon
@@ -122,5 +123,11 @@ test.snapshot([
 		if (a)
 			if (b) foo()
 		;[].forEach(bar)
+	`,
+	outdent`
+		if (a) {
+			if (b) foo()
+		}
+		;[].forEach(bar)
 	`
 ]);