feat(eslint-plugin): [no-unused-private-class-members] new extension rule #10913
Conversation
|
Thanks for the PR, @bradzacher! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
View your CI Pipeline Execution ↗ for commit 34b77b8.
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 34b77b8. ☁️ Nx Cloud last updated this comment at |
| // mark the first member we encounter as used. If you were to delete the | ||
| // member, then any subsequent usage could incorrectly mark the member of | ||
| // an encapsulating parent class as used, which is incorrect. | ||
| trackedClassMembersUsed.add(memberDefinition.declaredNode); |
There was a problem hiding this comment.
[Bug]: I've noticed some missed reports with functions that have a different this than the class instance (deploy preview playground link):
class Test1 {
// should be reported but doesn't?
private bar: number;
foo = function () {
return this.bar;
}
}There was a problem hiding this comment.
This is because the base rule doesn't support that case because this.#bar is a syntax error inside a function expression!!
It's worth noting that TS doesn't actually catch this case either and reports bar as unused because the type of the this in the function expression is any!
If you turn on noImplicitThis then TS will error on that this
We could support this if we wanted to. I'm leaning towards not because TS itself doesn't catch it either.
There was a problem hiding this comment.
Yes! I think the rule should report bar as unused in this case. I've mentioned this since TypeScript reports it as unused but the rule doesn't report it as unused (regardless of the type error).
| PrivateIdentifier(node): void { | ||
| processPrivateIdentifier(node); | ||
| }, |
There was a problem hiding this comment.
[Bug]: I think there's an inconsistency that creates false positives with static private properties (deploy preview playground link):
class Test1 {
public static foo() {
return Test1.#bar;
}
// correctly not reported
static #bar = 1;
}
class Test2 {
public static foo() {
return Test2.bar;
}
// reported but shouldn't?
private static bar = 1;
}I've noticed a similar false positive with the following (deploy preview playground link):
class Test1 {
public foo(a: Test1) {
return a.#bar;
}
// correctly not reported
#bar = 1;
}
class Test2 {
public foo(a: Test2) {
return a.bar;
}
// reported but shouldn't?
private bar = 1;
}There was a problem hiding this comment.
For your second example -- this is expected and is the same purposely not covered case as here:
https://github.com/typescript-eslint/typescript-eslint/pull/10913/files#diff-914330b2e3f9d71dfb628800b850412c3c5560900b13cf8a2925ff7af0905e53R50-R66
I made this rule purposely NOT use type info because the cases that type info enable are mostly going to be rarer edge cases.
There was a problem hiding this comment.
This first example of static private member access we could handle though with scope analysis.
| </TabItem> | ||
| </Tabs> | ||
|
|
||
| ## Limitations |
There was a problem hiding this comment.
What do you think about also mentioning assigning this to a variable as a limitaiton?
class Test1 {
private bar: number;
foo() {
const self = this;
return function () {
// ❌ NOT detected as a usage
self.bar;
}
}
}|
FYI I'm going to make @Josh-Cena very happy - I'm going to build a "class scope analyser" so we can do this right rather than just hacking it in. This does mean that I'm going to rewrite the rule from the ground up -- but it'll be worht it |
Do you intend for this to proceed as-is, with this work intended to happen in the future, or should we pause/draft it pending this work? |
|
Let's draft it. |
PR Checklist
Overview
This PR implements an extension rule for
no-unused-private-class-membersthat introduces support forprivatemembers.