Skip to content

Fix #4364: Try SAM type when no candidates found #4821

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

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions compiler/src/dotty/tools/dotc/core/Definitions.scala
Original file line number Diff line number Diff line change
Expand Up @@ -782,8 +782,10 @@ class Definitions {
def GetterMetaAnnot(implicit ctx: Context) = GetterMetaAnnotType.symbol.asClass
lazy val SetterMetaAnnotType = ctx.requiredClassRef("scala.annotation.meta.setter")
def SetterMetaAnnot(implicit ctx: Context) = SetterMetaAnnotType.symbol.asClass
lazy val ShowAsInfixAnotType = ctx.requiredClassRef("scala.annotation.showAsInfix")
def ShowAsInfixAnnot(implicit ctx: Context) = ShowAsInfixAnotType.symbol.asClass
lazy val ShowAsInfixAnnotType = ctx.requiredClassRef("scala.annotation.showAsInfix")
def ShowAsInfixAnnot(implicit ctx: Context) = ShowAsInfixAnnotType.symbol.asClass
lazy val FunctionalInterfaceAnnotType = ctx.requiredClassRef("java.lang.FunctionalInterface")
def FunctionalInterfaceAnnot(implicit ctx: Context) = FunctionalInterfaceAnnotType.symbol.asClass

// convenient one-parameter method types
def methOfAny(tp: Type) = MethodType(List(AnyType), tp)
Expand Down
16 changes: 15 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Applications.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1430,7 +1430,21 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
narrowByTypes(alts, args, resultType)

case pt =>
alts filter (normalizedCompatible(_, pt))
val alts1 = alts filter (normalizedCompatible(_, pt))
if (alts1.isEmpty) {
// We only consider SAM types when there is no alternative.
// In the example below, the second overload of `bar` should
// be preferred over the first one:
//
// def foo(c: Consumer[String]) = ...
// def bar(x: String): Unit = ...
// def bar: Consumer[String] = ...
// foo(bar)
pt match {
case SAMType(mtp) => narrowByTypes(alts, mtp.paramInfos, mtp.resultType)
case _ => alts1
}
} else alts1
}
val found = narrowMostSpecific(candidates)
if (found.length <= 1) found
Expand Down
8 changes: 6 additions & 2 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2353,9 +2353,13 @@ class Typer extends Namer
!tree.symbol.isConstructor &&
!tree.symbol.is(TransparentMethod) &&
!ctx.mode.is(Mode.Pattern) &&
!(isSyntheticApply(tree) && !isExpandableApply))
!(isSyntheticApply(tree) && !isExpandableApply)) {
if (!pt.classSymbol.hasAnnotation(defn.FunctionalInterfaceAnnot) &&
!defn.isFunctionType(pt) &&
SAMType.unapply(pt).isDefined)
ctx.warning(ex"${tree.symbol} is eta-expanded even though ${pt.classSymbol} does not have the @FunctionalInterface annotation.", tree.pos)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpick: I'd change the condition to

if (!defn.isFunctionType(pt))
  pt match {
    case SAMType(_) if !pt.classSymbol.hasAnnotation(defn.FunctionalInterfaceAnnot) =>
      ...
    case _ =>
 }

simplify(typed(etaExpand(tree, wtp, arity), pt), pt, locked)
else if (wtp.paramInfos.isEmpty && isAutoApplied(tree.symbol))
} else if (wtp.paramInfos.isEmpty && isAutoApplied(tree.symbol))
readaptSimplified(tpd.Apply(tree, Nil))
else if (wtp.isImplicitMethod)
err.typeMismatch(tree, pt)
Expand Down
9 changes: 9 additions & 0 deletions tests/neg-custom-args/fatal-warnings/i4364.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
object Test {
def foo(c: java.util.function.Consumer[Integer]) = c.accept(0)
def f(x: Int): Unit = ()

def main(args: Array[String]) = {
foo(f) // Ok: Consumer is @FunctionalInterface
new java.io.ObjectOutputStream(f) // error: OutputStream is not @FunctionalInterface
}
}
2 changes: 1 addition & 1 deletion tests/neg/i2033.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import collection._
object Test {
def check(obj: AnyRef): Unit = {
val bos = new ByteArrayOutputStream()
val out = new ObjectOutputStream(println) // error
val out = new ObjectOutputStream(println)
val arr = bos toByteArray ()
val in = (())
val deser = ()
Expand Down
13 changes: 13 additions & 0 deletions tests/run/i4364a.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import java.util.function.Consumer

object Test {
def f(): Unit = assert(false)
def f(x: Int): Unit = assert(false)
def f(x: String): Unit = ()

def foo(c: Consumer[String]) = c.accept("")

def main(args: Array[String]) = {
foo(f)
}
}
12 changes: 12 additions & 0 deletions tests/run/i4364b.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import java.util.function.Consumer

object Test {
Copy link
Contributor

@allanrenucci allanrenucci Jul 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear what you are testing here that is not tested in i4364a.scala. I suggest you add a test to tests/neg-custom-args/fatal-warnings to test the warnings:

class Test {
  def foo(c: java.util.function.Consumer[String]) = ()
  def bar(out: java.io.OutputStream) = ()

  bar(x => println(x))
  bar(println(_))
  bar(println) // error: OutputStream is not @FunctionalInterface

  def f(x: String) = ()
  foo(f) // Ok: Consumer is @FunctionalInterface
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

def f(x: Int): Unit = assert(false)
def f(x: String): Unit = assert(false)
def f: java.io.OutputStream = new java.io.OutputStream { def write(x: Int) = () }

val oosF = new java.io.ObjectOutputStream(f)
oosF.write(0)

Through the test, I tried to check that whether method overloading is correctly resolved when candidates exist without considering SAM type. In this case, the second f is valid choice when it is eta-expanded and OutputStream is considered as SAM type. However, the third f should be choosed. Therefore, I think this test needs to remain.

As you suggested, it would be better to check a warning in a seperate test. I'll remove

val oosG = new java.io.ObjectOutputStream(g)
oosG.write(0)
oosG.close()

from test b and add a test about a warning in tests/neg-custom-args/fatal-warnings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I suggest a simpler test though:

import java.util.function.Consumer

object Test {
  def foo(c: Consumer[String]) = c.accept("")

  def bar(x: String): Unit = ???
  def bar: Consumer[String] = new Consumer {  def accept(s: String) = () }

  def main(args: Array[String]) = {
    foo(bar)
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this one is simpler and clearer. I'll fix it. Thank you!

def f(x: String): Unit = assert(false)
def f: Consumer[String] = new Consumer { def accept(s: String) = () }

def foo(c: Consumer[String]) = c.accept("")

def main(args: Array[String]) = {
foo(f)
}
}