Remove unused terms

[olafurpg]

scalac can report warnings on unused terms like this

[warn] /Users/ollie/dev/scalafix/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala:211: local val originalLines in method unifiedDiff is never used
[warn]     val originalLines = original.asString.lines.toSeq.asJava
[warn]         ^
[warn] /Users/ollie/dev/scalafix/scalafix-core/shared/src/main/scala/scalafix/patch/Patch.scala:211: local val originalLines in method unifiedDiff is never used
[warn]     val originalLines = original.asString.lines.toSeq.asJava
[warn]         ^

it would be nice if scalafix could automatically remove those definitions. Note that the right-hand side of the definition may contain side-effects so we can't remove those unless they are 100% side-effect free (for example literals)

[gabro]

I like this! We could also have an "aggressive" option that removes them indiscriminately, for users that are confident no side-effects happen on the rhs.

[olafurpg]

That should be doable.

Note that this rewrite will need to provide some escape hatch for cases like this in the scalafix codebase

[warn] <macro>:3: private val FilterMatcherReader in class PrintStreamReporter is never used
[warn]   private val FilterMatcherReader = null
[warn]               ^
[warn] one warning found

That statement is necessary to shadow another implicit, removing the line will cause a compilation error. What would be a sane escape hatch?

private val FilterMatcherReader = null // scalafix: ignore
private val FilterMatcherReader = null // This line is necessary because ...

[gabro]

Ugh, I didn't know you could do something like that :D In that specific case, why isn't FilterMatcherReader implicit?

Anyway, a general scalafix: ignore would probably be useful in some other cases.

[ShaneDelmore]

+1 to scalafix: ignore. It follows the style of wartRemover suppressWarnings, scalafmt ignore, scalastyle ignore, etc. All of these tools end up needing an escape hatch sooner or later, ignore seems like the popular way to do it. Just two weeks ago I tried to run scalafix on sbt codebase and ended up wanting to ignore a bunch of generated files that were in source with a //Do not modify by hand comment on them.

[olafurpg]

@ShaneDelmore do you think the escape hatch message should be configurable? In order to accommodate code generated code like you mention. Implementing the actual escape hatch should not be too difficult, I believe nailing the interface is harder.

[ShaneDelmore]

I don’t understand the question about configuring the escape hatch message, do you mean a message scalafix provides, or the comment to trigger skipping a rewrite?

If you mean various levels of escaping, I think that makes sense.
//scalafix: off. –disables scalafix for the rest of the file until
//scalafix: on is found
//scalafix: ignore –don’t scalafix this line (or method?) at all
//scalafix: ignore.RemoveUnused –Only ignore the RemoveUnused rewrite but apply any other matching rewrites.
//scalafix: ignore.{ RemoveUnused, ExplicitReturnTypes } –handle multiple ignores with a syntax similar to imports maybe?

That covers all of the cases I can think of right now.

[olafurpg]

That's exactly what I was looking for @ShaneDelmore Thank you. I agree it would be nice to have fine-grained control over how to disable scalafix.

I think it's possible to let comments at the end of statements disable the whole statement

foo(
  1,
  bar
) // scalafix: ignore

[olafurpg]

I opened #241 to track the escape hatch

[olafurpg]

Fixed in #367