Fix problems with higher-kinded implicits #716

odersky · 2015-07-07T19:34:50Z

Closes #553 and #681. Review by @smarter

There ws a missing case, where a lambda projection `T#Apply` is compared to something to a type lambda. In that case we should compare the prefix `T` directly wiht the type lambda. With that modification i553 works if the canBuildFrom implicit is imported. But the implicit is not yet found in the type scope.

lookupRefined used to fall back to unbounded Wildcard, given a Wildcard prefix. But that loses bounds when called from wildApprox, which in turn loses companion objects in the implicit scope of a type. In `i553-shuffle.scala` this happened because we projected with #Apply on a type that approximated to a bounded wildcard. The fix keeps the bounds, projecting in turn on them.

odersky · 2015-07-07T20:20:13Z

/rebuild

odersky · 2015-07-07T21:33:38Z

/rebuild

odersky · 2015-07-07T21:37:38Z

@smarter:

([T] => List[T])[Int] # Apply   <:   [T] => Traversable[T]

or, expanded

Lambda$P { type Apply = List[$HK0] } { $HK0 = Int } # Apply 
<:
Lambda$P { type Apply = Traversable[$HK0] }

smarter · 2015-07-07T22:21:49Z

src/dotty/tools/dotc/core/Types.scala

+          pre.optBounds match {
+            case TypeBounds(lo, hi) if name.isTypeName =>
+              // for term names, forming a Termref with just the name risks losing overloaded instance
+              WildcardType(TypeBounds(NamedType(lo, name), NamedType(hi, name)))


The comment on https://github.com/dotty-staging/dotty/blob/fix/shuffle/src/dotty/tools/dotc/core/Types.scala#L1423-L1424 is now wrong since the wildcard is now bounded, not unbounded. it'd be nice to update it and maybe also document the handling of wildcard in lookupRefined itself.

So, does this mean that we consider (_ >: Foo <: Bar)#T to be equivalent to _ >: Foo#T <: Bar#T ? If so, isn't this wrong in general? For example, if T is contravariant we may have Foo <: Bar but Foo#T >: Bar#T so the bounds _ >: Foo#T <: Bar#T would not be satisfiable.

I think that having a few test cases would be helpful.

@smarter Good point about the change in lookupRefined. I have backed out of it, and replaced it with a different solution.

Test cases: We have Random.shuffle, I think that's already a good one. If you have more, please add them.

This reverts commit ec2d7d6.

lookupRefined on a Wildcard prefix will return an unbounded Wildcard type. This can destroy information that's important for determining the companions making up the implicit scope of a type. We handle this case by maintaining a collection for types that were discarded in that way and taking these types intoa ccount separately when computing companions. Examples where this is necessary include `i553-shuffle.scala` abd `vector1.scala`.

smarter · 2015-07-08T23:14:08Z

Otherwise LGTM, though I wonder if it's possible that we may add something to theMap.discarded in wildApprox which should not be part of the implicit scope (which is determined by ImplicitRunInfo#implicitScope).

smarter · 2015-07-08T23:42:51Z

I also suggest adding the test case https://raw.githubusercontent.com/retronym/scala/e439651befb85e6c3b131db6bf346513ecedc783/test/pending/pos/t6948.scala which fails in Scala 2.11 (see https://issues.scala-lang.org/browse/SI-6948?focusedCommentId=61848&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-61848) but works fine in Dotty (yay!)

retronym · 2015-07-09T00:45:05Z

@smarter I would suggest to add make that test case independent of the collections library before using it. It will be less fragile and easier to reason about in isolation.

Why does it work in dotty? Here's an approach to quickly and directly see how higher kinded inference works:

scala> def foo[M[_], A](m: M[A]): M[A] = ???
warning: there was one feature warning; re-run with -feature for details
foo: [M[_], A](m: M[A])M[A]

scala> def bar = foo(null: T[Int] with U[String])
bar: T[Int]

scala> def bar = foo(null: U[String] with T[Int])
bar: U[String]

scala> def bar = foo(null: U[String] with T[Int] {})
bar: U[String]

scala> type Alias = U[String]
defined type alias Alias

scala> def bar = foo(null: Alias)
bar: U[String]

The logic in play is here: https://github.com/scala/scala/blob/v2.11.6/src/reflect/scala/reflect/internal/Types.scala#L3067-L3087

odersky · 2016-02-16T15:18:40Z

I verified that the "shuffle" test no longer fails. So this seems to be fixed by other means now.

smarter · 2016-02-16T15:43:14Z

The shuffle test and many others were fixed by #799 .The bug that dotty-staging@ec2d7d6 fixes probably still exists but we don't see it anymore because #799 changed inference such that we no longer end up with wildcard prefixes for implicit higher-kinded types. I could try to construct a test case for it, however I'm aware of several issues with implicit scope caching that I'd like to fix first since they might overlap with this.

odersky · 2016-02-16T15:54:42Z

Should we leave the PR open then?

On Tue, Feb 16, 2016 at 4:43 PM, Guillaume Martres <notifications@github.com

wrote:

The shuffle test and many others were fixed by #799
#799 .The bug that dotty-staging@
ec2d7d6
dotty-staging@ec2d7d6
fixes probably still exists but we don't see it anymore because #799
#799 changed inference such that
we no longer end up with wildcard prefixes for implicit higher-kinded
types. I could try to construct a test case for it, however I'm aware of
several issues with im plicit scope caching that I'd like to fix first
since they might overlap with this.

—
Reply to this email directly or view it on GitHub
#716 (comment).

Martin Odersky
EPFL

smarter · 2016-02-16T15:58:03Z

Sure.

smarter · 2016-05-31T16:02:44Z

The bug that dotty-staging/dotty-1@ec2d7d6 fixes probably still exists but we don't see it anymore because #799 changed inference such that we no longer end up with wildcard prefixes for implicit higher-kinded types. I could try to construct a test case for it.

The only test cases I can come up are of this form:

class X {
  type Elem
}
object X {
  implicit def foo: X#Elem = ???
}

object Test {
  def get[T <: X](implicit ev: T#Elem) = ev
  get
}

Which does not compile anyway because of the restrictions on type projections that #1051 added, so we can probably close this for now, but keep this in mind if we ever loosen these restrictions.

odersky added 2 commits July 7, 2015 18:57

smarter reviewed Jul 7, 2015
View reviewed changes

odersky added 2 commits July 8, 2015 17:20

Revert "Refine lookupRefined for Wildcard prefixes"

87cfcf9

This reverts commit ec2d7d6.

smarter mentioned this pull request Jul 22, 2015

Inference of higher-kinded type parameters is not precise enough #732

Closed

odersky closed this Aug 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix problems with higher-kinded implicits #716

Fix problems with higher-kinded implicits #716

odersky commented Jul 7, 2015

odersky commented Jul 7, 2015

odersky commented Jul 7, 2015

odersky commented Jul 7, 2015

smarter Jul 7, 2015

smarter Jul 7, 2015

smarter Jul 7, 2015

odersky Jul 8, 2015

smarter commented Jul 8, 2015

smarter commented Jul 8, 2015

retronym commented Jul 9, 2015

odersky commented Feb 16, 2016

smarter commented Feb 16, 2016

odersky commented Feb 16, 2016

smarter commented Feb 16, 2016

smarter commented May 31, 2016

Fix problems with higher-kinded implicits #716

Fix problems with higher-kinded implicits #716

Conversation

odersky commented Jul 7, 2015

odersky commented Jul 7, 2015

odersky commented Jul 7, 2015

odersky commented Jul 7, 2015

smarter Jul 7, 2015

Choose a reason for hiding this comment

smarter Jul 7, 2015

Choose a reason for hiding this comment

smarter Jul 7, 2015

Choose a reason for hiding this comment

odersky Jul 8, 2015

Choose a reason for hiding this comment

smarter commented Jul 8, 2015

smarter commented Jul 8, 2015

retronym commented Jul 9, 2015

odersky commented Feb 16, 2016

smarter commented Feb 16, 2016

odersky commented Feb 16, 2016

smarter commented Feb 16, 2016

smarter commented May 31, 2016