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

[sammy] eta-expansion, overloading, existentials #4101

Merged
merged 3 commits into from Nov 10, 2014

Conversation

Projects
None yet
6 participants
@adriaanm
Member

adriaanm commented Nov 6, 2014

Review by @retronym, please.

While playing with Java 8 Streams from the repl,
I realized we weren't eta-expanding,
nor resolving overloading properly for SAMs.

Also, the way Java uses wildcards to represent use-site variance
stresses type inference (since it results in existentials).

Introduce wildcardExtrapolation to simplify the resulting types
(without losing precision): wildcardExtrapolation(tp) =:= tp.

For example, the MethodType given by def bla(x: (_ >: String)): (_ <: Int)
is both a subtype and a supertype of def bla(x: String): Int.

Translating http://winterbe.com/posts/2014/07/31/java8-stream-tutorial-examples/ into Scala:

scala> import java.util.Arrays
scala> import java.util.stream.Stream
scala> import java.util.stream.IntStream
scala> val myList = Arrays.asList("a1", "a2", "b1", "c2", "c1")
myList: java.util.List[String] = [a1, a2, b1, c2, c1]

scala> myList.stream.filter(_.startsWith("c")).map(_.toUpperCase).sorted.forEach(println)
C1
C2

scala> myList.stream.filter(_.startsWith("c")).map(_.toUpperCase).sorted
res8: java.util.stream.Stream[?0] = java.util.stream.SortedOps$OfRef@133e7789

scala> Arrays.asList("a1", "a2", "a3").stream.findFirst.ifPresent(println)
a1

scala> Stream.of("a1", "a2", "a3").findFirst.ifPresent(println)
a1

scala> IntStream.range(1, 4).forEach(println)
<console>:37: error: object creation impossible, since method accept in trait IntConsumer of type (x$1: Int)Unit is not defined
(Note that Int does not match Any: class Int in package scala is a subclass of class Any in package scala, but method parameter types must match exactly.)
              IntStream.range(1, 4).forEach(println)
                                            ^

scala> IntStream.range(1, 4).forEach(println(_: Int)) // TODO: can we avoid this annotation?
1
2
3

scala> Arrays.stream(Array(1, 2, 3)).map(n => 2 * n + 1).average.ifPresent(println(_: Double))
5.0

scala> Stream.of("a1", "a2", "a3").map(_.substring(1)).mapToInt(_.parseInt).max.ifPresent(println(_: Int)) // whoops!
ReplGlobal.abort: Unknown type: <error>, <error> [class scala.reflect.internal.Types$ErrorType$, class scala.reflect.internal.Types$ErrorType$] TypeRef? false
error: Unknown type: <error>, <error> [class scala.reflect.internal.Types$ErrorType$, class scala.reflect.internal.Types$ErrorType$] TypeRef? false
scala.reflect.internal.FatalError: Unknown type: <error>, <error> [class scala.reflect.internal.Types$ErrorType$, class scala.reflect.internal.Types$ErrorType$] TypeRef? false
    at scala.reflect.internal.Reporting$class.abort(Reporting.scala:59)

scala> IntStream.range(1, 4).mapToObj(i => "a" + i).forEach(println)
a1
a2
a3

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 6, 2014

Member

/cc FYI, @Ichoran

Member

adriaanm commented Nov 6, 2014

/cc FYI, @Ichoran

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 6, 2014

Member

I'll investigate the failures in the REPL transcript later. Need to get back to my talk :)

Member

adriaanm commented Nov 6, 2014

I'll investigate the failures in the REPL transcript later. Need to get back to my talk :)

@scala-jenkins scala-jenkins added this to the 2.11.5 milestone Nov 6, 2014

@gourlaysama

This comment has been minimized.

Show comment
Hide comment
@gourlaysama

gourlaysama Nov 6, 2014

Member

👍 Yay! Overloading is the thing that prevented SAMs from being really useful when calling into java.

BTW there was a ticket for that: SI-8310

Member

gourlaysama commented Nov 6, 2014

👍 Yay! Overloading is the thing that prevented SAMs from being really useful when calling into java.

BTW there was a ticket for that: SI-8310

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 6, 2014

Member

Ugh, the jenkins node I added is running an outdated version of java.

Member

adriaanm commented Nov 6, 2014

Ugh, the jenkins node I added is running an outdated version of java.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 6, 2014

Member

Thanks for the pointer @gourlaysama! I'll include the test case -- it passes now.

Member

adriaanm commented Nov 6, 2014

Thanks for the pointer @gourlaysama! I'll include the test case -- it passes now.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Nov 6, 2014

Member

I offlined them about 15 mins ago.

Member

retronym commented Nov 6, 2014

I offlined them about 15 mins ago.

[sammy] eta-expansion, overloading (SI-8310)
Playing with Java 8 Streams from the repl showed
we weren't eta-expanding, nor resolving overloading for SAMs.

Also, the way Java uses wildcards to represent use-site variance
stresses type inference past its bendiness point (due to excessive existentials).

I introduce `wildcardExtrapolation` to simplify the resulting types
(without losing precision): `wildcardExtrapolation(tp) =:= tp`.

For example, the `MethodType` given by `def bla(x: (_ >: String)): (_ <: Int)`
is both a subtype and a supertype of `def bla(x: String): Int`.

Translating http://winterbe.com/posts/2014/07/31/java8-stream-tutorial-examples/
into Scala shows most of this works, though we have some more work to do (see near the end).

```
scala> import java.util.Arrays
scala> import java.util.stream.Stream
scala> import java.util.stream.IntStream
scala> val myList = Arrays.asList("a1", "a2", "b1", "c2", "c1")
myList: java.util.List[String] = [a1, a2, b1, c2, c1]

scala> myList.stream.filter(_.startsWith("c")).map(_.toUpperCase).sorted.forEach(println)
C1
C2

scala> myList.stream.filter(_.startsWith("c")).map(_.toUpperCase).sorted
res8: java.util.stream.Stream[?0] = java.util.stream.SortedOps$OfRef@133e7789

scala> Arrays.asList("a1", "a2", "a3").stream.findFirst.ifPresent(println)
a1

scala> Stream.of("a1", "a2", "a3").findFirst.ifPresent(println)
a1

scala> IntStream.range(1, 4).forEach(println)
<console>:37: error: object creation impossible, since method accept in trait IntConsumer of type (x$1: Int)Unit is not defined
(Note that Int does not match Any: class Int in package scala is a subclass of class Any in package scala, but method parameter types must match exactly.)
              IntStream.range(1, 4).forEach(println)
                                            ^

scala> IntStream.range(1, 4).forEach(println(_: Int)) // TODO: can we avoid this annotation?
1
2
3

scala> Arrays.stream(Array(1, 2, 3)).map(n => 2 * n + 1).average.ifPresent(println(_: Double))
5.0

scala> Stream.of("a1", "a2", "a3").map(_.substring(1)).mapToInt(_.parseInt).max.ifPresent(println(_: Int)) // whoops!
ReplGlobal.abort: Unknown type: <error>, <error> [class scala.reflect.internal.Types$ErrorType$, class scala.reflect.internal.Types$ErrorType$] TypeRef? false
error: Unknown type: <error>, <error> [class scala.reflect.internal.Types$ErrorType$, class scala.reflect.internal.Types$ErrorType$] TypeRef? false
scala.reflect.internal.FatalError: Unknown type: <error>, <error> [class scala.reflect.internal.Types$ErrorType$, class scala.reflect.internal.Types$ErrorType$] TypeRef? false
	at scala.reflect.internal.Reporting$class.abort(Reporting.scala:59)

scala> IntStream.range(1, 4).mapToObj(i => "a" + i).forEach(println)
a1
a2
a3

```
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 6, 2014

Member

Thanks -- I push --forced to address review feedback (include SI-8310 test case).

Member

adriaanm commented Nov 6, 2014

Thanks -- I push --forced to address review feedback (include SI-8310 test case).

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Nov 6, 2014

Member

Need to update that check file. The new error message is correct, though suboptimal...

Member

adriaanm commented Nov 6, 2014

Need to update that check file. The new error message is correct, though suboptimal...

val tp1 = normalize(tp)
( (tp1 weak_<:< pt)
|| isCoercible(tp1, pt)
|| isCompatibleByName(tp, pt)
|| isCompatibleSam(tp, pt)

This comment has been minimized.

@retronym

retronym Nov 6, 2014

Member

I don't see where we predicate this on -Xexperimental. I believe we should. I'd also be interested in performance impact of this; we should have a fast path for non-SAMs.

@retronym

retronym Nov 6, 2014

Member

I don't see where we predicate this on -Xexperimental. I believe we should. I'd also be interested in performance impact of this; we should have a fast path for non-SAMs.

This comment has been minimized.

@retronym

retronym Nov 6, 2014

Member

Oh wait, I see samOf checks the settings. That will do.

@retronym

retronym Nov 6, 2014

Member

Oh wait, I see samOf checks the settings. That will do.

This comment has been minimized.

@adriaanm

adriaanm Nov 7, 2014

Member

I figured it's easier to flip the switch if it's only tested in one central spot.

@adriaanm

adriaanm Nov 7, 2014

Member

I figured it's easier to flip the switch if it's only tested in one central spot.

@@ -2818,6 +2849,11 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
)
}
// TODO: improve error reporting -- when we're in silent mode (from `silent(_.doTypedApply(tree, fun, args, mode, pt)) orElse onError`)
// the errors in the function don't get out...
if (block exists (_.isErroneous))

