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

value-discard warning is not suppressed for methods with implicit params #12011

Closed
enzief opened this issue May 20, 2020 · 9 comments · Fixed by scala/scala#10358
Closed

value-discard warning is not suppressed for methods with implicit params #12011

enzief opened this issue May 20, 2020 · 9 comments · Fixed by scala/scala#10358

Comments

@enzief
Copy link

enzief commented May 20, 2020

reproduction steps

using Scala (2.13.2),

scala> def foo(implicit x: Int): Boolean = x % 2 == 1
def foo(implicit x: Int): Boolean

scala> implicit val i = 1
val i: Int = 1

scala> def f(): Unit = foo: Unit
                       ^
       warning: discarded non-Unit value
def f(): Unit

problem

expect no warning: discarded non-Unit value as per scala/scala#7563

explicitly passing implicit argument works

scala> def f(): Unit = foo(2): Unit
def f(): Unit
@som-snytt
Copy link

The adapted f(x) doesn't receive the magic annotation on f.

[adapt] (implicit i: scala.this.Int): scala.this.Int adapted to { T.this.f(x); () } based on pt scala.this.Unit

@lrytz lrytz added the lint label Jun 5, 2020
@lrytz lrytz added this to the Backlog milestone Jun 5, 2020
@steinybot
Copy link

This is a real pain in the neck when using ScalaTest. For example:

import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers

class ShouldSpec extends AnyFlatSpec with Matchers {

  behavior of "Foo"

  it must "do foo" in {
    123.shouldBe(an[Int]): Unit
    123.shouldBe(an[Int])
  }
}

I'm not even sure why. shouldBe is overloaded but this overload doesn't have any implicit parameters (it is a macro though):

    def shouldBe(anType: ResultOfAnTypeInvocation[_]): Assertion = macro TypeMatcherMacro.shouldBeAnTypeImpl

There are other overloads that do have implicit parameters. I tried reproducing this without using ScalaTest and was unable to.

@steinybot
Copy link

I should point out that my example is not for value-discard but nonunit-statement although I believe the suppression mechanism should work the same.

@steinybot
Copy link

I also don't know of a workaround for my case. Specifying the implicit parameter is not an option because there is no implicit parameter.

@som-snytt
Copy link

som-snytt commented Mar 29, 2023

I believe the suppression mechanism should work the same.

They are different. I think there is a ticket somewhere for ScalaTest. Perhaps on sbt-tpolecat of all places. I hope to update nonunit-statement but there is not a tracking ticket.

typelevel/sbt-tpolecat#134

Edit: by mechanism I understood mechanism and not syntax. Yes, I think the syntax for expressing "I mean Unit here" is the same.

@som-snytt
Copy link

It seems this was fixed.

➜  scala -Wvalue-discard
Welcome to Scala 2.13.10 (OpenJDK 64-Bit Server VM, Java 19).
Type in expressions for evaluation. Or try :help.

scala> def foo(implicit x: Int): Boolean = x % 2 == 1
def foo(implicit x: Int): Boolean

scala> implicit def i: Int = 42
def i: Int

scala> foo(42)
val res0: Boolean = false

scala> foo
val res1: Boolean = false

scala> def u: Unit = foo
                     ^
       warning: discarded non-Unit value of type Boolean
def u: Unit

scala> def u: Unit = foo: Unit
def u: Unit

scala>

@som-snytt
Copy link

Actually the code to check for the annotation was updated by nonunit-statement PR so they share that much. That was 2.13.9. But the false positives for test frameworks is ongoing, as far as I know.

@steinybot
Copy link

Should I create another issue for the nonunit-statement problem?

@som-snytt
Copy link

@steinybot please feel free!

@SethTisue SethTisue added this to the 2.13.9 milestone Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants