scala / scala3

The Scala 3 compiler, also known as Dotty.
https://dotty.epfl.ch
Apache License 2.0
5.81k stars 1.05k forks source link

Compiler incorrectly tries to invoke context function passed as argument #10825

Closed elfprince13 closed 3 years ago

elfprince13 commented 3 years ago

Minimized code

trait A { def apply():Int }
trait B { def apply():Int }

class MWE2 {
  type Op = (A, B) ?=> Int

  def doOp(op:Op):Int = {
    implicit val a:A = new A{ def apply():Int = 2}
    implicit val b:B = new B{ def apply():Int = 3}
    op
  }

  def main(args:Array[String]):Unit = {
    val op:Op = {
      summon[A]() + summon[B]()
    }
    implicit val a:A = new A{ def apply():Int = 6}
    implicit val b:B = new B{ def apply():Int = 7}
    Console.println(doOp(op))
  }
}

Output

[error] 17 |      summon[A]() + summon[B]()
[error]    |               ^
[error]    |ambiguous implicit arguments: both value evidence$1 and value a match type A of parameter x of method summon in object Predef
[error] -- Error: src/main/scala/MWE2.scala:21:27
[error] 21 |    Console.println(doOp(op))
[error]    |                           ^
[error]    |ambiguous implicit arguments: both value evidence$5 and value a match type A of parameter of MWE2.this.Op
[error]    |which is an alias of: (A, B) ?=> Int
[error] two errors found
[error] (Compile / compileIncremental) Compilation failed
[error] Total time: 8 s, completed Dec 16, 2020 5:48:13 PM

Expectation

op should not be invoked in main but should instead be passed to doOp. Additionally, if the compiler is supposed to invoke it in main (which would be very surprising), the result should be a type error (found Int, required Op). Further a should not be ambiguous with some anonymous evidence when I'm not even using any context bounds or anything else implicits / givens related that would be generating evidence.

griggt commented 3 years ago

Just curious, does this issue only arise when compiling with -source:3.0-migration?

elfprince13 commented 3 years ago

I'll double check on that in the morning, but the issue does not arise if you comment out:

    implicit val a:A = new A{ def apply():Int = 6}
    implicit val b:B = new B{ def apply():Int = 7}
som-snytt commented 3 years ago

TIL implicit nesting level is tied to -source. My other idle question is where does evidence$5 come from?

The expectation is slightly off: an (a, b) ?=> op(a,b) is constructed in main. It is passed to doOp.

Also, doOp could be spelled dooWop.

LPTK commented 3 years ago

Your code compiles in Scastie. Are you using a language option?

The calling behavior does seem correct BTW. By specification, context functions are always called whenever they are referred to. The problem here is that the context parameters given by doOp are not prioritized over the outer implicits, which they normally should (but maybe not in Scala 2 compat mode).

som-snytt commented 3 years ago

Yes, implicit nesting level is not bumped under -source 3.0-migration. Otherwise the code works.

elfprince13 commented 3 years ago

By specification, context functions are always called whenever they are referred to.

So then when I comment out the implicit definitions in main why is there not an error that no implicit could be found?

And as @som-snytt says, where is evidence$5 coming from?

som-snytt commented 3 years ago

@elfprince13 see my previous comment and https://dotty.epfl.ch/docs/reference/contextual/context-functions.html

if the expected type of an expression E is a context function type (T_1, ..., T_n) ?=> U and E is not already an context function literal, E is converted to a context function literal... x_1, ..., x_n are available as givens in E.

I used -Xprint:typer to see some expansion, but I assume evidence$5 is generated perhaps by typechecking, such as when rewrites are attempted. But I don't know how to view that. The print output only shows evidence$1 in op, so I conjecture it is retrying the arg to doOp and showing only the next fresh name, which is evidence$5. Somewhere it says the names are arbitrary so we're not supposed to worry about them.

It would also be nice if Scala 3 had -Xlint so it could tell us that certain variables are unused.

LPTK commented 3 years ago

@elfprince13 In principle Console.println(doOp(op)) expands into something like Console.println(doOp(ev$0 ?=> op(summon))).

elfprince13 commented 3 years ago

It still seems like there's a bug here. If I bump the source-level to 3.0, then I get 5 regardless of whether main's set of implicit vals are defined. If I keep it on 3.0-migration, then I get 5 when they are not defined and a compilation error when they are. So obviously the implicit function is not evaluated when referred to in main, so what's the point of the overhead in creating a wrapper function to "cast" an Op to an Op?

LPTK commented 3 years ago

There is no bug at all. The old behavior when several implicits were in local scope was to raise an ambiguity error. That's why it doesn't compile. In the revised Scala 3 language, the compiler instead picks the innermost one (ev$0 in my example above).

elfprince13 commented 3 years ago

Okay, so I think I understand both the migration and 3.0 behavior, and maybe this deserves a fresh issue, but .... expanding doOp(op) to doOp(ev$0 ?=> op) breaks one of the fundamental use cases for contextual abstractions and doesn't appear to have any benefits.

Consider the following code, emulating the behavior of a very simple STM that never commits:

import scala.annotation.tailrec

object Explode {
  trait Txn {}
  type AtomicOp[Z] = Txn ?=> Z

  object AbortAndRetry extends scala.util.control.ControlThrowable("abort and retry this transaction") {}

  def beginTxn:Txn = new Txn {}
  def commit(using txn:Txn) = throw AbortAndRetry

  @tailrec def atomic[Z](txn:AtomicOp[Z]):Z = {
    try {
      given Txn = beginTxn
      val ret:Z = txn
      commit
      ret
    } catch {
      case AbortAndRetry => atomic(txn)
    }
  }

  def main(args:Array[String]) = {
    Console.println(atomic {
      "hello world"
    })
  }
}

Compared to the equivalent, slightly uglier Scala 2 code, this version crashes due to a Stack Overflow in the wrapper functions generated here:

      case AbortAndRetry => atomic(txn)
LPTK commented 3 years ago

This definitely deserves a separate issue (and I think this issue can be closed).

expanding doOp(op) to doOp(ev$0 ?=> op) breaks one of the fundamental use cases for contextual abstractions and doesn't appear to have any benefits.

You're welcome to make an alternative proposal for how this feature works in this case, but beware that it currently behaves like this for a very good reason. So you should read this first: https://infoscience.epfl.ch/record/229878?ln=en