`no-return-assign` behavior changed with arrow functions #5150

Closed
feross opened this Issue Feb 4, 2016 · 27 comments

Projects

None yet
@feross
Contributor
feross commented Feb 4, 2016

Using the latest eslint (2.0.0-rc.0).

Rule:

{
  "no-return-assign": 2
}

Code:

var data = ''
process.stdin.on('data', buffer => data += buffer.toString())

Produces the error:

  2:26  error  Arrow function should not return assignment  no-return-assign

The rule is functioning correctly, because there's an implicit return with arrow functions. But I'm wondering if there isn't a way to keep the rule enabled but accommodate this kind of usage with arrow functions.

The intent of the rule is, primarily, to catch cases where the user intended a comparison but accidentally did an assignment. There's no chance of that in this example because it's += not =, yet this is still considered an error because there's an assignment happening.

Possible ways to accommodate this:

  • add an except-arrow-function option, to disable checking on arrow functions
  • add a way to specify only checking for assignments with =

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@eslintbot eslintbot added the triage label Feb 4, 2016
@eslintbot

@feross Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@feross feross referenced this issue in feross/standard Feb 4, 2016
Closed

Release proposal: standard v6 #399

25 of 25 tasks complete
@nzakas nzakas added enhancement rule proposal and removed triage labels Feb 4, 2016
@nzakas
Member
nzakas commented Feb 4, 2016

If we omitted arrow functions, would it just be for concise bodies? Or also for block bodies?

@michaelficarra
Member

Since you're not using the return value here, it'd be appropriate for you to put the body in curlies. πŸ‘Ž This rule is doing exactly what was intended.

@platinumazure
Member

