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

Inconsistent behavior of -Ywarn-value-discard flag #11379

Closed
ChernikovP opened this issue Jan 23, 2019 · 6 comments · Fixed by scala/scala#10102
Closed

Inconsistent behavior of -Ywarn-value-discard flag #11379

ChernikovP opened this issue Jan 23, 2019 · 6 comments · Fixed by scala/scala#10102
Assignees
Labels
Milestone

Comments

@ChernikovP
Copy link

It looks like -Ywarn-value-discard flag doesn't always warn on value discarding:

bos-pchernikov-mac:personal pchernikov$ cat UnitOfTrust.scala 
object UnitOfTrust {
  import scala.util._

  private def unitRight[A]: Either[A, Unit] = Right(())

  // fails with:
  //   discarded non-Unit value
  // def test1: Either[Int, Unit] = Right(Right(()))
  // def test2: Either[Int, Unit] = Right(()).map(_ => unitRight[Int])
  // def test3: Either[Int, Unit] = Right(()).map { case _ => unitRight[Int] }

  // fails with:
  //   error: type mismatch;
  //   found   : scala.util.Right[Nothing,Unit]
  //   required: Unit => Unit
  // def test4: Either[Int, Unit] = Right(()).map(Right(()))

  // compiles just fine
  def test5: Either[Int, Unit] = Right(()).map { case _ => unitRight }
  def test6: Either[Int, Unit] = Right(()).map { _ => unitRight }
  def test7: Either[Int, Unit] = Right(()).map(_ => unitRight)
}

scalac output:

bos-pchernikov-mac:personal pchernikov$ ~/Downloads/scala-2.12.8/bin/scalac -Ywarn-value-discard -Xfatal-warnings -print UnitOfTrust.scala
[[syntax trees at end of                   cleanup]] // UnitOfTrust.scala
package <empty> {
  object UnitOfTrust extends Object {
    private def unitRight(): scala.util.Either = new scala.util.Right(scala.runtime.BoxedUnit.UNIT);
    def test5(): scala.util.Either = new scala.util.Right(scala.runtime.BoxedUnit.UNIT).map({
      ((x0$1: scala.runtime.BoxedUnit) => UnitOfTrust.this.$anonfun$test5$1(x0$1))
    });
    def test6(): scala.util.Either = new scala.util.Right(scala.runtime.BoxedUnit.UNIT).map({
      ((x$1: scala.runtime.BoxedUnit) => UnitOfTrust.this.$anonfun$test6$1(x$1))
    });
    def test7(): scala.util.Either = new scala.util.Right(scala.runtime.BoxedUnit.UNIT).map({
      ((x$2: scala.runtime.BoxedUnit) => UnitOfTrust.this.$anonfun$test7$1(x$2))
    });
    final <artifact> private[this] def $anonfun$test5$1(x0$1: scala.runtime.BoxedUnit): Unit = {
      case <synthetic> val x1: scala.runtime.BoxedUnit = x0$1;
      case4(){
        matchEnd3({
          UnitOfTrust.unitRight();
          scala.runtime.BoxedUnit.UNIT
        })
      };
      matchEnd3(x: scala.runtime.BoxedUnit){
        ()
      }
    };
    final <artifact> private[this] def $anonfun$test6$1(x$1: scala.runtime.BoxedUnit): Unit = {
      UnitOfTrust.unitRight();
      ()
    };
    final <artifact> private[this] def $anonfun$test7$1(x$2: scala.runtime.BoxedUnit): Unit = {
      UnitOfTrust.unitRight();
      ()
    };
    def <init>(): UnitOfTrust.type = {
      UnitOfTrust.super.<init>();
      ()
    }
  }
}

This may also have a regression from scala 2.11 to 2.12:

bos-pchernikov-mac:personal pchernikov$ cat UnitOfTrust2_11.scala
object UnitOfTrust {
  import scala.util._

  private def unitRight[A]: Either[A, Unit] = Right(())

  // fails with:
  //   discarded non-Unit value
  // def test1: Either[Int, Unit] = Right(Right(()))
  // def test2: Either[Int, Unit] = Right(()).right.map(_ => unitRight[Int])
  // def test3: Either[Int, Unit] = Right(()).right.map { case _ => unitRight[Int] }

  // fails with:
  //   error: type mismatch;
  //   found   : scala.util.Right[Nothing,Unit]
  //   required: Unit => Unit
  // def test4: Either[Int, Unit] = Right(()).right.map(Right(()))

  // compiles just fine
  def test5: Either[Int, Unit] = Right(()).right.map { case _ => unitRight }
  def test6: Either[Int, Unit] = Right(()).right.map { _ => unitRight }
  def test7: Either[Int, Unit] = Right(()).right.map(_ => unitRight)
}
bos-pchernikov-mac:personal pchernikov$ ~/Downloads/scala-2.11.12/bin/scalac -Ywarn-value-discard -Xfatal-warnings UnitOfTrust2_11.scala
bos-pchernikov-mac:personal pchernikov$
@ChernikovP
Copy link
Author

A bit richer example will be with scalaz.\/:

bos-pchernikov-mac:personal pchernikov$ cat UnitOfTrust.scala 
object UnitOfTrust {
  import scalaz._
  import scalaz.Scalaz._

  private def unitRight[A]: A \/ Unit = ().right[A]

  // fails with: discarded non-Unit value
  // def test1: \/[String, Unit] = \/-(\/-(()))
  // def test2: \/[String, Unit] = ().right[String].map(_ => unitRight[String])
  // def test3: \/[String, Unit] = ().right[String].map { case _ => unitRight[String] }

  // fails with: type mismatch;
  // def test4: \/[String, Unit] = ().right[String].right[String]

  // fails with: polymorphic expression cannot be instantiated to expected type;
  // def test5: \/[String, Unit] = ().right[String].map(().right)

  // compiles just fine
  def test6: \/[String, Unit] = ().right[String].map { case _ => unitRight }
  def test7: \/[String, Unit] = ().right[String].map { _ => unitRight}
  def test8: \/[String, Unit] = ().right[String].map(_ => unitRight)
}bos-pchernikov-mac:personal pchernikov$ ~/Downloads/scala-2.12.8/bin/scalac -classpath /Users/pchernikov/.ivy2/cache/org.scalaz/scalaz-core_2.12/bundles/scalaz-core_2.12-7.2.27.jar -Ywarn-value-discad -Xfatal-warnings UnitOfTrust.scala 
bos-pchernikov-mac:personal pchernikov$ 

@nivekastoreth
Copy link

nivekastoreth commented Jan 23, 2019

I've created an example that I think demonstrates that this issue is very likely related to having multiple type parameters:
Full gist is available here: https://gist.github.com/nivekastoreth/fbe5dfb12687c565587f279909ff04f0

Example specifically pertaining to this claim:

class OneTypeParam[B](value: B) {
  def map[B1](fn: B => B1): OneTypeParam[B1] = new OneTypeParam(fn(value))
  def unitValue: OneTypeParam[Unit] = new OneTypeParam(())
  def checkCompiler: OneTypeParam[Unit] = unitValue.map(_ => unitValue)
}

class TwoTypeParam[A, B](value: B) {
  def map[B1](fn: B => B1): TwoTypeParam[A, B1] = new TwoTypeParam(fn(value))
  def unitValue[C]: TwoTypeParam[C, Unit] = new TwoTypeParam(())
  def checkCompiler: TwoTypeParam[String, Unit] = unitValue.map(_ => unitValue)
}

Compiling this with both -Xfatal-warnings and -Ywarn-value-discard yields only a single warning for OneTypeParam but compiles TwoTypeParam even though they compile to identical class files:

$ rm -f *.class; /build/env/scala-2.12.8/bin/scalac -Xfatal-warnings -Ywarn-value-discard -deprecation -print TypeNumExample.scala
TypeNumExample.scala:4: warning: discarded non-Unit value
  def checkCompiler: OneTypeParam[Unit] = unitValue.map(_ => unitValue)
                                                             ^

@nivekastoreth
Copy link

nivekastoreth commented Jan 23, 2019

Upon further investigation, this also effects a method marked as having a type that needs solving, even if it isn't used:

class OneTypeParam[B](value: B) {
  def map[B1](fn: B => B1): OneTypeParam[B1] = new OneTypeParam(fn(value))

  def unitValue: OneTypeParam[Unit] = new OneTypeParam(())
  def typedValue[C]: OneTypeParam[Unit] = new OneTypeParam(())

  def checkCompilerUnTyped: OneTypeParam[Unit] = unitValue.map(_ => unitValue)
  def checkCompilerTypedInner: OneTypeParam[Unit] = unitValue.map(_ => typedValue)
  def checkCompilerTypedOuter: OneTypeParam[Unit] = typedValue.map(_ => unitValue)
  def checkCompilerTypedBoth: OneTypeParam[Unit] = typedValue.map(_ => typedValue)
}

This yields warnings on the original footprint (checkCompilerUnTyped as well as on the version with the typed value on the outside (checkCompilerTypedOuter):

$ /build/env/scala-2.11.12/bin/scalac -Xlint -Xfatal-warnings -Ywarn-value-discard -deprecation typeParamNumExample.scala
typeParamNumExample.scala:9: warning: discarded non-Unit value
  def checkCompilerUnTyped: OneTypeParam[Unit] = unitValue.map(_ => unitValue)
                                                                    ^
typeParamNumExample.scala:11: warning: discarded non-Unit value
  def checkCompilerTypedOuter: OneTypeParam[Unit] = typedValue.map(_ => unitValue)

Looking at the typer output yields this tree for checkCompilerUnTyped (warns):

|    |    |    |    |-- ((x$1) => unitValue) : pt=scala.this.Function1[scala.this.Unit,scala.this.Unit] BYVALmode-EXPRmode-POLYmode (site: method checkCompilerUnTyped in OneTypeParam, a MethodSymbol with flags <method>)
|    |    |    |    |    |-- unitValue : pt=scala.this.Unit EXPRmode (site: value $anonfun in OneTypeParam, a TermSymbol with flags <synthetic>)
typeParamNumExample.scala:9: warning: discarded non-Unit value
  def checkCompilerUnTyped: OneTypeParam[Unit] = unitValue.map(_ => unitValue)
                                                                    ^
|    |    |    |    |    |    |-- { OneTypeParam.this.unitValue; () } : pt=scala.this.Unit EXPRmode (site: value $anonfun in OneTypeParam, a TermSymbol with flags <synthetic>)
|    |    |    |    |    |    |    |-- () : pt=scala.this.Unit EXPRmode (site: value $anonfun in OneTypeParam, a TermSymbol with flags <synthetic>)
|    |    |    |    |    |    |    |    \-> scala.this.Unit
|    |    |    |    |    |    |    \-> scala.this.Unit
|    |    |    |    |    |    [adapt] => nonUnit.this.OneTypeParam[scala.this.Unit] adapted to { OneTypeParam.this.unitValue; () } based on pt scala.this.Unit
|    |    |    |    |    |    \-> scala.this.Unit
|    |    |    |    |    \-> scala.this.Function1[scala.this.Unit,scala.this.Unit]

Compared to checkCompilerTypedInner (does not warn)

|    |    |    |    |-- ((x$2) => typedValue) : pt=scala.this.Function1[scala.this.Unit,scala.this.Unit] BYVALmode-EXPRmode-POLYmode (site: method checkCompilerTypedInner in OneTypeParam, a MethodSymbol with flags <method>)
|    |    |    |    |    |-- typedValue : pt=scala.this.Unit EXPRmode (site: value $anonfun in OneTypeParam, a TermSymbol with flags <synthetic>)
|    |    |    |    |    |    [search #1] start `<notype>`, searching for adaptation to pt=scala.this.Function1[nonUnit.this.OneTypeParam[scala.this.Unit],scala.this.Unit] (silent: value $anonfun in OneTypeParam, a TermSymbol with flags <synthetic>) implicits disabled
|    |    |    |    |    |    [search #2] start `<notype>`, searching for adaptation to pt=scala.this.Function1[scala.this.<byname>[nonUnit.this.OneTypeParam[scala.this.Unit]],scala.this.Unit] (silent: value $anonfun in OneTypeParam, a TermSymbol with flags <synthetic>) implicits disabled
|    |    |    |    |    |    |-- { OneTypeParam.this.typedValue[C]; () } : pt=scala.this.Unit EXPRmode (silent: value $anonfun in OneTypeParam, a TermSymbol with flags <synthetic>)
|    |    |    |    |    |    |    |-- () : pt=scala.this.Unit EXPRmode (silent: value $anonfun in OneTypeParam, a TermSymbol with flags <synthetic>)
|    |    |    |    |    |    |    |    \-> scala.this.Unit
|    |    |    |    |    |    |    \-> scala.this.Unit
|    |    |    |    |    |    solving for (C: ?C)
|    |    |    |    |    |    |-- { OneTypeParam.this.typedValue[scala.this.Nothing]; () } : pt=scala.this.Unit EXPRmode (site: value $anonfun in OneTypeParam, a TermSymbol with flags <synthetic>)
|    |    |    |    |    |    |    |-- () : pt=scala.this.Unit EXPRmode (site: value $anonfun in OneTypeParam, a TermSymbol with flags <synthetic>)
|    |    |    |    |    |    |    |    \-> scala.this.Unit
|    |    |    |    |    |    |    \-> scala.this.Unit
|    |    |    |    |    |    [adapt] [C]=> nonUnit.this.OneTypeParam[scala.this.Unit] adapted to { OneTypeParam.this.typedValue[scala.this.Nothing]; () } based on pt scala.this.Unit
|    |    |    |    |    |    \-> scala.this.Unit
|    |    |    |    |    \-> scala.this.Function1[scala.this.Unit,scala.this.Unit]

@som-snytt
Copy link

As another data point, the minimized example progresses after M5. Maybe that's due to improvements in inference for functions.

$ scalacm -d /tmp -Ywarn-value-discard discarding.scala
discarding.scala:7: warning: discarded non-Unit value
  def checkCompiler: OneTypeParam[Unit] = unitValue.map(_ => unitValue)
                                                             ^
one warning found
$ scalacm -version
Scala compiler version 2.13.0-M5 -- Copyright 2002-2018, LAMP/EPFL and Lightbend, Inc.
$ skalac -version
Scala compiler version 2.13.0-20190122-201544-ecc7a70 -- Copyright 2002-2018, LAMP/EPFL and Lightbend, Inc.
$ skalac -d /tmp -Ywarn-value-discard discarding.scala
discarding.scala:7: warning: discarded non-Unit value
  def checkCompiler: OneTypeParam[Unit] = unitValue.map(_ => unitValue)
                                                             ^
discarding.scala:13: warning: discarded non-Unit value
  def checkCompiler: TwoTypeParam[String, Unit] = unitValue.map(_ => unitValue)
                                                                     ^
two warnings found

@SethTisue
Copy link
Member

SethTisue commented Jul 25, 2022

@som-snytt is this, by any chance, essentially a duplicate of #11998?

@som-snytt
Copy link

I pushed the tests for 2.13, but unlikely to backport a fix to 2.12 (which is probably a non-trivial change to type inference).

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

Successfully merging a pull request may close this issue.

4 participants