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

Separate compilation of inferred type constructors should not crash asSeenFrom (nor cause spurious recompilation) #7119

Merged
merged 3 commits into from Aug 27, 2018

Conversation

Projects
None yet
5 participants
@adriaanm
Copy link
Member

adriaanm commented Aug 22, 2018

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

@adriaanm adriaanm requested a review from retronym Aug 22, 2018

@scala-jenkins scala-jenkins added this to the 2.13.0-RC1 milestone Aug 22, 2018

@adriaanm

This comment was marked as resolved.

Copy link
Member

adriaanm commented Aug 22, 2018

I stumbled onto this while looking into scala/bug#10762

@adriaanm adriaanm force-pushed the adriaanm:t11103 branch 3 times, most recently from 01c172a to 531eaed Aug 22, 2018

@adriaanm adriaanm self-assigned this Aug 23, 2018

adriaanm added some commits Aug 22, 2018

Denote prefix of constructor pats in type params 1/2
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...
Denote prefix of constructor pats in type params 2/2
A similar problem as the previous commit arose in etaExpand.
Use the rewritten type parameters in typeNew.

Now we can add the test.
Eta expansion uses local owner for binders
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.

@adriaanm adriaanm force-pushed the adriaanm:t11103 branch from 74fbb94 to 6c20b59 Aug 23, 2018

@adriaanm adriaanm changed the title Honor prefix of type params for constructor patterns Separate compilation of inferred type constructors should not crash asSeenFrom (nor cause spurious recompilation) Aug 23, 2018

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Aug 23, 2018

@SethTisue

This comment has been minimized.

Copy link
Member

SethTisue commented Aug 23, 2018

51 projects are green, but then there are compilation errors in shapeless /cc @milessabin

one of the errors:

