Babel-eslint: Properties excluded through spread + destructuring assignment should be allowed to be unused

[ide]

Sample code is the easiest way for me to explain this. Say we have an object with a bunch of fields and you want to clone the object but exclude a couple of properties. We'd traditionally write:

var clone = Object.assign({}, originalObject);
delete clone.field1;
delete clone.field2;

With ES7 we can write:

var {field1, field2, ...clone} = originalObject;

Babel itself handles this correctly, but the linter complains if field1 and field2 are unused. Since the variable declarations have a meaningful side effect, I believe some code in the pipeline needs to tell eslint to ignore the unused variable declaration with /*eslint-disable no-unused-vars */, or perhaps Babel should omit the variable declaration if it lexically detects that the variable is used (although this may be undesirable if it breaks eval).

[sebmck]

You could probably patch escope (similar to #75) to register a binding as referenced. I've stemmed away from touching escope because of how hacky it'd be but I'm open to accepting PRs.

[AlexKVal]

And how about legitimately unused vars ?
What if the user has just removed another line of code ?

var {unusedVar, usedVar, ...clone} = originalObject;
// someFunction(usedVar); // removed

How could linter know which var is legitimately unused and which is just forgotten?

[sebmck]

@AlexKVal Left side properties of the object rest _should_ be assumed to be "used". This is also noted in the actual proposal which offers an alternate suggestion of just using _ and configuring your linter like so.

[ide]

@AlexKVal that code is still correct. Without the comment, two things were happening: usedVar was excluded from clone and usedVar was referenced to pass it to the function. With the comment, usedVar is still excluded from clone.

So in that sense, usedVar is actually still being used (that is, it's used to exclude the property from clone) even though it is no longer referenced. Does that distinction help clear things up?

[AlexKVal]

Yeah. Thanks guys ! :cherries:
Your answers about this particular case helps not only me.

[danielberndt]

I'd actually prefer the stricter version from the proposal @sebmck talked about above.
i.e. var {unusedVar1: _, usedVar, unusedVar2: _, ...clone} = originalObject;

this way the intent of the variables is very explicit and will cause less errors (I assume)

[sebmck]

@danielberndt Then you have duplicate declarations.

[danielberndt]

I feel it's acceptable to suppress the duplicate warning if

1) the variable name is _
2) and the declaration happens within a destructuring context with a rest spread

that's at least my interpretation of @sebmarkbage's proposal

[sebmarkbage]

This is only ok when you use var since let and const would not allow you to use duplicate declarations.

[hzoo]

Was going to start this, but after I updated ESLint (0.22.1) and thus escope (3.1.0) with babel-eslint (3.1.10) I don't get the no-unused-vars error for field1 and field2 anymore? Not sure exactly what changed fixed it.

[hzoo]

Is it still the case for you @ide?

[ide]

@hzoo it's fixed for me too (eslint 0.22.1, babel-eslint 3.1.11). Thanks for the notice.

[hzoo]

Ok the fix for https://github.com/babel/babel-eslint/issues/120 made this not work again (I forgot to add a test for this).

[jedwards1211]

I would be equally happy if it were possible to mark variables used via comment:

const {field1, field2, field3, ...clone} = originalObject
// eslint-ignore-unused field2, field3
const x = field1 + ...

This is longer but even more explicit and less questionable than _ and doesn't require var either.

import _ from 'lodash'
const {field1, field2: _, field3: _, ...clone} = originalObject
_.forEach(clone, ...) // uh...

It is currently possible to mark the variables used with no-ops like this, but also questionable:

const {field1, field2, field3, ...clone} = originalObject
field2
field3

[idolize]

If we could get the old behavior back that would be ideal IMO

[danielberndt]

Here's a solution that you can use today and that works around the issue quite well in my opinion:

you need to add this eslint-rule

    "no-unused-vars": [1, {
      "vars": "local",
      "varsIgnorePattern": "^_",
      "args": "after-used",
      "argsIgnorePattern": "^_"
    }]

which allows you to write code like this without warning

const {theme: _, children: _1, onSubmit: _2, ...rest} = this.props

[jedwards1211]

These days I just do

const {
  field1,
  field2, field3, // eslint-disable-line no-unused-vars
  ...rest
} = props

It may be a bit ugly, but I still like it better than using varsIgnorePattern.

[danez]

This is now fixed via https://github.com/eslint/eslint/pull/7968

[hzoo]

YES this was the worst 😛