Separate compilation of inferred type constructors should not crash asSeenFrom (nor cause spurious recompilation)#7119
Merged
adriaanm merged 3 commits intoscala:2.13.xfrom Aug 27, 2018
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
01c172a to
531eaed
Compare
One part of the problem was that when you drill down to the info of a member of a polymorphic class, you should remember to push down the type params of the class by re-wrapping the info of the member in a PolyType, so that, then when you relativize the info of the member relative to the class and its corresponding prefix (used as its this type, which should thus have kind *), you also rewrite the info of the type params, which may be affected as illustrated by the test case (coming in 2/2). Dotty has a neat solution through denotations, we need to take care...
A similar problem as the previous commit arose in etaExpand. Use the rewritten type parameters in typeNew. Now we can add the test.
Before, they kept their original owner (the eta expanded class), which caused an owner-structure mismatch in pickling. This resulted in spurious recompilation and asSeenFrom crashes. Changing the owner of the binders used for eta-expansion, weirdly revealed a problem with typing annotations, where, I assume, reuse of the class type params hid the missing logic for dealing with polymorphic annotation classes and the undetparams that arise of typing their instantiation.
Contributor
Author
|
https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-community-build/1383/console for 6c20b59 |
Member
|
51 projects are green, but then there are compilation errors in shapeless /cc @milessabin one of the errors: all of them: Details |
Contributor
|
I'll take a look. |
Contributor
|
I'm out of time for today, but I think I've got a handle on the problem. |
Contributor
Author
|
As reported on the ticket, this seems to have fixed the problem reported with the akka test suite. @milessabin, we're now thinking it would be best to include this PR in M5 to get more battle testing. What do you think? Happy to assist in resolving the shapeless issue next week, so that we can cut M5 as soon as we have confidence shapeless is ok. |
Contributor
|
@adriaanm I'm fine with that. I think I'm quite close to a fix ... I'll scream for help on Monday if I can't get there. |
retronym
approved these changes
Aug 27, 2018
Contributor
Author
|
@milessabin I'm happy to help out this afternoon, so that we can (hopefully) cut M5 tomorrow |
Contributor
Author
|
Looks like the issue is a wrong assumption about PolyTypes that's fixed by this PR. The type params bound by the PolyType have no meaning beyond the PolyType's result type. It looks like shapeless expects them to be the same as, say, an eta-expanded class's type params. Given All but one of the errors are fixed by def mkGeneric1Impl[T[_], FR[_[_]]](implicit tTag: WeakTypeTag[T[_]], frTag: WeakTypeTag[FR[Any]]): Tree = {
- val tpe = tTag.tpe
+ val tpe = tTag.tpe.etaExpandSo that the set of typeParams seen during synthesis are consistent. One error remains with Split -- looking at that now. |
Contributor
Author
|
Btw, I don't think appliedTypTree1;s case for def appliedTypTree1(tpe: Type, param: Type, arg: TypeName): Tree = {
tpe match {
case t if t =:= param =>
Ident(arg)
- case PolyType(params, body) if params.head.asType.toType =:= param =>
- appliedTypTree1(body, param, arg)
+ case PolyType(tps@(List(binder)), body) =>
+ appliedTypTree1(body.substituteTypes(tps, param :: Nil), param, arg) |
Contributor
Author
|
Ah, but that actually caused the remaining error I was seeing, so I guess this is an internal representation. It's not really how a PolyType is meant to be used, though: it makes no sense to compare the symbols of the poly type's binders to a symbol that comes from outside. So, that one-line patch actually make shapeless compile. It would probably be good to audit the use of polytypes a bit more, but I feel at least we could merge this PR, knowing its impact is fixable on the shapeless side. |
Contributor
|
You should go ahead and merge. A viable fix is probably somewhere in between what I've been tinkering with and your observations. FTR, I think that |
Contributor
Author
|
Ok, thanks! |
Contributor
|
It looks like I was reinventing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While trying to fix scala/bug#10762, I stumbled on scala/bug#11103, which we'll try/have to fix first.
When you drill down to the info of a member of a polymorphic class, you should remember to push down the type params of the class by re-wrapping the info of the member in a PolyType, so that, then when you relativize the info of the member relative to the class and its corresponding prefix (used as its this type, which should thus have kind *), you also rewrite the info of the type params, which may be affected as illustrated by the test case pos/t11103.scala.
A similar problem arose in etaExpand. Use the rewritten type parameters in typeNew.
Dotty has a neat solution through denotations, we need to take care...
The real problem we're trying to solve is spurious recompilation and ASF crashes due to eta expansion reusing the class owning the type params as the owner for the binders in the resulting PolyType.
Changing the owner of the binders used for eta-expansion, weirdly revealed a problem with typing annotations, where, I assume, reuse of the class type params hid the missing logic for dealing with polymorphic annotation classes and the undetparams that arise of typing their instantiation.
H/T @retronym for spotting the dodgy
tree.tpe.prefix memberType caseClass. As no good deed goes unpunished, also review by –.Fixes scala/bug#11103
Fixes scala/bug#10762