Update: no-unused-vars to account for rest property omissions #7968

zackargyle · 2017-01-23T23:58:24Z

What is the purpose of this pull request? (put an "X" next to item)
[X] Changes an existing rule ([template]

What rule do you want to change?
no-unused-vars

Does this change cause the rule to produce more or fewer warnings?
fewer

How will the change be implemented? (New option, new default behavior, etc.)?
new default behavior

Please provide some example code that this change will affect:

var data = { type: 'coordinates', x: 0, y: 0 };
var { type, ...coordinates } = data;
console.log(coordinates);

What does the rule currently do for this code?
Errors that type is unused.

What will the rule do after it's changed?
type will be considered to be passively used. Meaning that although it is not used afterwards, it's existence effects the declaration of coordinates. Much like _.omit(data, ['type']);

What changes did you make? (Give an overview)
The change is pretty simple. If the variable is unused, but has a sibling rest parameter, we now consider it to be used. The logic is found in the hasRestSpreadSibling function.

Is there anything you'd like reviewers to focus on?
Although the rest property is still considered experimental, it is stage-3 and widely adopted in the community. The ability to use it as a means of property omission is also becoming increasingly popular.

mention-bot · 2017-01-23T23:58:26Z

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

not-an-aardvark

Thanks for the pull request.

This seems like a reasonable use-case for having unused variables. Personally, though, I think it would be better to put it behind an option rather than changing the default behavior of the rule.

Previous discussion on this issue can be found here: #6563

not-an-aardvark · 2017-01-24T00:28:09Z

tests/lib/rules/no-unused-vars.js

@@ -333,6 +345,21 @@ ruleTester.run("no-unused-vars", rule, {
            ]
        },

+        // Using object rest for variable omission
+        {
+            code: "const data = { type: 'coords', x: 1, y: 2 };\nconst { type } = data;",


I'm confused by this testcase -- the comment indicates that it's using object rest, but it doesn't appear to do that.

Yeah I wasn't sure how necessary this was, but thought it would be nice to have a destructuring case without the rest sibling just for sanity. Need to change the comment for sure tho. Copy pasta.

not-an-aardvark · 2017-01-24T00:31:27Z

lib/rules/no-unused-vars.js

+              .some(def => (
+                def.node.id &&
+                def.node.id.type === "ObjectPattern" &&
+                def.node.id.properties.some(property => property.type === "ExperimentalRestProperty")


Can you also include a check for RestProperty in addition to ExperimentalRestProperty? I think parsers like babel-eslint output RestProperty, and we will too after it reaches stage 4.

not-an-aardvark · 2017-01-24T00:33:33Z

lib/rules/no-unused-vars.js

+              .some(def => (
+                def.node.id &&
+                def.node.id.type === "ObjectPattern" &&
+                def.node.id.properties.some(property => property.type === "ExperimentalRestProperty")


It's only necessary to check the last property of the declaration here rather than iterating through all the properties, because rest properties are always the last property of a declaration.

Great point.

eslintbot · 2017-01-24T00:49:10Z

Thanks for the pull request, @zackargyle! 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-24T00:54:12Z

Thanks for the pull request, @zackargyle! 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 · 2017-01-24T01:00:18Z

I think eslintbot is complaining because it's checking the message for the first commit of this PR (since that's the message that gets used when the PR is merged).

eslintbot · 2017-01-24T17:20:03Z

LGTM

zackargyle · 2017-01-24T17:39:02Z

@not-an-aardvark all fixed up :) Was missing the colon 😄👍🏻

nzakas · 2017-01-24T17:46:56Z

think it would be better to put it behind an option rather than changing the default behavior of the rule.

Agree. I don't think this should be the default behavior, but I think an option is fine.

not-an-aardvark · 2017-01-24T17:47:27Z

lib/rules/no-unused-vars.js

+                lastProperty.node.id.properties.some(property => (
+                    property.type === "ExperimentalRestProperty" ||
+                    property.type === "RestProperty"
+                ))


I think you might have removed the wrong iteration here -- it's still necessary to keep the filter statement from before (if a variable has multiple declarations, the rule should check all of them), but the node.id.properties.some() iteration is unnecessary because a rest property will always be the last property of a given node.

I think it should look roughly like this:

function hasRestSpreadSibling(variable) { return variable.defs .filter(def => def.name.type === "Identifier") .some(def => ( def.node.id && def.node.id.type === "ObjectPattern" && def.node.id.properties.length && new Set(["ExperimentalRestProperty", "RestProperty"]).has(def.node.id.properties[def.node.id.properties.length - 1].type) )); }

not-an-aardvark · 2017-01-24T17:58:38Z

Looking at the status of the proposal:

@alberto and @michaelficarra were 👍 for Add option to no-unused-vars when using object spreads to omit properties #6563.
I think @platinumazure expressed support for the proposal in Add option to no-unused-vars when using object spreads to omit properties #6563 (comment) (contingent on the feature being behind an option). @platinumazure, could you clarify if you're 👍 for this proposal?
@mysticatea added a 👎 to this PR. @mysticatea, are you still against this proposal if the new feature is behind an option rather than changing the default behavior?
I'll champion this proposal (assuming it's behind an option rather than changing the default behavior).

platinumazure · 2017-01-24T18:09:42Z

@not-an-aardvark I edited the comment. I'm 👍 only if the enhancement is behind an option, 👎 for default behavior change. Thanks for following up.

not-an-aardvark · 2017-01-24T18:15:25Z

Thanks for clarifying. So we now have a champion and 3 👍s for putting the behavior behind an option, which means we can accept the issue if @mysticatea's 👎 was just for changing the default behavior. (I'll wait for him to clarify, though.)

eslintbot · 2017-01-24T18:28:40Z

LGTM

eslintbot · 2017-01-24T18:29:55Z

LGTM

zackargyle · 2017-01-24T18:30:43Z

Fixed it up and put it behind a ignoreRestSiblings boolean option (default: false). Sounds like most everyone agrees the option is the best course of action!

not-an-aardvark

This mostly looks good to me, I just have some recommendations for additional tests.

not-an-aardvark · 2017-01-24T18:41:51Z

docs/rules/no-unused-vars.md

@@ -195,6 +195,16 @@ Examples of **correct** code for the `{ "args": "none" }` option:
 })();
 ```

+### ignoreRestSiblings
+
+The `ignoreRestSiblings` option is a boolean (default: `false`). With modern JavaScript it is possible to "omit" properties from an object by using a rest parameter, but by default the sibling properties are marked as "unused". With this option enabled the omitted properties are considered "used", as their existence has a side-effect. In the example, `type` is considered "used".


A few minor nitpicks:

I feel like "modern JavaScript" is kind of a vague term, especially since rest/spread properties aren't part of the official spec yet. Maybe it would be better to link to the rest/spread property spec instead.

I think this is technically referring to rest properties, not rest parameters.

It might be clearer to say that the unused variables are ignored by the rule, rather than saying that the variables are considered "used" because their existence has a side-effect. For example, this variable's existence also has a side-effect, but it's not considered "used":

var foo = doSomethingWithSideEffects();

not-an-aardvark · 2017-01-24T18:43:09Z

docs/rules/no-unused-vars.md

+
+The `ignoreRestSiblings` option is a boolean (default: `false`). With modern JavaScript it is possible to "omit" properties from an object by using a rest parameter, but by default the sibling properties are marked as "unused". With this option enabled the omitted properties are considered "used", as their existence has a side-effect. In the example, `type` is considered "used".
+
+```js


To make sure the code box ends up colored green on the site to indicate "correct" code, you should put something like this before the code box:

Examples of **correct** code for the `{ "ignoreRestSiblings": true }` option: ```js // ...example code ```

not-an-aardvark · 2017-01-24T18:47:18Z

lib/rules/no-unused-vars.js

+         * @private
+         */
+        function hasRestSpreadSibling(variable) {
+            if (config.ignoreRestSiblings === true) {


Nitpick: This can be replaced with if (config.ignoreRestSiblings) { (explicitly comparing to true isn't necessary).

not-an-aardvark · 2017-01-24T18:49:35Z

tests/lib/rules/no-unused-vars.js

+                    experimentalObjectRestSpread: true
+                }
+            }
+        },


Can you add a test that has code like this, but without the ignoreRestSiblings option enabled? It would be useful to verify that an error is reported in that case.

not-an-aardvark · 2017-01-24T18:54:33Z

tests/lib/rules/no-unused-vars.js

+            errors: [
+                { line: 2, column: 9, message: "'type' is assigned a value but never used." }
+            ]
+        },


Can you add a couple tests where the unused variable is the one with the rest property?

var { ...unused } = bar;

var { foo, ...unused } = baz; doSomethingWith(foo);

Both of these should result in an error getting reported.

not-an-aardvark · 2017-01-24T18:57:44Z

tests/lib/rules/no-unused-vars.js

+            errors: [
+                { line: 2, column: 9, message: "'type' is assigned a value but never used." }
+            ]
+        },


Can you add a couple tests where an unused variable is in the same declarator as a rest property, but it's not a sibling?

var { foo: [unused], ...bar } = baz; doSomethingWith(bar);

var [unused, { ...bar }] = baz; doSomethingWith(bar);

Both of these should result in an error getting reported.

eslintbot · 2017-01-24T21:51:35Z

LGTM

not-an-aardvark

Just one small correction, otherwise this LGTM.

not-an-aardvark · 2017-01-24T22:14:06Z

lib/rules/no-unused-vars.js

+                        def.node.id.type === "ObjectPattern" &&
+                        def.node.id.properties.length &&
+                        restProperties.has(def.node.id.properties[def.node.id.properties.length - 1].type) &&  // last property is a rest property
+                        def.node.id.properties.some(prop => prop.type === "Property" && prop.key.name === variable.name)  // variable is sibling of the rest property


I don't think comparing prop.key.name is correct, because the property names and the variable names are independent from each other:

// This will get reported, because prop.key.name is "foo" and variable.name is "_" var { foo: _, ...bar } = baz; doSomethingWith(bar);

I think the easiest way to fix this would be to replace the "variable is sibling of the rest property" line with something using def.name property, which is a reference to the Identifier node for the given variable. That way, we also wouldn't have to iterate over all the properties:

// verify that this definition is not part of a RestProperty !restProperties.has(def.name.parent.type)

I'm happy to change it, but I actually had purposefully wanted that example to be an error. My thinking was that you shouldn't rename a variable that's only used for omission. It would be slightly faster though to change it to check the parent type. Easy change, let me know :)

Oh I see, interesting point. I still think it would be better to check the parent type though -- renaming the variable might be necessary if a different variable is already defined with the given name. For example, in the code below, the variable has to be renamed to avoid shadowing the existing window variable:

var { window: _, ...otherProps } = unrelatedObj; // later... window.addEventListener(/* ... */);

Also, in some cases the property name is computed, so a name check won't work:

function omit(obj, key) { const { [key]: _, ...remainingProps } = obj; return remainingProps; }

👍 Too many awkward possibilities. Just switched to use the parent type.

mysticatea · 2017-01-25T08:05:02Z

@mysticatea added a 👎 to this PR. @mysticatea, are you still against this proposal if the new feature is behind an option rather than changing the default behavior?

No. I will not oppose this proposal strongly if it's an option.

However, "Rest/Spread Properties" proposal is still on stage 3, so I wonder if this is proper in core for right now (https://github.com/eslint/eslint/#what-about-experimental-features). Because we have context.markVariableAsUsed() API, I think plugins can use it for this.

Update docs Update: use RestProperty and only check last property Temp temp temp Use parent type

eslintbot · 2017-01-25T17:33:05Z

LGTM

not-an-aardvark

LGTM. Thanks!

not-an-aardvark · 2017-01-25T17:39:12Z

@mysticatea I suppose another option would be to mark the option as experimental, e.g. something along the lines of experimentalIgnoreRestSiblings (since we do the same thing with the experimentalObjectRestSpread parser option).

zackargyle · 2017-01-25T19:52:25Z

Anything else y'all need from me? I'd love to get this merged so we can remove a bunch of disabled linter statements here at Pinterest! 😁

platinumazure · 2017-01-25T20:55:46Z

@not-an-aardvark I'd rather not have an option name with "experimental" because it means we'd have to change it (and do a major release) sometime after the proposal hits stage 4. As far as I'm concerned, even though our parser option labels it "experimental", we're basically supporting it, so we should use a normal option name. Just my 2 cents.

not-an-aardvark · 2017-01-25T21:30:02Z

My thinking was that we would eventually introduce a new non-experimental flag when the feature reaches stage 4 (e.g. ignoreRestSiblings), and keep experimentalIgnoreRestSiblings indefinitely as an alias of ignoreRestSiblings. This would prevent us from having to do a breaking change.

That said, I'm also fine with keeping a normal option name.

zackargyle · 2017-01-26T23:12:55Z

All good with option naming?

not-an-aardvark · 2017-01-27T02:26:31Z

Fine by me. I'll leave this open for one more day in case anyone else ends up reviewing it, but unless something unexpected comes up this will get merged soon and make it into the next release. 👍

not-an-aardvark · 2017-01-28T05:07:49Z

Thanks for contributing!

not-an-aardvark suggested changes Jan 24, 2017

View reviewed changes

not-an-aardvark 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 24, 2017

zackargyle force-pushed the no-unused-vars-with-rest-property branch from f3a4d48 to ccdd96a Compare January 24, 2017 00:54

zackargyle force-pushed the no-unused-vars-with-rest-property branch from ccdd96a to 09fce25 Compare January 24, 2017 17:20

not-an-aardvark reviewed Jan 24, 2017

View reviewed changes

not-an-aardvark self-assigned this Jan 24, 2017

zackargyle force-pushed the no-unused-vars-with-rest-property branch from 09fce25 to 5aa0048 Compare January 24, 2017 18:28

zackargyle force-pushed the no-unused-vars-with-rest-property branch from 5aa0048 to c4efcec Compare January 24, 2017 18:29

not-an-aardvark suggested changes Jan 24, 2017

View reviewed changes

zackargyle force-pushed the no-unused-vars-with-rest-property branch from c4efcec to 55961cb Compare January 24, 2017 21:51

not-an-aardvark suggested changes Jan 24, 2017

View reviewed changes

Update: no-unused-vars to account for rest property omissions

31a1617

Update docs Update: use RestProperty and only check last property Temp temp temp Use parent type

zackargyle force-pushed the no-unused-vars-with-rest-property branch from 55961cb to 31a1617 Compare January 25, 2017 17:33

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

View reviewed changes

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 25, 2017

not-an-aardvark merged commit c59a0ba into eslint:master Jan 28, 2017

mysticatea mentioned this pull request Feb 3, 2017

no-unused-vars when destructuring #8022

Closed

This was referenced Feb 3, 2017

Update dependency eslint to version 3.15.0 singapore/lint-condo#231

Merged

Update dependency eslint to version 3.15.0 renovatebot/renovate#88

Merged

This was referenced Feb 5, 2017

[Chore] Update dependency eslint to version 3.15.0 SimpliField/sf-preferences#34

Merged

[Chore] Update dependency eslint to version 3.15.0 SimpliField/eslint-config-simplifield#49

Merged

danez mentioned this pull request Mar 6, 2017

Properties excluded through spread + destructuring assignment should be allowed to be unused babel/babel-eslint#95

Closed

windse7en mentioned this pull request Jun 13, 2017

Handling unused props in the render method cerner/terra-core#268

Closed

4 tasks

jsnajdr mentioned this pull request Sep 7, 2017

Simple Payments: Fix a couple of React warnings in SimplePaymentsDialog Automattic/wp-calypso#17875

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


		The `ignoreRestSiblings` option is a boolean (default: `false`). With modern JavaScript it is possible to "omit" properties from an object by using a rest parameter, but by default the sibling properties are marked as "unused". With this option enabled the omitted properties are considered "used", as their existence has a side-effect. In the example, `type` is considered "used".

		```js

Update: no-unused-vars to account for rest property omissions #7968

Update: no-unused-vars to account for rest property omissions #7968

Conversation

zackargyle commented Jan 23, 2017

mention-bot commented Jan 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jan 24, 2017

eslintbot commented Jan 24, 2017

not-an-aardvark commented Jan 24, 2017

eslintbot commented Jan 24, 2017

zackargyle commented Jan 24, 2017

nzakas commented Jan 24, 2017

Choose a reason for hiding this comment

not-an-aardvark commented Jan 24, 2017

platinumazure commented Jan 24, 2017

not-an-aardvark commented Jan 24, 2017

eslintbot commented Jan 24, 2017

eslintbot commented Jan 24, 2017

zackargyle commented Jan 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eslintbot commented Jan 24, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark Jan 24, 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

mysticatea commented Jan 25, 2017 • edited

eslintbot commented Jan 25, 2017

not-an-aardvark left a comment

Choose a reason for hiding this comment

not-an-aardvark commented Jan 25, 2017

zackargyle commented Jan 25, 2017

platinumazure commented Jan 25, 2017

not-an-aardvark commented Jan 25, 2017

zackargyle commented Jan 26, 2017

not-an-aardvark commented Jan 27, 2017

not-an-aardvark commented Jan 28, 2017

not-an-aardvark Jan 24, 2017 •

edited

mysticatea commented Jan 25, 2017 •

edited