Implement -Wvalue-discard warning

[cb372]

Warn when an expression e with non-Unit type is adapted by embedding it into a block { e; () } because the expected type is Unit.

TODO

• [ ] This could do with a few more tests. I'll look through the the Scala 2 tests and port any relevant ones
• [ ] The equivalent code in Scala 2 has a couple more boolean conditions that I don't fully understand the reasons for. I'd like to dig into that a bit more. Looking at the tests will probably shed some light on it.

[som-snytt]

It's documented somewhere, but Scala 2 avoids warning for methods that return this.type.

def append(x: X): Unit = buf.addOne(x)

The user can "annotate" that discarding is desired:

def f(): Unit = x.m(): Unit

That convention was devised before @nowarn and -Wconf, so I don't know what would be best now; the "explicit annotation" exception means a warning is never generated, instead of generating a warning that must be post-processed.

I'd suggest not supporting ancient compiler option aliases (the original -Ywarn).

If they want to support -Xlint (or -Wlint) as an option, it's worth considering calling it -Xlint:value-discard, where the distinction between -W and -Xlint is that -W warns about dubious language features or interactions (-Wnumeric-widen), whereas -Xlint warns about code that may be reasonable. (Maybe -Wvalue-discard is the better name under that criterion, who knows?)

[odersky]

This is a fine line to walk since lots of parts of the Scala library do return something that can safely be ignored. For instance put or remove on mutable collections, or the many functions that return this.

To make the test bullet-proof it would need to be augmented with effect checking. It makes indeed no sense to discard the value of a pure expression, but for side-effecting expressions it's common practice.

[SethTisue]

This is a fine line to walk since lots of parts of the Scala library do return something that can safely be ignored

Note that in Scala 2 these warnings aren't enabled by default, and they aren't enabled by -Xlint either. Users must explicitly opt-in, and it's expected that pure-functional coders are the main audience. Those coders are already avoiding "parts of the Scala library" such as mutable collections. (And in places where they must use those parts, they don't mind a little extra ceremony.)

the many functions that return this

I agree with Som that these are important to exempt.

[som-snytt]

Relatedly, -Wnonunit-statement resulted in narrowing the mutators of StringBuilder to this.type in https://github.com/scala/scala/pull/10047 which suggests that adding an annoying tool to the tool belt may result in useful behavior changes to avoid the annoyance.

My recent interaction with ConcurrentMap and the Java collection wrappers was a reminder that the half-life of a collections API may be half as long as previously estimated.

[gvolpe]

Apologies for rekindling this discussion, but I am currently finishing writing a Scala 3 book focused on purely functional programming, and I was wondering if there were any plans for Scala 3 to add support for -Ywarn-value-discard (the Scala 2 equivalent). This flag is enabled by default in sbt-tpolecat, a plugin used by most FP applications.

I've been following the work on #16157, but it seems this work is out of scope. Then Chris made me aware of this draft PR. I've also been pointed to the zerowaste plugin, ~~but unfortunately it doesn't seem to work~~ EDIT: After reaching out to the author, he made some fixes and it now works! More testing on other projects still required, though, if anyone would like to help test it.

I've been told this feature is planned to be implemented after 3.3, but I couldn't find any discussion about it. Is there any link related to this I can refer to in my book?

This flag is a must-have for production systems, and I believe it currently is the biggest blocker to Scala 3 adoption in FP shops.

[cb372]

I've ported over from Scala 2 the special-case handling of functions that return this.type, so I think this is now ready for a review.

[cb372]

Thank you! I was planning to address that comment but it was gradually moving further down my todo list 😄 so I'm happy for you to take over from here.