From da94ca939145a1146484d2ef8ffbe3e7f80fb0d1 Mon Sep 17 00:00:00 2001 From: fisker Cheung Date: Sat, 30 Jan 2021 02:48:30 +0800 Subject: [PATCH] `no-lonely-if`: Keep all comments (#1047) --- rules/no-lonely-if.js | 121 +++++++++++++++------ rules/utils/remove-spaces-after.js | 14 +++ test/no-lonely-if.js | 31 +++--- test/snapshots/no-lonely-if.js.md | 156 ++++++++++++++++------------ test/snapshots/no-lonely-if.js.snap | Bin 1398 -> 1439 bytes 5 files changed, 215 insertions(+), 107 deletions(-) create mode 100644 rules/utils/remove-spaces-after.js diff --git a/rules/no-lonely-if.js b/rules/no-lonely-if.js index 7fb7e2f9f5..124c060c83 100644 --- 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 new file mode 100644 index 0000000000..8b4829d162 --- /dev/null +++ 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 index c87e709200..cae9a842c4 100644 --- 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) ` ]); diff --git a/test/snapshots/no-lonely-if.js.md b/test/snapshots/no-lonely-if.js.md index cc6a97e656..f8d28ecb54 100644 --- a/test/snapshots/no-lonely-if.js.md +++ b/test/snapshots/no-lonely-if.js.md @@ -94,7 +94,7 @@ Generated by [AVA](https://avajs.dev). > Output `␊ - 1 | if (a && b) ;␊ + 1 | if (a && b);␊ ` > Error 1/1 @@ -157,9 +157,8 @@ Generated by [AVA](https://avajs.dev). `␊ 1 | function * foo() {␊ - 2 | if ((a || b) && (a ?? b) && (a ? b : c) && (a = b) && (a += b) && (a -= b) && (a &&= b) && (yield a) && (a, b))␊ - 3 | ;␊ - 4 | }␊ + 2 | if ((a || b) && (a ?? b) && (a ? b : c) && (a = b) && (a += b) && (a -= b) && (a &&= b) && (yield a) && (a, b));␊ + 3 | }␊ ` > Error 1/8 @@ -338,9 +337,8 @@ Generated by [AVA](https://avajs.dev). `␊ 1 | async function foo() {␊ - 2 | if (a && await a && a.b && a && b)␊ - 3 | ;␊ - 4 | }␊ + 2 | if (a && await a && a.b && a && b);␊ + 3 | }␊ ` > Error 1/3 @@ -388,7 +386,7 @@ Generated by [AVA](https://avajs.dev). > Output `␊ - 1 | if ((((a || b) && (c || d)))) ;␊ + 1 | if (((a || b)) && ((c || d)));␊ ` > Error 1/1 @@ -399,71 +397,79 @@ Generated by [AVA](https://avajs.dev). ` ## Invalid #10 - 1 | if /* will keep */ + 1 | if // 1 2 | ( - 3 | /* will keep */ - 4 | a /* will keep */ - 5 | .b /* will keep */ - 6 | ) /* keep */{ - 7 | /* will remove */ - 8 | if ( - 9 | /* will remove */ - 10 | c /* will keep */ - 11 | .d /* will remove */ - 12 | ) { - 13 | /* will keep */ - 14 | foo(); - 15 | /* will keep */ - 16 | } - 17 | /* will remove */ - 18 | } + 3 | // 2 + 4 | a // 3 + 5 | .b // 4 + 6 | ) // 5 + 7 | { + 8 | // 6 + 9 | if ( + 10 | // 7 + 11 | c // 8 + 12 | .d // 9 + 13 | ) { + 14 | // 10 + 15 | foo(); + 16 | // 11 + 17 | } + 18 | // 12 + 19 | } > Output `␊ - 1 | if /* will keep */␊ + 1 | if // 1␊ 2 | (␊ - 3 | /* will keep */␊ - 4 | a /* will keep */␊ - 5 | .b && c /* will keep */␊ - 6 | .d /* will keep */␊ - 7 | ) /* keep */{␊ - 8 | /* will keep */␊ - 9 | foo();␊ - 10 | /* will keep */␊ - 11 | }␊ + 3 | // 2␊ + 4 | a // 3␊ + 5 | .b // 4␊ + 6 | && // 5␊ + 7 | // 6␊ + 8 | ␊ + 9 | // 7␊ + 10 | c // 8␊ + 11 | .d // 9␊ + 12 | ) {␊ + 13 | // 10␊ + 14 | foo();␊ + 15 | // 11␊ + 16 | }␊ + 17 | // 12␊ ` > Error 1/1 `␊ - 1 | if /* will keep */␊ + 1 | if // 1␊ 2 | (␊ - 3 | /* will keep */␊ - 4 | a /* will keep */␊ - 5 | .b /* will keep */␊ - 6 | ) /* keep */{␊ - 7 | /* will remove */␊ - > 8 | if (␊ + 3 | // 2␊ + 4 | a // 3␊ + 5 | .b // 4␊ + 6 | ) // 5␊ + 7 | {␊ + 8 | // 6␊ + > 9 | if (␊ | ^^^^␊ - > 9 | /* will remove */␊ - | ^^^^^^^^^^^^^^^^^^^␊ - > 10 | c /* will keep */␊ - | ^^^^^^^^^^^^^^^^^^^␊ - > 11 | .d /* will remove */␊ - | ^^^^^^^^^^^^^^^^^^^␊ - > 12 | ) {␊ - | ^^^^^^^^^^^^^^^^^^^␊ - > 13 | /* will keep */␊ - | ^^^^^^^^^^^^^^^^^^^␊ - > 14 | foo();␊ - | ^^^^^^^^^^^^^^^^^^^␊ - > 15 | /* will keep */␊ - | ^^^^^^^^^^^^^^^^^^^␊ - > 16 | }␊ + > 10 | // 7␊ + | ^^^^^^␊ + > 11 | c // 8␊ + | ^^^^^^␊ + > 12 | .d // 9␊ + | ^^^^^^␊ + > 13 | ) {␊ + | ^^^^^^␊ + > 14 | // 10␊ + | ^^^^^^␊ + > 15 | foo();␊ + | ^^^^^^␊ + > 16 | // 11␊ + | ^^^^^^␊ + > 17 | }␊ | ^^^ Unexpected `if` as the only statement in a `if` block without `else`.␊ - 17 | /* will remove */␊ - 18 | }␊ + 18 | // 12␊ + 19 | }␊ ` ## Invalid #11 @@ -475,8 +481,8 @@ Generated by [AVA](https://avajs.dev). > Output `␊ - 1 | if (a && b) foo();␊ - 2 | [].forEach(bar)␊ + 1 | if (a && b) foo()␊ + 2 | ;[].forEach(bar)␊ ` > Error 1/1 @@ -497,9 +503,8 @@ Generated by [AVA](https://avajs.dev). > Output `␊ - 1 | if (a && b)␊ - 2 | foo()␊ - 3 | ;[].forEach(bar)␊ + 1 | if (a && b) foo()␊ + 2 | ;[].forEach(bar)␊ ` > Error 1/1 @@ -511,3 +516,26 @@ Generated by [AVA](https://avajs.dev). > 3 | ;[].forEach(bar)␊ | ^^ Unexpected `if` as the only statement in a `if` block without `else`.␊ ` + +## Invalid #13 + 1 | if (a) { + 2 | if (b) foo() + 3 | } + 4 | ;[].forEach(bar) + +> Output + + `␊ + 1 | if (a && b) foo()␊ + 2 | ;[].forEach(bar)␊ + ` + +> Error 1/1 + + `␊ + 1 | if (a) {␊ + > 2 | if (b) foo()␊ + | ^^^^^^^^^^^^ Unexpected `if` as the only statement in a `if` block without `else`.␊ + 3 | }␊ + 4 | ;[].forEach(bar)␊ + ` diff --git a/test/snapshots/no-lonely-if.js.snap b/test/snapshots/no-lonely-if.js.snap index 35462f869c64d46b6ea60ba6ebbeb9b29ee62f1a..7b027cd53d3af24334b4b8fa9b62eeb3b056709b 100644 GIT binary patch literal 1439 zcmV;Q1z`F?RzVIfaRlA>}yTRZ3`{iyIy--T1rBVDMm0#f++O%K1z?achFm?O|Bx7siNPO86K7}k`{u4oxNA$?~dqes+9S)qUvd;d2jv3PJDS)`w82U(Vs6|k>6jF`ebB* z|48aGN4&^I?Hz2(dwn159zH+Xe`)yj@{=!Yk6p@1?%_mZ+vroLyAWU33 z&JnMyT6qaVdu|Fl9y@kp_4mn5*ILHD+sYB|F4g$u2z`3-t?joS-Pu-<*xL2&zn6PB z;;mNg47RB)Q|0G-o?E@MrTB-oijt=|;;mRgZShJRaijUr!N5P8eo1|idU@i_Dvo%I zR#E%46rtb#>XX*rE^hI!Ik-949$m%}Z)q8|i`Z_yG(Gub`}pS5hqmpQ{QIjej(De4 zJNpnq3s51^+_exbbyXGth%gAT4sg5O5)2MPND!d15;wz2DUVYY^<$ZrcUhM~E5Tqo?ol!`{yZa$2OLDX)8kZp!2Z{EFx)b3p z=!?mni32hOqut49u*Q<3tIUK;Qs9IH=tv~ob}X+UbD#hN7+M`}@=Duyh63U9kv4z_ z4?Sc}>s>9ajsx+q981KZil%}S1S3I>d!m;F8#kKnAp{#BtTmgAo@&$mI@5h+rQYa| zMY|&)sXc2Ibpz}~CtV6R88bCfRWrr(l3`RI$(XQfqgRRZn&MP6TBvOXDbe*tbdzaN zMx@V(tezpX!3bS9LujKBN}0?O>(^p6qBF;e>-BLJ9RjS#oXtd0Ojr2hJ5?AM3TIc7 zPebiMO@~T4RMDw|PSu+a%AL%Ga==zDvqZd=EGLW2=}L3F%p9+=`5@CPd6Ws#Wy+$7 zeFOggYLc8?O@N_E7XIxzIeYaufAP!*1={Cc0bnSQjcxwkWxH*DvJEV@vk_~TV9ts4 z-b2I!i`dK@^0Pys>uL2UBqjUfVS`4-aflrBq&_Jo1HG*Ykso^g%W#7E9XTIgp~Fyz z{*k~s9O5`bv6_$;WA3lFWF68EnU=jFcg*_Ik^gu{$bo^Jjgp^ND8IgIlwd=d85KWE zpqzf*a1EKh)JXILjs2sBdgVh8ODc~rQr6kGX?AReuQCISndYJ6=+i4_^n26kU!RG7 z6;I&#T{Rtv-c~8x=?+P~f)(AU6&>q~Q-(do!x~#doDb3N4(NMMKCb0;>CpY2YM3=w zS4lcj+gjSsT1)Izln*KP+PQ?ZB|(+|Ut0^$;9>CVJD!lCH@Yv;D{Fh0!iBPV4T16( zWUR0PHbMvRG1lVMG!b+4$8b-6`FH(rimi z2%<)e21-iODB(@TG??OpCXFG+X!NNS42cAz(ZrY-W5WyD#5?!S|J3b` z3LU?iJr?C~>o@C($(0P#QPb7c{{H6Op#}cTsj45UINUm^nr|b+bjDw)o_XtIxN~~; za%1Z3VGg(MQ*9nuT#lUEc6NID(vz>Z24=6F;c#o)1H^tp_R;5W?!WWs!R``i&%oDz zT~Beib=4+fCz0LSeXHh5>cGPXca(qI{n&r%ZlNmA6VDXWyHW3pP zK8RJHB>4k2F0V3XU;wg++T-YwSFVjdga}^d$m#GLtHda$ z$A-(Q$0*EXJko|e$5FP0S;2)&IF*$KwPlimZZXiEra>vluz{>!1lnpqw=4o}GoU1* zF4%Srs|7qaHrS+(tLV`|L*^|bgF&alA1qa2peXFFChuVFK~0ZJdQ{P?f?m}t2IVI5 zp&Zba%PkRa4VIV5=5?ldo@SosSTV?SvVbyyQzjQp>;?G$t4U^gH35nyF7dbP6j5l%4ZS&;1h<0gbd`gA&P&DP+K>JukI77LHsD?4$Z?SkE z)jv8dH6niz4m+8DwAa;Af;o%{B!hj=T)N;}Y`_M4xg)}Y6Xr2v9 zL$bCSE7_w|g)vYbfyas|Y~y5rFeOuOIx;n14<=c!zk$c;S>IZytfv&6rrbq`kX}l< z)BhzY=sOSjkA#HW%~iJEtyJlZDSx?KV5s#rOPvNiS^zBN*gDI{@;$Bp0Sm%ZM4%l2 E0A--D5&!@I