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 no-identical-alternatives-in-ternary rule #2430

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions docs/rules/no-identical-alternatives-in-ternary.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Disallow nested ternary expressions with repeated alternatives

💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/sindresorhus/eslint-plugin-unicorn#preset-configs-eslintconfigjs).

🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).

<!-- end auto-generated rule header -->
<!-- Do not manually modify this header. Run: `npm run fix:eslint-docs` -->

Disallow nested ternary expressions with repeated alternatives, and simplify to a more readable format with logical operators.

## Examples

```js
a ? b ? c : 1 : 1; //
a && b ? c : 1; //
```

```js
a ? b ? c : { foo } : { foo }; //
a && b ? c : { foo }; //
```

```js
a ? b ? c : sameReference : sameReference; //
a && b ? c : sameReference; //
```

```js
a ? b ? c : foo.bar() : foo.bar(); //
a && b ? c : foo.bar(); //
```
1 change: 1 addition & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ If you don't use the preset, ensure you use the same `env` and `parserOptions` c
| [no-empty-file](docs/rules/no-empty-file.md) | Disallow empty files. || | |
| [no-for-loop](docs/rules/no-for-loop.md) | Do not use a `for` loop that can be replaced with a `for-of` loop. || 🔧 | 💡 |
| [no-hex-escape](docs/rules/no-hex-escape.md) | Enforce the use of Unicode escapes instead of hexadecimal escapes. || 🔧 | |
| [no-identical-alternatives-in-ternary](docs/rules/no-identical-alternatives-in-ternary.md) | Disallow nested ternary expressions with repeated alternatives. || 🔧 | |
| [no-instanceof-array](docs/rules/no-instanceof-array.md) | Require `Array.isArray()` instead of `instanceof Array`. || 🔧 | |
| [no-invalid-fetch-options](docs/rules/no-invalid-fetch-options.md) | Disallow invalid options in `fetch()` and `new Request()`. || | |
| [no-invalid-remove-event-listener](docs/rules/no-invalid-remove-event-listener.md) | Prevent calling `EventTarget#removeEventListener()` with the result of an expression. || | |
Expand Down
57 changes: 57 additions & 0 deletions rules/no-identical-alternatives-in-ternary.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
'use strict';

const {isNodesEqual, findIndexOfQuestionMarkInConditionalExpression, getParenthesizedRange} = require('./utils/index.js');

const MESSAGE_ID = 'no-identical-alternatives-in-ternary';
const messages = {
[MESSAGE_ID]:
'Replace repeated alternatives of ternary expressions with a logical expression.',
};

/** @param {import('eslint').Rule.RuleContext} context */
const create = context => ({
/** @param {import('estree').ConditionalExpression} node */
ConditionalExpression(node) {
// Check if the alternate is a ConditionalExpression
if (node.alternate && node.consequent.type === 'ConditionalExpression') {
Copy link
Collaborator

@fisker fisker Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should go deeper,

a ? (b ? 1 : 2) : (c ? 1 : 3);
a ? (b ? 1 : 2) : (c ? 3 : 1)

should also be reported. I'm not sure about the equivalent... but there must be a better way to write?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is too complicated, I don't think it can be done easily.

Copy link
Contributor Author

@axetroy axetroy Aug 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should ignore this situation, it is almost impossible to happen in the real world.

Or create another rule that names no-complex-ternary to disallow it.

const {test: _outerTest, consequent: outerConsequent, alternate: outerAlternate} = node;
const {test: _innerTest, consequent: _innerConsequent, alternate: innerAlternate} = node.consequent;

const {sourceCode} = context;

if (isNodesEqual(outerAlternate, innerAlternate)) {
context.report({
node,
messageId: MESSAGE_ID,
* fix(fixer) {
// Find the index of the question mark in the outer ConditionalExpression
const questionMarkIndex = findIndexOfQuestionMarkInConditionalExpression(node, sourceCode);

// Replace the ? with &&
yield fixer.replaceTextRange([questionMarkIndex, questionMarkIndex + 1], '&&');

// Remove the repeated alternative
const [alternativeStart] = getParenthesizedRange(innerAlternate, sourceCode);
const alternativeEnd = getParenthesizedRange(outerConsequent, sourceCode)[1];

yield fixer.replaceTextRange([alternativeStart, node.range[1]], sourceCode.getText().slice(alternativeStart, alternativeEnd));
},
});
}
}
},
});

/** @type {import('eslint').Rule.RuleModule} */
module.exports = {
create,
meta: {
type: 'problem',
docs: {
description: 'Disallow nested ternary expressions with repeated alternatives.',
recommended: true,
},
fixable: 'code',
messages,
},
};
36 changes: 36 additions & 0 deletions rules/utils/conditional-expression.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
'use strict';

const {getParenthesizedRange} = require('./parentheses.js');

/**
Find the index of the question mark in a ConditionalExpression node

@param {import('estree').ConditionalExpression} node
@param {import('eslint').SourceCode} sourceCode
@returns {number}
*/
function findIndexOfQuestionMarkInConditionalExpression(node, sourceCode) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sourceCode.getTokenAfter(node.test, () => ...isQuestionMarkToken)

should enough

const testAfterComments = sourceCode.getCommentsAfter(node.test);
const consequentBeforeComments = sourceCode.getCommentsBefore(node.consequent);

let start = getParenthesizedRange(node.test, sourceCode)[1];
let end = getParenthesizedRange(node.consequent, sourceCode)[0];

if (testAfterComments.length > 0) {
const lastComment = testAfterComments.at(-1);

start = lastComment.range[1];
}

if (consequentBeforeComments.length > 0) {
const firstComment = consequentBeforeComments.at(0);

end = firstComment.range[0];
}

return start + sourceCode.getText().slice(start, end).indexOf('?');
}

