Update: add fixer for `arrow-body-style` #7240

not-an-aardvark · 2016-09-25T18:19:12Z

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:

Please check each item to ensure your pull request is ready:

I've read the pull request guide
I've included tests for my change
I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)

This adds a fixer for arrow-body-style.

/* eslint arrow-body-style: [2, "as-needed"] */

var foo = () => { return 5; };
var bar = () => { return {ok: true}; };

// gets fixed to:

var foo = () =>   5 ;
var bar = () =>   ({ok: true}) ;

/* eslint arrow-body-style: [2, "always"] */

var foo = () => 5;
var bar = () => ({ok: true});

// gets fixed to:

var foo = () => {return 5};
var bar = () => {return ({ok: true})};

/* eslint arrow-body-style: [2, "never"] */

var foo = () => { bar(); return baz(); }; // not fixed

The fixer preserves all comments and whitespace. On the next pass, the fixers for brace-style, indent, no-multi-spaces, no-extra-parens, semi, and keyword-spacing will be able to clean up stylistic issues from the outputted code.

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

We should make sure there are no cases where replacing a block body with a literal (or vice versa) causes a SyntaxError.

~~The build will fail at the moment due to #7239. I'll rebase onto master after that's fixed.~~

mention-bot · 2016-09-25T18:19:14Z

@not-an-aardvark, thanks for your PR! By analyzing the annotation information on this pull request, we identified @ajhyndman, @alberto and @vitorbal to be potential reviewers

eslintbot · 2016-09-25T18:19:14Z

LGTM

not-an-aardvark · 2016-09-30T02:04:17Z

I'll champion this.

nzakas · 2016-09-30T18:10:16Z

