Skip to content

Summoning Mirror for ADT defined in trait #13332

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

Closed
TimWSpence opened this issue Aug 18, 2021 · 9 comments · Fixed by #15847
Closed

Summoning Mirror for ADT defined in trait #13332

TimWSpence opened this issue Aug 18, 2021 · 9 comments · Fixed by #15847

Comments

@TimWSpence
Copy link
Contributor

TimWSpence commented Aug 18, 2021

Compiler version

scala-3.1.0-RC1-bin-20210817-f27c250-NIGHTLY (I tried to use the most recent I could find as I thought there's a chance that #11686 might have fixed this as it's in a similar area. Not entirely sure if that made it into this nightly or not)

Minimized code

import scala.deriving.Mirror

class Scope extends IListDefn {

  type Of = Mirror { type MirroredType[X] = IList[X]; type MirroredMonoType = IList[Any] ; type MirroredElemTypes[_] <: Tuple }

  val M = summon[Of]
}


trait IListDefn {
  sealed trait IList[A]
  case class INil[A]() extends IList[A]
  case class ICons[A](h: A, t: IList[A]) extends IList[A]
}

Output

[error] -- Error: /private/tmp/scala/Minimization.scala:7:20 ---------------------------
[error] 7 |  val M = summon[Of]
[error]   |                    ^
[error]   |no implicit argument of type Scope.this.Of was found for parameter x of method summon in object Predef
[error] one error found

Expectation

Compilation should succeed (and does if you move the IList definition out of the trait and up to top-level)

@bishabosha
Copy link
Member

bishabosha commented Aug 18, 2021

workaround is to give a companion object to IList (which acts as the Mirror object that will be summoned). Otherwise e.g. the child INil fails the accessibility test for generating an anonymous class Mirror type at the callsite M, perhaps that test is too restrictive in some cases

@TimWSpence
Copy link
Contributor Author

Thanks @bishabosha! Unfortunately this is ultimately for https://github.com/typelevel/kittens where we summon Mirror instances via shapeless 3 for typeclass derivation so we have no control over how users define their ADTs.

@milessabin
Copy link
Contributor

@bishabosha is there any chance we could tweak the accessibility test before 3.1?

@bishabosha bishabosha added this to the 3.1.0 milestone Aug 23, 2021
@bishabosha bishabosha added the Spree Suitable for a future Spree label Aug 23, 2021
@bishabosha bishabosha self-assigned this Aug 23, 2021
bishabosha added a commit to dotty-staging/dotty that referenced this issue Aug 26, 2021
bishabosha added a commit to dotty-staging/dotty that referenced this issue Aug 26, 2021
bishabosha added a commit to dotty-staging/dotty that referenced this issue Aug 26, 2021
@bishabosha
Copy link
Member

bishabosha commented Aug 27, 2021

if we tweak the accessibility rules to allow this case, then changing IList to be monomorphic will crash the compiler:

class Scope2 extends ZListDefn {

  type Of = Mirror { type MirroredType = ZList; type MirroredMonoType = ZList ; type MirroredElemTypes <: Tuple }

  val M = summon[Of]

}

class ZListDefn {
  sealed trait ZList
  case class ZNil() extends ZList
  case class ZCons(h: Boolean, t: ZList) extends ZList
}

with the same cause as #12328, summarised by my comment here #12328 (comment), the fix is more involved as it requires substituting the correct prefix into the types of the sealed children

@odersky
Copy link
Contributor

odersky commented Aug 29, 2021

Yes, for the moment this is simply out of spec. We generate mirrors only for statically accessible classes. I would advise to go slowly here and make absolutely sure that each generalization step can be made without negative consequences.

@bishabosha bishabosha removed this from the 3.1.0 milestone Aug 30, 2021
bishabosha added a commit to dotty-staging/dotty that referenced this issue Sep 3, 2021
bishabosha added a commit to dotty-staging/dotty that referenced this issue Sep 3, 2021
add a new method healPrefix which converts
ThisTypes wrapping a Module into a TermRef
and subsitutes ThisType of enclosing classes
with matching parts of the prefix of the
summoned mirror

