Allow suppressing value-discard warning via type ascription to Unit

[som-snytt]

Extend (e: Unit) to mean no-warn for both value discard and expression in statement position warnings.

[dwijnand]

Nice one. Thanks, Som.

[dwijnand]

/rebuild 38d0b82

[dwijnand]

Going to rebase to give my commit a fresh SHA (can't get the CLA validation to pass...) and to squash Nth's suggestion.

[som-snytt]

I'm going to stop checking the box that allows pushy pushes.

It's nowarn because of -nowarn. Consult the state of the art.

[dwijnand]

Apologies. It was "no warn" on the line below so I assumed it wasn't intentional.

[som-snytt]

@dwijnand I was just rationalizing. Happy New Year to U both. I can't wait to use Using and also Unit attribution. I can't use my shiny new toys until Jenkins comes out of hibernation, of course.

[adriaanm]

All I wanted for Xmas was a happy jenkins. The socks are nice too, I guess. (See also scala/scala-dev#594)

[dwijnand]

Review by @scala/team-core-scala?

[NthPortal]

@dwijnand oops, looks like @adriaanm removed that team

[SethTisue]

we're "@scala/lightbend" now, I see

[adriaanm]

Let's use @scala/committers for that -- the lightbend team is probably only useful for yelling at us :-p

[dwijnand]

Thanks for the review and merge (and the pairing session!)

[maphi]

Just saw this merge and i thinks it's not a good idea to use this syntax for explicit discarding.

1. : Unit would behave differently than any other type

"a": String // compiles - fine
"a": Int //does not compile - fine
"a": Unit // returns Unit & compiles - strange

2. How would one check that something is actually Unit at compile time?
3. Would this fail to compile:
   Future(Future(someStuff)).map((_: Unit) => ())

[adriaanm]

Note that this PR only influences whether we emit an optional warning.

Even before this PR, "a": Unit would compile and evaluate as you described (albeit with a warning that you opted in to). Now the value discarding warning is suppressed because you've explicitly ascribing : Unit, which seems like a good indication you know what you're doing and don't want the warning.

[adriaanm]

To answer 2.:

( () : Any) match { case u : Unit => }

[maphi]

Hm i think : is widely used for upcasting or typechecking, or just as additional ascription for the human reader to give a hint which types are involved. Unit now behaves totally different.

What would happen when writing this:

val myValue = 5: Unit

myValue match {
  case _: Int => println("is an int")
  case _: Unit => println("is unit")
}

And to you answer for 2. :

( 5 : Any) match { case u : Unit => } // this would give compile error?

[valenterry]

Agreeing with @maphi here. To my knowledge, value discarding has always been just a quirk to help those coming from the Java void world. But Unit is not void. I question the whole value-discarding construct.
If we really want to have such a construct in Scala (and I think we don't), then at the very least generalize it. Make it so that every type (also Custom types like Akkas Done) with only one value can be used with the value-discard feature.

However, I vote for not cementing the decision of the value-discard feature. This special rule is not worth the gain at all. If possible, it should be removed.

[adriaanm]

I think there must be a misunderstanding here somewhere. Again, this PR is about warning suppression. Warnings do not influence the outcome of type checking or codegen.

Also, a type ascription is not an upcast. It's also not just a hint -- it tells the compiler which type the ascribed expression has to meet, and this may trigger conversions (e.g. value discarding, implicit conversion or numeric widening).

[maphi]

Ok agreeing that value discard is not different from implicit conversion. For me it feels like this MR takes same safety apart to not accidently discard something by ascribing it with : Unit. E.g. there might have been a method returning Unit but after refactoring returning Future[Unit] and a : Unit ascription at use site would discard that Future. But i see this is a rare case.

A good way might be what @valenterry said. Configuring if value discard should happen at all would solve this.

How is this implemented right now. Is there an implicit conversion Any -> Unit in Predef?

[adriaanm]

It's in the spec: https://www.scala-lang.org/files/archive/spec/2.12/06-expressions.html#value-discarding, implementation:

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

Lines 1119 to 1121 in 38cd84d

	pt.dealias match {
	case TypeRef(_, UnitClass, _) if anyTyped => // (12)
	warnValueDiscard() ; tpdPos(gen.mkUnitBlock(tree))

[som-snytt]

I agree that there is some confusion about the mechanisms involved, but I understand that folks are confused by Future[Unit]. I'll explore whether -Ywarn-value-discard:strict would help by turning off the escape hatches. (Disabling value discard conversion is not an option. Pun intended.) Strict mode would be there if you needed it.

As an example of what's been implemented previously, it doesn't warn when discarding this.type:

scala 2.13.0-M5> import collection.mutable
import collection.mutable

scala 2.13.0-M5> val g: mutable.Growable[Int] = mutable.ListBuffer(0)
g: scala.collection.mutable.Growable[Int] = ListBuffer(0)

scala 2.13.0-M5> def f(i: Int): Unit = g += i
f: (i: Int)Unit

scala 2.13.0-M5> def f(i: Int): Unit = g += i  //print
   def f(i: scala.Int): scala.Unit = {
  g.+=(i);
  ()
} // : <notype>

The current change is eliminate these warnings:

scala 2.13.0-M5> 42: Unit
                 ^
                 warning: discarded non-Unit value
                 ^
                 warning: a pure expression does nothing in statement position

The test example is ignoring the boolean returned by set.remove(element). Obviously, this trick is in lieu of SupressWarnings.

[som-snytt]

There's an old thread where I commented naively. There the problem was unwittingly Future[Unit](Future(e)).

There's a blog about unwittingly val f: Future[Unit] = for (x <- Future(42)) yield Future(x).

(Edit: internet search engine reminded me scala/bug#9822)

-Ywarn-value-discard warns in these cases, even if you Future(x): Unit!

That's because of the implicit arg, but I haven't looked closely at what it's doing yet:

scala> def f(s: String)(implicit n: Int) = s * n
f: (s: String)(implicit n: Int)String

scala> f("x")(10)
res3: String = xxxxxxxxxx

scala> { implicit val i: Int = 10 ; f("x") }
res4: String = xxxxxxxxxx

scala> { implicit val i: Int = 10 ; val u: Unit = f("x") }
                                                   ^
       warning: discarded non-Unit value

scala> { implicit val i: Int = 10 ; val u: Unit = f("x"): Unit }
                                                   ^
       warning: discarded non-Unit value

scala> { implicit val i: Int = 10 ; val u: Unit = f("x")(10): Unit }

Probably the tree attachment is lost during a tree copy during adaptation.

Anyway, strict mode is at #7716