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

Propagate overloaded function type to expected arg type #5307

Merged
merged 1 commit into from Aug 13, 2016

Conversation

Projects
None yet
5 participants
@adriaanm
Member

adriaanm commented Jul 27, 2016

Infer missing parameter types for function literals passed
to higher-order overloaded methods by deriving the
expected argument type from the function types in the
overloaded method type's argument types.

This eases the pain caused by methods becoming overloaded
because SAM types and function types are compatible,
which used to disable parameter type inference because
for overload resolution arguments are typed without
expected type, while typedFunction needs the expected
type to infer missing parameter types for function literals.

It also aligns us with dotty. The special case for
function literals seems reasonable, as it has precedent,
and it just enables the special case in typing function
literals (derive the param types from the expected type).

Since this does change type inference, you can opt out
using the Scala 2.11 source level.

Fix scala/scala-dev#157

@adriaanm adriaanm changed the title from Issue 157 to Issue 157 [ci: last-only] Jul 27, 2016

@adriaanm adriaanm changed the title from Issue 157 [ci: last-only] to Propagate overloaded function type to expected arg type Jul 28, 2016

@adriaanm adriaanm added this to the 2.12.0-RC1 milestone Jul 28, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jul 28, 2016

Member

@DarkDimius, @smarter -- does this sound like what you are doing in Dotty? I tried to understand how this is implemented in dotty (lampepfl/dotty#1378?) but couldn't figure out how you actually propagate the argument types of the function types in the arguments of the overloaded method type.

Member

adriaanm commented Jul 28, 2016

@DarkDimius, @smarter -- does this sound like what you are doing in Dotty? I tried to understand how this is implemented in dotty (lampepfl/dotty#1378?) but couldn't figure out how you actually propagate the argument types of the function types in the arguments of the overloaded method type.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Jul 28, 2016

Member

IIRC, dotty does something questionable for:

trait C[A] {
  def foo(a: A => String, z: A): Unit
  def foo(a: Int => String, z: Int): Unit
}

trait Test {
  def test(c: C[Int]) = c.foo(x => x.toString, 0)
}

When computing the members of C[Int], it considers the two foos to overlap and picks one over the other. This happens in:

      at dotty.tools.dotc.core.Denotations$DenotUnion.matches(Denotations.scala:970)
      at dotty.tools.dotc.core.Denotations$SingleDenotation.filterDisjoint(Denotations.scala:817)
      at dotty.tools.dotc.core.Denotations$SingleDenotation.mapInherited(Denotations.scala:821)
      at dotty.tools.dotc.core.Denotations$SingleDenotation.mapInherited(Denotations.scala:457)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation$$anonfun$computeNPMembersNamed$1.collect$1(SymDenotations.scala:1578)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation$$anonfun$computeNPMembersNamed$1.apply(SymDenotations.scala:1586)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation$$anonfun$computeNPMembersNamed$1.apply(SymDenotations.scala:1563)
      at dotty.tools.dotc.util.Stats$.track(Stats.scala:36)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation.computeNPMembersNamed(SymDenotations.scala:1562)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation.nonPrivateMembersNamed(SymDenotations.scala:1552)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation.membersNamed(SymDenotations.scala:1539)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation.findMember(SymDenotations.scala:1591)

I remember discussing this on a previous dotty ticket or PR, but I can't find it at the moment.

Member

retronym commented Jul 28, 2016

IIRC, dotty does something questionable for:

trait C[A] {
  def foo(a: A => String, z: A): Unit
  def foo(a: Int => String, z: Int): Unit
}

trait Test {
  def test(c: C[Int]) = c.foo(x => x.toString, 0)
}

When computing the members of C[Int], it considers the two foos to overlap and picks one over the other. This happens in:

      at dotty.tools.dotc.core.Denotations$DenotUnion.matches(Denotations.scala:970)
      at dotty.tools.dotc.core.Denotations$SingleDenotation.filterDisjoint(Denotations.scala:817)
      at dotty.tools.dotc.core.Denotations$SingleDenotation.mapInherited(Denotations.scala:821)
      at dotty.tools.dotc.core.Denotations$SingleDenotation.mapInherited(Denotations.scala:457)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation$$anonfun$computeNPMembersNamed$1.collect$1(SymDenotations.scala:1578)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation$$anonfun$computeNPMembersNamed$1.apply(SymDenotations.scala:1586)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation$$anonfun$computeNPMembersNamed$1.apply(SymDenotations.scala:1563)
      at dotty.tools.dotc.util.Stats$.track(Stats.scala:36)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation.computeNPMembersNamed(SymDenotations.scala:1562)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation.nonPrivateMembersNamed(SymDenotations.scala:1552)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation.membersNamed(SymDenotations.scala:1539)
      at dotty.tools.dotc.core.SymDenotations$ClassDenotation.findMember(SymDenotations.scala:1591)

I remember discussing this on a previous dotty ticket or PR, but I can't find it at the moment.

@odersky

This comment has been minimized.

Show comment
Hide comment
@odersky

odersky Jul 29, 2016

Contributor

@adriaanm The dotty PR that introduced something similar was:

lampepfl/dotty#1160

Hope this helps!

Contributor

odersky commented Jul 29, 2016

@adriaanm The dotty PR that introduced something similar was:

lampepfl/dotty#1160

Hope this helps!

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Jul 29, 2016

Member

It does, thanks Martin! At first sight, looks like we ended up at the same solution, but I'll experiment a bit more to make sure.

Member

adriaanm commented Jul 29, 2016

It does, thanks Martin! At first sight, looks like we ended up at the same solution, but I'll experiment a bit more to make sure.

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Aug 10, 2016

Member

I started reviewing this and hit a weird case. Not introduced by this PR, it already exists in M5, but it looks related:

scala> trait MF1[-A,+R] { def apply(a: A): R }
defined trait MF1

scala> (x => x) : MF1[String, String]
res0: MF1[String,String] = $$Lambda$1140/992955027@213bd3d5

scala> trait A extends MF1[String, String]
defined trait A

scala> (x => x) : A
res1: A = $$Lambda$1139/1562090557@69e05f61

scala> (x => x) : Function1[String, String]
res2: String => String = $$Lambda$1149/102709691@249e0271

So far so good, but now:

scala> trait B extends Function1[String, String]
defined trait B

scala> (x => x) : B
<console>:13: error: missing parameter type
       (x => x) : B
        ^

Adding the param type: note that we don't get an indyLMF-lambda, but an AOT anonymous class

scala> ((x: String) => x) : B
res4: B = <function1>
Member

lrytz commented Aug 10, 2016

I started reviewing this and hit a weird case. Not introduced by this PR, it already exists in M5, but it looks related:

scala> trait MF1[-A,+R] { def apply(a: A): R }
defined trait MF1

scala> (x => x) : MF1[String, String]
res0: MF1[String,String] = $$Lambda$1140/992955027@213bd3d5

scala> trait A extends MF1[String, String]
defined trait A

scala> (x => x) : A
res1: A = $$Lambda$1139/1562090557@69e05f61

scala> (x => x) : Function1[String, String]
res2: String => String = $$Lambda$1149/102709691@249e0271

So far so good, but now:

scala> trait B extends Function1[String, String]
defined trait B

scala> (x => x) : B
<console>:13: error: missing parameter type
       (x => x) : B
        ^

Adding the param type: note that we don't get an indyLMF-lambda, but an AOT anonymous class

scala> ((x: String) => x) : B
res4: B = <function1>
@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Aug 10, 2016

Member

The problem of requiring a param type seems to be here:

if (samSym.exists && samSym.owner != correspondingFunctionSymbol) // don't treat Functions as SAMs

The sam symbol is the apply method inherited from Function1, so sam.owner == correspondingFunctionSymbol. The expected type is therefore set to NoType.

This simple fix seems fine: lrytz@b7a5f0e

Member

lrytz commented Aug 10, 2016

The problem of requiring a param type seems to be here:

if (samSym.exists && samSym.owner != correspondingFunctionSymbol) // don't treat Functions as SAMs

The sam symbol is the apply method inherited from Function1, so sam.owner == correspondingFunctionSymbol. The expected type is therefore set to NoType.

This simple fix seems fine: lrytz@b7a5f0e

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Aug 10, 2016

Member

The problem of not using indyLMF is because the parent Function1 has a constructor method $init$, so compilesToPureInterface is false (

ok(tpSym) && tpSym.ancestors.forall(sym => (sym eq AnyClass) || (sym eq ObjectClass) || ok(sym))
). It seems that the presence of any concrete trait method causes an $init$ method to be added, which disqualifies the interface for LMF:

scala> trait A { def f(x: Int): Int }
defined trait A

scala> (x => x) : A
res0: A = $$Lambda$1722/816302479@36361ddb

scala> trait B { def f(x: Int): Int ; def blip = 1 }
defined trait B

scala> (x => x) : B
res1: B = $anonfun$1@7a2b1eb4

Related to scala/scala-dev#191

Maybe we should allow no-op init methods?

Member

lrytz commented Aug 10, 2016

The problem of not using indyLMF is because the parent Function1 has a constructor method $init$, so compilesToPureInterface is false (

ok(tpSym) && tpSym.ancestors.forall(sym => (sym eq AnyClass) || (sym eq ObjectClass) || ok(sym))
). It seems that the presence of any concrete trait method causes an $init$ method to be added, which disqualifies the interface for LMF:

scala> trait A { def f(x: Int): Int }
defined trait A

scala> (x => x) : A
res0: A = $$Lambda$1722/816302479@36361ddb

scala> trait B { def f(x: Int): Int ; def blip = 1 }
defined trait B

scala> (x => x) : B
res1: B = $anonfun$1@7a2b1eb4

Related to scala/scala-dev#191

Maybe we should allow no-op init methods?

@lrytz

This comment has been minimized.

Show comment
Hide comment
@lrytz

lrytz Aug 10, 2016

Member

the change looks good so far - i'll avoid the magic letters for now as you mention you'd like to experiment a bit more.

Member

lrytz commented Aug 10, 2016

the change looks good so far - i'll avoid the magic letters for now as you mention you'd like to experiment a bit more.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 10, 2016

Member

Thanks, I'll also incorporate your fix. Regarding no-op ctors, maybe we should look into that before RC1? :-s OR we could reuse FunctionalInterface as an indication that a trait should not receive a ctor, and add it as an other sufficient condition for sammyness.

Member

adriaanm commented Aug 10, 2016

Thanks, I'll also incorporate your fix. Regarding no-op ctors, maybe we should look into that before RC1? :-s OR we could reuse FunctionalInterface as an indication that a trait should not receive a ctor, and add it as an other sufficient condition for sammyness.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 10, 2016

Member

OR we could reuse FunctionalInterface as an indication that a trait should not receive a ctor, and add it as an other sufficient condition for sammyness.

I like this idea a lot.

Member

retronym commented Aug 10, 2016

OR we could reuse FunctionalInterface as an indication that a trait should not receive a ctor, and add it as an other sufficient condition for sammyness.

I like this idea a lot.

@retronym

This comment has been minimized.

Show comment
Hide comment
@retronym

retronym Aug 10, 2016

Member

Maybe we should allow no-op init methods?

The problem here is separate compilation.

Member

retronym commented Aug 10, 2016

Maybe we should allow no-op init methods?

The problem here is separate compilation.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 10, 2016

Member

Tracking the @FunctionalInterface idea as scala/scala-dev#197

Member

adriaanm commented Aug 10, 2016

Tracking the @FunctionalInterface idea as scala/scala-dev#197

@adriaanm

This comment has been minimized.

Show comment
Hide comment
Member

adriaanm commented Aug 11, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 11, 2016

Member

With the community build green, that concludes my experimentation/sanity checking :-)

Member

adriaanm commented Aug 11, 2016

With the community build green, that concludes my experimentation/sanity checking :-)

