accessor-pairs option to check classes

[mdjermanovic]

What rule do you want to change?

accessor-pairs

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

More if the option is set.

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

New option.

Please provide some example code that this change will affect:

/*eslint accessor-pairs: ["error", { "class": true }]*/

class foo {
  set a(bar) {}
}

What does the rule currently do for this code?

Nothing

What will the rule do after it's changed?

Warn that the 'getter is not present', as it already does for object literals and property descriptors.

I guess there is no reason to skip classes in this rule, the option instead of the default behavior is there just to avoid a breaking change.

Are you willing to submit a pull request to implement this change?

Yes, I'll be glad to do it. #12062 fix should be merged first.

[kaicataldo]

I imagine the only reason this doesn't check this case now is that it was written before the class syntax was added to the language. I think it makes sense to add this behind an option, as suggested, and then to change the default behavior in a future major release.

[kaicataldo]

I'll champion this!

[mdjermanovic]

The code will not mix static and prototype accessors, e.g.:

/*eslint accessor-pairs: ["error", 
 { "getWithoutSet": true, "setWithoutGet": true, "class": true }]*/

class foo {
  get a() {}
  static set a(bar) {}
}

Both will be reported, as these are two unrelated elements that just happen to have the same name.

Also, rule should skip new elements for now, like private instance accessors, e.g.:

class foo {
  get #a() {}
}

I guess the filter by type = "MethodDefinition" will skip these, but I just can't make babel-eslint to work with this syntax to be sure (I tried to configure both the actual and 11 beta versions, there is always some error). What is the type of this node?

[kaicataldo]

@mdjermanovic ESLint core doesn't support syntax features until they reach stage 4, so we can cross that bridge when we get to it.

[mdjermanovic]

@mdjermanovic ESLint core doesn't support syntax features until they reach stage 4, so we can cross that bridge when we get to it.

Yes, but still shouldn't crash on input that comes through babel-eslint? This probably wouldn't, I just thought to check could there be some false positives/negatives (most likely not, though).

[platinumazure]

@mdjermanovic

Yes, but still shouldn't crash on input that comes through babel-eslint? This probably wouldn't, I just thought to check could there be some false positives/negatives (most likely not, though).

I really appreciate your commitment to quality on this front. That said, I don't personally interpret our policy to mean that we should punish ourselves if we accidentally release an enhancement that does cause a crash for someone using a stage 3 feature. I see our policy as more reactive: If we discover, or are informed about, an issue where a stage 3 feature crashes, we can fix it with a semver-patch change. But that doesn't mean we need to worry too much about testing every possible experimental feature on every PR. This project would be completely unmaintainable if we tried to do that. 😄

So my suggestion here is, spend as much or as little time as you want on trying to test babel-eslint here, but we don't need babel-eslint tests in the PR and we definitely don't want you to burn yourself out trying to account for every corner case in experimental syntax.

Just my two cents. Other team members may have different thoughts.

[mdjermanovic]

@platinumazure

That was very helpful, now I have a much better understanding of the stage 3 policy :)

[platinumazure]

@eslint/eslint-team We just need two more 👍s to accept this enhancement!

[g-plane]

It's accepted now.

[mdjermanovic]

I'm working on this.

What would be a name, maybe enforceForClassMembers ?