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
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects

Comments

@TimvdLippe
Copy link
Contributor

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.

@TimvdLippe TimvdLippe added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules triage An ESLint team member will look at this issue soon labels Jul 30, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jul 30, 2021
@nzakas
Copy link
Member

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?

@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Aug 2, 2021
@nzakas nzakas removed the triage An ESLint team member will look at this issue soon label Aug 2, 2021
@TimvdLippe
Copy link
Contributor Author

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

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

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.

@mdjermanovic
Copy link
Member

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.

@snitin315
Copy link
Contributor

Makes sense to add this rule 👍

@nzakas
Copy link
Member

nzakas commented Aug 4, 2021

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

Makes sense to me.

@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

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 😄

@mdjermanovic mdjermanovic added feature This change adds a new feature to ESLint and removed enhancement This change enhances an existing feature of ESLint labels Aug 6, 2021
@nzakas
Copy link
Member

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.

@nzakas
Copy link
Member

nzakas commented Aug 26, 2021

@eslint/eslint-tsc still looking for approval here

@btmills btmills added the accepted There is consensus among the team that this change meets the criteria for inclusion label Aug 26, 2021
@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

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 😄

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Apr 21, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Apr 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

No branches or pull requests

5 participants