This comment has been minimized.

@retronym

retronym Nov 6, 2014

Member

Traversing the typechecked expansion in search of errors seems a bit wasteful. If this is erroneous, there will have been an error issues (admittedly about synthetic code). Would be better to have this explanation automatically attached as an addendum to any errors issued during typechecking the expansion (in a manner similar to names/defaults.)

@retronym

retronym Nov 6, 2014

Member

Traversing the typechecked expansion in search of errors seems a bit wasteful. If this is erroneous, there will have been an error issues (admittedly about synthetic code). Would be better to have this explanation automatically attached as an addendum to any errors issued during typechecking the expansion (in a manner similar to names/defaults.)

This comment has been minimized.

@adriaanm

adriaanm Nov 7, 2014

Member

The problem is that we don't get any errors and the back-end freaks out. I have spent some time debugging this, but couldn't figure it out. I agree this isn't ready for production, but I think it's better than a crash for now.

@adriaanm

adriaanm Nov 7, 2014

Member

The problem is that we don't get any errors and the back-end freaks out. I have spent some time debugging this, but couldn't figure it out. I agree this isn't ready for production, but I think it's better than a crash for now.

This comment has been minimized.

@retronym

retronym Nov 7, 2014

Member

Fair enough. Can you include a test case that gets to this spot so we can easily pick up the investigation later?

@retronym

retronym Nov 7, 2014

Member

Fair enough. Can you include a test case that gets to this spot so we can easily pick up the investigation later?

This comment has been minimized.

@adriaanm

adriaanm Nov 7, 2014

Member

Will do.

@adriaanm

adriaanm Nov 7, 2014

Member

Will do.

@@ -422,6 +422,22 @@ private[internal] trait TypeMaps {
}
}
/**
* Get rid of BoundedWildcardType where variance allows us to do so.
* Invariant: `wildcardExtrapolation(tp) =:= tp`

This comment has been minimized.

@retronym

retronym Nov 6, 2014

Member

Some unit tests to show this would be handy.

@retronym

retronym Nov 6, 2014

Member

Some unit tests to show this would be handy.

}
if (samSym.exists && samSym.owner != correspondingFunctionSymbol) // don't treat Functions as SAMs
wildcardExtrapolation(normalize(tp memberInfo samSym))

This comment has been minimized.

@retronym

retronym Nov 6, 2014

Member

Why normalize?

@retronym

retronym Nov 6, 2014

Member

Why normalize?

This comment has been minimized.

@adriaanm

adriaanm Nov 7, 2014

Member

to turn it into a function type

@adriaanm

adriaanm Nov 7, 2014

Member

to turn it into a function type

@@ -2848,15 +2884,11 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
* Note that the arity of the sam must correspond to the arity of the function.
*/
val samViable = sam.exists && sameLength(sam.info.params, fun.vparams)
val ptNorm = if (samViable) samToFunctionType(pt, sam) else pt

This comment has been minimized.

@retronym

retronym Nov 6, 2014

Member

This will limit us to SAMs of 22 parameters, if we're happy with that restriction we'll at least need a good error message / test.

@retronym

retronym Nov 6, 2014

Member

This will limit us to SAMs of 22 parameters, if we're happy with that restriction we'll at least need a good error message / test.

This comment has been minimized.

@retronym

retronym Nov 7, 2014

Member

Which reminds me, we also need to consider by-name and repeated parameters, which are under-specced when it comes to anonymous function params, and have been a source of bugs.

With 2.11.4:

scala> trait LazySink { def accept(a: => Any): Unit }
defined trait LazySink

scala> val f: LazySink = (a) => (a, a)
f: LazySink = $anonfun$1@1fb26910

scala> f(println("!"))
<console>:10: error: LazySink does not take parameters
              f(println("!"))
               ^

scala> f.accept(println("!"))
!
!

This looks like a bug:

scala> trait RepeatedSink { def accept(a: Any*): Unit }
defined trait RepeatedSink

scala> val f: RepeatedSink = (a) => println(a)
f: RepeatedSink = $anonfun$1@4799abc2

scala> f.accept(1)
WrappedArray(WrappedArray(1))

scala> f.accept(1, 2)
WrappedArray(WrappedArray(1, 2))
@retronym

retronym Nov 7, 2014

Member

Which reminds me, we also need to consider by-name and repeated parameters, which are under-specced when it comes to anonymous function params, and have been a source of bugs.

With 2.11.4:

scala> trait LazySink { def accept(a: => Any): Unit }
defined trait LazySink

scala> val f: LazySink = (a) => (a, a)
f: LazySink = $anonfun$1@1fb26910

scala> f(println("!"))
<console>:10: error: LazySink does not take parameters
              f(println("!"))
               ^

scala> f.accept(println("!"))
!
!

This looks like a bug:

scala> trait RepeatedSink { def accept(a: Any*): Unit }
defined trait RepeatedSink

scala> val f: RepeatedSink = (a) => println(a)
f: RepeatedSink = $anonfun$1@4799abc2

scala> f.accept(1)
WrappedArray(WrappedArray(1))

scala> f.accept(1, 2)
WrappedArray(WrappedArray(1, 2))

This comment has been minimized.

@retronym

retronym Nov 7, 2014

Member

This bug comes from:

Apply(Ident(bodyName), fun.vparams map (p => Ident(p.name)))

Rather than something like:

// TreeGen
  def mkForwarder(target: Tree, vparamss: List[List[Symbol]]) =
    (target /: vparamss)((fn, vparams) => Apply(fn, vparams map paramToArg))
@retronym

retronym Nov 7, 2014

Member

This bug comes from:

Apply(Ident(bodyName), fun.vparams map (p => Ident(p.name)))

Rather than something like:

// TreeGen
  def mkForwarder(target: Tree, vparamss: List[List[Symbol]]) =
    (target /: vparamss)((fn, vparams) => Apply(fn, vparams map paramToArg))

This comment has been minimized.

@adriaanm

adriaanm Nov 7, 2014

Member

I'll have to defer this one. I tried Apply(Ident(bodyName), fun.vparams map gen.paramToArg), but no dice. I'm not sure how we would handle CBN params, but I guess we should support repeated ones somehow if java does?

@adriaanm

adriaanm Nov 7, 2014

Member

I'll have to defer this one. I tried Apply(Ident(bodyName), fun.vparams map gen.paramToArg), but no dice. I'm not sure how we would handle CBN params, but I guess we should support repeated ones somehow if java does?

This comment has been minimized.

@retronym

retronym Nov 7, 2014

Member

Fair enough. I tried this and it seems to work and pass partest-ack sam, but we can come back to it.

% git diff head
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index 5eee600..48345ab 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -2848,7 +2848,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
           Nil,
           List(fun.vparams),
           TypeTree(samMethTp.finalResultType) setPos sampos.focus,
-          Apply(Ident(bodyName), fun.vparams map (p => Ident(p.name)))
+          Apply(Ident(bodyName), fun.vparams map gen.paramToArg)
         )

       val serializableParentAddendum =

% cat sandbox/test.scala
trait RepeatedSink { def accept(a: Any*): Unit }

object Test {
  def main(args: Array[String]): Unit = {
    val f: RepeatedSink = (a) => println(a)
    f.accept(1)
  }
}
% qscalac -Xexperimental sandbox/test.scala && qscala Test
WrappedArray(1)
@retronym

retronym Nov 7, 2014

Member

Fair enough. I tried this and it seems to work and pass partest-ack sam, but we can come back to it.

% git diff head
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index 5eee600..48345ab 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -2848,7 +2848,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
           Nil,
           List(fun.vparams),
           TypeTree(samMethTp.finalResultType) setPos sampos.focus,
-          Apply(Ident(bodyName), fun.vparams map (p => Ident(p.name)))
+          Apply(Ident(bodyName), fun.vparams map gen.paramToArg)
         )

       val serializableParentAddendum =

% cat sandbox/test.scala
trait RepeatedSink { def accept(a: Any*): Unit }

object Test {
  def main(args: Array[String]): Unit = {
    val f: RepeatedSink = (a) => println(a)
    f.accept(1)
  }
}
% qscalac -Xexperimental sandbox/test.scala && qscala Test
WrappedArray(1)

This comment has been minimized.

@adriaanm

adriaanm Nov 7, 2014

Member

Sorry, not sure how I botched trying that. You're right. Added a commit.

@adriaanm

adriaanm Nov 7, 2014

Member

Sorry, not sure how I botched trying that. You're right. Added a commit.

[sammy] use correct type for method to override
Don't naively derive types for the single method's signature
from the provided function's type, as it may be a subtype
of the method's MethodType.

Instead, once the sam class type is fully defined, determine
the sam's info as seen from the class's type, and use those
to generate the correct override.

```
scala> Arrays.stream(Array(1, 2, 3)).map(n => 2 * n + 1).average.ifPresent(println)
5.0

scala> IntStream.range(1, 4).forEach(println)
1
2
3
```

Also, minimal error reporting

Can't figure out how to do it properly, but some reporting
is better than crashing. Right? Test case that illustrates
necessity of the clumsy stop gap `if (block exists (_.isErroneous))`
enclosed as `sammy_error_exist_no_crash`

added TODO for repeated and by-name params
@adriaanm

This comment has been minimized.

Show comment
Hide comment
Member

adriaanm commented Nov 7, 2014

@scala-jenkins scala-jenkins added tested and removed needs-attention labels Nov 7, 2014

[sammy] support repeated params
Generate correct trees to refer to repeated params using `gen.paramToArg`.
Based on retronym's review feedback.

@scala-jenkins scala-jenkins added tested and removed tested labels Nov 7, 2014

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Nov 9, 2014

Member

LGTM

Member

retronym commented Nov 9, 2014

LGTM

lrytz added a commit that referenced this pull request Nov 10, 2014

Merge pull request #4101 from adriaanm/sam-ex
[sammy] eta-expansion, overloading, existentials

@lrytz lrytz merged commit 5a7875f into scala:2.11.x Nov 10, 2014

1 check passed

default pr-scala Took 76 min.
Details
@@ -2701,6 +2725,22 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
* However T must be fully defined before we type the instantiation, as it'll end up as a parent type,
* which must be fully defined. Would be nice to have some kind of mechanism to insert type vars in a block of code,
* and have the instantiation of the first occurrence propagate to the rest of the block.
*
* TODO: by-name params

This comment has been minimized.

@VladUreche

VladUreche Jan 14, 2015

Member

@adriaanm, by this you mean automatically applying the SAM implementation's abstract method?

@VladUreche

VladUreche Jan 14, 2015

Member

@adriaanm, by this you mean automatically applying the SAM implementation's abstract method?

This comment has been minimized.

@adriaanm

adriaanm Jan 14, 2015

Member

Not sure what you're referring to, but I meant that f(println("!")) should behave the same as f.accept(println("!")) below.

@adriaanm

adriaanm Jan 14, 2015

Member

Not sure what you're referring to, but I meant that f(println("!")) should behave the same as f.accept(println("!")) below.

This comment has been minimized.

@VladUreche

VladUreche Jan 14, 2015

Member

Got it now, thanks @adriaanm!

@VladUreche

VladUreche Jan 14, 2015

Member

Got it now, thanks @adriaanm!

@VladUreche

This comment has been minimized.

Show comment
Hide comment
@VladUreche

VladUreche Jan 14, 2015

Member

Cool patch! I'm super excited about this making its way to 2.11.5 👍

Member

VladUreche commented Jan 14, 2015

Cool patch! I'm super excited about this making its way to 2.11.5 👍

ilinum added a commit to ilinum/intellij-scala that referenced this pull request Jul 29, 2015

SAM Overloading and existential bounds
In some cases existential bounds can be simplified without losing precision.

For example:
trait Blargle[T] { def compare(a: T, b: T): Int }

trait Test {
  def foo(a: Blargle[_ >: String]): Int
}

can be simplified to:

trait Test {
  def foo(a: Blargle[String]): Int
}

see: scala/scala#4101
#SCL8956 Fixed

@ilinum ilinum referenced this pull request Jul 29, 2015

Merged

SAM Fixes #189

ilinum added a commit to ilinum/intellij-scala that referenced this pull request Jul 31, 2015

SAM Overloading and existential bounds
In some cases existential bounds can be simplified without losing precision.

For example:
trait Blargle[T] { def compare(a: T, b: T): Int }

trait Test {
  def foo(a: Blargle[_ >: String]): Int
}

can be simplified to:

trait Test {
  def foo(a: Blargle[String]): Int
}

see: scala/scala#4101
#SCL8956 Fixed

ilinum added a commit to ilinum/intellij-scala that referenced this pull request Jul 31, 2015

SAM Overloading and existential bounds
In some cases existential bounds can be simplified without losing precision.

For example:
trait Blargle[T] { def compare(a: T, b: T): Int }

trait Test {
  def foo(a: Blargle[_ >: String]): Int
}

can be simplified to:

trait Test {
  def foo(a: Blargle[String]): Int
}

see: scala/scala#4101
#SCL8956 Fixed

@adriaanm adriaanm deleted the adriaanm:sam-ex branch Aug 5, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment