Fixes #7863; Add fixer for no-else-return #7864

xdumaine · 2017-01-05T18:35:54Z

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[x] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Added a fixer for the no-else-return rule, updated documentation, and updated tests.

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

mention-bot · 2017-01-05T18:35:55Z

@xdumaine, thanks for your PR! By analyzing the history of the files in this pull request, we identified @not-an-aardvark, @elwayman02 and @jrvidal to be potential reviewers.

jsf-clabot · 2017-01-05T18:35:55Z

All committers have signed the CLA.

eslintbot · 2017-01-05T18:35:55Z

Thanks for the pull request, @xdumaine! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

not-an-aardvark

Thanks for the pull request. I think adding autofix support for this rule is a good idea, but there are a few places where this implementation will accidentally change the behavior of the code.

not-an-aardvark · 2017-01-06T00:12:53Z

lib/rules/no-else-return.js

+                    let fixedSource;
+
+                    if (startToken.type === "Punctuator") {
+                        fixedSource = source.substr(1, source.length - 2);


This will cause an error if the else statement doesn't have a block:

if (foo) bar() else return baz // gets fixed to if (foo) bar() return ba

if (foo) bar() else return // gets fixed to if (foo) bar() retur

Also, it will cause an error if there are more statements on the same line as the else block.

if (foo) { bar() } else { return baz } qux() // gets fixed to: if (foo) { bar() } return baz qux()

@not-an-aardvark I'll check out your third example, but are you sure the first two are true? The tests have these examples in them, and pass, without making those corrections. They're handled by the check on L48 that the start token is a punctuator.

By the way, thanks for the quick responses and feedback. I'll give this another go and update. :)

Ok, took a longer look at these:

Your first example:

if (foo) bar() else return baz

Is not an error according to the rule, so this one won't get fixed at all. You can't remove the else in this case, as it changes the meaning of the code.

Your second example is the same. It's not an error according to the rule because the else is not extra, it's necessary to the meaning of the code.

However, if you change both of these examples to replace bar() with return bar() that would make them error states. In doing that, your assertion about how it gets fixed is incorrect. The else case in my code (L51) handles it, and it fixes the code correctly.

Your third example is also not applicable unless you change bar() to return bar(). If that's the case you end up with this:

if (foo) { return bar() } else { return baz } qux()

Where qux() is unreachable code. So this would cause an unexpected token error if 1. you have unreachable code and 2. don't use semicolons.

Looks like you're right about these, my mistake. I missed the Punctuator check.

However, I noticed a few other errors (and double-checked them with the fixer this time):

At the moment the Punctuator check is too loose -- it will treat any Punctuator node as if it was a block, even if it's not an opening curly. As a result, the following code causes a syntax error when fixed:

function foo() { if (foo) return bar; else (foo).bar(); } // gets fixed to: function foo() { if (foo) return bar; foo).bar() }

To fix this, I would recommend modifying the check to

if (startToken.type === "Punctuator" && startToken.value === "{")

Also, this will cause ASI issues if the statement in the if statement does not end in a semicolon. For example, the following code will be fixed incorrectly:

function foo() { if (foo) return bar else { [1, 2, 3].map(foo) } } // gets fixed to function foo() { if (foo) return bar [1, 2, 3].map(foo) // this is parsed as a computed property access rather than an array }

To avoid this problem, I would recommend that the fixer avoid creating a fix when all of the following are true:

The if statement does not have a block

The body of the if statement does not end in a semicolon

The first character of the else statement is an unsafe character for starting a line ((, [, /, `, +, or -)

A similar check can be found in the no-lonely-if fixer.

In the third example from the previous comment, I think we should still use a workaround to avoid causing a syntax error, even if it would only occur when the user has very strange code.

function foo() { if (foo) return bar else { baz } qux } // gets fixed to function foo() { if (foo) return bar baz qux }

Similarly to the example above, this will also cause an ASI issue if the statement after the else starts with a character like [:

function foo() { if (foo) return bar else { baz() } [1, 2, 3].map(foo) } // gets fixed to function foo() { if (foo) return bar baz() [1, 2, 3].map(foo) // this gets parsed as a computed property access rather than an array }

To avoid this problem, I would recommend that the fixer avoid creating a fix when all of the following are true:

The last statement in the else block does not end in a semicolon

There is a statement after the else block, and it either (a) starts with an unsafe character ((, [, /, `, +, or -), or (b) is on the same line as the last statement in the else block.

As an aside, hopefully it doesn't seem like I'm coming up with exceedingly unlikely code examples that no one would actually use. In general, we try to uphold the guarantee that an autofixer should never change the behavior of working code under any circumstances, so sometimes there are unfortunately a lot of edge cases for fixers like this (particularly when a fix involves removing a block statement).

I definitely appreciate the feedback. This type of semantic parsing type of code, and code modification is new to me, and I'm not familiar with all the ins and outs so I'm grateful for your detailed feedback. I'll give it another go later and update. Thanks again for your time!

eslintbot · 2017-01-17T22:28:21Z

Thanks for the pull request, @xdumaine! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2017-01-17T22:35:08Z

Thanks for the pull request, @xdumaine! I took a look to make sure it's ready for merging and found some changes are needed:

The commit summary needs to begin with a tag (such as Fix: or Update:). Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

eslintbot · 2017-01-17T22:39:53Z

LGTM

not-an-aardvark

This mostly looks good, I just have a few more suggestions.

not-an-aardvark · 2017-01-19T17:37:17Z

lib/rules/no-else-return.js

+                        if (lastIfToken.value !== "}" &&
+                                lastIfToken.value !== ";" &&
+                                /^[([/+`-]/.test(firstTokenOfElseBlock.value)) {
+                            return null;


It might be helpful to future contributors to add a comment describing the case that this logic is trying to catch.

not-an-aardvark · 2017-01-19T17:41:19Z

lib/rules/no-else-return.js

+                        if (lastTokenOfElseBlock.value !== ";") {
+                            const nextToken = sourceCode.getTokenAfter(endToken);
+
+                            if (/^[([/+`-]/.test(nextToken.value) ||


This will throw an error if nextToken doesn't exist.

{ code: "if (foo) return bar; else { baz }", parserOptions: { ecmaFeatures: { globalReturn: true } } }

Easy enough to add a check, but Is that actually possible? Since return must always be inside a function, so there will always be at least one more token: } closing the function

@xdumaine The globalReturn option on espree begs to differ.

not-an-aardvark · 2017-01-19T17:44:17Z

lib/rules/no-else-return.js

+                message: "Unnecessary 'else' after 'return'.",
+                fix: fixer => {
+                    const sourceCode = context.getSourceCode();
+                    const startToken = sourceCode.getTokenByRangeStart(node.start);


It might be clearer to use sourceCode.getFirstToken(node) here instead.

not-an-aardvark · 2017-01-19T17:50:44Z

lib/rules/no-else-return.js

+                    if (startToken.type === "Punctuator" && startToken.value === "{") {
+                        const firstTokenOfElseBlock = sourceCode.getTokenAfter(startToken);
+
+                        if (lastIfToken.value !== "}" &&


This check will work incorrectly here:

if (foo) return function() {} // note: the last token is '}' else [1, 2, 3].map(bar) // gets fixed to if (foo) return function() {} [1, 2, 3].map(bar) // this is parsed as a property access instead of an array

To fix this, I would replace the lastIfToken.value !== "}" check with

node.parent.consequent.type !== "BlockStatement"

Hmm makes sense, but node.parent is not defined here - could you recommend a better way to do that check?

Good point -- two potential workarounds I can think of:

In this call to displayReport, pass node instead of alternate (and then access the property accordingly within the displayReport function to make sure everything else still works). That way, node will refer to the IfStatement, so you could check node.consequent.type.

Change this listener to "IfStatement:exit" instead of IfStatement.

Explanation: The parent property is added to each node as ESLint enters it while traversing. Since this listener is fired when the IfStatement node is entered, the parent property hasn't been added to the else node yet. If the listener is fired when the IfStatement node is exited instead, the else node will have its parent property.

not-an-aardvark · 2017-01-19T17:51:40Z

lib/rules/no-else-return.js

+                            }
+                        }
+
+                        fixedSource = source.substr(1, source.length - 2);


Nitpick: Can you replace this with source.slice(1, -1)?

not-an-aardvark · 2017-01-19T18:08:13Z

tests/lib/rules/no-else-return.js

+        {
+            code: "function foo() { if (foo) return bar \nelse { baz() } \n[1, 2, 3].map(foo) }",
+            output: "function foo() { if (foo) return bar \nelse { baz() } \n[1, 2, 3].map(foo) }",
+            errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]


Can you add test cases like the two above, but with a semicolon?

"function foo() { if (foo) return bar; \nelse { [1, 2, 3].map(foo) } }" "function foo() { if (foo) return bar \nelse { baz(); } \n[1, 2, 3].map(foo) }"

Both of these cases should result in a fix being applied.

not-an-aardvark · 2017-01-19T18:10:52Z

tests/lib/rules/no-else-return.js

+        },
+        {
+            code: "function foo() { if (foo) return bar \nelse { baz() } \nqaz() }",
+            output: "function foo() { if (foo) return bar \n baz()  \nqaz() }",
            errors: [{ message: "Unnecessary 'else' after 'return'.", type: "BlockStatement" }]
        }


Can you add a test case where a semicolon is missing and the following statement is on the same line?

"function foo() { if (foo) return bar; else { baz } qux }"

This should cause the fix to get skipped.

eslintbot · 2017-01-19T22:58:55Z

LGTM

eslintbot · 2017-01-19T23:05:45Z

LGTM

not-an-aardvark

LGTM. Thanks for contributing!

I have two final nitpicks, but they're very minor, so they shouldn't block this from being merged.

not-an-aardvark · 2017-01-20T04:33:15Z

lib/rules/no-else-return.js

+                    const sourceCode = context.getSourceCode();
+                    const alternateStartToken = sourceCode.getFirstToken(node.parent.alternate);
+                    const elseToken = sourceCode.getTokenBefore(alternateStartToken);
+                    const source = sourceCode.getText(node.parent.alternate);


This line (and the alternateStartToken line) is a bit redundant because node.parent.alternate === node.

Good call. Confused myself when trying out returning the original node instead of the alternate. Fixed and simplified.

not-an-aardvark · 2017-01-20T04:34:09Z

lib/rules/no-else-return.js

+                        const nextTokenUnsafe = nextToken && /^[([/+`-]/.test(nextToken.value);
+                        const nextTokenOnSameLine = nextToken && nextToken.loc.start.line === lastTokenOfElseBlock.loc.start.line;
+
+                        if (nextTokenUnsafe || (nextTokenOnSameLine && nextToken.value !== "}")) {


A comment here would also be useful.

Comment added.

eslintbot · 2017-01-20T14:07:24Z

LGTM

not-an-aardvark · 2017-01-20T15:45:24Z

Note to whoever ends up merging this: make sure to use the Update: commit prefix since it's a semver-minor change.

not-an-aardvark suggested changes Jan 6, 2017

View reviewed changes

ilyavolodin added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules labels Jan 6, 2017

xdumaine force-pushed the issue7863 branch from 12912dc to 7c08544 Compare January 17, 2017 22:28

xdumaine force-pushed the issue7863 branch from 7c08544 to 019b712 Compare January 17, 2017 22:35

xdumaine force-pushed the issue7863 branch from 019b712 to f4f8ac4 Compare January 17, 2017 22:39

not-an-aardvark suggested changes Jan 19, 2017

View reviewed changes

xdumaine force-pushed the issue7863 branch from f4f8ac4 to cb6fb13 Compare January 19, 2017 22:58

xdumaine force-pushed the issue7863 branch from cb6fb13 to 4be98de Compare January 19, 2017 23:05

not-an-aardvark approved these changes Jan 20, 2017

View reviewed changes

Fix: Add fixer for no-else-return (fixes eslint#7863)

ac25c41

xdumaine force-pushed the issue7863 branch from 4be98de to ac25c41 Compare January 20, 2017 14:07

not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jan 20, 2017

gyandeeps merged commit 243e47d into eslint:master Jan 20, 2017

This was referenced Jan 20, 2017

Update dependency eslint to version 3.14.1 singapore/lint-condo#218

Merged

Update dependency eslint to version 3.14.0 renovatebot/renovate#63

Merged

eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #7863; Add fixer for no-else-return #7864

Fixes #7863; Add fixer for no-else-return #7864

xdumaine commented Jan 5, 2017

mention-bot commented Jan 5, 2017

jsf-clabot commented Jan 5, 2017 •

edited

eslintbot commented Jan 5, 2017

not-an-aardvark left a comment

not-an-aardvark Jan 6, 2017 •

edited

xdumaine Jan 6, 2017

xdumaine Jan 6, 2017

xdumaine Jan 6, 2017 •

edited

not-an-aardvark Jan 6, 2017

xdumaine Jan 6, 2017

eslintbot commented Jan 17, 2017

eslintbot commented Jan 17, 2017

eslintbot commented Jan 17, 2017

not-an-aardvark left a comment

not-an-aardvark Jan 19, 2017

not-an-aardvark Jan 19, 2017 •

edited

xdumaine Jan 19, 2017

platinumazure Jan 19, 2017

not-an-aardvark Jan 19, 2017

not-an-aardvark Jan 19, 2017

xdumaine Jan 19, 2017

not-an-aardvark Jan 19, 2017 •

edited

not-an-aardvark Jan 19, 2017

not-an-aardvark Jan 19, 2017

not-an-aardvark Jan 19, 2017

eslintbot commented Jan 19, 2017

eslintbot commented Jan 19, 2017

not-an-aardvark left a comment

not-an-aardvark Jan 20, 2017

xdumaine Jan 20, 2017

not-an-aardvark Jan 20, 2017

xdumaine Jan 20, 2017

eslintbot commented Jan 20, 2017

not-an-aardvark commented Jan 20, 2017

Fixes #7863; Add fixer for no-else-return #7864

Fixes #7863; Add fixer for no-else-return #7864

Conversation

xdumaine commented Jan 5, 2017

mention-bot commented Jan 5, 2017

jsf-clabot commented Jan 5, 2017 • edited

eslintbot commented Jan 5, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark Jan 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xdumaine Jan 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jan 17, 2017

eslintbot commented Jan 17, 2017

eslintbot commented Jan 17, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark Jan 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

not-an-aardvark Jan 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jan 19, 2017

eslintbot commented Jan 19, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jan 20, 2017

not-an-aardvark commented Jan 20, 2017

jsf-clabot commented Jan 5, 2017 •

edited

not-an-aardvark Jan 6, 2017 •

edited

xdumaine Jan 6, 2017 •

edited

not-an-aardvark Jan 19, 2017 •

edited

not-an-aardvark Jan 19, 2017 •

edited