Add `-Wnonunit-statement` to warn about discarded values in statement position #9893

som-snytt · 2022-01-05T19:15:52Z

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 · 2022-01-05T20:21:47Z

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 · 2022-01-19T03:25:39Z

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 · 2022-01-19T03:50:49Z

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 · 2022-06-08T17:47:26Z

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 · 2022-06-09T22:13:26Z

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] = _

SethTisue · 2022-07-25T20:54:30Z

It still nags at me that the distinction between -Wvalue-discard and this seems rather academic. How many actual users would want to enable -Wvalue-discard without also enabling this? In other words: shouldn't the functionality of -Wnonunit-statement simply be folded into -Wvalue-discard? (Perhaps with an opt-out if someone really wants to split this hair, comparable to the -Wnonunit-if:false opt-out you added.)

(Less importantly, I'm having doubts about whether we want to use the word "statement" in the flag name and documentation. And namingwise, I guess "statement" bothers me because it seems like importing terminology from other programming languages outside of the functional tradition. In Scala, a few things like val and import actually are statements... everything else is just expressions.)

som-snytt · 2022-07-25T22:22:53Z

I would distinguish -Wvalue-discard only because it is well-defined and less prone to heuristic false positives.

They are statements when in statement position. The spec for "block" calls them statements, for example. Maybe time for a neologism. Blockpression. Exstatement.

I'll tear myself away from Fringe season 5 to update the message with the fine suggestions to suggest something.

Edit: OK, I figured out that I was confusing Georgina Haig from Fringe and Katherine McNamara from Arrow, they both play daughter heroines in the future.

SethTisue · 2022-07-25T23:00:14Z

I would distinguish -Wvalue-discard only because it is well-defined and less prone to heuristic false positives

Okay, I'll take that (but perhaps we'll merge them Someday™️).

The spec for "block" calls them statements

Touché. I yield! (https://scala-lang.org/files/archive/spec/2.13/06-expressions.html#blocks)

som-snytt · 2022-07-26T00:12:15Z

Som day, Som day.

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

SethTisue · 2022-07-26T03:17:50Z

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 · 2022-07-26T09:01:16Z

src/reflect/scala/reflect/internal/TreeInfo.scala

+              receiver match {
+                case Apply(_, _) => Precedence(op.decoded).level == 0         // xs(i) += x
+                case _ => receiver.symbol != null &&
+                  (receiver.symbol.isGetter || receiver.symbol.isField)       // xs.addOne(x) for var xs


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?

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.

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

armanbilge · 2022-07-26T14:54:25Z

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 · 2022-07-27T15:34:28Z

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 · 2022-07-27T15:41:18Z

@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.

src/compiler/scala/tools/nsc/typechecker/RefChecks.scala

lrytz · 2022-07-28T08:20:56Z

src/reflect/scala/reflect/internal/TreeInfo.scala

+              receiver match {
+                case Apply(_, _) => Precedence(op.decoded).level == 0         // xs(i) += x
+                case _ => receiver.symbol != null &&
+                  (receiver.symbol.isGetter || receiver.symbol.isField)       // xs.addOne(x) for var xs


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

SethTisue · 2022-07-28T15:13:40Z

needs rebase

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

som-snytt · 2022-07-28T15:39:45Z

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 · 2022-07-29T00:27:04Z

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

som-snytt · 2022-07-29T00:46:21Z

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

Warn about any interesting value in statement position, for some definition of interest. Generalizes the current warning for "pure" expressions. By default, warns about unibranch if, which lack an else in user code. Specify -Wnonunit-if:false to turn off warning about unibranch if. (That also turns off -Wvalue-discard for those cases.) Improve explicit Unit check Accommodate field when method is this.type Handle Match

SethTisue · 2022-07-29T05:11:59Z

🎉

sideeffffect · 2022-07-29T12:55:19Z

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

SethTisue · 2022-07-29T14:23:50Z

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

scala-jenkins added this to the 2.13.9 milestone Jan 5, 2022

som-snytt force-pushed the topic/warn-nonunit-statement branch from abe01d8 to ac7478d Compare Jan 5, 2022

som-snytt marked this pull request as ready for review Jan 5, 2022

som-snytt marked this pull request as draft Jan 5, 2022

SethTisue added the release-notes worth highlighting in next release notes label Jan 5, 2022