Add -Wnonunit-statement to warn about discarded values in statement position

[som-snytt]

Summary

New compiler option -Wnonunit-statement warns about any interesting expression whose value is ignored because it is followed by another expression.

The new warnings are not included in -Xlint; they must be separately enabled with -Wnonunit-statement. Users of -Wnonunit-statement may also wish to enable a related option, -Wvalue-discard.

Example

scala> :settings -Wnonunit-statement
scala> Some(3); "hello"
           ^
       warning: unused value
val res0: String = hello

How to silence

The warning can be suppressed at a particular site by explicit ascription: e: Unit. This is the same convention used to ignore warnings about "value discard" conversions.

As always, any warning is also suppressible with @nowarn.

Relation to other warnings

This generalizes the current warning for "pure" expressions, which only warns for limited cases where the expression can be proved not to be side-effecting.

-Wvalue-discard remains separate. It warns only when a value is discarded because it's the final expression in a block whose expected type is Unit, as specified by the language.

Details on what warns or doesn't

"Statement position" means statements in templates (i.e., constructors) and statements in blocks (everything in a block except the result expression). If a statement is an expression (and not a definition or assignment), then -Wnonunit-statement warns if the type of the expression is not Unit. There are heuristic exceptions, such as for methods that return this.type, where there is no loss of information. (The method is inherently side-effecting.) There is an additional flag whether to take an if statement as side-effecting, so that its result can be ignored.

By default, warns about if without else. Specify -Wnonunit-if:false to turn that off. (That also turns off -Wvalue-discard for those cases.)

The heuristic for this.type-returning methods intends to accommodate usual idioms updating unstable values.

Motivation

As requested on Discord!

This is especially useful in pure-functional code where it's unusual for values ever to be intentionally discarded.

[som-snytt]

Update to handle Future(42): Unit where the attachment is on the user expression, but we're linting the implicit Future(42)(ec) in { Future(42)(ec) ; () }.

Typing the user expression yields the block, so additional work would be needed to copy attachments from the explicit apply to the implicit.

[SethTisue]

I haven't seen so many yellow thumbs since my last all-day Simpsons-watching marathon. Looks like this will be popular if you can finish it.

[som-snytt]

It's a bit niggling, but the warm reception means I can't stop at mile 18 despite the pain in my side.

[som-snytt]

It would be nice to provide an audit of Unit ascriptions that suppress a warning (or respectively suppress nothing). Maybe -Y.

Also a sibling option for nowarn. Otherwise, once you suppress, there is no opportunity to revisit. Grepping is harder than for nowarn usages.

I just needed that feature, A method was used a few times but only one warned because I'd previously slapped on the Unit.

What should the output look like? Just turning off suppression could result in many errors. Possibly specify a region of audit using -opt:inline syntax?

Or a general mechanism -Yaudit-nowarn where all suppressions are gathered, then reported by type and location.

[som-snytt]

This will require re-tweaks, as it still yields what I assume are spurious warnings on the standard library.

hashMapBuilder.addAll(xs)

Probably because it doesn't have a getter.

private[this] var hashMapBuilder: HashMapBuilder[K, V] = _

[som-snytt]

Som day, Som day.

https://www.youtube.com/watch?v=UiF0FMs16dU

[SethTisue]

youtube.com/watch?v=UiF0FMs16dU

🎵 you gave me no warnin’ 🎵

[lrytz]

Question: this warns - should it count as a Unit ascription?

class C {
  def f(x: Int): Int = 1 + x
  def g: Unit = f(0)
}

[lrytz]

This part I don't understand... I don't see anything about var or addOne.

IIUC, it catches any method call on a field, no?

How about calls on local values / variables?

Also addOne and += are defined with return type this.type - why do you need to handle them separately?

[som-snytt]

If a value is not stable, in v().addOne(x), I can witness different values in v() and the result. I think that is the gist of why, even if addOne is side-effecting and returns this, I've lost the value produced by v; I can't assume I can recover it by calling v again.

I see a test class Variant. I'll add more variants to ensure this check is not too wide a net. I think it applies only to this-typed methods. But is that even correct? I'm sure I noticed it by testing on this code base. But maybe v.addOne(x) is bad form.

[lrytz]

👍 thanks. what i missed is that we're already narrowed down to a method with a this.type / singleton type here.

[armanbilge]

I just took this for a spin on Skunk. Only two (spurious?) warnings!

https://github.com/tpolecat/skunk/blob/14c756d68ba88da4415699bea388149c43e4e962/modules/core/shared/src/main/scala-2/syntax/StringContextOps.scala#L79-L82

[error] /workspace/skunk/modules/core/shared/src/main/scala-2/syntax/StringContextOps.scala:79:36: unused value
[error]       val EncoderType      = typeOf[Encoder[_]]
[error]                                    ^
[error] /workspace/skunk/modules/core/shared/src/main/scala-2/syntax/StringContextOps.scala:81:36: unused value
[error]       val FragmentType     = typeOf[Fragment[_]]
[error]                                    ^
[error] two errors found

Seems like the issue is the _, if I change it to Any then the warning goes away.

[som-snytt]

The Skunk warning is due to this call in the expansion. I added a commit that tweaks the signature, which may pass MiMA. There are probably other bits of reflect API that deserve similar tweaking.

$u.internal.reificationSupport.setInfo[$u.Symbol](symdef$EncoderType1, $u.NoType);

[som-snytt]

@lrytz As to whether explicit result type means "I intend to discard on RHS", I think the answer is no, because -Wvalue-discard warns for this case.

Explicit result types are supplied often: to specify API, or to accommodate recursion.

I like the "transposed" form:

def g = f(42): Unit

which is somewhat equivalent but does show "intention to discard", and also is not redundant.

[lrytz]

👍 thanks. what i missed is that we're already narrowed down to a method with a this.type / singleton type here.

[SethTisue]

needs rebase

@som-snytt is #9893 (comment) considered resolved?

[som-snytt]

Since I have to rebase anyway, sigh, I'll add a code comment where lrytz had to read it twice. I had to read it more than that.

Edit: instead of a comment, added self-documenting method name.

[SethTisue]

needs checkfile update, looks like. once CI likes it let's merge

[som-snytt]

because the other warning jumped the queue, need to update on the first commit

!!    1 - neg/nonunit-if.scala                      [output differs]
% diff /home/jenkins/workspace/scala-2.13.x-validate-main/test/files/neg/nonunit-if.check /home/jenkins/workspace/scala-2.13.x-validate-main/test/files/neg/nonunit-if-neg.log
@@ -1,7 +1,7 @@
-nonunit-if.scala:58: warning: discarded non-Unit value
+nonunit-if.scala:58: warning: discarded non-Unit value of type U
     if (!isEmpty) f(a)      // warn, check is on
                    ^
-nonunit-if.scala:62: warning: discarded non-Unit value
+nonunit-if.scala:62: warning: discarded non-Unit value of type Boolean
       f(a)                  // warn, check is on
        ^
 nonunit-if.scala:13: warning: unused value

[SethTisue]

🎉

[sideeffffect]

Is there a chance that we will have something like this in Dotty?

[SethTisue]

@sideeffffect thread is https://contributors.scala-lang.org/t/replacement-for-xlint-in-scala3/5241