gvolpe / blog-comments

Principled Software Craftsmanship (source moved permanently to https://github.com/gvolpe/gvolpe.github.io)
https://gvolpe.com/blog/
MIT License
2 stars 1 forks source link

[Comments] - Scala 3: the missing compiler plugin #19

Open gvolpe opened 1 year ago

kubukoz commented 1 year ago

In the snippet:

val program: IO[Unit] =
  IO.ref(List.empty[Int]).flatMap { ref =>
    IO.println("performing critical work")
    ref.set(List.range(0, 11))
  }

I don't think -Ywarn-value-discard actually does anything. What you want is -Wnonunit-statement:

[warn] ./main.scala:10:5: unused value of type cats.effect.IO[Unit] (add `: Unit` to discard silently)
[warn]     IO.println("performing critical work")
[warn]     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

It's a relatively new feature, too (2.13.9 / 2.13.10), so probably not that much of a deal breaker when it's missing in Scala 3 :)

Value discard is this:

//> using scala "2.13.10"
//> using option "-Ywarn-value-discard"
import cats.effect.IO

object demo {

  val program: IO[Unit] = IO.ref(List.empty[Int]).map { ref =>
    ref.set(List.range(0, 11))
  }
}
[warn] ./main.scala:9:5: discarded non-Unit value of type cats.effect.IO[Unit]
[warn]     ref.set(List.range(0, 11))
[warn]     ^^^^^^^^^^^^^^^^^^^^^^^^^^
gvolpe commented 1 year ago

Oh that's definitely a new name for it, I haven't used Scala 2 in a while :)

ghik commented 1 year ago

Interesting, none of the standard Scala options work for me.

//> using scala "2.13.10"
//> using option "-Wvalue-discard"
//> using option "-Wnonunit-statement"

object foo {
  val x: Unit = {
    "first".trim
    "second".trim
  }
}
❯ scala-cli compile foo.scala
Compiling project (Scala 2.13.10, JVM)
[warn] ./foo.scala:4:5: discarded non-Unit value of type String
[warn]     "second".trim
[warn]     ^^^^^^^^^^^^^
Compiled project (Scala 2.13.10, JVM)

None of these options report a warning on the "first".trim expression. Zerowaste plugin reports them both.

kubukoz commented 1 year ago

Seems specific to trim being a Java method (or something). Using a Scala-defined one emits warnings:

//> using scala "2.13.10"
//> using option "-Wnonunit-statement"
//> using option "-Ywarn-value-discard"

object demo {

  implicit class StringOpsss(s: String) {
    def trimmed: String = s.trim
  }

  val s: Unit = {
    "foo".trimmed
    "bar".trimmed
  }
}
scala-cli compile .
[warn] ./foo.scala:13:5: unused value of type String (add `: Unit` to discard silently)
[warn]     "bar".trimmed
[warn]     ^^^^^^^^^^^^^
[warn] ./foo.scala:12:5: unused value of type String (add `: Unit` to discard silently)
[warn]     "foo".trimmed
[warn]     ^^^^^^^^^^^^^
[warn] ./foo.scala:13:5: discarded non-Unit value of type String
[warn]     "bar".trimmed
[warn]     ^^^^^^^^^^^^^
objektwerks commented 1 year ago

Using zerowaste, I caught 2 errors in a ZIO project. Well done!

ghik commented 1 year ago

@kubukoz yes, it looks like the compiler ignores Java methods by design

kubukoz commented 1 year ago

ugh. I wish I could opt out of that (not all Java methods are side-effecting!)

SethTisue commented 1 year ago

ugh. I wish I could opt out of that (not all Java methods are side-effecting!)

I believe a Scala 2 PR adding that as an additional opt-in flag would be accepted. (I think @som-snytt's decision about the default behavior here was correct, which is why I'm suggesting an additional flag here, rather than suggesting changing the default.)

SethTisue commented 1 year ago

This is just a nitpick, but note that -Ywarn-value-discard is a legacy name and -Wvalue-discard is the recommended name.

som-snytt commented 1 year ago

Thanks for advertising linting; I just commented on the other social media that I think all warnings should be external: as motivation, the scala 3 warning effort is pushed to a phase, so there is no gain in efficiency or correctness; PRs like my previous version of unused imports for scala 3 sit ignored on the queue or at best await the next release cycle. There is a long history ("Abide") of wishing to push warnings to a third party.

I have fixes for -Wnonunit-statements, I'd be glad to field feature suggestions. I am not opinionated.

I didn't know about zero waste, otherwise I would have named the option --zero-waste.

I agree that the previous title was problematic. In fact, I would have suggested inverting the lede as an ad for third-party linting.

There is a mistaken idea that "standard plugins" should be "made official" by ingestion, such as -explain for splain. Perhaps the "toolkit" approach will allow the "core ecosystem" to be more decoupled in future.

som-snytt commented 1 year ago

@SethTisue Thanks again, you pick all the right nits. Note that -Y connotes "private option" or perhaps "forking option" (Y looks like a fork in the road). -W serves us better. (But see my opinion about "zero warn" compiler. Plugins need a way to "own" options.)