Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect unused private members in classes #14859

Closed
TimvdLippe opened this issue Jul 30, 2021 · 10 comments
Closed

Detect unused private members in classes #14859

TimvdLippe opened this issue Jul 30, 2021 · 10 comments

Comments

@TimvdLippe
Copy link
Contributor

@TimvdLippe TimvdLippe commented Jul 30, 2021

What rule do you want to change?
no-unused-vars (arguably this could be a separate rule as well, something like no-unused-private-fields)
Does this change cause the rule to produce more or fewer warnings?
More
How will the change be implemented? (New option, new default behavior, etc.)?
New default behavior
Please provide some example code that this change will affect:

class MyClass {
  #unusedField = 5;
  #usedFieldOnlyInWrite = 42;
  #usedWhenReadingAndWriting = 500;
  someMethod() {
    this.#usedFieldOnlyInWrite = 42;
    this.#usedWhenReadingAndWriting += 1;
  }
  someOtherMethod() {
    return this.#usedWhenReadingAndWriting;
  }
}

What does the rule currently do for this code?
Nothing
What will the rule do after it's changed?
Report unused fields declared in a class. In the above example, it should report that both #unusedField and #usedFieldOnlyInWrite are unused. This should be possible, as private fields are only concerned in a particular class context, so you can collect all fields in a class, iterate through all usages and mark does that are never read as unused.
Are you willing to submit a pull request to implement this change?
I would be able to give it a go, but I am not sure how difficult this would be.

@nzakas
Copy link
Member

@nzakas nzakas commented Aug 2, 2021

I don't think we want to update no-unused-vars to cover class fields, as this is two steps beyond the intent of the rule and doesn't really apply the same logic of checking scopes (which we use to determine unused variables).

I could see the utility of a new rule, no-unused-private-fields, that just checks private fields for usage. Would that solve your use case?

Loading

@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Aug 2, 2021
@nzakas nzakas removed the triage label Aug 2, 2021
@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented Aug 2, 2021

Yes a new rule would be absolutely fine by me. Would you be okay if I submit a PR with that?

Loading

@nzakas nzakas self-assigned this Aug 3, 2021
@nzakas
Copy link
Member

@nzakas nzakas commented Aug 3, 2021

I will champion this as a new rule. We still need to get feedback from the team so please hold off on a PR until we mark this as accepted.

Loading

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Aug 3, 2021

Makes sense to add this rule. If we want to include private methods in the same rule (I don't see a reason why we shouldn't), a name like no-unused-private-class-members might be more appropriate.

Loading

@snitin315
Copy link
Contributor

@snitin315 snitin315 commented Aug 3, 2021

Makes sense to add this rule 👍

Loading

@nzakas
Copy link
Member

@nzakas nzakas commented Aug 4, 2021

a name like no-unused-private-class-members might be more appropriate.

Makes sense to me.

Loading

@nzakas nzakas changed the title Detect unused private fields in classes Detect unused private members in classes Aug 4, 2021
TimvdLippe added a commit to TimvdLippe/eslint that referenced this issue Aug 6, 2021
For any private class property or method, report those that are
unused. Since private class members can only be accessed in the
same class body, we can safely assume that all usages are processed
when we leave the ClassBody scope.
@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented Aug 6, 2021

I know this issue hasn't been marked as accepted yet. However, given the positive response thus far and me having a Friday to work on open source contributions, I figured I can implement the rule relatively straightforwardly (#14895). If this rule ends up not being accepted, I am planning to copy it into our DevTools rules anyways. So the work won't be thrown away for me personally.

P.S. What are the odds that this issue has number 14859 and the PR I made number 14895. Made me chuckle 😄

Loading

@nzakas
Copy link
Member

@nzakas nzakas commented Aug 6, 2021

I'm reasonably certain this will be accepted, it just takes time to get in front of more of the team.

Loading

@nzakas
Copy link
Member

@nzakas nzakas commented Aug 26, 2021

@eslint/eslint-tsc still looking for approval here

Loading

@btmills btmills moved this from Feedback Needed to Pull Request Opened in Triage Aug 29, 2021
Triage automation moved this from Pull Request Opened to Complete Oct 22, 2021
@TimvdLippe
Copy link
Contributor Author

@TimvdLippe TimvdLippe commented Dec 1, 2021

I included this rule in DevTools today and it found a whole bunch of unused fields 🎉 https://crrev.com/c/3310610 I didn't encounter any further issues nor false positives, so I think we are good here 😄

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Triage
Complete
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants