I've also rebased #4767 (enable SAM by default) on top of this PR, and have triggered a build.

That build showed a flaw in my first attempt, I've just pushed an improved fix. The enable-SAM-by-default build is running again on top of 639e52a.

Do I understand correctly that the following code won't compile anymore?

object T {
  def bar(r: Runnable): Unit = r.run()
  def f() = () => println()
  bar(f)
}

Because in bar(f) f is a parameterless method type

I may be misunderstanding it, not very familiar with scalac sources :)

That's right:

scala> object T {
     |   def bar(r: Runnable): Unit = r.run()
     |   def f() = () => println()
     |   bar(f)
     | }
<console>:14: error: type mismatch;
 found   : () => Unit
 required: Runnable
         bar(f)
             ^

If only I knew this several months earlier. Spent a lot of time trying to support eta expansion of SAMs in Intellij IDEA. That makes it simpler. If you are interested, here are tests on SAM for IntelliJ IDEA.

Assuming this PR passes the tests, will this fix be backported to Scala 2.11?

Thanks for that work, really great to have that done ahead of the 2.12 release! We hang out over at https://gitter.im/scala/contributors, so feel free to drop by with those sort of questions.

Not sure about 2.11 yet, let's see how this proposal fares in 2.12.

I will, thanks!

Hopefully this proposal makes it into both 2.11 and 2.12!

I've modified that job to archive JVM crash logs as an build artifact to get a bit more information it if happens again.

/rebuild

/rebuild - scaboooou only reads the headlines.

The Function0 example you provide in the ticket remains the same?

object Test {
  def foo(): () => String = () => ""
  val f: () => Any = foo

  def main(args: Array[String]): Unit = {
    println(f()) // <function0>
  }
}

Should we emit a deprecation in this case? It'd be good to add the example as a test case. LGTM otherwise, but I'll let @adriaanm take a look.

I've added the test case to show that the status quo is preserved. If we have a consensus to deprecate let's do so under the banner of a new PR.

My vote is +1 unless we learn something from running the community build with the deprecation warning (ramped up to an error so we see where if/where it is used.)

I'm taking another look at this failure to compile the library with the locker in the followup change, which enables SAM by default:

quick.lib:
    [mkdir] Created dir: /home/jenkins/workspace/scala-2.12.x-validate-publish-core/build/quick/classes/library
[quick.library] Compiling 584 files to /home/jenkins/workspace/scala-2.12.x-validate-publish-core/build/quick/classes/library
[quick.library] OpenJDK 64-Bit Server VM warning: ignoring option MaxPermSize=512M; support was removed in 8.0
[quick.library] OpenJDK 64-Bit Server VM warning: Using the ParNew young collector with the Serial old collector is deprecated and will likely be removed in a future release
[quick.library] /home/jenkins/workspace/scala-2.12.x-validate-publish-core/src/library/scala/collection/parallel/ParIterableLike.scala:737: error: recursive method bf2seq needs result type
[quick.library]   def scanLeft[S, That](z: S)(op: (S, T) => S)(implicit bf: CanBuildFrom[Repr, S, That]) = setTaskSupport(seq.scanLeft(z)(op)(bf2seq(bf)), tasksupport)
[quick.library]                                                                                                                               ^

Looks like that is a general problem with the SAM implementation. In order to support let SAMs participate in overload resolution, isCompatible was changed to call isCompatibleSAM. This computes the set of members of the corresponding formal parameter type; if there is exactly one then and some other conditions hold, SAM conversion is tried.

This seems pretty heaviweight from a performance perspective, and as the build failure shows, it also treads into cycles.

We might be able to easily make this leaner by instead looking at the decls of all the base classes in the hope that we'll find more than one abstract method that would disqualify this as a SAM. Generalizing this, we could expose a variant of findMembers that returns an iterator of members.

LGTM

cool! @adriaanm wdyt about deprecating the Function0 case (#4822 (comment))?

Agreed with deprecating. @retronym, could you follow up with a PR for that?

Also, SAM needs to be enabled by default?

@ilinum I'm trying that in #4871

Is this why, under 2.12, the following code compiles:

package object mypackage {
  def render() = {}

  val handler: Runnable = () => render
}

yet this code does not?

package object mypackage {
  def render() = {}

  val handler: Runnable = render _
}

[error] /home/vossad01/dev/untitled/src/main/scala/mypackage/Main.scala:10: type mismatch;
[error]  found   : Unit
[error]  required: Runnable
[error]   val handler: Runnable = render _
[error]                           ^

Under 2.11, with the addition of an implicit conversion either form was valid.

Originally noticed in the context of Scala.js (scala-js/scala-js#2656)

Yes, you have to write the function literal in full now in the zero-arg
case. Also, in 2.12, implicit conversion will not kick in when the expected
type is a SAM and the expression is a function literal. This conversion is
handled by SAM conversion. The release notes have more info and a pointer
to the spec.
On Sat, Nov 19, 2016 at 07:47 Adam Voss [email protected] wrote:

Is this why, under 2.12, the following code compiles:

package object mypackage {
def render() = {}

val handler: Runnable = () => render
}

yet this code does not?

package object mypackage {
def render() = {}

val handler: Runnable = render _
}

[error] /home/vossad01/dev/untitled/src/main/scala/mypackage/Main.scala:10: type mismatch;
[error] found : Unit
[error] required: Runnable
[error] val handler: Runnable = render _
[error] ^

Under 2.11, with the addition of an implicit conversion either form was
valid.

Originally noticed in the context of Scala.js (scala-js/scala-js#2656
scala-js/scala-js#2656)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4822 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFjy9sntoVBjrsVHtjEjRGsERva4Igzks5q_xoEgaJpZM4GWMra
.

@adriaanm I have finally muddled my way through the spec. I will admit it is a little over my head, but this stackoverflow post on method type vs function type helped a lot. From what I read, it seems like the case I described above would be a bug.

package object mypackage {
  def render() = {}
  val handler: Function0[Any] = render _
  val handler2 = render _
  val handler3: Runnable = handler2
  val handler4: Runnable = render _


  implicit def runnable(f: () => Unit): Runnable =
    new Runnable() { def run() = f() }
}

of contention is how render _ it interpreted.

In the above code, only the line of handler4 fails to compile. Based on both the -Xprint:typer output and the error message, the compiler seems to have chosen to do function application on that line rather than to treat it as a method value

The spec says:

Expression forms are discussed subsequently in decreasing order of precedence.

Since function application is listed first, it would seem that if function application was appropriate for handler4 it should have been used for handler and handler2.

What I expect is render _ is treated as a method value. Looking at the spec:

Method Values
The expression e _ is well-formed if e is of method type or if e is a call-by-name parameter. If e is a method with parameters, e _ represents e converted to a function type by eta-expansion. [...]

Based on the this issue, I suppose it could be argued that because the method value explicitly triggers eta-expansion and Runnable is Function0-like it is covered by issue. However, that seems inconsistent with how #5327 treats method values as distinct from eta-expansion nor did I see how that is specified in the spec.

Continuing under the assumption that render _ is a method value, its function type would be () => Unit. Because it is a method value, it is not a anonymous function (lambda) per 6.23 and the SAM-conversion via translation (6.23.1) would not be applicable. () => Unit is not assignable to Runnable, but there are still Implicit Conversions to consider.

SAM conversion
An expression (p1, ..., pN) => body of function type (T1, ..., TN) => T is sam-convertible to the expected type S [...]

render _ has a function type suitable for the SAM conversion; however, render _ does not match the form (p1, ..., pN) => body

It would then advance onto View Application, for which there is a suitable implicit defined.

My interpretation that the implicit SAM conversion would not apply would seem consistent with the release note,

Note that SAM conversion only applies to lambda expressions, not to arbitrary expressions with Scala FunctionN types: [...]

Thank you for digging in here -- this is tricky stuff. I think you're right that something's iffy here. Must indeed be something to do with it being 0-ary. Both these examples compile:

class Ok1 {
  trait Sam1 { def apply(x: Any): Unit }
  def render(x: Any) = {}
  val handler: Sam1 = render _
}

class Ok2 {
  implicit def conv(x: Function0[Unit]): Runnable = () => x()
  def render() = {}
  val handler: Runnable = (render _): Function0[Unit] // suppress sam conversion by typing the method value `render _` as a function-type, which will then expand to `() => render()` via eta-expansion, which will ultimately be converted via the implicit view `conv` to the expected type `Runnable`
}

Could you file a ticket over at scala/scala-dev? I think the right behavior is that the nullary case should behave just like the unary case in Ok1. (I.e., sam conversion applies)