Skip to content
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

Make FieldType and @@ abstract #807

Closed
wants to merge 14 commits into from
Closed

Conversation

joroKr21
Copy link
Collaborator

@joroKr21 joroKr21 commented Jan 13, 2018

aka. "You shall not dealias"

Fixes a bunch of problems caused by:

  1. the compiler normalizing types behind your back;
  2. the compiler failing to unify the normalized types.

Also has the additional benefit of printing more nicely in the REPL.
This will always box however. Would that be a problem?

TODO: Check which bugs are fixed, reference them here and add regression tests:

@joroKr21
Copy link
Collaborator Author

Sadly I think this is not allowed on Scala 2.10 so this has to wait for #655


import scala.reflect.macros.whitebox

object labelled {
/**
* The type of fields with keys of singleton type `K` and value type `V`.
*/
type FieldType[K, +V] = V with KeyTag[K, V]
type FieldType[K, +V] <: V with KeyTag[K, V]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the purpose of V in KeyTag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about calling FieldType[K, V] K ->> V?

@joroKr21 joroKr21 force-pushed the type-aliases branch 3 times, most recently from f1a4c77 to 602ce99 Compare January 14, 2018 12:48
@codecov-io
Copy link

codecov-io commented Jan 14, 2018

Codecov Report

Merging #807 into master will decrease coverage by 2.01%.
The diff coverage is 10.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
- Coverage   89.86%   87.85%   -2.02%     
==========================================
  Files          34       65      +31     
  Lines        1470     1498      +28     
  Branches        1        3       +2     
==========================================
- Hits         1321     1316       -5     
- Misses        149      182      +33     
Impacted Files Coverage Δ
core/src/main/scala/shapeless/generic.scala 0.00% <0.00%> (ø)
core/src/main/scala/shapeless/singletons.scala 0.00% <0.00%> (ø)
core/src/main/scala/shapeless/typeoperators.scala 0.00% <0.00%> (ø)
core/src/main/scala/shapeless/labelled.scala 85.71% <66.66%> (-14.29%) ⬇️
core/src/main/scala/shapeless/hlists.scala 63.15% <0.00%> (-3.51%) ⬇️
...re/src/main/scala/shapeless/syntax/coproduct.scala 97.87% <0.00%> (-0.05%) ⬇️
core/src/main/scala/shapeless/fin.scala 0.00% <0.00%> (ø)
core/src/main/scala/shapeless/annotation.scala 0.00% <0.00%> (ø)
core/src/main/scala/shapeless/poly.scala 0.00% <0.00%> (ø)
core/src/main/scala/shapeless/sybclass.scala 0.00% <0.00%> (ø)
... and 28 more

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 85fb65f...7192c0e. Read the comment docs.

@joroKr21 joroKr21 force-pushed the type-aliases branch 4 times, most recently from b2a53cd to a42e14f Compare January 14, 2018 19:52
@joroKr21
Copy link
Collaborator Author

ClassTag magic doesn't work. HALP!

@joroKr21 joroKr21 force-pushed the type-aliases branch 2 times, most recently from 56469d0 to 59edbc8 Compare January 16, 2018 01:36
@joroKr21 joroKr21 force-pushed the type-aliases branch 3 times, most recently from c8d15d9 to e7c72d6 Compare February 28, 2018 14:33
def apply[U] = new Tagger[U]

private[shapeless] trait Tag {
type @@[+T, U] <: T with Tagged[U]
Copy link
Contributor

Choose a reason for hiding this comment

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

I have been giving this encoding a spin, for encoding/decoding tagged types to/from json:

  • with the @@ part of the private Tag class, I run into a problem when using play-json's format macro:

No instance of play.api.libs.json.Format is available for xxx.Tag.$at$at[java.lang.String, TaggingTest.FooTag] in the implicit scope (Hint: if declared in the same file, make sure it's declared before)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I introduced the trait only because Scala 2.10 doesn't support abstract types in objects / classes, but somehow it supports them when inherited.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also it looks like type @@[+T, U] <: T with Tagged[U] now boxes tagged primitives, while type @@[+T, U] = T with Tagged[U] does not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, that's true unfortunately.

aka. "You shall not dealias"

Fixes a bunch of problems caused by:
1. the compiler normalizing types behind your back;
2. the compiler failing to unify the normalized types.
@milessabin
Copy link
Owner

Continuing the discussion from scala/community-build#1047 (comment).

I think it's worth investigating which approach most smoothly transfers to Dotty. At least some uses of tagged types will be replaced by opaque types, but probably not all. It'd be good to see what works best for the cases which aren't handled well by opaque types.

@joroKr21
Copy link
Collaborator Author

Progress!

The boxing issue is solved by scala/scala#8720 so we can have our cake and eat it too.

Dotty has better type inference and some simple cases that I tried work better than in Scala 2 with the current encoding of tagged types. I still need to come up with a better comparison (and also check how they are erased in dotty).

I agree that perhaps tagged types will not be completely replaced (because they are subtypes) but as a native solution I expect opaque types to become much more widely used, perhaps even as much as to make tagged types on shapeless 3 unwarranted.

@joroKr21 joroKr21 modified the milestones: shapeless-2.4.0, shapeless-2.5.0 Mar 16, 2020
@joroKr21
Copy link
Collaborator Author

Changed milestone to 2.5.0 which will be 2.13 only.
If scala/scala#8720 is merged (I see no reason why not) then this has no downsides.

@codecov-commenter
Copy link

codecov-commenter commented Aug 16, 2020

Codecov Report

Merging #807 into master will decrease coverage by 2.01%.
The diff coverage is 10.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #807      +/-   ##
==========================================
- Coverage   89.86%   87.85%   -2.02%     
==========================================
  Files          34       65      +31     
  Lines        1470     1498      +28     
  Branches        1        3       +2     
==========================================
- Hits         1321     1316       -5     
- Misses        149      182      +33     
Impacted Files Coverage Δ
core/src/main/scala/shapeless/generic.scala 0.00% <0.00%> (ø)
core/src/main/scala/shapeless/singletons.scala 0.00% <0.00%> (ø)
core/src/main/scala/shapeless/typeoperators.scala 0.00% <0.00%> (ø)
core/src/main/scala/shapeless/labelled.scala 85.71% <66.66%> (-14.29%) ⬇️
core/src/main/scala/shapeless/hlists.scala 63.15% <0.00%> (-3.51%) ⬇️
...re/src/main/scala/shapeless/syntax/coproduct.scala 97.87% <0.00%> (-0.05%) ⬇️
core/src/main/scala/shapeless/annotation.scala 0.00% <0.00%> (ø)
core/src/main/scala/shapeless/fin.scala 0.00% <0.00%> (ø)
...re/src/main/scala/shapeless/hlistconstraints.scala 0.00% <0.00%> (ø)
core/src/main/scala/shapeless/zipper.scala 0.00% <0.00%> (ø)
... and 28 more

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 e8b6028...edc47fa. Read the comment docs.

Base automatically changed from master to main March 19, 2021 12:59
@joroKr21
Copy link
Collaborator Author

Changed milestone to 2.5.0 which will be 2.13 only.
If scala/scala#8720 is merged (I see no reason why not) then this has no downsides.

In the end it was not merged. In Scala 3 we have opaque types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants