Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some test specs trigger compiler bug which eats warnings #2056

Open
blast-hardcheese opened this issue Aug 9, 2021 · 5 comments
Open

Some test specs trigger compiler bug which eats warnings #2056

blast-hardcheese opened this issue Aug 9, 2021 · 5 comments

Comments

@blast-hardcheese
Copy link

In attempting to track down why an obviously invalid construction was a runtime error instead of a type error, I stumbled upon scala/bug#12441 .

A minimal reproduction is included in the linked repo, but I'll additionally talk about a workaround here.

The core issue seems to come from when values that are attempting to emit a warning are coerced to Unit, wherein their warnings are suppressed:

object Bogus {
  Option.empty[String].contains(1234) // Emits a warning

  def should(func: => Unit): Unit = func

  should {
    Option.empty[String].contains(1234) // Does not warn
  }
}

An interesting observation is that this only happens at the block site, so a perfectly valid workaround for this is to apply:

diff --git a/src/main/scala/Bogus.scala b/src/main/scala/Bogus.scala
index a94fb91..7304326 100644
--- a/src/main/scala/Bogus.scala
+++ b/src/main/scala/Bogus.scala
@@ -1,10 +1,10 @@
 object Bogus {
   Option.empty[String].contains(1234) // Emits a warning
 
-  def should(func: => Unit): Unit = func
+  def should[A](func: => A): Unit = func
 
   should {
-    Option.empty[String].contains(1234) // Does not warn
+    Option.empty[String].contains(1234) // This now emits a warning!
   }
 
   println(Option.empty[String].contains(1234)) // does not warn

Impacted specs

I copied the different spec examples from Selecting a Style and applied the offending Option.empty[String].contains(1234) in each of the test blocks (gist). What I found was that not all tests exhibited this bug:

Bug No Bug Partial Bug
AnyFlatSpec AnyFunSuite
AnyWordSpec AnyFunSpec
AnyFreeSpec AnyPropSpec

Resolution

I think a reasonable course of action here would be to sprinkle some unbounded type parameters on the functions that currently accept Unit. Unfortunately, my tests show that it's not possible to have both def should[A](fun: => A): Unit and def should(fun: => Unit): Unit visible at the same time, so care will need to be taken to ensure bincompat.

@blast-hardcheese
Copy link
Author

This change would also resolve #1717, though that's a different situation.

@cheeseng
Copy link
Contributor

@blast-hardcheese Interesting, thanks for the detailed analysis, I'll look at the style traits to try to understand why some have problem while some do not.

@cheeseng
Copy link
Contributor

@blast-hardcheese I did a quick try and your workaround did work, but ideally, imho this is best fixed in the scala compiler, hopefully, it will be fixed, :)

@bvenners
Copy link
Contributor

Rather than an unbounded and unused type parameter, I wonder if changing Unit to Any would solve the problem. We've done that in other places, so maybe we missed a few spots.

@blast-hardcheese
Copy link
Author

@bvenners

I wonder if changing Unit to Any would solve the problem

I don't believe so -- in my reproduction repo, both cases are actually represented:

  • should(func: Unit): Unit
  • println(x: Any): Unit

That being said, nothing wrong with being explicit, so I updated the sample repo to include what I believe to be representative of your suggestion: blast-hardcheese/scala-bug-xlint-infer-any@cd131ea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants