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

Medowhill · 2018-07-20T16:54:13Z

No description provided.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

smarter · 2018-07-20T17:02:06Z

compiler/src/dotty/tools/dotc/typer/Applications.scala

+            case SAMType(mtp) =>
+              val sam = narrowByTypes(alts, mtp.paramInfos, mtp.resultType)
+              if (sam.nonEmpty && !pt.classSymbol.hasAnnotation(defn.FunctionalInterfaceAnnot))
+                ctx.warning(ex"$pt does not have the @FunctionalInterface annotation.", ctx.tree.pos)


I still don't think this is worth adding a warning for. Even if it is, there should be a simple way to turn off this warning.

The simple way is to do explicit eta-expension, no?

new ObjectOutputStream(println) // warning: are you sure this is what you want to do? new ObjectOutputStream(() => println()) // Ok new ObjectOutputStream(println _) // Ok

Right, but if I remember correctly, we plan to deprecate "f _" since just using "f" should work in all situations

allanrenucci · 2018-07-21T14:45:09Z

compiler/src/dotty/tools/dotc/typer/Applications.scala

+        val noSam = alts filter (normalizedCompatible(_, pt))
+        if (noSam.isEmpty) {
+          pt match {
+            case SAMType(mtp) =>


I would move this case to the enclosing match

val candidates = pt match { ... case SAMType(mtp) => val sam = narrowByTypes(alts, mtp.paramInfos, mtp.resultType) if (sam.nonEmpty && !pt.classSymbol.hasAnnotation(defn.FunctionalInterfaceAnnot)) ctx.warning(...) sam case pt => alts filter (normalizedCompatible(_, pt)) }

Do you mean fixing the code like the above one?
I think this code would choose the second f instead of the third f for the below code example.

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)

Do I understand your comment correctly?

I see. This deserves a comment with an example in the code

allanrenucci · 2018-07-21T14:48:29Z

compiler/src/dotty/tools/dotc/typer/Applications.scala

+            case SAMType(mtp) =>
+              val sam = narrowByTypes(alts, mtp.paramInfos, mtp.resultType)
+              if (sam.nonEmpty && !pt.classSymbol.hasAnnotation(defn.FunctionalInterfaceAnnot))
+                ctx.warning(ex"$pt does not have the @FunctionalInterface annotation.", ctx.tree.pos)


ctx.tree.pos here will not be very precise. Ideally it should be the position of the tree being expended:

-- Warning: tests/allan/Test.scala:5:6 ----------------------------------------- 5 | val out2 = new ObjectOutputStream(println) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Should be:

-- Warning: tests/allan/Test.scala:5:6 ----------------------------------------- 5 | val out2 = new ObjectOutputStream(println) | ^^^^^^^

allanrenucci · 2018-07-21T14:58:24Z

tests/run/i4364a.scala

@@ -0,0 +1,13 @@
+object Test {
+  var flag = false


I suppose you can remove this

allanrenucci · 2018-07-21T14:58:57Z

tests/run/i4364a.scala

+
+  def f(): Unit = assert(false)
+  def f(x: Int): Unit = assert(false)
+  def f(x: String): Unit = flag = true


def f(x: String): Unit = ()

allanrenucci · 2018-07-21T16:20:04Z

tests/run/i4364b.scala

@@ -0,0 +1,23 @@
+object Test {


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 }

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.

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) } }

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

allanrenucci · 2018-07-23T11:41:14Z

compiler/src/dotty/tools/dotc/typer/Applications.scala

@@ -1274,7 +1274,7 @@ trait Applications extends Compatibility { self: Typer with Dynamic =>
   *  Two trials: First, without implicits or SAM conversions enabled. Then,
   *  if the fist finds no eligible candidates, with implicits and SAM conversions enabled.
   */
-  def resolveOverloaded(alts: List[TermRef], pt: Type)(implicit ctx: Context): List[TermRef] = track("resolveOverloaded") {
+  def resolveOverloaded(alts: List[TermRef], pt: Type, pos: Position = NoPosition)(implicit ctx: Context): List[TermRef] = track("resolveOverloaded") {


Do you need a default value here?

allanrenucci · 2018-07-23T11:42:07Z

compiler/src/dotty/tools/dotc/typer/Applications.scala


      case defn.FunctionOf(args, resultType, _, _) =>
        narrowByTypes(alts, args, resultType)

      case pt =>
-        alts filter (normalizedCompatible(_, pt))
+        val noSam = alts filter (normalizedCompatible(_, pt))


I think noSam is not a good variable name. Maybe alts1

allanrenucci · 2018-07-23T11:50:48Z

compiler/src/dotty/tools/dotc/typer/Applications.scala

+        val noSam = alts filter (normalizedCompatible(_, pt))
+        if (noSam.isEmpty) {
+          /*
+           * the case should not be moved to the enclosing match


Maybe instead:

// 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)

allanrenucci · 2018-07-23T11:51:24Z

tests/neg-custom-args/fatal-warnings/i4364.scala

+    foo(f) // Ok: Consumer is @FunctionalInterface
+
+    val oos = new java.io.ObjectOutputStream(f) // error: OutputStream is not @FunctionalInterface
+    oos.write(0)


You can remove this line and the next one

allanrenucci · 2018-07-23T11:53:51Z

tests/neg-custom-args/fatal-warnings/i4364.scala

+object Test {
+  def foo(c: java.util.function.Consumer[String]) = c.accept("")
+
+  def f(x: String): Unit = ()


You can remove the first overload of f

allanrenucci · 2018-07-23T12:13:42Z

compiler/src/dotty/tools/dotc/typer/Applications.scala

+            case SAMType(mtp) =>
+              val sam = narrowByTypes(alts, mtp.paramInfos, mtp.resultType)
+              if (sam.nonEmpty && !pt.classSymbol.hasAnnotation(defn.FunctionalInterfaceAnnot))
+                ctx.warning(ex"${sam.head.designator} is eta-expanded even though $pt does not have the @FunctionalInterface annotation.", pos)


I am not 100% sure this is the right place to emit the warning. This assumes that this overload will be chosen, and its argument will be eta-expanded . Although I couldn't come up with an example where the warning is emitted but no eta-expansion happen.

I would emit the warning where the eta-expansion is performed

allanrenucci · 2018-07-23T15:42:39Z

compiler/src/dotty/tools/dotc/typer/Typer.scala

@@ -2321,9 +2321,11 @@ class Typer extends Namer
      if (arity >= 0 &&
          !tree.symbol.isConstructor &&
          !ctx.mode.is(Mode.Pattern) &&
-          !(isSyntheticApply(tree) && !isExpandableApply))
+          !(isSyntheticApply(tree) && !isExpandableApply)) {
+        if (!pt.classSymbol.hasAnnotation(defn.FunctionalInterfaceAnnot))


You need to exclude function classes (i.e. defn.isFunctionClass)

Medowhill · 2018-07-23T16:03:39Z

I found the following comment from the 1331st line of Applications.scala.

This private version of resolveOverloaded does the bulk of the work of overloading resolution, but does not do result adaptation. It might be called twice from the public resolveOverloaded method, once with implicits and SAM conversions enabled, and once without.

In the current version of the public resolveOverloaded method, the private resolveOverloaded method is called without Mode.ImplicitsEnabled at the first time. If there are no candidates, it is called again with Mode.ImplicitsEnabled. I think that currently there is no mode corresponding to SAM conversion.

How do you think about the following fix?

Adding

val SAMConversionEnabled = newMode(...)

to Mode.scala.

Changing public resolveOverloaded to

var found = resolveOverloaded(alts, pt, Nil)(ctx.retractMode(Mode.ImplicitsEnabled).retractMode(Mode.SAMConversionEnabled)
if (found.isEmpty && (ctx.mode.is(Mode.ImplicitsEnabled) || ctx.mode.is(Mode.SAMConversionEnabled)))
  found = resolveOverloaded(alts, pt, Nil)

Changing private resolveOverloaded to

val candidates = pt match {
  ...
  case SAMType(mtp) if ctx.mode.is(Mode.SAMConversionEnabled) =>
    narrowByTypes(alts, mtp.paramInfos, mtp.resultType)
  case pt =>
    alts filter (normalizedCompatible(_, pt))
}

Medowhill · 2018-07-23T16:13:13Z

I thought again about the above fix. It will result unnecessary redundant calculations. Would it be better to fix the comment to explain the current code correctly and keep the current code?

allanrenucci · 2018-07-24T12:47:49Z

I thought again about the above fix. It will result unnecessary redundant calculations. Would it be better to fix the comment to explain the current code correctly and keep the current code?

Your proposed fix looked reasonable to me (except that you can reuse Mode.ImplicitsEnabled and don't need to introduce a new mode). I tried it out and was able to come up with a test that would not compile:

import java.util.function.Consumer

class Test {
  def foo(c: Consumer[String]) = 1
  def f(x: Int): Unit = ()
  def f: Int = 1
  implicit def bar(x: Int): Consumer[String] = ???

  foo(f) // error: none of the overloaded alternatives of method f match expected type Consumer[String]
}

So I would say, we leave it as is for now. #4831 is a related issue I found while looking at it

allanrenucci · 2018-07-24T13:24:19Z

compiler/src/dotty/tools/dotc/typer/Typer.scala

-          !(isSyntheticApply(tree) && !isExpandableApply))
+          !(isSyntheticApply(tree) && !isExpandableApply)) {
+        pt match {
+          case SAMType(_) if !defn.isFunctionType(pt) && !pt.classSymbol.hasAnnotation(defn.FunctionalInterfaceAnnot) =>


The extractor is more expensive than the other tests, I would reorder as follow:

/** Is this a SAM type without the @FunctionalInterface annotation? */ def isNonFunctionalSamType = !pt.classSymbol.hasAnnotation(defn.FunctionalInterfaceAnnot) && !defn.isFunctionType(pt) && SAMType.unapply(pt).isDefined

odersky

The change to overloading looks like the right thing.

I'd like to discuss a bit more the warning if the expected type of an eta expansion is not a FunctionalInterface. It might end up to be the best thing we can do, but we should discuss it once more.

Looking at the documentation of @FunctionalInterface, it seems to play a role much like @tailrec in Scala: Whether an interface counts as a functional interface is decided structurally, but a @FunctionalInterface annotation makes sure that the user gets an error of a type is not structurally a functional interface.

It seems an overstretch to use the same annotation for deciding whether to warning when eta-expanding to yield instances of SAM types. On the other hand, the status quo where we do so silently can give nasty surprises. So, possible choices are:

don't warn (the status quo)
make it a warning, as this PR does
make it an error

I also have a tendency to make it a warning. But would like to get feedback from others. /cc @adriaanm

odersky · 2019-01-12T10:17:56Z

compiler/src/dotty/tools/dotc/typer/Typer.scala

+        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)


Small nitpick: I'd change the condition to

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

adriaanm · 2019-01-14T10:24:28Z

I think it's a good idea to warn. Since Java users are encouraged (though not required) to express their intent with the @functionalinterface annotation it's likely that they will. So, the omission of the annotation is a good indication we shouldn't be treating the interface like a SAM. I'd be happy to have this improvement backported to Scala 2!

odersky · 2019-01-15T14:49:30Z

Superseded by #5717.

dottybot reviewed Jul 20, 2018

View reviewed changes

smarter requested changes Jul 20, 2018

View reviewed changes

allanrenucci suggested changes Jul 21, 2018

View reviewed changes

allanrenucci reviewed Jul 21, 2018

View reviewed changes

allanrenucci reviewed Jul 23, 2018

View reviewed changes

allanrenucci reviewed Jul 24, 2018

View reviewed changes

Medowhill added 7 commits July 26, 2018 19:44

Fix #4364: Try SAM type when no candidates found

0ef2206

better warning, fix tests, & add comments

770e49f

simpler tests

b4d8776

fix test, change val name, & change warning point

5685a99

check function type before warning

653b2e4

check SAM type also

f5758a5

reordered checks

ac97d90

OlivierBlanvillain assigned allanrenucci Oct 2, 2018

odersky approved these changes Jan 12, 2019

View reviewed changes

odersky assigned odersky and unassigned allanrenucci Jan 12, 2019

odersky reviewed Jan 12, 2019

View reviewed changes

odersky mentioned this pull request Jan 15, 2019

Fix #4364: Try SAM type when no candidates are found #5717

Merged

odersky closed this Jan 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Medowhill commented Jul 20, 2018

dottybot left a comment

smarter Jul 20, 2018

allanrenucci Jul 20, 2018 •

edited

Loading

smarter Jul 20, 2018

allanrenucci Jul 21, 2018

Medowhill Jul 23, 2018

allanrenucci Jul 23, 2018

allanrenucci Jul 21, 2018

allanrenucci Jul 21, 2018

allanrenucci Jul 21, 2018

allanrenucci Jul 21, 2018 •

edited

Loading

Medowhill Jul 23, 2018

allanrenucci Jul 23, 2018

Medowhill Jul 23, 2018

allanrenucci Jul 23, 2018

allanrenucci Jul 23, 2018

allanrenucci Jul 23, 2018

allanrenucci Jul 23, 2018

allanrenucci Jul 23, 2018

allanrenucci Jul 23, 2018 •

edited

Loading

allanrenucci Jul 23, 2018

Medowhill commented Jul 23, 2018

Medowhill commented Jul 23, 2018

allanrenucci commented Jul 24, 2018

allanrenucci Jul 24, 2018

odersky left a comment

odersky Jan 12, 2019

adriaanm commented Jan 14, 2019

odersky commented Jan 15, 2019

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

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

Conversation

Medowhill commented Jul 20, 2018

dottybot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allanrenucci Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allanrenucci Jul 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allanrenucci Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Medowhill commented Jul 23, 2018

Medowhill commented Jul 23, 2018

allanrenucci commented Jul 24, 2018

Choose a reason for hiding this comment

odersky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriaanm commented Jan 14, 2019

odersky commented Jan 15, 2019

allanrenucci Jul 20, 2018 •

edited

Loading

allanrenucci Jul 21, 2018 •

edited

Loading

allanrenucci Jul 23, 2018 •

edited

Loading