Skip to content

Fix #1490: type test of union types via type alias #1491

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 1 commit into from
Sep 1, 2016

Conversation

liufengyun
Copy link
Contributor

Make type erasure accept one additional parameter:

  • eraseOrType: Whether erase OrType to its lowest uppper bound

*/
def erasure(tp: Type)(implicit ctx: Context): Type =
erasureFn(isJava = false, semiEraseVCs = false, isConstructor = false, wildcardOK = false)(tp)(erasureCtx)
* @param eraseOrType Whether erase OrType to its lowest uppper bound
Copy link
Member

Choose a reason for hiding this comment

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

I would write Whether OrType should be erased to its lowest upper bound

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will address in the next commit.

@odersky
Copy link
Contributor

odersky commented Sep 1, 2016

I am missing the context. Why do we need to make this distinction? The way it is done it adds a significant cost to compiler complexity. Adding a new boolean parameter to a core method should not be taken lightly, because you just have doubled all possible error scenarios!

@smarter
Copy link
Member

smarter commented Sep 1, 2016

transformIsInstanceOf takes erased arguments but sometimes you don't actually want to erase things because you're going to do transformations on the isInstanceOf checks like for union types and singleton types. The alternative would be to rewrite transformIsInstanceOf to expect non-erased arguments and only erase them after doing the rewritings it needs to do, that might be better indeed.

@smarter
Copy link
Member

smarter commented Sep 1, 2016

By the way, the same thing is true in https://github.com/lampepfl/dotty/blob/master/src/dotty/tools/dotc/transform/IsInstanceOfEvaluator.scala which deals with erased values and so might end up doing an incorrect rewriting.

@liufengyun
Copy link
Contributor Author

The alternative would be to rewrite transformIsInstanceOf to expect non-erased arguments and only erase them after doing the rewritings it needs to do, that might be better indeed.

@smarter That is how the previous implementation was done, but it doesn't handle the case in #1490 . A proper handling of #1490 would require us to recursively get rid of all type alias, that's part of the logic of type erasure. Either we need to find a way to reuse the type erasure code, or we have to duplicate some of its code.

@odersky
Copy link
Contributor

odersky commented Sep 1, 2016

Question: What should the imlementation

xs.isInstanceOf[List[A | B]] 

be? Erase | or not? I would assume erase. In which case, the whole thing does belong in isInstanceOfEvaluator. Can #1490 not be addressed by dealiasing?

@smarter
Copy link
Member

smarter commented Sep 1, 2016

I think that in your example, A | B is not erased if eraseOrType is false, but List[A | B] is erased to a raw List anyway so it doesn't matter

@liufengyun
Copy link
Contributor Author

I agree it's better to make the core simpler. Will follow the original approach by dealiasing.

@smarter
Copy link
Member

smarter commented Sep 1, 2016

LGTM

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.

4 participants