Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add prefer-logical-operator-over-ternary rule #1830

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions configs/recommended.js
Expand Up @@ -83,6 +83,7 @@ module.exports = {
'unicorn/prefer-includes': 'error',
'unicorn/prefer-json-parse-buffer': 'off',
'unicorn/prefer-keyboard-event-key': 'error',
'unicorn/prefer-logical-operator-over-ternary': 'error',
'unicorn/prefer-math-trunc': 'error',
'unicorn/prefer-modern-dom-apis': 'error',
'unicorn/prefer-modern-math-apis': 'error',
Expand Down
52 changes: 52 additions & 0 deletions docs/rules/prefer-logical-operator-over-ternary.md
@@ -0,0 +1,52 @@
# Prefer using a logical operator over a ternary

<!-- Do not manually modify RULE_NOTICE part. Run: `npm run generate-rule-notices` -->
<!-- RULE_NOTICE -->
✅ *This rule is part of the [recommended](https://github.com/sindresorhus/eslint-plugin-unicorn#recommended-config) config.*

💡 *This rule provides [suggestions](https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions).*
<!-- /RULE_NOTICE -->

Disallow ternary operators when simpler logical operator alternatives exist.

Ideally, most reported cases have an equivalent [`Logical OR(||)`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Logical_OR) expression. The rule intentionally provides suggestions instead of auto-fixes, because in many cases, the [nullish coalescing operator (`??`)](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing_operator) should be preferred.

## Fail

```js
foo ? foo : bar;
```

```js
foo.bar ? foo.bar : foo.baz
```

```js
foo?.bar ? foo.bar : baz
```

```js
!bar ? foo : bar;
```

## Pass

```js
foo ?? bar;
```

```js
foo || bar;
```

```js
foo ? bar : baz;
```

```js
foo.bar ?? foo.baz
```

```js
foo?.bar ?? baz
```
1 change: 1 addition & 0 deletions readme.md
Expand Up @@ -123,6 +123,7 @@ Each rule has emojis denoting:
| [prefer-includes](docs/rules/prefer-includes.md) | Prefer `.includes()` over `.indexOf()` and `Array#some()` when checking for existence or non-existence. | ✅ | 🔧 | 💡 |
| [prefer-json-parse-buffer](docs/rules/prefer-json-parse-buffer.md) | Prefer reading a JSON file as a buffer. | | 🔧 | |
| [prefer-keyboard-event-key](docs/rules/prefer-keyboard-event-key.md) | Prefer `KeyboardEvent#key` over `KeyboardEvent#keyCode`. | ✅ | 🔧 | |
| [prefer-logical-operator-over-ternary](docs/rules/prefer-logical-operator-over-ternary.md) | Prefer using a logical operator over a ternary. | ✅ | | 💡 |
| [prefer-math-trunc](docs/rules/prefer-math-trunc.md) | Enforce the use of `Math.trunc` instead of bitwise operators. | ✅ | 🔧 | 💡 |
| [prefer-modern-dom-apis](docs/rules/prefer-modern-dom-apis.md) | Prefer `.before()` over `.insertBefore()`, `.replaceWith()` over `.replaceChild()`, prefer one of `.before()`, `.after()`, `.append()` or `.prepend()` over `insertAdjacentText()` and `insertAdjacentElement()`. | ✅ | 🔧 | |
| [prefer-modern-math-apis](docs/rules/prefer-modern-math-apis.md) | Prefer modern `Math` APIs over legacy patterns. | ✅ | 🔧 | |
Expand Down
155 changes: 155 additions & 0 deletions rules/prefer-logical-operator-over-ternary.js
@@ -0,0 +1,155 @@
'use strict';
const {isParenthesized, getParenthesizedText} = require('./utils/parentheses.js');
const isSameReference = require('./utils/is-same-reference.js');
const shouldAddParenthesesToLogicalExpressionChild = require('./utils/should-add-parentheses-to-logical-expression-child.js');
const needsSemicolon = require('./utils/needs-semicolon.js');

const MESSAGE_ID_ERROR = 'prefer-logical-operator-over-ternary/error';
const MESSAGE_ID_SUGGESTION = 'prefer-logical-operator-over-ternary/suggestion';
const messages = {
[MESSAGE_ID_ERROR]: 'Prefer using a logical operator over a ternary.',
[MESSAGE_ID_SUGGESTION]: 'Switch to `{{operator}}` operator.',
};

function isSameNode(left, right, sourceCode) {
if (isSameReference(left, right)) {
return true;
}

if (left.type !== right.type) {
return false;
}

switch (left.type) {
case 'AwaitExpression':
return isSameNode(left.argument, right.argument, sourceCode);

case 'LogicalExpression':
return (
left.operator === right.operator
&& isSameNode(left.left, right.left, sourceCode)
&& isSameNode(left.right, right.right, sourceCode)
);

case 'UnaryExpression':
return (
left.operator === right.operator
&& left.prefix === right.prefix
&& isSameNode(left.argument, right.argument, sourceCode)
);

case 'UpdateExpression':
return false;

// No default
}

return sourceCode.getText(left) === sourceCode.getText(right);
}

function fix({
fixer,
sourceCode,
conditionalExpression,
left,
right,
operator,
}) {
let text = [left, right].map((node, index) => {
const isNodeParenthesized = isParenthesized(node, sourceCode);
let text = isNodeParenthesized ? getParenthesizedText(node, sourceCode) : sourceCode.getText(node);

if (
!isNodeParenthesized
&& shouldAddParenthesesToLogicalExpressionChild(node, {operator, property: index === 0 ? 'left' : 'right'})
) {
text = `(${text})`;
}

return text;
}).join(` ${operator} `);

// According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence#table
// There should be no cases need add parentheses when switching ternary to logical expression

// ASI
if (needsSemicolon(sourceCode.getTokenBefore(conditionalExpression), sourceCode, text)) {
text = `;${text}`;
}

return fixer.replaceText(conditionalExpression, text);
}

function getProblem({
sourceCode,
conditionalExpression,
left,
right,
}) {
return {
node: conditionalExpression,
messageId: MESSAGE_ID_ERROR,
suggest: ['??', '||'].map(operator => ({
messageId: MESSAGE_ID_SUGGESTION,
data: {operator},
fix: fixer => fix({
fixer,
sourceCode,
conditionalExpression,
left,
right,
operator,
}),
})),
};
}

/** @param {import('eslint').Rule.RuleContext} context */
const create = context => {
const sourceCode = context.getSourceCode();

return {
ConditionalExpression(conditionalExpression) {
const {test, consequent, alternate} = conditionalExpression;

// `foo ? foo : bar`
if (isSameNode(test, consequent, sourceCode)) {
return getProblem({
sourceCode,
conditionalExpression,
left: test,
right: alternate,
});
}

// `!bar ? foo : bar`
if (
test.type === 'UnaryExpression'
&& test.operator === '!'
&& test.prefix
&& isSameNode(test.argument, alternate, sourceCode)
) {
return getProblem({
sourceCode,
conditionalExpression,
left: test.argument,
right: consequent,
});
}
},
};
};

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
create,
meta: {
type: 'suggestion',
docs: {
description: 'Prefer using a logical operator over a ternary.',
},

hasSuggestions: true,
messages,
},
};
13 changes: 10 additions & 3 deletions rules/utils/should-add-parentheses-to-logical-expression-child.js
Expand Up @@ -7,10 +7,17 @@ Check if parentheses should be added to a `node` when it's used as child of `Log
@returns {boolean}
*/
function shouldAddParenthesesToLogicalExpressionChild(node, {operator, property}) {
// When operator or property is different, need check `LogicalExpression` operator precedence, not implemented
// We are not using this, but we can improve this function with it
/* c8 ignore next 3 */
if (operator !== '??' || property !== 'left') {
throw new Error('Not supported.');
if (!property) {
throw new Error('`property` is required.');
}

if (
node.type === 'LogicalExpression'
&& node.operator === operator
) {
return false;
}

// Not really needed, but more readable
Expand Down
45 changes: 45 additions & 0 deletions test/prefer-logical-operator-over-ternary.mjs
@@ -0,0 +1,45 @@
import outdent from 'outdent';
import {getTester} from './utils/test.mjs';

const {test} = getTester(import.meta);

test.snapshot({
valid: [
'foo ? foo1 : bar;',
'foo.bar ? foo.bar1 : foo.baz',
'foo.bar ? foo1.bar : foo.baz',
'++foo ? ++foo : bar;',

// Not checking
'!!bar ? foo : bar;',
],
invalid: [
'foo ? foo : bar;',
'foo.bar ? foo.bar : foo.baz',
'foo?.bar ? foo.bar : baz',
'!bar ? foo : bar;',
'!!bar ? foo : !bar;',

'foo() ? foo() : bar',

// Children parentheses
'foo ? foo : a && b',
'foo ? foo : a || b',
'foo ? foo : a ?? b',
'a && b ? a && b : bar',
'a || b ? a || b : bar',
'a ?? b ? a ?? b : bar',
'foo ? foo : await a',
'await a ? await a : foo',

// ASI
outdent`
const foo = []
!+a ? b : +a
`,
outdent`
const foo = []
a && b ? a && b : 1
`,
],
});