Propagate overloaded function type to expected arg type
Infer missing parameter types for function literals passed
to higher-order overloaded methods by deriving the
expected argument type from the function types in the
overloaded method type's argument types.

This eases the pain caused by methods becoming overloaded
because SAM types and function types are compatible,
which used to disable parameter type inference because
for overload resolution arguments are typed without
expected type, while typedFunction needs the expected
type to infer missing parameter types for function literals.

It also aligns us with dotty. The special case for
function literals seems reasonable, as it has precedent,
and it just enables the special case in typing function
literals (derive the param types from the expected type).

Since this does change type inference, you can opt out
using the Scala 2.11 source level.

Fix scala/scala-dev#157
@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 12, 2016

Member

Rebased.

Member

adriaanm commented Aug 12, 2016

Rebased.

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 13, 2016

Member

/rebuild

Member

adriaanm commented Aug 13, 2016

/rebuild

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Aug 13, 2016

Member

LGTM-echo of Lukas's earlier review (which was pending my experimenting a bit with the community build)

Member

adriaanm commented Aug 13, 2016

LGTM-echo of Lukas's earlier review (which was pending my experimenting a bit with the community build)

@adriaanm adriaanm merged commit 3cb45e1 into scala:2.12.x Aug 13, 2016

5 checks passed

cla @adriaanm signed the Scala CLA. Thanks!
Details
integrate-ide [2889] SUCCESS. Took 5 s.
Details
validate-main [3273] SUCCESS. Took 116 min.
Details
validate-publish-core [3207] SUCCESS. Took 3 min.
Details
validate-test [2816] SUCCESS. Took 76 s.
Details

SethTisue added a commit to SethTisue/make-release-notes that referenced this pull request Sep 15, 2016

@adriaanm

This comment has been minimized.

Show comment
Hide comment
@adriaanm

adriaanm Oct 3, 2016

Member

This caused a regression. Maybe we need to follow dotty more closely and require identical argument types.. :-/

Member

adriaanm commented Oct 3, 2016

This caused a regression. Maybe we need to follow dotty more closely and require identical argument types.. :-/

adriaanm added a commit to adriaanm/scala that referenced this pull request Oct 4, 2016

SI-9943 sealed class does not yield SAM type
This is a bit of a sneaky fix for this bug, but our hand
is pretty much forced by other constraints, in this intersection
of overload resolution involving built-in function types and SAMs,
and type inference for higher-order function literals (#5307).

Luckily, in this particular issue, the overloading clash seems accidental.
The `<:<` class is not a SAM type as it cannot be subclassed outside
of `Predef`. For simplicity, we don't consider where the SAM conversion
occurs and exclude all sealed classes from yielding SAM types.

adriaanm added a commit to adriaanm/scala that referenced this pull request Oct 4, 2016

SI-9943 final/sealed class does not yield SAM type
Cannot subclass such a class. (Well, we could subclass a sealed
class in the same compilation unit. We ignore this for simplicity.)

This is a bit of a sneaky fix for this bug, but our hand
is pretty much forced by other constraints, in this intersection
of overload resolution involving built-in function types and SAMs,
and type inference for higher-order function literals (#5307).

Luckily, in this particular issue, the overloading clash seems
accidental. The `sealed` `<:<` class is not a SAM type as it
cannot be subclassed outside of `Predef`. For simplicity,
we don't consider where the SAM conversion occurs and exclude
all sealed classes from yielding SAM types.

Thanks to Miles for pointing out that `final` was missing in my
first iteration of this fix.

@adriaanm adriaanm modified the milestone: 2.12.0-RC1 Oct 29, 2016

@adriaanm adriaanm added the 2.12 label Oct 29, 2016

@lrytz lrytz referenced this pull request Nov 1, 2016

Closed

notes on possible 2.12 release notes improvements #202

12 of 16 tasks complete

szeiger added a commit to szeiger/scala that referenced this pull request Feb 15, 2017

Extend propagation of overloaded function types to partial functions
The original implementation of this feature was done in
scala#5307. This change extends the
function type propagation to pattern-matching anonymous functions.

szeiger added a commit to szeiger/scala that referenced this pull request Mar 20, 2017

Eliminate non-matching overloads early for partial function literals
This was already done for function literals. It is important in cases
where only one overload of a method takes a `Function1` or
`PartialFunction`, for example:

    def map[U](f: T => U): R[U]
    def map[U](f: (A, B) => U): R[C]

By recognizing that a partial function literal always conforms to
`PartialFunction[Any, Nothing]`, in a call such as

    m.map { case (a, b) => a }

the non-matching overload (which takes a `Function2`) can be eliminated
early, thus allowing the known types `A` and `B` to be used during
type-checking of the partial function (which would otherwise fail).

Propagation of overloaded function types (originally implemented in
scala#5307) is extended to cover pattern-
matching anonymous functions, too.

@szeiger szeiger referenced this pull request Jul 4, 2017

Merged

Add collect #117

@adriaanm adriaanm referenced this pull request Nov 1, 2017

Merged

Akka Typed #3

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