Fix: allow instance and proto def in no-dupe-class-members (refs #14857)

[mdjermanovic]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an 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:

refs #14857, fixes #14591 (comment)

What changes did you make? (Give an overview)

Fixed the no-dupe-class-members to not report instance field and method/getter/setter with the same name as duplicates.

class Foo {
  bar; // defines property 'bar' on instances of Foo
  bar() { } // defines property 'bar' on Foo.prototype
}

class Foo {
  bar; // defines property 'bar' on instances of Foo
  get bar() { } // defines property 'bar' on Foo.prototype
}

class Foo {
  bar; // defines property 'bar' on instances of Foo
  set bar(value) { } // defines property 'bar' on Foo.prototype
}

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

• Does everyone agree that this shouldn't be reported, or at least not by default? Unlike all other combinations, these definitions do not target the same property. It can be argued that field definition mistakenly overrides prototype definitions in lookups, but reporting this might flag valid code such as:

class Foo {
    handleClick = this.handleClick.bind(this);
    handleClick() {
        // ....
    }
}

• All added invalid tests are regression tests, this change produces only fewer warnings. Some tests that appear as deleted in the diff were just moved to a different position.

[nzakas]

I think we should report duplicate fields by default and have an option to disable the check. From the docs:

If there are declarations of the same name in class members, the last declaration overwrites other declarations silently. It can cause unexpected behaviors.

I think the same type of hazard exists when a field and a method have the same name, only now, you may not know that the field wins. I think doing this is most likely an error so I’d prefer to warn by default.

[mdjermanovic]

I think the same type of hazard exists when a field and a method have the same name, only now, you may not know that the field wins. I think doing this is most likely an error so I’d prefer to warn by default.

That's a good point. My only concern is this pattern:

class Foo {
    handleClick = this.handleClick.bind(this);
    handleClick() {}
}

It's a possible variant of the common pattern to bind some methods in the constructor (e.g., https://reactjs.org/docs/handling-events.html recommends binding event handlers in the constructor).

This rule is classified as type: "problem" so it aims to report possible errors, but this is a valid code. It could be just a stylistic issue.

I have another proposal:

• Allow this and only this particular combination by default: method foo and instance field foo initialized to this.foo.bind(this).
• Start without an option to disable instance vs prototype check. I'm not aware of any other cases that would be common enough to warrant disabling this check in entire codebase, so using eslint-disable-* comments might be more appropriate.

[nzakas]

That pattern still seems like an edge case to me. I think it’s fine to flag it and let people use disable comments to work around it. If we start getting feedback indicating that pattern is becoming common, we can always add an option later.

[mdjermanovic]

Okay, I still think the rule just shouldn't report this pattern by default, but we can reconsider that later.