thrownBy results in "discarded non-Unit value" warnings

[jawshooah]

I add -Ywarn-value-discard to scalacOptions in my projects to help enforce a more explicit and less error-prone coding style.

Unfortunately, this results in compiler warnings whenever I use the an [Exception] must be thrownBy { ... } syntax in my tests. The thrownBy method of ResultOfBeWordForAnType expects an expression of type => Unit rather than => Any, so in order to suppress these warnings, I have to use syntax like { 1 / 0; () } or { val _ = 1 / 0 }.

This could be avoided by changing the method signature of thrownBy from (fun: => Unit): Unit to (fun: => Any): Unit. Interestingly enough, that's exactly the signature of the corresponding method of ResultOfBeWordForAType.

[bvenners]

Interesting. Happy to change that to Any, but what happens at the end of a test, which is also Unit?

[bvenners]

Actually, now that I think about it, not that we won't change it to Any, but Unit is actually more appropriate because thrownBy don't use the value. Unit says that whereas Any does not.

[jawshooah]

I'm not sure that ignoring the value of the expression makes Unit more appropriate. To me, Unit means Unit. The only reason the method works at all is because the compiler (dangerously) inserts a Unit value in place of the real value.

Besides, the thrownBy methods of Matchers, PureMatchers, ResultOfTheTypeInvocation, and ResultOfBeWordForAType all already expect an expression of type => Any rather than => Unit. ResultOfBeWordForAnType and ResultOfBeWordForNoException are different for no apparent reason.

[bvenners]

I'm curious why you think it is more dangerous to throw away an Any than it is a more specific type? Essentially, we don't need that value, so we throw it away. That's what Unit means. Perhaps could you give an example of when a value is thrown away accidentally that you'll get a warning for with -Ywarn-value-discard?

[jawshooah]

There's a reason the flag was introduced in the first place 😉

Here's one example.

Keep in mind that type signatures are not meant to express how a method will use arguments, but rather the type of arguments it requires, and the type of value it returns.

In this case, thrownBy accepts an argument of any type, so that should be expressed in the type signature. The fact that it doesn't actually use the argument except to evaluate it is incidental, and opaque to the user.

[jawshooah]

Also note that checkExpectedException, the method that each of these thrownBy methods call, expects an f: => Any. So the compiler goes to the trouble of inserting a Unit value for nothing!

[bvenners]

The other thing is that I measured it once, and Any compiled slightly faster than Unit. The problem in the gist you linked to is that side effects aren't type checked, and Unit means the method (or function) you are calling most likely is being executed only for a side effect, because it isn't returning anything. So the warning shows up if you are calling a method that takes Unit, but you don't do something that results in Unit (like forgetting to assign a variable).

Do you not sometimes get the -Ywarn-value-discard warning at the end of tests? Because regular (not fixture) style traits have Unit result type. Assertions currently have Unit result type, but in 3.0 have a Succeeded result type. But even now, if you end your test with an expression that produces anything but a Unit value, I think you'll get the warning.

[bvenners]

I just tried this and indeed 3.0.0-M11 would give lots and lots of warnings for traditional styles under -Ywarn-value-discard:

scala> class MySpec extends FunSuite {
     |   test("hi") { assert(1 + 1 == 2) }
     | }
<console>:11: warning: discarded non-Unit value
         test("hi") { assert(1 + 1 == 2) }
                        ^
defined class MySpec

The new "safe" styles won't have the problem because their result type is Assertion:

scala> class MySpec extends SafeFunSuite {
     |   test("hi") { assert(1 + 1 == 2) }
     | }
defined class MySpec

Of course if you really have something else at the end of the test you'll need to add a succeed, but that's by design. Value discarding is explicit in safe styles, which is the point of them:

scala> class MySpec extends SafeFunSuite {
     |   test("hi") { println() }
     | }
<console>:11: error: type mismatch;
 found   : Unit
 required: org.scalatest.Assertion
    (which expands to)  org.scalatest.Succeeded.type
         test("hi") { println() }
                         ^

scala> class MySpec extends SafeFunSuite {
     |   test("hi") { println(); succeed }
     | }
defined class MySpec

Thus I think I went in the wrong direction in my attempt to make the result type of the "classic" (existing in 1.0 and 2.0) styles by making them all ...Unit. I should probably make them all ...Any. Historically the result type of org.scalatest styles has been => Unit but org.scalatest.fixture styles has been FixtureParam => Any. In 3.0 I tried to make them consistent by making org.scalatest.fixture styles become FixtureParam => Unit, but I should probably go the other way: leave org.scalatest.fixture styles Any and make classic styles => Any.

[jawshooah]

Unit means the method (or function) you are calling most likely is being executed only for a side effect, because it isn't returning anything

That's exactly my point. thrownBy can be used with a value of any type, so it is a bit misleading to state in its type signature that it requires a value of type Unit, which implies a purely side-effecting function.

Do you not sometimes get the -Ywarn-value-discard warning at the end of tests?

Nope. I use WordSpec with MustMatchers, and all the assertions I'm using explicitly return Unit (x must be y, etc), so there's no value discarding.

Also, regarding the FunSuite example, I'm not sure how much has changed between 2.2.0 and 3.0.0-M11, but your code doesn't generate any warnings for me. Looks like on 2.2.0, FunSuite#test expects a => Unit and Assertions#assert returns Unit, so there's no problem.

[jawshooah]

I'm also currently locked into using 2.2.0, since I'm using Play 2.3.x with ScalaTest + Play 😢

[bvenners]

In 3.0.0, ScalaTest assertions now return the Succeeded singleton instead of the Unit singleton:

scala> assert(1 + 1 == 2)
res1: org.scalatest.Assertion = Succeeded

scala> 1 + 1 should equal (2)
res2: org.scalatest.Assertion = Succeeded

There's a type alias like this in the org.scalatest package object:

type Assertion = Succeeded.type

There's a set of async styles in which the result type of tests is Future[Assertion], which was the main motivation for changing the result singleton of assertions and matcher expressions. We added a set of safe styles in which the result type is Assertion, but left the tradtional styles at Unit so no code would break. However, turns out lots of code would at least start generating tons of warnings for people using -Ywarn-value-discard. So it isn't just thrownBy, but also test bodies of classic styles that should probaly be taking Any not Unit.

[jawshooah]

Gotcha, that makes perfect sense.

Any chance of backporting such a change to 2.2.x? And cutting a patch-level 1.2.x release of ScalaTest + Play?

[jawshooah]

Alternatively, is there a way to use the convenience traits from ScalaTest + Play with a newer version of ScalaTest? The changes discussed in this issue do me no good if I can't upgrade 😞

[bvenners]

We are very close to an RC1 release of 3.0.0. Doing mainly documentation and modularization now. But I think we could make a ScalaTest+Play release. That way we;d be moving forwards not backwards, and ScalaTest + Play (and then your project) would be a good test case. We'll give that a shot and I'll post here once it is out. May take a day or two.

[bvenners]

Actually, I just realized we should make the Unit to Any changes and do an M12 first. Otherwise you'll end up with all the warnings. So maybe 2 days, beacuse I'd like to try our work in progress modularization. Do you use any other style than WordSpec.

[jawshooah]

I think we could make a ScalaTest+Play release.

Awesome! Will there be a release for Play 2.3.x in addition to 2.4.x? I haven't yet done the refactoring necessary to upgrade.

Do you use any other style than WordSpec?

Not at the moment. All my specs derive from either PlaySpec (which itself derives from WordSpec) or my own custom UnitSpec:

abstract class UnitSpec extends WordSpec
  with MustMatchers
  with OptionValues
  with TryValues
  with Inside
  with Inspectors
  with ScalaFutures

[bvenners]

Sure, we can do a release for ScalaTest 3.0.0-SNAPx and Play 2.3.x, because I'd like you to try it in your project.

[bvenners]

Oddly enough a few hours after this conversation another user reported the same issue but on style traits:

https://groups.google.com/forum/#!topic/scalatest-users/mltBJPFiWiQ

I have deployed a 3.0.0-SNAP9 with the Units changed to Anys. The commit is here:

0c98b80

I need to leave now but will attempt to get a ScalaTest + Play release based on this out today too.

Bill

[bvenners]

Hi,

I released 3.0.0-SNAP11 with the ResultOfBeWordForNoException Unit changed to Any. Please give this a try in your project and let me know if it gets rid of the warnings.

Thanks.

Bill

[jawshooah]

Is there a corresponding ScalaTest + Play release compatible with Play 2.3.x?

[bvenners]

Not yet. I just released SNAP11. But my next task was going to be attempting to build a ScalaTest + Play snap release. Will post here once that's out.

[bvenners]

I've released ScalaTest + Play 1.5.0-SNAP1 for Scala 2.10 and 2.11. It intersects ScalaTest 3.0.0-SNAP11 and Play 2.3.0. Please give that a try and let me know if it removes those warnings.

[jawshooah]

Looks to be working fine! And no more discarded value warnings 😄

[bvenners]

Great, thanks for letting me know. I'll merge that over to 3.0.x once we have our modularization duck in a row, and it will be included in the next milestone, which with luck will be the last milestone release before RC1.