Update: Fix accessor-pairs to enforce pairs per property in literals

[mdjermanovic]

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

[X] Bug fix

This is a bug fix, not an enhancement, but it will produce more errors by default.

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.

/*eslint accessor-pairs: "error"*/

var o = { get a() {}, set b(foo) {} };

What did you expect to happen?

By default, this rule reports setters without getters.

I would expect 1 error for the b setter.

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

No warnings.

The rule doesn't read property names in object literals. An object literal with a getter and a setter that are unrelated to each other is never reported, regardless of the settings.

What changes did you make? (Give an overview)

Fixed the logic for object literals.

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

• The code which handles property descriptors is also changed (it had to be, because the same code was handling both property descriptors and 'usual' object literals), but the logic isn't. This code could also use astUtils.getStaticPropertyName() but I didn't change this as it would be an enhancement, not just a bug fix. I can do this in another PR, and there should be also more test cases for property descriptors (e.g. there are no test cases for getters without setters).
• In the new code which handles object literals, computed property keys that cannot be statically resolved to a string are compared by their tokens. This could be seen as an enhancement, but the rule didn't compare names anyway. This allows the rule to report cases like { set [a]() {} }.
• The code is organized in a way that an option to check classes can be easily added, I'll open another issue for this option.
• There is one edge case which isn't handled (needs ES2015+ syntax enabled):

var obj = {
  get a() {},
  a: 1,
  set a() {}
}

This is interpreted as 3 unrelated attempts to create the same key, the last wins and creates a setter without getter. I guess this isn't so important as the no-dupe-keys will report this literal.

As a side note, this could also happen in cases like this:

var obj = {
  get a() {},
  ...anotherObj,
  set a() {}
}

perhaps it would be good to have a rule like no-loose-accessors (I'd be glad to implement it)

[mdjermanovic]

Just to note that this is a major bug for this rule, i.e. the logic is broken.

[platinumazure]

Some minor changes requested to make the tests a little clearer.

I am also slightly worried about comparing token lists in computed properties in case of side effects (e.g., ({ get [a++]() {}, set [a++](foo) {} })). That said, maybe the best way to handle that going forward would be to have an ESLint rule that focused on computed property/method keys with side effects, and then we would be able to do whatever is easiest here (whether that is to report all equivalent token sequences, or to restrict reporting to identifiers and literals).

What do you think?

[platinumazure]

It would be useful if the test could confirm that a is the property missing a setter. Could you please add that to the test? (Request applies to similar tests below)

[mdjermanovic]

Yes, this is definitely an incomplete test that doesn't show much.

This could be tested using location, but it might be also good to change not just the test but the rule itself, to show a message with the name instead of just Setter is not present.?

[platinumazure]

I would love to see the property name added to the message, but I won't insist on it happening in this PR.

[mdjermanovic]

I'll do it, better now in this PR where the tests have to be modified anyway than to fix the whole test file again later.

Just a small question, as there will be different messages for property descriptors (the old generic ones) and object literal properties (which will have the name). Is messageId a public data, can I change it?

[platinumazure]

Great question. We haven't really established that messageId is in the public API, because we don't yet support message handlers/translators anyway, so I think we can consider the message IDs an implementation detail for now.

[mdjermanovic]

This is all done now, using astUtils.getFunctionNameWithKind().

The function doesn't return full name for computed ones, just "getter" or "setter". This might be a future enhancement, maybe like how no-self-assign shows source code without whitespace in the message.

There is a minor bug in getFunctionNameWithKind() related to empty string names, I'll fix that in another PR.

Also, the error will now highlight only the header (e.g. get a) instead of the whole property, as there is nothing wrong with the code in the getter, it's just missing the setter.

Tests are fixed and should be much better now.

[platinumazure]

I think it would be useful to include location data in these errors, to confirm that the same node is not reported twice. Could you please add location data to this test and the one below?

[mdjermanovic]

This is a very good point, it would be very useful indeed. I'll fix this, just to see first what other changes should be made.

[mdjermanovic]

This is also done :)

[mdjermanovic]

As noted in #12070 comparing tokens is not always safe, but it might be useful to analyze how this imperfection affects the particular rule.

E.g. in no-dupe-keys it could produce false positives in cases when a user wants to create an object using side effects. The user could solve this by disabling the rule for the particular part of the code, which might be okay as that code is indeed an exception. Perhaps this isn't even that bad, the eslint-disable feature is made for such exceptions?

In this accessor-pairs rule it could produce false negatives which are, I guess, in general worse thing than false positives. However, the alternative is to not check computed properties at all.

Overall, I'm not sure what would be more in line with the ESLint's standards - to not enforce the rule on something because it isn't 100% reliable (but still pretty good), or enforce knowing that some (very) edge cases will not be caught.

[mdjermanovic]

There are already some rules that compare tokens and thus have small flaws, e.g. these are all false positives:

/*eslint no-useless-call:error*/

a[++i].foo.call(a[++i])

/*eslint prefer-spread:error*/

a[++i].foo.apply(a[++i], args)

/*eslint no-self-compare:error*/

a[++i] < a[++i]

while someone could see these as false negatives:

/*eslint no-useless-call:error*/

a[i+1].foo.call(a[1+i])

/*eslint prefer-spread:error*/

a[i+1].foo.apply(a[1+i], args)

/*eslint no-self-compare:error*/

a[i+1] < a[1+i]

There is a known limitation section in prefer-spread:

Known limitations:

This rule analyzes code statically to check whether or not the this argument is changed. So, if the > this argument is computed in a dynamic expression, this rule cannot detect a violation.

/eslint prefer-spread: "error"/

// This warns.
a[i++].foo.apply(a[i++], args);

// This does not warn.
a[++i].foo.apply(a[i], args);

(Offtopic, I don't think that these examples match the description which says that the rule cannot detect some violations. The first example is detected while it shouldn't be, the second is not detected and it actually isn't a violation.)

Edit: my mistake, the example is ok.

[mdjermanovic]

Given all this, perhaps the rule could indeed check computed keys (i.e. compare tokens) and have a 'known limitations' section in the docs with an example when it would fail to warn:

var foo = {
   get [i++]() {
   },
   set [i++]() {
   }
}

[platinumazure]

Given all this, perhaps the rule could indeed check computed keys (i.e. compare tokens) and have a 'known limitations' section in the docs with an example when it would fail to warn:

That works for me.

Thanks for the great discussion!

[platinumazure]

LGTM, thanks for contributing! Really appreciate the extra details in the tests.

[mdjermanovic]

I've added the "Known Limitations" section, I guess it should be okay.

@platinumazure thank you for the detailed review, I didn't realize how bad was the first version of the tests. And the rule itself got some significant improvements :)

[platinumazure]

I've reproduced the bug here. Marking as accepted.

[platinumazure]

Latest changes LGTM, thanks! Waiting for another team member to review before merging.

[kaicataldo]

One suggestion about documentation, but this LGTM. Thanks for the extensive test suite - it'll make it so much easier for the next person to touch the code to understand what the rule's expected behavior is!

[kaicataldo]

Do we want to add this to the "known limitations "documentation as well? I could see someone reporting this as a bug in the future.

[kaicataldo]

I don't want to block this from landing in this release, so I'm going to go ahead and merge this. If we decide the documentation could be improved, we can always do that in a follow up PR.

[mdjermanovic]

Was about to reply when you merged :)

I think this is a good idea, I'll make a small docs PR to add this.

[kaicataldo]

Sounds good!