@michaelficarra I agree with your reasoning but I can see why it could be confusing or frustrating for users since there is no way to "omit the return" without going to braces (and the rule messaging doesn't make that clear either). Surely this still merits a change of some kind? Either:

  1. Change the messaging for concise arrow functions
  2. Allow an option to suppress warnings on concise arrow functions
@feross
Contributor
feross commented Feb 6, 2016

An option to suppress warnings on concise arrow functions would be perfect.

@despairblue

No idea if that is important for this, but using brackets also suppresses the warning, although it's still returning the assignment.

// Arrow function should not return assignment
const a = (v) => v.b = 1
// passes
const a = (v) => (v.b = 1)
@michaelficarra
Member

@despairblue That's an intentional exception to this rule.

@nzakas
Member
nzakas commented Feb 9, 2016

I think that adding an exception just for arrow functions, or just for arrow functions with concise bodies, is a bit too specific an exception. We do have ways around this now, both by adding parentheses, adding braces, or using /*eslint-disable*/. I think @michaelficarra is correct in that this is really the type of nonobvious issue that the rule is trying to flag.

@feross
Contributor
feross commented Feb 9, 2016

@nzakas I get where you're coming from. That's what the rule is for.

This could be an accidental return:

const a = (v) => v.b = 1

But this is almost certainly intentional:

const a = (v) => v.b += 1

An option to ignore +=, -=, etc. and just do = would be nice. In any case, I don't feel too strongly about this. Feel free to close if this is out of the question.

@nzakas
Member
nzakas commented Feb 10, 2016

At the moment, it doesn't seem like there's much support for adding an exception to the rule, so closing (as always, we'll monitor to see if similar issues pop up in the future.)

@nzakas nzakas closed this Feb 10, 2016
@scottohara
Contributor

Please note that some of the suggested workarounds (adding parens/curlies around the expression) may conflict with other rules, such as no-extra-parens, brace-style (1tbs) and/or semi.

Example:

/*eslint no-return-assign: 2,
         no-extra-parens: 2,
         brace-style: 2,
         semi: 2
*/

somePromise.then(foo => foo.bar = 1);

As expected, the above code fails with:

error  Arrow function should not return assignment  no-return-assign

Workaround 1: Wrap the assignment expression in parens:

/*eslint no-return-assign: 2,
         no-extra-parens: 2,
         brace-style: 2,
         semi: 2
*/

somePromise.then(foo => (foo.bar = 1));

We now fail with:

error  Gratuitous parentheses around expression     no-extra-parens

Workaround 2: Wrap the assignment expression in curlies:

/*eslint no-return-assign: 2,
         no-extra-parens: 2,
         brace-style: 2,
         semi: 2
*/

somePromise.then(foo => {foo.bar = 1});

We now fail with:

error  Statement inside of curly braces should be on next line  brace-style
error  Missing semicolon                                        semi

The semi error could be mitigated by including omitLastInOneLineBlock: true; but it would still violate the 1tbs brace style

Workaround 3: Convert from concise body to full block body:

/*eslint no-return-assign: 2,
         no-extra-parens: 2,
         brace-style: 2,
         semi: 2
*/

somePromise.then(foo => {
  foo.bar = 1;
});

Success! Although I would argue that the arrow function has lost some of it's original beauty here.

Based on this, I don't think any of the suggested workarounds (except perhaps temporarily disabling, via eslint-disable) are particularly appealing, considering their impact on other rules.

If you're still interested in votes for some kind of exception to the rule; consider this my +1.

@crookedneighbor

I would also like an exception to this rule for arrow functions.

@crookedneighbor crookedneighbor added a commit to HabitRPG/eslint-config that referenced this issue Apr 15, 2016
@crookedneighbor crookedneighbor fix: Remove no-return-assign 1e34daf
@aarongreenwald

I'm going to add to the chorus requesting an exception (and if it would be accepted, I'd be willing to work on a PR to get it done).

I like having no-return-assign, but I also want to error when braces are used in arrow functions that contain only one line. And I don't want to add visual weight to my code. For me, one of the key benefits of arrow functions is the ability to write clean, concise code:

someArray.forEach(item => item.prop = 'val')

Yes, I'm technically returning an assignment, but it's a common pattern that's not a mistake. If the spirit of no-return-assign is to prevent accidents, I think the rule should be limited to where there's an explicit return statement, not implicit returns. I think that's a logical and defensible distinction, not too specific an exception to carve out.

@mikecousins
mikecousins commented Aug 2, 2016 edited

This conflicts pretty badly with this: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-string-refs.md

Not doing your refs like:
<div ref={c => this.hello = c}>Hello, world.</div>
causes an eslint error.

@gaearon
gaearon commented Aug 31, 2016

To be clear, you can use callback refs just fine if you add { and }:

<div ref={c => { this.hello = c }}>Hello, world.</div>

It’s kinda verbose so I agree it would be great if this option existed.

@gaearon
gaearon commented Aug 31, 2016

Also, FYI, we removed this rule in Create React App because it is too inconvenient/surprising to React users who want to use callback refs (which by themselves are quite verbose so this makes it worse). Relevant discussion: facebookincubator/create-react-app#528. Would be happy to provide more context on our decision if you’d like to chat!

@Kerumen
Kerumen commented Sep 3, 2016

Another way to do it, maybe a bit less verbose, is with parenthesis:

<div ref={c => (this.hello = c)}>Hello, world.</div>
@vitorbal
Member
vitorbal commented Sep 4, 2016

It looks like there's been a lot of support from the community lately for an option to ignore concise arrow-functions.
Since we originally closed this citing lack of support, I am hoping we can reach a consensus for a solution here, so I'm reopening this. If we can make it happen I'm willing to work on a PR for it.

@vitorbal vitorbal reopened this Sep 4, 2016
@mysticatea
Member

I'm πŸ‘Ž.
This request sounds the opposite of this rule's purpose.
Such change/option will make a hole to allow the list.filter(x => x.code = 1)-like mistakes.

Use of braces/parentheses makes clear the code.

@scottohara
Contributor

@mysticatea Please note my earlier comment that the use of braces/parens conflicts with other rules: #5150 (comment)

@platinumazure
Member
platinumazure commented Sep 4, 2016 edited

@scottohara Have you looked at the options for no-extra-parens? I believe it is possible to suppress a warning of that rule for assignments in particular using an option. In that regard, both no-return-assign and no-extra-parens will happily allow use of parens to denote an intentional assignment here. (Compare with no-cond-assign.)

If it turns out that the current behavior does not match no-cond-assign, then perhaps we need to make a change. However, I'm otherwise πŸ‘Ž with @mysticatea: the rule is doing exactly what it's supposed to here.

EDIT: This is the option I was thinking of in no-extra-parens: http://eslint.org/docs/rules/no-extra-parens#returnassign

@mysticatea
Member
mysticatea commented Sep 4, 2016 edited

@scottohara no-extra-parens has the option to allow parentheses around such assignments.

/*eslint no-return-assign: 2,
         no-extra-parens: [2, all, {returnAssign: false}],
         brace-style: 2,
         semi: 2
*/

somePromise.then(foo => (foo.bar = 1));

EDIT: The following options for braces.

/*eslint no-return-assign: 2,
         no-extra-parens: 2,
         brace-style: [2, 1tbs, {allowSingleLine: true}],
         semi: [2, always, {omitLastInOneLineBlock: true}]
*/

somePromise.then(foo => { foo.bar = 1 });
@scottohara
Contributor

OK, thanks @platinumazure / @mysticatea ; it looks like that is a new option that has been added since my original comment.

@btmills
Member
btmills commented Sep 4, 2016 edited

While I agree with the rest of the team here that the rule is behaving correctly and that disabling it defeats the purpose of the rule, given the choice between users disabling the rule entirely or just disabling the check for concise arrow functions, I'd rather they use part of the rule than none of it. Since so many people want it, I would support adding an option that would disable this check only arrow functions with expression bodies. I'd still like to see a proposal for what that option would look like, given the two current options "always" and "except-parens".

Edit: If the no-extra-parens option pointed out by @platinumazure and @mysticatea is sufficient to indicate intentional assignment, then we wouldn't need a new option at all, and everyone could use no-return-assign with no changes today.

@kaicataldo
Member

Has this been resolved?

@nzakas
Member
nzakas commented Oct 1, 2016

With two πŸ‘Žs, and @btmills last comment, I think this can be closed.

@nzakas nzakas closed this Oct 1, 2016
@lukeschunk lukeschunk referenced this issue in mxenabled/eslint-config-mx Oct 4, 2016
Merged

removed rule #17

@roderickObrist
roderickObrist commented Dec 12, 2016 edited

Hi All,

I may be a little late on this thread, but I find most of my angst with this rule can be determined based on the way the array is being used. For example:

// Good
someArray.forEach(val => str += val);
someObject.on('event', val => str += val);
someObject.then(val => str += val);

// Bad
someArray.map(val => str += val);
someObject.then(val => str += val)
  .then(hello => world);

To be specific, it would be a best of both worlds fix if:

  1. There was a list of all Array methods that did not expect a return value and made an exception
  2. If we can detect Promise chaining (.then().then().then()) made exception to the last arrow in the chain
  3. In my experience, the return type does not matter in event handlers (.addEventListener, .on)
@fenivana fenivana added a commit to fenivana/eslint-config-enough that referenced this issue Dec 13, 2016
@fenivana fenivana semi: never. more and more people prefer ASI now...
remove no-return-assign. eslint/eslint#5150
68edc43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment