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

New: Add no-dupe-else-if rule (fixes #12469) #12504

Merged
merged 4 commits into from Nov 18, 2019
Merged

New: Add no-dupe-else-if rule (fixes #12469) #12504

merged 4 commits into from Nov 18, 2019

Conversation

@mdjermanovic
Copy link
Member

mdjermanovic commented Oct 29, 2019

What is the purpose of this pull request? (put an "X" next to item)

[X] New rule #12469

Examples of incorrect code for this rule:

/*eslint no-dupe-else-if: "error"*/

if (isSomething(x)) {
    foo();
} else if (isSomething(x)) {
    bar();
}

if (a) {
    foo();
} else if (b) {
    bar();
} else if (c && d) {
    baz();
} else if (c && d) {
    quux();
} else {
    quuux();
}

if (n === 1) {
    foo();
} else if (n === 2) {
    bar();
} else if (n === 3) {
    baz();
} else if (n === 2) {
    quux();
} else if (n === 5) {
    quuux();
}

What changes did you make? (Give an overview)

New rule no-dupe-else-if

Is there anything you'd like reviewers to focus on?

@eslint eslint bot added the triage label Oct 29, 2019
Copy link
Member

mysticatea left a comment

Looks very good to me, but I have a question around || expression.

while (current.parent && current.parent.type === "IfStatement" && current.parent.alternate === current) {
current = current.parent;

if (astUtils.equalTokens(node.test, current.test, sourceCode)) {

This comment has been minimized.

Copy link
@mysticatea

mysticatea Oct 29, 2019

Member

I'm guessing that to check each condition of the || expression individually is useful if the test nodes are LogicalExpression with || operator. Thoughts?

For example:

if (a || b) {
    // ...
} else if (a) { // duplicated condition
    // ...
}

I'm imaging code like:

                    const nodeConds = getOrConditions(node.test)
                    const currentConds = getOrConditions(current.test)
                    if (nodeConds.every(a => currentConds.some(b => astUtils.equalTokens(a, b, sourceCode)))) {

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Oct 29, 2019

Author Member

I think it's a very good idea.

We could even compare with the union of || conditions from all parents in the chain?

For example:

if (a) {
    // ...
} else if (b) {
    // ...
} else if (a || b) { // duplicated condition: { a, b } is a subset of { a, b }
    // ...
}

That would also catch things like this:

if (a || b) {
    // ...
} else if (c || d) {
    // ...
} else if (a || d) { // duplicated condition: { a, d } is a subset of { a, b, c, d }
    // ...
}

This comment has been minimized.

Copy link
@mysticatea

This comment has been minimized.

Copy link
@platinumazure

platinumazure Oct 29, 2019

Member

Is this really necessary? This seems like it would add some complexity and go beyond the original proposal.

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Oct 29, 2019

Author Member

Looks like there are 3 options at the moment:

  1. To check for equal ancestors only, as originally proposed in #12469
  2. To check for a subset of an ancestor as in this comment
  3. To check for a subset of the union of all ancestors as in this comment

I'm fine with all three 👍

Options 2. and 3. look very nice, but only 1. is accepted, so I don't how to proceed 😕

It doesn't look too complicated to do 2. or 3. Though, compared to the simple 1. it would be certainly much more code. Not sure how to weight how useful these enhancements would be.

This comment has been minimized.

Copy link
@platinumazure

platinumazure Nov 4, 2019

Member

I don't have any complexity concerns anymore based on the implementation shared earlier.

I don't understand why this only applies to || operators only, though. Shouldn't we look at && as well?

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Nov 4, 2019

Member

I like this addition! Agreed with @platinumazure. It does seem like it could be useful to also check for and warn on:

if (a && b) {} 
else if (b && a) {}

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Nov 4, 2019

Author Member

👍 to account for && as well.

The logic for && in a chain is a bit different, so option 3 wouldn't apply well. For instance, the following is valid code:

if (a && b) {
    // ...
} else if (a) {
    // ...
} else if (b) {
    // ... 
}

if (a && b) {
    // ...
} else if (b && c) {
    // ...
} else if (a && c) {
    // ...
}

On the other hand, 'inverted' option 2 should work quite well for && and is easy to implement. If the condition split by && is a superset of one of the previous conditions, then it's a 'duplicate'. For example, all these are invalid:

if (a) {
    // ...
} else if (a && b) {
    // ...
}

if (a && b) {
    // ...
} else if (b && a) {
    // ...
}

if (a && b) {
    // ...
} else if (a && b && c) {
    // ...
}

In addition, it also looks easy to split the original condition by && and do the || check with each element of the resulting array, which would catch cases that might not even look like a bug at first glance:

if (a || b) {
    // ...
} else if (b && c) { // perhaps a not-so-obvious bug
    // ...  
}

if (a) {
    // ...
} else if (b) {
    // ...
} else if ((a || b) && c) {
   // ...
}

That shouldn't even add new lines, just .map and .some to the existing lines in this draft

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Nov 5, 2019

Author Member

This is done now, the rule should be able to catch some relatively complex errors ('more complex errors with || and &&' section in invalid tests).

Examples from the documentation:

/*eslint no-dupe-else-if: "error"*/

if (a || b) {
    foo();
} else if (a) {
    bar();
}

if (a) {
    foo();
} else if (b) {
    bar();
} else if (a || b) {
    baz();
}

if (a) {
    foo();
} else if (a && b) {
    bar();
}

if (a && b) {
    foo();
} else if (a && b && c) {
    bar();
}

if (a || b) {
    foo();
} else if (b && c) {
    bar();
}

if (a) {
    foo();
} else if (b && c) {
    bar();
} else if (d && (c && e && b || a)) {
    baz();
}

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Nov 5, 2019

Author Member

Tried the rule on ESLint codebase, reports only 1 bug at indent-legacy.js#L779 and it's indeed a bug because of !parentVarNode which is used in the previous condition with ||.

Copy link
Member

aladdin-add left a comment

LGTM, but a small suggestion, thanks!

errors: [{ messageId: "unexpected", type: "LogicalExpression", column: 35 }]
},
{
code: "if (a) {} else if (a || a) {}",

This comment has been minimized.

Copy link
@aladdin-add

aladdin-add Nov 12, 2019

Member

unrelated: is there a rule to warn something like: a || a; -- it seems a common mistake.

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Nov 12, 2019

Author Member

I don't think so. There was an idea to check this in #12097 (comment)

This comment has been minimized.

Copy link
@mdjermanovic

mdjermanovic Nov 13, 2019

Author Member

Off-topic, maybe we should reopen #12097 or open a new issue.

lib/rules/no-dupe-else-if.js Outdated Show resolved Hide resolved
lib/rules/no-dupe-else-if.js Outdated Show resolved Hide resolved
@mysticatea mysticatea self-requested a review Nov 13, 2019
Copy link
Member

mysticatea left a comment

LGTM, thank you so much!

Copy link
Member

kaicataldo left a comment

One small comment, but otherwise this LGTM!

lib/rules/no-dupe-else-if.js Outdated Show resolved Hide resolved
Co-Authored-By: Kai Cataldo <kaicataldo@gmail.com>
Copy link
Member

kaicataldo left a comment

LGTM, thank you!

@kaicataldo kaicataldo merged commit 17a8849 into master Nov 18, 2019
18 checks passed
18 checks passed
Verify Files
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 8.10.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191118.2 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@kaicataldo kaicataldo deleted the no-dupe-else-if branch Nov 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.