Skip to content

Commit b0279dd

Browse files
costajohntsindresorhus
costajohnt
and
sindresorhus
authored
Add prefer-simple-condition-first rule (#2902)
Co-authored-by: Sindre Sorhus <[email protected]>
1 parent 8d5d487 commit b0279dd

12 files changed

Lines changed: 560 additions & 6 deletions

‎docs/rules/prefer-simple-condition-first.md‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
# Prefer simple conditions first in logical expressions
2+
3+
💼 This rule is enabled in the following [configs](https://github.com/sindresorhus/eslint-plugin-unicorn#recommended-config): ✅ `recommended`, ☑️ `unopinionated`.
4+
5+
🔧💡 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix) and manually fixable by [editor suggestions](https://eslint.org/docs/latest/use/core-concepts#rule-suggestions).
6+
7+
<!-- end auto-generated rule header -->
8+
<!-- Do not manually modify this header. Run: `npm run fix:eslint-docs` -->
9+
10+
When writing multiple conditions in a logical expression (`&&`, `||`), simple conditions should come first. This can improve readability and performance, since simple checks like identifiers and strict equality comparisons are cheaper to evaluate and can short-circuit before expensive operations.
11+
12+
A condition is considered "simple" if it is:
13+
14+
- A bare identifier (`foo`)
15+
- A strict equality/inequality comparison between identifiers and/or literals (`foo === 1`, `a !== b`)
16+
17+
## Examples
18+
19+
### `&&`
20+
21+
```js
22+
// ❌
23+
if (check(foo) && bar);
24+
25+
// ✅
26+
if (bar && check(foo));
27+
```
28+
29+
```js
30+
// ❌
31+
if (foo.bar.baz === 1 && bar === 2);
32+
33+
// ✅
34+
if (bar === 2 && foo.bar.baz === 1);
35+
```
36+
37+
### `||`
38+
39+
```js
40+
// ❌
41+
const x = foo() || bar;
42+
43+
// ✅
44+
const x = bar || foo();
45+
```
46+
47+
## Fix safety
48+
49+
Expressions with side effects or throwing potential are not flagged, since reordering would change program behavior:
50+
51+
- Assignment expressions (`state.ready = true`)
52+
- Update expressions (`++counter`)
53+
- Deep member expression chains (`object.deep.value`)
54+
- Tagged template expressions (`` tag`x` ``)
55+
56+
When the complex side contains function calls or `new` expressions, the fix is provided as a **suggestion** rather than an auto-fix, because reordering changes when the call executes due to short-circuit evaluation.
57+
58+
When both sides are side-effect-free (identifiers, simple member expressions, literals), the fix is applied automatically.

‎eslint.dogfooding.config.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ const config = [
3737
'unicorn/consistent-function-scoping': 'off',
3838
// Annoying
3939
'unicorn/no-keyword-prefix': 'off',
40+
// Suggestion-only violations remain in call-based patterns
41+
'unicorn/prefer-simple-condition-first': 'off',
4042
// https://github.com/sindresorhus/eslint-plugin-unicorn/issues/2833
4143
'unicorn/template-indent': ['error', {indent: '\t'}],
4244
},

‎readme.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,7 @@ export default [
177177
| [prefer-response-static-json](docs/rules/prefer-response-static-json.md) | Prefer `Response.json()` over `new Response(JSON.stringify())`. | ✅ ☑️ | 🔧 | |
178178
| [prefer-set-has](docs/rules/prefer-set-has.md) | Prefer `Set#has()` over `Array#includes()` when checking for existence or non-existence. | ✅ ☑️ | 🔧 | 💡 |
179179
| [prefer-set-size](docs/rules/prefer-set-size.md) | Prefer using `Set#size` instead of `Array#length`. | ✅ ☑️ | 🔧 | |
180+
| [prefer-simple-condition-first](docs/rules/prefer-simple-condition-first.md) | Prefer simple conditions first in logical expressions. | ✅ ☑️ | 🔧 | 💡 |
180181
| [prefer-single-call](docs/rules/prefer-single-call.md) | Enforce combining multiple `Array#push()`, `Element#classList.{add,remove}()`, and `importScripts()` into one call. | ✅ ☑️ | 🔧 | 💡 |
181182
| [prefer-spread](docs/rules/prefer-spread.md) | Prefer the spread operator over `Array.from(…)`, `Array#concat(…)`, `Array#{slice,toSpliced}()` and `String#split('')`. | ✅ | 🔧 | 💡 |
182183
| [prefer-string-raw](docs/rules/prefer-string-raw.md) | Prefer using the `String.raw` tag to avoid escaping `\`. | ✅ ☑️ | 🔧 | |

‎rules/index.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ export {default as 'prefer-regexp-test'} from './prefer-regexp-test.js';
120120
export {default as 'prefer-response-static-json'} from './prefer-response-static-json.js';
121121
export {default as 'prefer-set-has'} from './prefer-set-has.js';
122122
export {default as 'prefer-set-size'} from './prefer-set-size.js';
123+
export {default as 'prefer-simple-condition-first'} from './prefer-simple-condition-first.js';
123124
export {default as 'prefer-single-call'} from './prefer-single-call.js';
124125
export {default as 'prefer-spread'} from './prefer-spread.js';
125126
export {default as 'prefer-string-raw'} from './prefer-string-raw.js';

‎rules/no-instanceof-builtins.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ const create = context => {
113113
context.on('BinaryExpression', /** @param {import('estree').BinaryExpression} node */ node => {
114114
const {right, operator} = node;
115115

116-
if (right.type !== 'Identifier' || operator !== 'instanceof' || exclude.includes(right.name)) {
116+
if ((operator !== 'instanceof') || (right.type !== 'Identifier') || exclude.includes(right.name)) {
117117
return;
118118
}
119119

‎rules/prefer-at.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ function checkSliceCall(node) {
102102
&& (
103103
firstElementGetMethod === 'zero-index'
104104
|| firstElementGetMethod === 'shift'
105-
|| (startIndex === -1 && firstElementGetMethod === 'pop')
105+
|| ((firstElementGetMethod === 'pop') && (startIndex === -1))
106106
)
107107
) {
108108
return {safeToFix: true, firstElementGetMethod};

‎rules/prefer-export-from.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,10 @@ function getFixFunction({
7676
let exportDeclaration;
7777
if (shouldExportAsType) {
7878
// If a type export declaration already exists, reuse it, else use a value export declaration with an inline type specifier.
79-
exportDeclaration = exportDeclarations.find(({source, exportKind}) => source.value === sourceValue && exportKind === 'type');
79+
exportDeclaration = exportDeclarations.find(({source, exportKind}) => (exportKind === 'type') && (source.value === sourceValue));
8080
}
8181

82-
exportDeclaration ||= exportDeclarations.find(({source, exportKind}) => source.value === sourceValue && exportKind !== 'type');
82+
exportDeclaration ||= exportDeclarations.find(({source, exportKind}) => (exportKind !== 'type') && (source.value === sourceValue));
8383

8484
/** @param {import('eslint').Rule.RuleFixer} fixer */
8585
return function * (fixer) {

‎rules/prefer-simple-condition-first.js‎

Lines changed: 258 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,258 @@
1+
import {
2+
isParenthesized,
3+
getParenthesizedText,
4+
getParenthesizedRange,
5+
shouldAddParenthesesToLogicalExpressionChild,
6+
} from './utils/index.js';
7+
8+
const MESSAGE_ID = 'prefer-simple-condition-first';
9+
const MESSAGE_ID_SUGGESTION = 'prefer-simple-condition-first/suggestion';
10+
11+
const messages = {
12+
[MESSAGE_ID]: 'Prefer simple condition first in `{{operator}}` expression.',
13+
[MESSAGE_ID_SUGGESTION]: 'Swap conditions.',
14+
};
15+
16+
/**
17+
Check if a node is a "simple" condition:
18+
1. Bare identifier (`foo`)
19+
2. A binary `===`/`!==` where each operand is an identifier, a literal, or a signed
20+
numeric literal (`-1`, `+0`), and at least one operand is an identifier.
21+
*/
22+
function isSimpleOperand(node) {
23+
if (node.type === 'Identifier' || node.type === 'Literal') {
24+
return true;
25+
}
26+
27+
// Negative/positive numeric literals: `-1`, `+0`
28+
return node.type === 'UnaryExpression'
29+
&& (node.operator === '-' || node.operator === '+')
30+
&& node.argument.type === 'Literal'
31+
&& typeof node.argument.value === 'number';
32+
}
33+
34+
function isSimple(node) {
35+
if (node.type === 'Identifier') {
36+
return true;
37+
}
38+
39+
if (
40+
node.type === 'UnaryExpression'
41+
&& node.operator === '!'
42+
) {
43+
return isSimple(node.argument);
44+
}
45+
46+
if (
47+
node.type === 'BinaryExpression'
48+
&& (node.operator === '===' || node.operator === '!==')
49+
) {
50+
return isSimpleOperand(node.left) && isSimpleOperand(node.right)
51+
&& (node.left.type === 'Identifier' || node.right.type === 'Identifier');
52+
}
53+
54+
// A chain of all-simple conditions is considered simple to prevent fix oscillation
55+
if (node.type === 'LogicalExpression') {
56+
return isSimple(node.left) && isSimple(node.right);
57+
}
58+
59+
return false;
60+
}
61+
62+
const sideEffectTypes = new Set([
63+
'AssignmentExpression',
64+
'UpdateExpression',
65+
'TaggedTemplateExpression',
66+
'AwaitExpression',
67+
'YieldExpression',
68+
'ImportExpression',
69+
]);
70+
71+
/**
72+
Check if an AST subtree contains side effects or throwing potential
73+
(assignments, updates, member access, tagged templates, in/instanceof, await, yield, dynamic import).
74+
These patterns are not flagged, since reordering would change program behavior.
75+
*/
76+
function hasSideEffectsOrThrows(node) {
77+
if (!node || typeof node !== 'object') {
78+
return false;
79+
}
80+
81+
if (
82+
sideEffectTypes.has(node.type)
83+
// Any non-optional member access can throw if the object is nullish
84+
|| (node.type === 'MemberExpression' && !node.optional)
85+
// `in` and `instanceof` throw if the right operand is not an object/constructor
86+
|| (node.type === 'BinaryExpression' && (node.operator === 'in' || node.operator === 'instanceof'))
87+
) {
88+
return true;
89+
}
90+
91+
for (const key of Object.keys(node)) {
92+
if (key === 'parent') {
93+
continue;
94+
}
95+
96+
const value = node[key];
97+
if (Array.isArray(value)) {
98+
if (value.some(child => hasSideEffectsOrThrows(child))) {
99+
return true;
100+
}
101+
} else if (value && typeof value.type === 'string' && hasSideEffectsOrThrows(value)) {
102+
return true;
103+
}
104+
}
105+
106+
return false;
107+
}
108+
109+
/**
110+
Check if an AST subtree contains call or new expressions.
111+
*/
112+
function hasCallOrNew(node) {
113+
if (!node || typeof node !== 'object') {
114+
return false;
115+
}
116+
117+
if (node.type === 'CallExpression' || node.type === 'NewExpression') {
118+
return true;
119+
}
120+
121+
for (const key of Object.keys(node)) {
122+
if (key === 'parent') {
123+
continue;
124+
}
125+
126+
const value = node[key];
127+
if (Array.isArray(value)) {
128+
if (value.some(child => hasCallOrNew(child))) {
129+
return true;
130+
}
131+
} else if (value && typeof value.type === 'string' && hasCallOrNew(value)) {
132+
return true;
133+
}
134+
}
135+
136+
return false;
137+
}
138+
139+
function getSwapText(node, context, {operator, property}) {
140+
const isNodeParenthesized = isParenthesized(node, context);
141+
let text = isNodeParenthesized
142+
? getParenthesizedText(node, context)
143+
: context.sourceCode.getText(node);
144+
145+
if (
146+
!isNodeParenthesized
147+
&& shouldAddParenthesesToLogicalExpressionChild(node, {operator, property})
148+
) {
149+
text = `(${text})`;
150+
}
151+
152+
return text;
153+
}
154+
155+
/**
156+
Check if a LogicalExpression is used in a boolean context where the
157+
produced value is only tested for truthiness, not consumed as a value.
158+
*/
159+
function isBooleanContext(node) {
160+
const {parent} = node;
161+
162+
if (!parent) {
163+
return false;
164+
}
165+
166+
if (
167+
(parent.type === 'IfStatement' && parent.test === node)
168+
|| (parent.type === 'WhileStatement' && parent.test === node)
169+
|| (parent.type === 'DoWhileStatement' && parent.test === node)
170+
|| (parent.type === 'ForStatement' && parent.test === node)
171+
|| (parent.type === 'ConditionalExpression' && parent.test === node)
172+
|| (parent.type === 'UnaryExpression' && parent.operator === '!')
173+
) {
174+
return true;
175+
}
176+
177+
// A LogicalExpression nested inside another LogicalExpression inherits its context
178+
if (parent.type === 'LogicalExpression') {
179+
return isBooleanContext(parent);
180+
}
181+
182+
return false;
183+
}
184+
185+
/** @param {import('eslint').Rule.RuleContext} context */
186+
const create = context => {
187+
const {sourceCode} = context;
188+
189+
context.on('LogicalExpression', node => {
190+
if (node.operator !== '&&' && node.operator !== '||') {
191+
return;
192+
}
193+
194+
if (!isSimple(node.right) || isSimple(node.left)) {
195+
return;
196+
}
197+
198+
// Only flag in boolean contexts — reordering in value-producing contexts changes the result
199+
if (!isBooleanContext(node)) {
200+
return;
201+
}
202+
203+
// Skip expressions with side effects or throwing potential entirely
204+
if (hasSideEffectsOrThrows(node.left)) {
205+
return;
206+
}
207+
208+
const rightText = getSwapText(node.right, context, {operator: node.operator, property: 'left'});
209+
const leftText = getSwapText(node.left, context, {operator: node.operator, property: 'right'});
210+
211+
const fix = fixer => fixer.replaceTextRange(
212+
[getParenthesizedRange(node.left, context)[0], getParenthesizedRange(node.right, context)[1]],
213+
`${rightText} ${node.operator} ${leftText}`,
214+
);
215+
216+
// Use suggestion (not auto-fix) when left contains calls/new or is a chain
217+
const isChain = node.left.type === 'LogicalExpression' && node.left.operator === node.operator;
218+
if (isChain || hasCallOrNew(node.left)) {
219+
return {
220+
node,
221+
loc: sourceCode.getLoc(node.right),
222+
messageId: MESSAGE_ID,
223+
data: {operator: node.operator},
224+
suggest: [
225+
{
226+
messageId: MESSAGE_ID_SUGGESTION,
227+
fix,
228+
},
229+
],
230+
};
231+
}
232+
233+
return {
234+
node,
235+
loc: sourceCode.getLoc(node.right),
236+
messageId: MESSAGE_ID,
237+
data: {operator: node.operator},
238+
fix,
239+
};
240+
});
241+
};
242+
243+
/** @type {import('eslint').Rule.RuleModule} */
244+
const config = {
245+
create,
246+
meta: {
247+
type: 'suggestion',
248+
docs: {
249+
description: 'Prefer simple conditions first in logical expressions.',
250+
recommended: 'unopinionated',
251+
},
252+
fixable: 'code',
253+
hasSuggestions: true,
254+
messages,
255+
},
256+
};
257+
258+
export default config;

‎rules/utils/is-number.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -200,8 +200,8 @@ export default function isNumber(node, scope) {
200200
if (
201201
testStaticValueResult !== null
202202
&& (
203-
(testStaticValueResult.value && isConsequentNumber)
204-
|| (!testStaticValueResult.value && isAlternateNumber)
203+
(isConsequentNumber && testStaticValueResult.value)
204+
|| (isAlternateNumber && !testStaticValueResult.value)
205205
)
206206
) {
207207
return true;

0 commit comments

Comments
 (0)