@not-an-aardvark championing also means you need to drum up support for this amongst the team, gathering the necessary 👍s. (Just don't want this to sit around ignored)

mysticatea · 2016-10-01T20:35:12Z

We have received many questions "how to fix this arrow-body-style error?".
Also, #5498 still has top traffic in all issues.

So I think the autofix of arrow-body-style would help users great.

mysticatea · 2016-10-01T20:44:47Z

lib/rules/arrow-body-style.js

+                }
+
+                if (asNeeded && requireReturnForObjectLiteral && blockBody[0].type === "ReturnStatement" &&
+                    blockBody[0].argument.type === "ObjectExpression") {


ReturnStatement#argument property needs null check.
Chould you add tests like a => { return }?

mysticatea · 2016-10-01T20:48:39Z

lib/rules/arrow-body-style.js

+                            const sourceText = sourceCode.getText();
+                            const textBeforeReturn = sourceText.slice(arrowBody.range[0] + 1, blockBody[0].range[0]);
+                            const textBetweenReturnAndValue = sourceText.slice(sourceCode.getFirstToken(blockBody[0]).range[1], returnValue.range[0]);
+                            const returnValueText = returnValue.type === "ObjectExpression" ? `(${sourceCode.getText(returnValue)})` : sourceCode.getText(returnValue);


This can cause a syntax error after autofix. It needs parentheses if the first token of returnValue is {.

key => { return {a: 1, b: 2}[key] }

Also, it seems wrong if the returnValue is parenthesized.

() => { return ({a: 1, b: 2}) } // ↓ fixed () => (({a: 1, b: 2})

mysticatea · 2016-10-01T21:03:06Z

lib/rules/arrow-body-style.js

+
+                            return fixer.replaceTextRange(
+                                [firstBodyToken.range[0], node.range[1]],
+                                `{return ${sourceCode.getText().slice(firstBodyToken.range[0], node.range[1])}}`


In my ideal, it removes parentheses which are enclosing the body if the first token of the body is { because it's restricted by syntax.

I thought of doing that, but that could cause comments to get removed:

var foo = () => ( /* comment */ {ok: true}); // gets fixed to: var foo = () => {return ( /* comment */ {ok: true});};

I think users can use no-extra-parens if they want to avoid the unnecessary parens.

Fair enough.
Thank you!

eslintbot · 2016-10-01T22:10:45Z

LGTM

not-an-aardvark · 2016-10-01T22:12:32Z

I think the new commit addresses all of the issues.

eslintbot · 2016-10-01T22:20:25Z

LGTM

mysticatea · 2016-10-01T23:12:10Z

Great!

mysticatea

LGTM, thank you!

mysticatea

I found a problem now

let a = () => {
    return 1
}
(() => doSomething())()

mysticatea

Never mind...
In this case, originate code has a problem...

not-an-aardvark · 2016-10-02T22:38:03Z

I think that is actually a problem with this fixer:

var x = () => { return 1 } // <-- ASI inserts a semicolon here, x will always return 1
(2) // <-- This is an ExpressionStatement with the literal 2

// ↓ fixed

var x = () =>   1  // <-- ASI does not insert a semicolon here, x will throw a TypeError when called
(2)  // <-- This is an argument of calling 1 as a function

So I suppose we should avoid fixes if the next line begins with (, [, or ```, ~~or if the returned value ends with ++ or `--`~~.

edit: Actually, we don't need to account for ++ and --, because if x++ or x-- are used in a ReturnExpression then ++ has to be on the same line as x.

mysticatea · 2016-10-02T22:48:59Z

My brain had been confused since it was right after waking. 😅

mysticatea

There is the ASI problem.

platinumazure · 2016-10-02T22:56:18Z

A thought: Should we just always wrap the value in parentheses, and then let no-extra-parens decide if the parentheses can be removed?

not-an-aardvark · 2016-10-02T23:00:06Z

@platinumazure Could you elaborate on why always wrapping the value in parentheses would be beneficial? I don't understand why we would want to do that.

platinumazure · 2016-10-02T23:07:17Z

Hmm. Originally it was for ASI reasons, but I'm realizing that it doesn't fix all such situations.

Useful fix:

() => { return x++ } // becomes: () => (x++), avoids ASI problems on next line

Less useful:

() => { return x } // becomes () => (x)\n(1), does not solve ASI problem
(1)

So my suggestion wasn't as good as I thought.

not-an-aardvark · 2016-10-02T23:09:42Z

I don't think () => { return x++ } is an ASI problem anyway:

() => { return x++ }
foo

// ↓ fixed

() =>   x++ // this still increases x and not foo, because the ++ is on the same line as x.
foo

eslintbot · 2016-10-03T05:51:57Z

LGTM

eslintbot · 2016-10-03T05:58:28Z

LGTM

eslintbot · 2016-10-03T05:59:15Z

LGTM

nzakas · 2016-10-07T17:03:48Z

tests/lib/rules/arrow-body-style.js

@@ -72,6 +84,7 @@ ruleTester.run("arrow-body-style", rule, {
        },
        {
            code: "var foo = () => { return bar(); };",
+            output: "var foo = () =>   bar() ;",


Can we strip the extra spaces before bar()? It seems like you'd expect just one space because that's what is between return and bar().

The two extra spaces are from after the arrow, and after the opening curly. At the moment, the fixer keeps all text between the tokens in case it contains comments, newlines, etc. I think people can use no-multi-spaces and semi-spacing if they don't want the extra spaces.

Would you recommend that we special-case the fixes where there are only space characters between these tokens?

Yeah, I think we have to special case this. I can't see anyone being happy with more than one space after =>.

Updated to add a special case for this.

eslintbot · 2016-10-13T15:21:40Z

LGTM

nzakas

LGTM. Thanks.

Update: add fixer for arrow-body-style

2eb4882

jquerybot added the CLA: Valid label Sep 25, 2016

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

not-an-aardvark self-assigned this Sep 30, 2016

mysticatea 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 Oct 1, 2016

mysticatea suggested changes Oct 1, 2016

View reviewed changes

(squash) Make sure the fixer doesn't cause SyntaxErrors

8e74916

not-an-aardvark force-pushed the arrow-body-style-fixer branch from 5d04d67 to 8e74916 Compare October 1, 2016 22:20

mysticatea approved these changes Oct 1, 2016

View reviewed changes

mysticatea suggested changes Oct 2, 2016

View reviewed changes

mysticatea approved these changes Oct 2, 2016

View reviewed changes

mysticatea suggested changes Oct 2, 2016

View reviewed changes

not-an-aardvark force-pushed the arrow-body-style-fixer branch from f6703c5 to a2ea590 Compare October 3, 2016 05:58

Avoid fixes when ASI would cause errors

ce9fbcf

not-an-aardvark force-pushed the arrow-body-style-fixer branch from a2ea590 to ce9fbcf Compare October 3, 2016 05:59

mysticatea approved these changes Oct 5, 2016

View reviewed changes

nzakas requested changes Oct 7, 2016

View reviewed changes

Avoid unnecessary spaces when replacing arrow body

87915db

ilyavolodin approved these changes Oct 16, 2016

View reviewed changes

nzakas approved these changes Oct 18, 2016

View reviewed changes

nzakas merged commit e064a25 into eslint:master Oct 18, 2016

not-an-aardvark deleted the arrow-body-style-fixer branch October 18, 2016 18:11

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

Update: add fixer for `arrow-body-style` #7240

Update: add fixer for `arrow-body-style` #7240

not-an-aardvark commented Sep 25, 2016 •

edited

mention-bot commented Sep 25, 2016

eslintbot commented Sep 25, 2016

not-an-aardvark commented Sep 30, 2016

nzakas commented Sep 30, 2016

mysticatea commented Oct 1, 2016 •

edited

mysticatea Oct 1, 2016 •

edited

mysticatea Oct 1, 2016 •

edited

mysticatea Oct 1, 2016

mysticatea Oct 1, 2016

not-an-aardvark Oct 1, 2016

mysticatea Oct 1, 2016

eslintbot commented Oct 1, 2016

not-an-aardvark commented Oct 1, 2016

eslintbot commented Oct 1, 2016

mysticatea commented Oct 1, 2016

mysticatea left a comment

mysticatea left a comment

mysticatea left a comment

not-an-aardvark commented Oct 2, 2016 •

edited

mysticatea commented Oct 2, 2016

mysticatea left a comment

platinumazure commented Oct 2, 2016

not-an-aardvark commented Oct 2, 2016

platinumazure commented Oct 2, 2016

not-an-aardvark commented Oct 2, 2016

eslintbot commented Oct 3, 2016

eslintbot commented Oct 3, 2016

eslintbot commented Oct 3, 2016

nzakas Oct 7, 2016

not-an-aardvark Oct 7, 2016

nzakas Oct 10, 2016

not-an-aardvark Oct 13, 2016

eslintbot commented Oct 13, 2016

nzakas left a comment

Update: add fixer for arrow-body-style #7240

Update: add fixer for arrow-body-style #7240

Conversation

not-an-aardvark commented Sep 25, 2016 • edited

mention-bot commented Sep 25, 2016

eslintbot commented Sep 25, 2016

not-an-aardvark commented Sep 30, 2016

nzakas commented Sep 30, 2016

mysticatea commented Oct 1, 2016 • edited

mysticatea Oct 1, 2016 • edited

Choose a reason for hiding this comment

mysticatea Oct 1, 2016 • 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

eslintbot commented Oct 1, 2016

not-an-aardvark commented Oct 1, 2016

eslintbot commented Oct 1, 2016

mysticatea commented Oct 1, 2016

mysticatea left a comment

Choose a reason for hiding this comment

mysticatea left a comment

Choose a reason for hiding this comment

mysticatea left a comment

Choose a reason for hiding this comment

not-an-aardvark commented Oct 2, 2016 • edited

mysticatea commented Oct 2, 2016

mysticatea left a comment

Choose a reason for hiding this comment

platinumazure commented Oct 2, 2016

not-an-aardvark commented Oct 2, 2016

platinumazure commented Oct 2, 2016

not-an-aardvark commented Oct 2, 2016

eslintbot commented Oct 3, 2016

eslintbot commented Oct 3, 2016

eslintbot commented Oct 3, 2016

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 Oct 13, 2016

nzakas left a comment

Choose a reason for hiding this comment

Update: add fixer for `arrow-body-style` #7240

Update: add fixer for `arrow-body-style` #7240

not-an-aardvark commented Sep 25, 2016 •

edited

mysticatea commented Oct 1, 2016 •

edited

mysticatea Oct 1, 2016 •

edited

mysticatea Oct 1, 2016 •

edited

not-an-aardvark commented Oct 2, 2016 •

edited