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

Fix ops.record.Selector with tagged type for Scala 2.12+ #726

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@krzemin
Copy link

krzemin commented Jun 2, 2017

I've played a bit with record types that contain tagged types inside. It seems that there are inconsistencies of how they are handled between Scala 2.{10,11} and 2.12+ when it comes to ops.record.Selector macro. I found that shape of RefinedType in https://github.com/milessabin/shapeless/blob/master/core/src/main/scala/shapeless/generic.scala#L516 is different in newer versions of the compiler.

This PR contains a test case that failed under 2.12+ and workaround solution that seems to fix the issue and works under all supported Scala versions.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jun 2, 2017

Codecov Report

Merging #726 into master will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #726      +/-   ##
==========================================
+ Coverage   86.81%   86.83%   +0.01%     
==========================================
  Files          89       89              
  Lines        2518     2521       +3     
  Branches       54       60       +6     
==========================================
+ Hits         2186     2189       +3     
  Misses        332      332
Impacted Files Coverage Δ
core/src/test/scala/shapeless/records.scala 100% <ø> (ø) ⬆️
core/src/main/scala/shapeless/generic.scala 0% <0%> (ø) ⬆️
core/src/test/scala/shapeless/generic.scala 83.87% <0%> (+0.17%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 39f7330...b89e529. Read the comment docs.

@milessabin

This comment has been minimized.

Copy link
Owner

milessabin commented Jun 13, 2017

Thanks for the diagnosis and the patch!

From the diff it looks as though there an additional parent type in 2.12.x ... have you investigated what it is?

@krzemin

This comment has been minimized.

Copy link

krzemin commented Jun 13, 2017

It seems that in 2.12+ tag type alias (@@) is resolved and particular parents appear as inlined on the same level in RefinedType, but up to 2.11 another RefinedType for the tagged type is nested as first parent.

I prepared minimal code example that briefly illustrates the difference.

import scala.reflect.runtime.universe._
import shapeless._
import shapeless.tag._

trait Tag
case class Test(field: String @@ Tag)

val x = tag[Tag]("abc")

reify(LabelledGeneric[Test].to(Test(x))).staticType.asInstanceOf[TypeRef].typeArgs.head.asInstanceOf[RefinedType].parents

This snippet can be run in Scastie:

I don't think that Shapeless handles tagged types / field types any different between Scala versions. But so far I didn't find any possible root cause of the difference.

@krzemin

This comment has been minimized.

Copy link

krzemin commented Jun 13, 2017

What's interesting - when trying to construct LabelledGeneric for product by hand by composing DefaultSymbolicLablleing, Generic and ops.hlist.ZipWithKeys, we have:

val dsl = DefaultSymbolicLabelling[Test]
val gen = Generic[Test]
reify(ops.hlist.ZipWithKeys[dsl.Out, gen.Repr].apply(gen.to(Test(x)))).staticType.asInstanceOf[TypeRef].typeArgs.head.asInstanceOf[RefinedType].parents

which behaves identically on both 2.11/2.12, resulting with:

List(
  java.lang.String,
  shapeless.tag.Tagged[Playground.this.Tag],
  shapeless.labelled.KeyTag[Symbol with shapeless.tag.Tagged[java.lang.String("field")],java.lang.String with shapeless.tag.Tagged[Playground.this.Tag]]
): List[Type]

But when having a function similar to LabelledGeneric.materializeProduct which takes all these input type classes as implicit instances with Aux pattern:

def materializeProduct[T, K <: HList, V <: HList, R <: HList]
  (obj: T)
  (implicit lab: DefaultSymbolicLabelling.Aux[T, K],
   gen: Generic.Aux[T, V],
   zip: ops.hlist.ZipWithKeys.Aux[K, V, R],
   ev: R <:< V) = zip.apply(gen.to(obj))

reify(materializeProduct(Test(x))).staticType.asInstanceOf[TypeRef].typeArgs.head.asInstanceOf[RefinedType].parents

The same difference between 2.11/2.12 is present.

@milessabin

This comment has been minimized.

Copy link
Owner

milessabin commented Sep 11, 2017

I apologies for not getting back to you on this sooner ... I must have missed a notification from github :-(

#761 looks a little more general than this fix ... I'll ask the submitter to include your test case.

@krzemin

This comment has been minimized.

Copy link

krzemin commented Sep 11, 2017

Sure, I'm also for merging more general solution instead of this quick fix.

@joroKr21 feel free to check your fix also on this test:
https://github.com/milessabin/shapeless/pull/726/files#diff-45761b3cb0d957479a96d46507f6b3cbR1100

Closing.

@krzemin krzemin closed this Sep 11, 2017

@milessabin

This comment has been minimized.

Copy link
Owner

milessabin commented Sep 11, 2017

Continued in #761.

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