Fix: prefer-const false positive with object spread (fixes #8187)

[vitorbal]

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

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

This fixes a false-positive with prefer-const and the destructuring: "all" option. Essentially, the option was not taking into account destructurings that contained a rest operator.
The original bug-report is here: #8187 (comment)

What changes did you make? (Give an overview)
I've added a couple of test cases that reproduce the bug, then modified the prefer-const rule accordingly to fix the bug.

Is there anything you'd like reviewers to focus on?
I'm not sure, but I think not. Maybe suggestions for more test cases?

[eslintbot]

LGTM

[mention-bot]

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

[platinumazure]

Might be worth writing a test based on babel-eslint's parse output if you can-- I don't know if they use ExperimentalRestProperty or just RestProperty (and I think eventually the Stage 3 proposal will use the latter).

[not-an-aardvark]

Actually, ESTree ended up switching to RestElement. I think babel-eslint might still use RestProperty though.

[vitorbal]

Actually, ESTree ended up switching to RestElement. I think babel-eslint might still use RestProperty though.

That's a good point. This issue leads me to believe babel-eslint indeed is still using RestProperty. ping @hzoo can you confirm?

[vitorbal]

I reached out to @hzoo and it looks like babel-eslint still uses RestProperty, so I'm gonna update the PR to add a check for it as well.

As far as I understand, the RestProperty usage will be cleaned up after babel/babel-eslint#440, so we could do a sweeping change and clean up the occurences in eslint too once that's done.

[platinumazure]

@vitorbal Thanks for checking! I think removing RestProperty from every rule should probably be considered a semver-major change since we don't know what other parsers might use it and rely on ESLint supporting that node type.

[vitorbal]

@platinumazure Hmm that's a very good point, thanks for pointing it out!

[platinumazure]

Given what we've uncovered in the discussion, this should probably include RestProperty as well.

If at all possible, a test which simulates the babel-eslint parser's output would also be awesome, but don't put too much effort into that.

[eslintbot]

LGTM

[vitorbal]

Ok so I did a bit more investigating and I found out that babel-eslint already outputs ExperimentalRestProperty since v6.x (babel/babel-eslint@31dd811).

On previous versions of babel-eslint, the node type was being set to SpreadProperty (instead of RestProperty, like we previously believed).

I added SpreadProperty to the regexp pattern, since I assume babel-eslint 5.x still sees some usage (it only supports ESLint 1.x, though...).
I also wrote a stub parser fixture for the ast output from babel-eslint 5.x, and a couple of tests.
If folks think the tests are not necessary I can remove them, but I figured it wouldn't hurt.

[vitorbal]

ping @not-an-aardvark @platinumazure. Any chance you guys could take another look at this? Would like to get it merged before the release this Friday, if possible.

[platinumazure]

My concerns are addressed, thanks!