Update: Fix handling of property names in no-self-assign

[mdjermanovic]

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

[X] Bug fix

This is a bug fix to remove false positives, but it's fixed in a way that can produce a few more warnings.

Please see is it ok, the details are below.

Tell us about your environment

• ESLint Version: 6.1.0
• Node Version: 10.16.0
• npm Version: 6.9.0

What parser (default, Babel-ESLint, etc.) are you using?

default

Please show your full configuration:

Configuration

module.exports = {
  parserOptions: {
    ecmaVersion: 2015,
  },
};

What did you do? Please include the actual source code causing the issue.

An example:

/*eslint no-self-assign: "error"*/

({ "foo": a } = { "bar": a });
({ 1: a } = { 2: a });
({ "foo": a } = { 1: a });
({ 1: a } = { "bar": a });

Demo link

What did you expect to happen?

No warnings.

What actually happened? Please include the actual, raw output from ESLint.

  3:26  error  'a' is assigned to itself  no-self-assign
  4:18  error  'a' is assigned to itself  no-self-assign
  5:22  error  'a' is assigned to itself  no-self-assign
  6:22  error  'a' is assigned to itself  no-self-assign

What changes did you make? (Give an overview)

The code was assuming that all non-computed keys are identifiers and matched them by undefined === undefined.

It's fixed to compare keys by their getStaticPropertyName values.

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

These would be new warnings:

/*eslint no-self-assign: "error"*/

({ "foo": a } = { foo: a });
({ ["foo"]: a } = { foo: a });
({ ["foo"]: a } = { "foo": a });
({ ["foo"]: a } = { ["foo"]: a });
({ [1]: a } = { 1: a });

Is it okay for a semver-minor fix? I can modify the code to just remove false positives.

[platinumazure]

Is it okay for a semver-minor fix?

I think the question is, should the rule definitely be reporting those issues (could there be a good reason we weren't reporting those cases)? If the current behavior is not according to design, then this is acceptable as a semver-minor fix.

I lean toward that interpretation, but please let me know if I missed something.

[mdjermanovic]

I've edited the post to add ({ ["foo"]: a } = { ["foo"]: a }); to the list of new warnings.

As for ({ "foo": a } = { foo: a }); it was probably due to the missed fact that non-computed keys can be literals. I guess it could be ok to add at least these warnings, this was a bug that also produced false positives.

As for why 'simple' computed keys were not compared, the code is very explicit and it was certainly by design, probably before they were treated as statically known in other rules as well.

[mdjermanovic]

Most of the rules use getStaticPropertyName and treat ["a"] same as "a" and a.

Was there a precedent of this particular change in a minor version?

The safest thing here would be to revert that part and ignore all computed keys, it would be similar to how no-dupe-class-members works at the moment.

On the other hand, that rule does match "a" with a, I believe that part can be seen as a bug fix for a minor version (that's basically the same bug that caused false positives).

[mysticatea]

LGTM, thank you!

[platinumazure]

LGTM, thanks!

[mdjermanovic]

Are all new warnings acceptable for semver-minor?

(I didn't change anything after the initial commit)

[platinumazure]

@mdjermanovic No, but I thought about this case and I can't see any reason why the rule would want to distinguish between property assignments vs literals as a matter of design.

In this case, I evaluated the probability that people were depending on the current, specific behavior with differences between literals and property names, and decided that the probability was very low. But again, that's just my own evaluation.

We do have precedent about adding warnings in semver-minor where the enhancement just seems natural. (E.g., a stylistic rule about var declarations could be naturally expanded to let and const declarations.)

If other team members feel differently, I certainly have no problem with adding an option.

[mdjermanovic]

@platinumazure I agree, this is a too 'small' change in behavior to be an option.

[mysticatea]

I'm fine with semver-minor (A bug fix in a rule that results in ESLint reporting more errors). If the rest of the team have no objection, I will merge this PR within the next release.

[kaicataldo]

LGTM, thanks!