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

unwrap tag (tagged) #538

Merged
merged 1 commit into from
May 6, 2016
Merged

Conversation

etaty
Copy link
Contributor

@etaty etaty commented Jan 29, 2016

@clhodapp
Copy link
Contributor

Can you add a test similar to https://github.com/etaty/shapeless/blob/features/tagged-unwrap/core/src/test/scala/shapeless/unwrapped.scala#L109 ? I'm concerned about whether it will pass.

@etaty
Copy link
Contributor Author

etaty commented Jan 29, 2016

Yep good catch.
test.illTyped("unwrapped: String @@ TestTag") don't work
unless I write
val unwrapped : String = wrapped.unwrap

@etaty
Copy link
Contributor Author

etaty commented Jan 29, 2016

that's weird, I am curious for the reason why it keeps the tagged type.

@clhodapp
Copy link
Contributor

It's because e.g. String @@ TestTag is the most-specific T that can be extracted from T @@ TestTag, so that's what the compiler chooses to infer (as strange as it is, (String @@ TestTag) @@ TestTag is actually the same as String @@ TestTag and the compiler is able to see that this is the case and reason from it). As far as I currently know, you actually need to be working in the context of a macro to actually strip tags off in the general case, such as what's needed here.

@clhodapp
Copy link
Contributor

Also, the version of this in your PR actually leaves off the maybe-important feature of being able to see through multiple wrappers over the same type. This is the purpose of all of those chain parameters in the other instance providers. Also, rather than constructing a new instance, you can take advantage of a micro-optimization I make in some of my other instances: Taking an implicit chain and simply casting it. This works because Unwrapped instances for e.g. tagged types don't have to actually change the object, they just need to make the types line up.

@milessabin
Copy link
Owner

What's the status of this? I'm aiming for an RC1 before the end of the week and it'd be nice if we could get this in by then.

@milessabin milessabin modified the milestone: shapeless-2.3.0 Feb 8, 2016
@etaty
Copy link
Contributor Author

etaty commented Feb 8, 2016

Hi Miles,
I know nothing about macro.
And I did not realise that in my case, I did not need the unwrap. (I am also not sure it is a needed feature).
So I won't probably finish the pull-request by the end of the week.
(you can close/reject)

@milessabin
Copy link
Owner

No worries ... if you'd like to work on this I'll punt it to 2.3.1?

@milessabin
Copy link
Owner

Moved to 2.3.1 ...

@clhodapp
Copy link
Contributor

clhodapp commented Apr 15, 2016

Just got around to checking how easy it is to define an untagger with Miles's higher order unification patch (scala/scala#5102). It's pretty easy, I think: https://gist.github.com/clhodapp/a2f1b6fc011df9b60b107bd3034aef6d

@clhodapp
Copy link
Contributor

Actually, that technique seems to work even without higher order unification!

@clhodapp
Copy link
Contributor

@milessabin
Copy link
Owner

Do either or both of you have time to try and get this over the finish line for 2.3.1?

@clhodapp
Copy link
Contributor

I'll make a PR this evening or next.

@milessabin
Copy link
Owner

Oh, blimey! Missed this ... sorry :-(

LGTM. Many thanks ... merging :-)

@milessabin milessabin merged commit c332f90 into milessabin:master May 6, 2016
@milessabin milessabin modified the milestone: shapeless-2.3.1 May 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants