Skip to content
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

Regression with type inference and type constructors #7517

Closed
scabug opened this issue May 25, 2013 · 20 comments
Closed

Regression with type inference and type constructors #7517

scabug opened this issue May 25, 2013 · 20 comments
Assignees
Labels
Milestone

Comments

@scabug
Copy link

@scabug scabug commented May 25, 2013

The following compiles in 2.10.1, but not in 2.10.2-RC1. This has a straightforward workaround (see comments) and is not urgent.

trait Box[ K[A[x]] ]

object Box {
   // type constructor composition
   sealed trait [A[_], B[_]] { type l[T] = A[B[T]] }

   // composes type constructors inside K
   type SplitBox[K[A[x]], B[x]] = Box[ ({ type l[A[x]] = K[ (AB)#l] })#l ]

   def split[ K[A[x]], B[x] ](base: Box[K]): SplitBox[K,B] = ???

   class Composed[B[_], K[A[x]] ] {
      val box: Box[K] = ???

      type Split[ A[x] ] = K[ (AB)#l ]
      val a: Box[Split] = Box.split(box)

      //Either of these work:
      //val a: Box[Split] = Box.split[K,B](box)
      //val a: Box[ ({ type l[A[x]] = K[ (A ∙ B)#l ] })#l ] = Box.split(box)
   }
}

Error:

Box.scala:16: type mismatch;
 found   : Box[K]
 required: Box[K with Composed.this.Split]
Note: K >: K with Composed.this.Split, but trait Box is invariant in type K.
You may wish to define K as -K instead. (SLS 4.5)
      val a: Box[Split] = Box.split(box)
                                    ^
@scabug
Copy link
Author

@scabug scabug commented May 25, 2013

Imported From: https://issues.scala-lang.org/browse/SI-7517?orig=1
Reporter: @harrah
Affected Versions: 2.10.2-RC1

@scabug
Copy link
Author

@scabug scabug commented May 25, 2013

@paulp said:
This one is 7e52fb910b . I think it's a certifiable miracle that it used to compile.

@scabug
Copy link
Author

@scabug scabug commented May 25, 2013

@paulp said:
Ha, look at paulp calling it in #7226. "and it's not a risk-free fix"

Of course one can usually take that position without extending one's neck too far (with the compiler, one always can) and yet, somehow it has the air of true prophecy.

@scabug
Copy link
Author

@scabug scabug commented May 25, 2013

@retronym said:
It will be interesting to trace this through at and just before scala/scala@0cde930. 7e52fb910b was designed to restore the pre-0cde930b state of affairs.

@scabug
Copy link
Author

@scabug scabug commented May 25, 2013

@paulp said:
That's why I was surprised it used to compile!

@scabug
Copy link
Author

@scabug scabug commented May 25, 2013

@paulp said:
It compiles both before and after 0cde930b . The point between 2.9 and 2.10 when it started working was 658ba1b4e6, which makes sense given the test case:

// test/files/pos/hkrange.scala
class A {
  def f[CC[X] <: Traversable[X]](x: CC[Int]) = ()
  
  f(1 to 5)
}
 commit 658ba1b4e6
 Author: Paul Phillips <paulp@improving.org>
 Date:   1 year, 10 months ago
 
     Fixed adriaan's patch for type constructor infe...
     
     Fixed adriaan's patch for type constructor inference. The problem with
     haranguing people in bars about bugs is that the fixes with which they
     provide you may be flawed. Fortunately moors has this novelist on
     retainer. Review by moors.

@scabug
Copy link
Author

@scabug scabug commented May 26, 2013

@retronym said:
Marking as critical for 2.10.2, not for SBT, but because we need to characterize the nature of the regression more precisely. I'll do that in the coming days.

@scabug
Copy link
Author

@scabug scabug commented May 27, 2013

@retronym said:
One more data point: Applying 7e52fb910b immediately after 0cde930b compiles the test case successfully. Another change also appears to be in play.

@scabug
Copy link
Author

@scabug scabug commented May 28, 2013

@retronym said:
The actual regression (or progression, perhaps), comes from the use of dealiasWidenChain in unifyFull.

scala/scala@e5da30b#L5L3176

// old
tpes = List(Composed.this.Split[A], K[[T]A[B[T]]])

//new 
tpe.dealiasWidenChain.toList = List(Composed.this.Split[A], K[[T]A[B[T]]])

@scabug
Copy link
Author

@scabug scabug commented May 28, 2013

@paulp said:
Have I gone blind, or are those the same lists of types?

@scabug
Copy link
Author

@scabug scabug commented May 28, 2013

@paulp said:
This fixes it:

 -        tpe.dealiasWidenChain exists unifySpecific
 +        val tpes = if (isLowerBound) tpe.dealiasWidenChain else tpe :: Nil
 +        tpes exists unifySpecific

@scabug
Copy link
Author

@scabug scabug commented May 28, 2013

@paulp said:
But, unsurprisingly, breaks other things.

% qscalac -d /tmp test/files/pos/t6846.scala 
test/files/pos/t6846.scala:6: error: polymorphic expression cannot be instantiated to expected type;
 found   : [M[_], A]Test.Arb[M[A]]
 required: Test.Arb[Test.ListInt]
  foo: Arb[ListInt]
  ^
test/files/pos/t6846.scala:25: error: polymorphic expression cannot be instantiated to expected type;
 found   : [M[_], A]Test2.Carb[M[A]]
 required: Test2.Carb[Test2.ListInt]
  bar: Carb[ListInt]
  ^
test/files/pos/t6846.scala:26: error: polymorphic expression cannot be instantiated to expected type;
 found   : [M[_], A]Test2.Carb[M[A]]
 required: Test2.Carb[Test2.ListSingletonX]
  bar: Carb[ListSingletonX]
  ^
test/files/pos/t6846.scala:27: error: polymorphic expression cannot be instantiated to expected type;
 found   : [M[_], A]Test2.Carb[M[A]]
 required: Test2.Carb[Test2.ListSingletonY]
  bar: Carb[ListSingletonY]
  ^
four errors found

@scabug
Copy link
Author

@scabug scabug commented May 28, 2013

@retronym said:
Sorry, I pasted the wrong snippet.

Here are all of the before/after pairs. I'm not sure about the difference between the first and second attempt to do the old ad-hoc version of dealiasWidenChain for Composed.this.Split[A].


tpes = List(Composed.this.Split[A], K[[T]A[B[T]]])
tpe.dealiasWidenChain.toList = List(Composed.this.Split[A], K[[T]A[B[T]]])

tpes = List(T)
tpe.dealiasWidenChain.toList = List(T)
tpes = List(Any)
tpe.dealiasWidenChain.toList = List(Any)
tpes = List(B[T])
tpe.dealiasWidenChain.toList = List(B[T])
tpes = List(B[T])
tpe.dealiasWidenChain.toList = List(B[T])
tpes = List(B[T])
tpe.dealiasWidenChain.toList = List(B[T])
tpes = List(B[T])
tpe.dealiasWidenChain.toList = List(B[T])

tpes = List(Composed.this.Split[A])
tpe.dealiasWidenChain.toList = List(Composed.this.Split[A], K[[T]A[B[T]]])

tpes = List(x)
tpe.dealiasWidenChain.toList = List(x)
tpes = List(B[T])
tpe.dealiasWidenChain.toList = List(B[T])
tpes = List(B[T])
tpe.dealiasWidenChain.toList = List(B[T])
tpes = List(B[T])
tpe.dealiasWidenChain.toList = List(B[T])
tpes = List(B[T])
tpe.dealiasWidenChain.toList = List(B[T])

@scabug
Copy link
Author

@scabug scabug commented May 28, 2013

@retronym said (edited on May 28, 2013 9:48:42 PM UTC):
Here's another idea.

-addBound(tp.typeConstructor)
-isSubArgs(lhs, rhs, params, AnyDepth)
+isSubArgs(lhs, rhs, params, AnyDepth) && {addBound(tp.typeConstructor); true}

With this patch, both this test and t6846.scala compile. A quick run of pt --pos --neg is also green.

What I'm trying to avoid, is registering both type constructors K and Split as bounds, which leads to them later being glb-ed in a seemingly nonsensical way. (What does RefinementType(parents = K, Split) mean, anyway?)

@scabug
Copy link
Author

@scabug scabug commented May 28, 2013

@retronym said:
Oh, and we should stay open to the idea of a conservative partial-revert for 2.10.2; leaving the a progression for 2.11.x. I'm not quite sure how that would play out as #6846 itself fixed a regression.

I suppose it would be good to try to construct a simpler test that exhibits the same problem to gauge where this lands on the esoteric-o-meter.

Paul, perhaps could you discuss this one with Adriaan?

@scabug
Copy link
Author

@scabug scabug commented May 29, 2013

@JamesIry said (edited on May 29, 2013 8:31:37 PM UTC):
I like the proposed fix (as opposed to reverting), but I'm not qualified to like it. I'll talk to Adriaan.

@scabug
Copy link
Author

@scabug scabug commented May 29, 2013

@adriaanm said:
I think Jason's fix makes sense.

More generally, I think this is a good reminder that we've hit the limit of what we can fix in 2.10.x, especially regarding type checking.
We should probably start targeting master with fixes by default, and stop merging 2.10.x into master when it becomes too burdensome.

@scabug
Copy link
Author

@scabug scabug commented May 29, 2013

@retronym said:
I'm about to submit a PR with this change. Just collecting a trace unify* for this test case to help to explain things.

@scabug
Copy link
Author

@scabug scabug commented May 29, 2013

@scabug scabug closed this May 30, 2013
@scabug
Copy link
Author

@scabug scabug commented May 31, 2013

@paulp said:
Also: scala/scala#2615

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

Successfully merging a pull request may close this issue.

None yet
2 participants