fix(eslint-plugin): [no-unused-private-class-members] private destructured class member is defined but used #11785
Conversation
|
Thanks for the PR, @YuyaYoshioka! 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 project configuration. |
|
View your CI Pipeline Execution ↗ for commit 83c15c3
☁️ Nx Cloud last updated this comment at |
bradzacher
left a comment
There was a problem hiding this comment.
quickly looking, this misses the following cases:
class Foo {
property = 1;
caseOne() {
let property;
({ property } = this);
}
caseTwo() {
const foo = (
{ property } = this,
) => {}
}
}Instead of declaring an intermediate class property destructuredThisProperties and then writing to it, could we instead do it more localised -- eg do the countReference entirely within VariableDeclarator instead of splitting it across VariableDeclarator and Identifier?
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (82.00%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #11785 +/- ##
==========================================
+ Coverage 90.49% 90.53% +0.04%
==========================================
Files 522 523 +1
Lines 53360 53146 -214
Branches 8911 8853 -58
==========================================
- Hits 48286 48114 -172
+ Misses 5059 5019 -40
+ Partials 15 13 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
(testing)
Please also include a few tests around the long form destructuring assignment, i.e.
class Foo {
private privateMember;
method() {
const { unused: privateNember } = this;
}
}class Foo {
private privateMember;
method() {
const { privateNember: used } = this;
}
}
PR Checklist
Overview
Solution
This PR adds support for tracking destructured properties from
thisand counts them as member access at the point of destructuring.Key insight: Destructuring from
thisis semantically equivalent to property access.const { a } = thisis equivalent toconst a = this.a(readsthis.a)({ a } = this)is equivalent toa = this.a(readsthis.a)Implementation Details
Added a shared
handleThisDestructuringmethod that:ObjectPatterndestructuring fromthiscountReferencefor each destructured property at the point of destructuringconst { a } = this({ a } = this)(param = { a } = this) => {}Handles all destructuring patterns:
const { a } = thisconst { a: x } = this(still counts as accessingthis.a)const { a, b } = thisconst { staticMember } = thisin static methodsTest Coverage
Added comprehensive test cases:
({ property } = this))const { a: x } = this)