Skip to content

Conversation

liufengyun
Copy link
Contributor

@liufengyun liufengyun commented Nov 8, 2017

Fix #3443

Previously, the child compatibility check will result in a check like CNil <: TVar(Nothing..Coproduct), which failed. Now we expose type param according to its variance. We only resort to TVar if the variance is 0.

This PR also removes the @unchecked check, as agreed with @allanrenucci the check should be somewhere else and handle more complex cases.

Copy link
Contributor

@OlivierBlanvillain OlivierBlanvillain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! It looks cleaner without the refineCtx 😄

@@ -778,41 +780,41 @@ class SpaceEngine(implicit ctx: Context) extends SpaceLogic {
*
* A <: X :> Y B <: U :> V M { type T <: A :> B } ~~> M { type T <: X :> V }
*/
def expose(tp: Type, refineCtx: Boolean = false, up: Boolean = true): Type = tp match {
def expose(tp: Type, up: Boolean = true): Type = tp match {
Copy link
Contributor

@OlivierBlanvillain OlivierBlanvillain Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the default = true here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just removed the whole method :)

else if (variance == 1) expose(tp, true)
else expose(tp, false)

debug.println(s"$tp exposed to =====> " + exposed)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could put $exposed inside the string

@odersky odersky merged commit 6dab4c2 into scala:master Nov 11, 2017
@liufengyun liufengyun deleted the fix-3443 branch November 12, 2017 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants