no-else-return doesn't trigger for else if.

[graingert]

Tell us about your environment

• ESLint Version: 4.6.1
• Node Version: 8.4.0
• npm Version: 5.3.0

What parser (default, Babel-ESLint, etc.) are you using? see config
Please show your full configuration: see eslint demo URLs.
What did you do? Please include the actual source code causing the issue.

const res = (() => {
  if (error) {
    return 'It failed';
  } else if (loading) {
    return "It's still loading";
  }

  return result;
})();

What did you expect to happen?

no-else-return should trigger.

What actually happened? Please include the actual, raw output from ESLint.

no-else-return did not trigger.

Demo urls:

doesn't detect error
does detect error

[platinumazure]

I believe this is working as designed. With an else if, there is another code path (both conditionals not met), which results in an implicit return undefined. Additionally, the rule is designed to call out else blocks that could be removed (around return statements) without changing the meaning of the code. Removing else if changes the meaning of the code.

[graingert]

@platinumazure not in the example I gave.

[graingert]

@platinumazure can you give a code example of what you mean and I'll add it to my tests?

[platinumazure]

No, I see the hook in your example. I'm not sure how easy it will be to enhance the rule for your case without potentially breaking other cases, but that's why we have unit tests.

[graingert]

The hook?

[graingert]

I believe this is working as designed. With an else if, there is another code path (both conditionals not met), which results in an implicit return undefined. Additionally, the rule is designed to call out else blocks that could be removed (around return statements) without changing the meaning of the code. Removing else if changes the meaning of the code.

I'm pretty sure that's not true. If you transform the code to explicit block statements no-else-return detects it correctly

[platinumazure]

Sorry- I see what I had missed. I see why you are asking if the rule should be enhanced.

I'm worried about what rabbit hole we might be opening up if we expand these cases too much.

I guess in general, we could conceivably flag all else if statements with a single return statement, but maybe only working our way from the bottom up?

if (foo) return true;
else if (bar) { return true; }   // can't be flagged due to next line?
else if (baz) { doSomething(); return true; }
else if (x) return true;  // could be flagged?
else if (y) { return true; } // could be flagged?

I think the team might just decide that the rule is only designed to handle the specific case of else followed by return.

[platinumazure]

I'm pretty sure that's not true. If you transform the code to explicit block statements no-else-return detects it correctly

So it does. I stand corrected.

I think we can accept this as a bug. Please feel free to continue work on your PR and thanks for your patience!

[graingert]

@platinumazure fyi, with explicit block statements eslint flags them all.

[graingert]

Without else:

() => {
  if (foo) {
    return true;
  }

  if (bar) {
    return true;
  }

  if (baz) {
    doSomething();
    return true;
  }

  if (x) {
    return true;
  }

  if (y) {
    return true;
  }
}

And here it is without an extra return Which cases eslint not to flag it.

[graingert]

@platinumazure is there a way to run only one test? Currently it takes too long to drop to a debugger on one rule test.

[platinumazure]

@graingert Use Mocha's --grep flag to dig down to a specific test (and make sure you run Mocha against just the one test file, if you aren't already).

[graingert]

@platinumazure how do I get it to skip running eslint on itself?

[platinumazure]

Just run Mocha directly:

./node-modules/mocha/bin/_mocha tests/lib/rules/no-else-return.js --grep "Name of test"

[graingert]

@platinumazure thanks, that was super handy.

[graingert]

@platinumazure I've taken my PR out of WIP and it's ready for review.