fixes scala#12328
fixes scala#11174
@joroKr21
Copy link
Member

joroKr21 commented Nov 1, 2021

Not sure how that's related to accessibility - it sounds like incorrect handling of prefixes when generating a mirror. In general I think Scala users expect nesting to just work in most cases. Restricting certain features to only statically accessible symbols sounds very hard to explain.

@joroKr21
Copy link
Member

joroKr21 commented Nov 1, 2021

I think if we look for a mirror of foo.bar.baz.Qux from foo.bar.x.y then the path should be baz.Qux - i.e. go up the owner chain until you reach a common owner - if it's static of course you can shortcut to the fully qualified path.

@bishabosha
Copy link
Member

I think if we look for a mirror of foo.bar.baz.Qux from foo.bar.x.y then the path should be baz.Qux - i.e. go up the owner chain until you reach a common owner - if it's static of course you can shortcut to the fully qualified path.

I am trying this is #13502, I still need to test in more cases

@joroKr21
Copy link
Member

joroKr21 commented Nov 1, 2021

We can dig out some test cases from Shapeless 2 or Magnolia for Scala 2

bishabosha added a commit to dotty-staging/dotty that referenced this issue Mar 2, 2022
add a new method healPrefix which converts
ThisTypes wrapping a Module into a TermRef
and subsitutes ThisType of enclosing classes
with matching parts of the prefix of the
summoned mirror

fixes scala#12328
fixes scala#11174
@bishabosha bishabosha modified the milestone: 3.2.0-RC1 Mar 7, 2022
bishabosha added a commit to dotty-staging/dotty that referenced this issue Apr 1, 2022
add a new method healPrefix which converts
ThisTypes wrapping a Module into a TermRef
and subsitutes ThisType of enclosing classes
with matching parts of the prefix of the
summoned mirror

fixes scala#12328
fixes scala#11174
bishabosha added a commit to dotty-staging/dotty that referenced this issue Apr 1, 2022
add a new method healPrefix which converts
ThisTypes wrapping a Module into a TermRef
and subsitutes ThisType of enclosing classes
with matching parts of the prefix of the
summoned mirror

fixes scala#12328
fixes scala#11174
@bishabosha bishabosha added area:typeclass-derivation and removed Spree Suitable for a future Spree labels Apr 21, 2022
bishabosha added a commit to dotty-staging/dotty that referenced this issue May 16, 2022
add a new method healPrefix which converts
ThisTypes wrapping a Module into a TermRef
and subsitutes ThisType of enclosing classes
with matching parts of the prefix of the
summoned mirror

fixes scala#12328
fixes scala#11174
bishabosha added a commit to dotty-staging/dotty that referenced this issue May 16, 2022
add a new method healPrefix which converts
ThisTypes wrapping a Module into a TermRef
and subsitutes ThisType of enclosing classes
with matching parts of the prefix of the
summoned mirror

fixes scala#12328
fixes scala#11174
bishabosha added a commit to dotty-staging/dotty that referenced this issue May 18, 2022
add a new method healPrefix which converts
ThisTypes wrapping a Module into a TermRef
and subsitutes ThisType of enclosing classes
with matching parts of the prefix of the
summoned mirror

fixes scala#12328
fixes scala#11174

use do not force symbols

add safety for unknown type

experiment with TypeOps.refineUsingParent

support hk types

rebase fixes

disable for scala2x generic product nonstatic

simplify a bit

add more tests

find common prefix of and/or types

refine implementation based on runtime tests

experiment with supertypes

remove prefix splice in companionref
@Kordyjan Kordyjan added this to the 3.2.1 milestone Aug 1, 2023
EugeneFlesselle added a commit to dotty-staging/dotty that referenced this issue Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment