From a1b60ade0c49acbf522075ed060ff770f6030776 Mon Sep 17 00:00:00 2001 From: fisker Cheung Date: Wed, 23 Dec 2020 14:08:54 +0800 Subject: [PATCH] `explicit-length-check`: Check unsafe `LogicalExpression`s (#952) Co-authored-by: Sindre Sorhus --- docs/rules/explicit-length-check.md | 24 +++++++++- readme.md | 2 +- rules/explicit-length-check.js | 36 ++++++++++++--- test/explicit-length-check.js | 44 ++++++++++++++++++- test/snapshots/explicit-length-check.js.md | 5 ++- test/snapshots/explicit-length-check.js.snap | Bin 2221 -> 2233 bytes 6 files changed, 100 insertions(+), 11 deletions(-) diff --git a/docs/rules/explicit-length-check.md b/docs/rules/explicit-length-check.md index 1ac2d77ede..631dbb208d 100644 --- a/docs/rules/explicit-length-check.md +++ b/docs/rules/explicit-length-check.md @@ -2,7 +2,7 @@ Enforce explicitly checking the length of an object and enforce the comparison style. -This rule is fixable. +This rule is fixable, unless it's [unsafe to fix](#unsafe-to-fix-case). ## Zero comparisons @@ -141,3 +141,25 @@ The `non-zero` option can be configured with one of the following: - Enforces non-zero to be checked with: `foo.length !== 0` - `greater-than-or-equal` - Enforces non-zero to be checked with: `foo.length >= 1` + +## Unsafe to fix case + +`.length` check inside `LogicalExpression`s are not safe to fix. + +Example: + +```js +const bothNotEmpty = (a, b) => a.length && b.length; + +if (bothNotEmpty(foo, bar)) {} +``` + +In this case, the `bothNotEmpty` function returns a `number`, but it will most likely be used as a `boolean`. The rule will still report this as an error, but without an auto-fix. You can apply a [suggestion](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions) in your editor, which will fix it to: + +```js +const bothNotEmpty = (a, b) => a.length > 0 && b.length > 0; + +if (bothNotEmpty(foo, bar)) {} +``` + +The rule is smart enough to know some `LogicalExpression`s are safe to fix, like when it's inside `if`, `while`, etc. diff --git a/readme.md b/readme.md index 933096126a..d372f3dcad 100644 --- a/readme.md +++ b/readme.md @@ -112,7 +112,7 @@ Configure it in `package.json`. - [error-message](docs/rules/error-message.md) - Enforce passing a `message` value when creating a built-in error. - [escape-case](docs/rules/escape-case.md) - Require escape sequences to use uppercase values. *(fixable)* - [expiring-todo-comments](docs/rules/expiring-todo-comments.md) - Add expiration conditions to TODO comments. -- [explicit-length-check](docs/rules/explicit-length-check.md) - Enforce explicitly comparing the `length` property of a value. *(fixable)* +- [explicit-length-check](docs/rules/explicit-length-check.md) - Enforce explicitly comparing the `length` property of a value. *(partly fixable)* - [filename-case](docs/rules/filename-case.md) - Enforce a case style for filenames. - [import-index](docs/rules/import-index.md) - Enforce importing index files with `.`. *(fixable)* - [import-style](docs/rules/import-style.md) - Enforce specific import styles per module. diff --git a/rules/explicit-length-check.js b/rules/explicit-length-check.js index 9dd6178ae6..1cb404478f 100644 --- a/rules/explicit-length-check.js +++ b/rules/explicit-length-check.js @@ -5,9 +5,11 @@ const isLiteralValue = require('./utils/is-literal-value'); const TYPE_NON_ZERO = 'non-zero'; const TYPE_ZERO = 'zero'; +const MESSAGE_ID_SUGGESTION = 'suggestion'; const messages = { [TYPE_NON_ZERO]: 'Use `.length {{code}}` when checking length is not zero.', - [TYPE_ZERO]: 'Use `.length {{code}}` when checking length is zero.' + [TYPE_ZERO]: 'Use `.length {{code}}` when checking length is zero.', + [MESSAGE_ID_SUGGESTION]: 'Replace `.length` with `.length {{code}}`.' }; const isLogicNot = node => @@ -172,7 +174,7 @@ function create(context) { const nonZeroStyle = nonZeroStyles.get(options['non-zero']); const sourceCode = context.getSourceCode(); - function reportProblem({node, isZeroLengthCheck, lengthNode}) { + function reportProblem({node, isZeroLengthCheck, lengthNode, autoFix}) { const {code, test} = isZeroLengthCheck ? zeroStyle : nonZeroStyle; if (test(node)) { return; @@ -187,17 +189,33 @@ function create(context) { fixed = `(${fixed})`; } - context.report({ + const fix = fixer => fixer.replaceText(node, fixed); + + const problem = { node, messageId: isZeroLengthCheck ? TYPE_ZERO : TYPE_NON_ZERO, - data: {code}, - fix: fixer => fixer.replaceText(node, fixed) - }); + data: {code} + }; + + if (autoFix) { + problem.fix = fix; + } else { + problem.suggest = [ + { + messageId: MESSAGE_ID_SUGGESTION, + data: {code}, + fix + } + ]; + } + + context.report(problem); } return { [lengthSelector](lengthNode) { let node; + let autoFix = true; let {isZeroLengthCheck, node: lengthCheckNode} = getLengthCheckNode(lengthNode); if (lengthCheckNode) { @@ -211,11 +229,15 @@ function create(context) { if (isBooleanNode(ancestor)) { isZeroLengthCheck = isNegative; node = ancestor; + } else if (lengthNode.parent.type === 'LogicalExpression') { + isZeroLengthCheck = isNegative; + node = lengthNode; + autoFix = false; } } if (node) { - reportProblem({node, isZeroLengthCheck, lengthNode}); + reportProblem({node, isZeroLengthCheck, lengthNode, autoFix}); } } }; diff --git a/test/explicit-length-check.js b/test/explicit-length-check.js index a82c0035fc..7bba751bd1 100644 --- a/test/explicit-length-check.js +++ b/test/explicit-length-check.js @@ -1,6 +1,22 @@ import {outdent} from 'outdent'; import {test} from './utils/test'; +const suggestionCase = ({code, output, desc, options = []}) => { + const suggestion = {output}; + if (desc) { + suggestion.desc = desc; + } + + return { + code, + output: code, + options, + errors: [ + {suggestions: [suggestion]} + ] + }; +}; + const nonZeroCases = [ 'foo.length', '!!foo.length', @@ -82,7 +98,33 @@ test({ 'if (foo.length > 1) {}', 'if (foo.length < 2) {}' ], - invalid: [] + invalid: [ + suggestionCase({ + code: 'const x = foo.length || bar()', + output: 'const x = foo.length > 0 || bar()', + desc: 'Replace `.length` with `.length > 0`.' + }), + suggestionCase({ + code: 'const x = foo.length || bar()', + output: 'const x = foo.length !== 0 || bar()', + desc: 'Replace `.length` with `.length !== 0`.', + options: [{'non-zero': 'not-equal'}] + }), + suggestionCase({ + code: 'const x = foo.length || bar()', + output: 'const x = foo.length >= 1 || bar()', + desc: 'Replace `.length` with `.length >= 1`.', + options: [{'non-zero': 'greater-than-or-equal'}] + }), + suggestionCase({ + code: '() => foo.length && bar()', + output: '() => foo.length > 0 && bar()' + }), + suggestionCase({ + code: 'alert(foo.length && bar())', + output: 'alert(foo.length > 0 && bar())' + }) + ] }); test.visualize([ diff --git a/test/snapshots/explicit-length-check.js.md b/test/snapshots/explicit-length-check.js.md index 9d9b383f10..480d3dab2c 100644 --- a/test/snapshots/explicit-length-check.js.md +++ b/test/snapshots/explicit-length-check.js.md @@ -871,9 +871,12 @@ Generated by [AVA](https://avajs.dev). Output:␊ 1 | bar(foo.length === 0 || foo.length)␊ ␊ - Error 1/1:␊ + Error 1/2:␊ > 1 | bar(!foo.length || foo.length)␊ | ^^^^^^^^^^^ Use `.length === 0` when checking length is zero.␊ + Error 2/2:␊ + > 1 | bar(!foo.length || foo.length)␊ + | ^^^^^^^^^^ Use `.length > 0` when checking length is not zero.␊ ` ## explicit-length-check - #14 diff --git a/test/snapshots/explicit-length-check.js.snap b/test/snapshots/explicit-length-check.js.snap index da87f52b4a69ce8264965892adeef19533965f32..c3544cac12e6df653595235a34f8d611c971c0c6 100644 GIT binary patch literal 2233 zcmV;q2uAloRzV7lutgM+!Pv@HH``e{QMO@tsUV3) z5CVY+64=DRj1gi+d|*HgF+!q=KrqArMv$mH#K0fMdr!Ca-g_RsZEv@B^d=DUedpeD zzTfvd-#Pbpdq^7wz!;D+f&2Bmd;=2 z+E;c}z}7Fs2KSjy-}rdLZ%LG%#n) zm#4nkw)xW$2U5pvec?!}fUT_=05sUPr{A3ZM#}iQEPJ|P>SzI5tM3J%!`-^0c6ZGk zrIuZ@pPJJ6vw*FYnE-U26b{W>Rn?uA4rX_be!pJ8*0wAF);+iU+}t1b>^U>;m9vwGYv-|v;lg3=j+MWi#s}-=-GY)`_D{X6ksa|>+u7^ib?@nkIxiQO?&O= zxhp*xJ(+u_-Wu7MDqw4G6#!qK*txR$lKs+|uJW-{3^~UIZ0(%|z&96~i!w)^SkQdF zXY(Vy?}=lRtgB~>qnP6}y^oK3%}p4b=erVeMYlxXZK7y)Wb7&8x?{ z1Z?fD0bn>7i8C*xfKkIv0Ar=Qwar&*gwO&zpldNSo6L(np2;mvccZTfR#a5Lg1kJ~ z-f1*ewfR&{Wet(o47Zx$^?JO}GR0DAL?NFE#j8X53)Yese0qr!E|hh!5H4+Ux?z2j zv%cBoZiIav*AnRV_~0_9*E8A34R2|atHr4tb_KN1qNB8A2FQ78Fg`NG=)?P06<)o^ zy~HQBpYdX=?^RdO4{6)zpn5a>#&yf&=c9jRXaS3se*q z)p^CCz3g!{K-sv;=;6y6lSD0JLcr>S9y`=A=G425RjodkIEqr^Jh8ua+-J7iT*+D} zSxqLwUZiSmlH%#}6%)RIt3>5O0gp}c6yWC&o!R?+q|4!Vl@lIIf#k8ka`{b?T?#%A zJ}iPz3atuqXv0OC=yGuAE;T+fml__Ki-0HSvawxuw#&hE`SDS?X!!I`1P22m#G;Ng zAq>pgC-$bL_}vZ^^doF#B21s0h>eU2`6#d-Vi*^Oi7|_Ph>r|`e^fb0K%9$o!kNcF z#6^adf`~H^uP_%60L&Z$A|5i71VoJg;G$eIxG1yvhxo|oTZLkevn^h6<}na)k)Sq5n<*K5OI*vvkHZWvSWy$%;q0rBBLw))i5jGjqexes>n=_Ppw$U2H(*kz0Nb` z5&q2*oPh^0^%89Z@FT^!nD9lQAl0d`5=S7Hail?Uv?HmtS}q}O>4(GG4@ZbTg7HVE zm-N3#h2L)s@%~YzRW*2EdnYL%<`Fs-qaju$X#y4d!IU`ljPUmmnxt&(<

=UUp2BvhUBMhGjDuSV@C26^jzLBIuBKN65k^9uLwA`oGrR6@gFfH>D$~5*Vks(EC3=Tz$_IJnC zqD}mA7D9|3Dr_TFXwJN&iqBN2qXB!UfLf>xR2IFcYI7jo&@WlxK%P*z7Db0EhhY?P z8SxO09Pl`VwK!(5vI$1hl7mR-%kja&wPX-*m5h1^%J{JoRNpbFZj2wT9(_)H-Z45|2q z8JQ=qa(LXBDF$MGS*-qWhzVvXRJWJEpAx|ar=NugHaN>H{n65LLP1?1Upuz5^~I z3fVPt&a6$QjiGL1@+F)2xxfu~E6{&eBg1Qa64)Up&Io05cy31Nx{lrY??fcQz) zag9szC1Spm#1CJ1MFFvstlt8cq)SM;*b;*-;noDiN;1K&h<@5}o07#oG|9rK2#B3z zN{xz?M?;t{j@ybZ%$k7sN!GLp*UqQJri`T^$(fMr2;yd0%RXE?of3dDA=KR@WkRea zh?`}7=@ciFiBRNCQbxmCg7{h1dAS=n&cs0IO>#!dVuH9?*0(df)|1y&=TmI(|pNfMu5s1W?3(2#_b2Rdf$^UK!zocSL~H zC()9qQzRgg^XjoWF-Yr)nokN8`oM8Vh+k}meTGtp|Jji2i;f8e5E?9+zTEr|MF4|) Hwp9QC`Cd@S literal 2221 zcmV;e2vYY!RzVOL1VapbMDt3viboq3}j52yyE87oJ-p(zL_v5Cv&R+r0cZ!xg+Dg;-M!lymo!@?A7i) z6;}jo{X}eVm-*!N_uu^Gkqal^Ta$Zf!bJgF4~x&spI$wSishg z`U7A;Qn+m6p#iPe&NkmOb=U`jfUQ4d15j(?9=uvHuyn6dKH z6Q6C~@Zo@cnIktneYjP?*7M@0!M-K?#>7`LM%CpyvJK;h3fOuy2Y`U5b?clRwFAqo z+owG`uJK0!TMyj{K>Kmw;H(ujyRx#uwDzHI*9+MC`)~kOKe_bG%x`z@JT>z9RZVTV zFA3QCMJ@nqwyyhQ*V$Lc47-}UB@2E%N5Ix!M*y&HxqW5x6aH28W17tQd!4fdY@KNa zU`OWqL+^Cdo*R$c4?X_h=A{C*uC)Nr8JM_nNbcV+-@E&^hxZA~ss(JFJVit`>%}8y zE_dd14&Ocg=Agz*0b6Y~0DOLI+w$ob9T!h^RE`*D$p22jR@+nnzC71lGJMdnxy@%g zH#~UYEpcp;^8ms;YuT77J`^ zHyUde`4vrN4UyOkx0>Yhd4142&RT9ns?UVt6_EacbI1!mw$KIV$vT(^7dN>)u)fJv z-|Y4@!Y+?{A@q3taEZ(39c$!Lwy`$bf)?1MZZK1OO6&BU!m*SMRhHCV!L+ZY-1QPSvX<{VmM~WXcLVF84?8rlC@Z} znoNYfMA6zL#na_0C43=QnZkua9=qfz!p|W(b9DPimow<9Bs|t4$zz3;@|z^P6ubpK zAc9a1Z4u1g=N_=E4B|I`00Z-UvXS*D1my_uV;-hfU@adiiP6kAXMIC8E z7?`zB>`hDYyA>!HMA*hem_9iX8yN-iabVxaFfNV~V;1`m9~lDwxN?w!IG5;zGmn9Y ziwrFVF=rrIVJ__fm^lPQJY*;dh#CLhMY*hZQD*ZG@sZKD3MC$Ad$QupV<6%pqiYq? z31&ws!ptEc;vl1E6^agJXCFhE%|FCMMpybPVUBn=zFVNHB2&D6rD7o)d>|mb&NJl^ z{>>7cp$9Pa5^VzrBE`9w@Wr4Yols~e;t1q2jx;Eab|ke{$|b}t1RN@TgyYvyFZo^( zXM-2EwUfe~h0uo>4Y3+X6NcCirsP%6OrBTK0%c>bgjR+Q#+CuuA{e_1I+J-7Ao|2A zC;I|%h~^fTI4mfQ8k!}IuAo4!&si!&=jhQi)t7ZQsR_y+LEsYQ82A%jN36c0iTx(C ze3j8b>8CxAI;V9aevW#%H&M!MFpnC_ePra?$~&h+X10~c=!sE%ma1GV-pQ4SrQFiC zD}exgyArgM+ZDV*&~_^*_mK^J|u67*4bD?t}+w}L$?`?$!tz{SB-6%I`)1Ib za^@XX^rb=_57>PL)M9O*vgk!sc|-B4zuZIvc}n405+AM{hEYUa#6vuGz>^Tx(uBdv zCKye_4I`m1$9oIcvR=TI!!V|7%f>to&=U!)J-LA8QH-V2CW7a2mB`vk0Pg2ko8MuT?<*N8XHQH-~8;tiSos+@?Qs%h#rh>-H4znk+=j9#I6H=p;` zZ8Dk6Awn7$-whZd0*d@@K<=xI{GE}=peou|0QQiR@%4a6Fr?rYW~4=4I8 z$uug(#-ub|dY&#N`qPC|6Oc-hsbwifHl+!Z5WpJbc%AaEMlm5aMHm~jR(vqw@^=;@j3}&AYE|+5E1HSMyRrr zg5sZDJ@A61-k@^0j^9x@VA-Vu0V3i*NI5aApu5^RVT2FfQO^YvPogDJr$|67=fo3r vVvyDoHJ=nH^nqib$}cvfK0~R)|0qcIMaP5!2n`lZ-`xBU+g+`}E>!>k?-3==