module.exports = {
findIndexOfQuestionMarkInConditionalExpression,
};
3 changes: 3 additions & 0 deletions rules/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ const {
} = require('./array-or-object-prototype-property.js');
const {isNodeMatches, isNodeMatchesNameOrPath} = require('./is-node-matches.js');
const {isBooleanNode, getBooleanAncestor} = require('./boolean.js');
const {findIndexOfQuestionMarkInConditionalExpression} = require('./conditional-expression.js');

module.exports = {
avoidCapture: require('./avoid-capture.js'),
Expand Down Expand Up @@ -43,6 +44,7 @@ module.exports = {
isParenthesized,
isSameIdentifier: require('./is-same-identifier.js'),
isSameReference: require('./is-same-reference.js'),
isNodesEqual: require('./is-node-equals.js'),
isShadowed: require('./is-shadowed.js'),
isValueNotUsable: require('./is-value-not-usable.js'),
needsSemicolon: require('./needs-semicolon.js'),
Expand All @@ -52,5 +54,6 @@ module.exports = {
singular: require('./singular.js'),
toLocation: require('./to-location.js'),
getAncestor: require('./get-ancestor.js'),
findIndexOfQuestionMarkInConditionalExpression,
};

56 changes: 56 additions & 0 deletions rules/utils/is-node-equals.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
'use strict';

const ignoreKeys = new Set(['loc', 'start', 'end', 'parent', 'range']);

/**
Compare two AST nodes for equality by structure and value.
Different from `deepStrictEqual` in that it ignores `loc`, `start`, `end`, `parent`, and `range` properties.
Different from `isSameReference` in that `isSameReference` just checks if the nodes are the same reference, but cannot compare two identical data.
eg.:
```js
const node1 = { foo: bar }
const node2 = { foo: bar }
```
isNodesEqual(node1, node2) => true
isSameReference(node1, node2) => false
@param {import('estree').Node} node1 - The first AST node.
@param {import('estree').Node} node2 - The second AST node.
@returns {boolean} - True if the nodes are structurally and value-wise equal, false otherwise.
*/
function isNodesEqual(node1, node2) {
if (node1 === node2) {
return true;
}

if (typeof node1 === 'string' || typeof node1 === 'boolean' || typeof node1 === 'number') {
return node1 === node2;
}

// If one of them is null or undefined, they are not equal
if (!node1 || !node2) {
return false;
}

// If they are of different types, they are not equal
if (node1.type !== node2.type) {
return false;
}

if (node1.type === 'Literal') {
return node1.value === node2.value;
}

// Compare properties recursively
for (const key in node1) {
if (Object.hasOwn(node1, key) && !ignoreKeys.has(key) && !isNodesEqual(node1[key], node2[key])) {
return false;
}
}

return true;
}

module.exports = isNodesEqual;
75 changes: 75 additions & 0 deletions test/no-identical-alternatives-in-ternary.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import {outdent} from 'outdent';
import {getTester} from './utils/test.mjs';

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

test.snapshot({
valid: [
'a ? b ? c : 1 : 2',
'a && b ? c : 1',
],
invalid: [
'a?b?c:1:1',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a? b?1:c :1
a? 1: b?c:1
a? 1: b?1:c

Should be invalid.

Copy link
Contributor Author

@axetroy axetroy Sep 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a ? b ? (1: c :1) fix to (a && b) ? 1 : c

a ? 1: (b ? c : 1) fix to (a || !b) ? 1 : c

a ? 1 : (b ? 1 : c) fix to (a || b) ? 1 : c

This implementation is too complicated

I don't think this is easy to do. It cannot retain original comments.

Rather than implementing such a complex rule, I prefer closing this PR

'a ? b ? c : 1 : 1',
'a ? b ? c : "str" : "str"',
'a ? b ? c : sameReference : sameReference',
'a ? b ? c : { foo: 1 } : { foo: 1 }',
outdent`
const foo = { bar: 1 };
a ? b ? c : foo.bar : foo.bar
`,
// With comments
'/** comment before a */ a /** comment after a */ ? b ? c : 1 : 1',
outdent`
/** comment before a */
a ?
/** comment after a */ b ? c : 1 : 1
`,
outdent`
/** comment before a */
a ?
/** comment after a */ b /** comment after b */ ? c /** comment after c */ : 1 : 1 /** comment after value */
`,
'/** comment includes ? */ a /** comment includes ? */ ? b ? c : 1 : 1',

// Don't use outdent for the following cases, outdent will causes eslint parsing errors
`
a ?
b ?
c :
1 :
1
`,
`
a ? // comment a
b ? // comment b
c : // comment c
1 : // comment 1
1 // comment repeat 1
`,
`
/** comment before a */ a ? // comment a
/** comment before b */ b ? // comment b
/** comment before c */ c : // comment c
/** comment before 1 */ 1 : // comment 1
/** comment before repeat 1 */ 1 // comment repeat 1
`,

// Edge cases
'(a ? b ? c : 1 : 1)',
'a ? b ? c : (1, 2) : (1, 2)',
'a ? (b ? c : 1) : 1',
'(a ? (b ? c : 1) : 1)',
'a ? b ? c : a === 100 : a === 100',
outdent`
async function foo() {
return a ? b ? c : await 1 : await 1;
}
`,
outdent`
function* foo() {
return a ? b ? c : yield 1 : yield 1;
}
`,
],
});
1 change: 1 addition & 0 deletions test/package.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const RULES_WITHOUT_PASS_FAIL_SECTIONS = new Set([
'prefer-modern-math-apis',
'prefer-math-min-max',
'consistent-existence-index-check',
'no-identical-alternatives-in-ternary',
]);

test('Every rule is defined in index file in alphabetical order', t => {
Expand Down
Loading