[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/jvm/src/test/scala/shapeless/serialization.scala:952:32: type mismatch;
[shapeless] [error]  found   : pat$macro$16.type (with underlying type A)
[shapeless] [error]  required: fresh$macro$17
[shapeless] [error]     assertSerializable(Generic1[Some, TC1])
[shapeless] [error]                                ^

all of them:

[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/jvm/src/test/scala/shapeless/serialization.scala:952:32: type mismatch;
[shapeless] [error]  found   : pat$macro$16.type (with underlying type A)
[shapeless] [error]  required: fresh$macro$17
[shapeless] [error]     assertSerializable(Generic1[Some, TC1])
[shapeless] [error]                                ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:242:13: type mismatch;
[shapeless] [error]  found   : pat$macro$1.type (with underlying type T)
[shapeless] [error]  required: fresh$macro$2
[shapeless] [error]     Generic1[Foo, TC1]
[shapeless] [error]             ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:243:13: type mismatch;
[shapeless] [error]  found   : shapeless.Generic1TestsAux.Box[T]
[shapeless] [error]  required: shapeless.Generic1TestsAux.Box[fresh$macro$8]
[shapeless] [error]     Generic1[Bar, TC1]
[shapeless] [error]             ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:244:13: type mismatch;
[shapeless] [error]  found   : pat$macro$13.type (with underlying type T)
[shapeless] [error]  required: fresh$macro$15
[shapeless] [error]     Generic1[Baz, TC1]
[shapeless] [error]             ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:246:13: type mismatch;
[shapeless] [error]  found   : pat$macro$29.type (with underlying type A)
[shapeless] [error]  required: fresh$macro$30
[shapeless] [error]     Generic1[Some, TC1]
[shapeless] [error]             ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:257:13: type mismatch;
[shapeless] [error]  found   : shapeless.Generic1TestsAux.IList[T]
[shapeless] [error]  required: shapeless.Generic1TestsAux.IList[fresh$macro$72]
[shapeless] [error]     Generic1[PList, TC1]
[shapeless] [error]             ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:259:13: type mismatch;
[shapeless] [error]  found   : pat$macro$77.type (with underlying type T)
[shapeless] [error]  required: fresh$macro$79
[shapeless] [error]     Generic1[PIdList, TC1]
[shapeless] [error]             ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:265:24: type mismatch;
[shapeless] [error]  found   : pat$macro$98.type (with underlying type T)
[shapeless] [error]  required: fresh$macro$100
[shapeless] [error]     val gen0 = Generic1[Prod, TC2]
[shapeless] [error]                        ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:269:37: type mismatch;
[shapeless] [error]  found   : gen0.R[Int]
[shapeless] [error]     (which expands to)  T :: List[T] :: shapeless.HNil
[shapeless] [error]  required: Int :: List[Int] :: shapeless.HNil
[shapeless] [error]     typed[Int :: List[Int] :: HNil](r)
[shapeless] [error]                                     ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:274:58: type mismatch;
[shapeless] [error]  found   : shapeless.Generic1TestsAux.TC2[gen0.R]
[shapeless] [error]  required: shapeless.Generic1TestsAux.TC2[[t]t :: List[t] :: shapeless.HNil]
[shapeless] [error]     typed[TC2[({ type λ[t] = t :: List[t] :: HNil })#λ]](fr)
[shapeless] [error]                                                          ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:429:22: type mismatch;
[shapeless] [error]  found   : pat$macro$1.type (with underlying type T)
[shapeless] [error]  required: fresh$macro$2
[shapeless] [error]     val g0 = Generic1[Foo, FI]
[shapeless] [error]                      ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:435:22: type mismatch;
[shapeless] [error]  found   : pat$macro$7.type (with underlying type T)
[shapeless] [error]  required: fresh$macro$8
[shapeless] [error]     val g1 = Generic1[Foo, IF]
[shapeless] [error]                      ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:441:22: type mismatch;
[shapeless] [error]  found   : pat$macro$13.type (with underlying type T)
[shapeless] [error]  required: fresh$macro$14
[shapeless] [error]     val g2 = Generic1[Foo, FL]
[shapeless] [error]                      ^
[shapeless] [error] /home/jenkins/workspace/scala-2.13.x-integrate-community-build/target-0.9.14/project-builds/shapeless-e4a6decd71d57d40b9bb17029a4686e060aca535/core/src/test/scala/shapeless/generic1.scala:447:22: type mismatch;
[shapeless] [error]  found   : pat$macro$19.type (with underlying type T)
[shapeless] [error]  required: fresh$macro$20
[shapeless] [error]     val g3 = Generic1[Foo, LF]
[shapeless] [error]                      ^
[shapeless] [error] 14 errors found
@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Aug 23, 2018

I'll take a look.

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Aug 23, 2018

I'm out of time for today, but I think I've got a handle on the problem.

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Aug 24, 2018

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.

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Aug 24, 2018

@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.

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Aug 27, 2018

@milessabin I'm happy to help out this afternoon, so that we can (hopefully) cut M5 tomorrow

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Aug 27, 2018

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 class C[T], symbolOf[C].typeParams does not share symbols with typeOf[C[_]].typeConstructor.etaExpand.typeParams

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.etaExpand

So that the set of typeParams seen during synthesis are consistent. One error remains with Split -- looking at that now.

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Aug 27, 2018

Btw, I don't think appliedTypTree1;s case for PolyTypes is correct:

   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)
@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Aug 27, 2018

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.

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Aug 27, 2018

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 Generic1 needs a complete overhaul.

@adriaanm

This comment has been minimized.

Copy link
Member

adriaanm commented Aug 27, 2018

Ok, thanks!

@adriaanm adriaanm merged commit 08afd2f into scala:2.13.x Aug 27, 2018

4 checks passed

cla @adriaanm signed the Scala CLA. Thanks!
Details
combined All previous commits successful.
continuous-integration/travis-ci/pr The Travis CI build passed
Details
validate-main [4140] SUCCESS. Took 39 min.
Details

@adriaanm adriaanm modified the milestones: 2.13.0-RC1, 2.13.0-M5 Aug 27, 2018

@milessabin

This comment has been minimized.

Copy link
Contributor

milessabin commented Aug 27, 2018

It looks like I was reinventing etaExpand ... I've gone with your fix, tweaked to cross-compile for 2.10.x ... many thanks!

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