Skip to content

Fix #3443: adapt child compatibility check #3446

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

Merged
merged 3 commits into from
Nov 11, 2017

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