#238 Rule to remove unused local val and var #367
Conversation
There was a problem hiding this comment.
Thank you for your contribution @DanielaSfregola! 🙏
I left a few comments ;)
| } | ||
|
|
||
| override def fix(ctx: RuleCtx): Patch = { | ||
| ctx.debugIndex() |
There was a problem hiding this comment.
Yes, my bad! Removing it
| object RemoveUnusedTerms { | ||
|
|
||
| def foo { | ||
|
|
There was a problem hiding this comment.
You may want to remove the leading spaces including the newline (like I did in my presentation from yesterday)
There was a problem hiding this comment.
We definitely need a ctx.removeLeading or something
There was a problem hiding this comment.
Yes, I suggested at the spree that we leave the leading space unchanged until we add better utilities to handle this.
| println(1) | ||
| } | ||
|
|
||
| val unused = 0 |
There was a problem hiding this comment.
As far as I could see, this wasn't flagged by the compiler as unused. But I have to admit this looks a bit weird. // cc @olafurpg ?
There was a problem hiding this comment.
This is a globally accessible val, it could be used by another project.
| private def removeLiterals(rhs: Term): String = | ||
| rhs match { | ||
| case Lit(_) => "" | ||
| case r => r.syntax |
There was a problem hiding this comment.
You're potentially modifying the syntax of the original rhs here, I would probably change approach and produce an Option[Patch]
There was a problem hiding this comment.
I propose that we only remove the necessary tokens (val/identifier/=) and leave the rhs unchanged, there may be other rules touching the tokens inside the rhs.
There was a problem hiding this comment.
Thank you for this contribution @DanielaSfregola ! If we can minimize the number of removed tokens, that is, only remove val x = in val x = rhs.blah(..) instead of replacing all tokens then I think we are good to merge this 😄
If you are feeling ambitious, we can also handle a few other unused cases such as unused parameters. But we can leave that for a separate PR
| private def removeLiterals(rhs: Term): String = | ||
| rhs match { | ||
| case Lit(_) => "" | ||
| case r => r.syntax |
There was a problem hiding this comment.
I propose that we only remove the necessary tokens (val/identifier/=) and leave the rhs unchanged, there may be other rules touching the tokens inside the rhs.
| override def fix(ctx: RuleCtx): Patch = { | ||
| ctx.debugIndex() | ||
| ctx.tree.collect { | ||
| case i: Defn.Val if isUnused(i) => ctx.replaceTree(i, removeLiterals(i.rhs)) |
There was a problem hiding this comment.
It requires a bit more work, but as I mention above, I think it makes sense to ctx.removeToken only for the tokens that should be removed.
There was a problem hiding this comment.
Maybe something like i.tokens.filterNot(i.rhs.tokens.contains).map(ctx.removeToken) might do the trick.
| object RemoveUnusedTerms { | ||
|
|
||
| def foo { | ||
|
|
There was a problem hiding this comment.
Yes, I suggested at the spree that we leave the leading space unchanged until we add better utilities to handle this.
| println(1) | ||
| } | ||
|
|
||
| val unused = 0 |
There was a problem hiding this comment.
This is a globally accessible val, it could be used by another project.
There was a problem hiding this comment.
LGTM 👍 Great work @DanielaSfregola !
Only need to fix the build on 2.11 and then we're good to merge :)
build.sbt
Outdated
| @@ -306,7 +306,7 @@ lazy val testsInput = project | |||
| scalacOptions += s"-P:semanticdb:sourceroot:${sourceDirectory.in(Compile).value}", | |||
| scalacOptions ~= (_.filterNot(_ == "-Yno-adapted-args")), | |||
| scalacOptions += "-Ywarn-adapted-args", // For NoAutoTupling | |||
| scalacOptions += "-Ywarn-unused-import", // For RemoveUnusedImports | |||
| scalacOptions += "-Ywarn-unused", // For RemoveUnusedImports and RemoveUnusedTerms | |||
There was a problem hiding this comment.
Seems we need to keep -Ywarn-unused-import for 2.11 https://travis-ci.org/scalacenter/scalafix/jobs/278586505
| unusedTerms.contains(defn.pos) | ||
|
|
||
| private def removeDeclarationTokens(i: Defn, rhs: Term): Tokens = { | ||
| val startDef = i.tokens.start |
There was a problem hiding this comment.
TIL I have not noticed Tokens.start 😄
There was a problem hiding this comment.
Yeah, I struggled a bit with this function as Tokens.filter returns a IndexSeq[Token] rather than a Tokens. Also, the constructor of Tokens is private to the package meta, so writing something like
i.tokens.filterNot(rhs.tokens.contains)
would have been nicer but unfortunately it didn't quite work :(
|
Thank you! 🎆 |
This PR implements the rule described in #238.
In the specific, it removed unused local vals and vars. If litteral expressions are involved, the whole expression is removed, otherwise only the val definition is removed.
I'll try to send another PR for unused def/trait/classes in the near future.