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

fix bug in hlist.{Intersection, Union} #563

Merged
merged 4 commits into from
Mar 20, 2016

Conversation

aryairani
Copy link
Contributor

Fix bugs described in #562.

Currently Intersection is witnessed by three cases:
hnilIntersection says ∅ ∩ M = ∅, which is good.
hlistIntersection1 says (H :: T ∩ M) = T ∩ M
hlistIntersection2 says: if H ∈ M, (H :: T ∩ M) = H :: (T ∩ (M - R))

Unfortunately, (H :: T ∩ M) = T ∩ M is false if H ∉ M. This patch adds the H ∉ M condition to hlistIntersection1 using a FilterNot.Aux[M,H,M] witness.

The patch also fixes an equivalent bug in Union.


I had a couple additional questions:

  1. Should hlistIntersection1 and hlistIntersection2 reside in the same trait now, eliminating the need for LowPriorityIntersection?
  2. Why is it that Intersection extends DepFn1 but Union extends DepFn2? It seems like they should be the same, but I don't know what the right answer is.

@aryairani aryairani changed the title fix bug in for hlist.{Intersection, Union} fix bug in hlist.{Intersection, Union} Mar 19, 2016
@aryairani
Copy link
Contributor Author

Fails mima check — this is tricky, the patch not bincompat with existing, but existing is buggy. :-\

@OlivierBlanvillain
Copy link
Contributor

Hi @refried, thanks for fixing this, your changes are LGTM!

Regarding your 2nd question, I think A ∩ B only contains elements from A, while A U B could have elements from both A and B, which would explain why you don't need the same number of arguments to produce the output. Note that for A - B, the type of B is also sufficient, as the output will only contain values from A.

@alexarchambault
Copy link
Collaborator

About MiMa, it is possible keep binary compatibility. You just have to keep the former methods as is, possibly modifying a bit their implementation, but removing their implicit modifier. (All of this along side the new methods you added)

This way, code compiled against older versions of shapeless will still find the methods they're looking for at runtime, and code compiled against the newer version will only find the newest methods, which are the only ones flagged as implicit.

@milessabin
Copy link
Owner

+1 to @alexarchambault's advice ... I'll merge when MiMa passes ... thanks! :-)

@aryairani
Copy link
Contributor Author

@alexarchambault @milessabin Ok, I can make that MiMa change, but please advise/confirm: the binary-compatible version will necessarily continue to be buggy, which is pretty scary to me. (The fix was to add the additional witness.) My instinct to add a @deprecation warning won't help if there's no recompilation.

Do I add @deprecation for now we delete the buggy method in the next minor version? Is there a process for remembering to have that happen? Thanks!

@alexarchambault
Copy link
Collaborator

@refried As the methods kept for binary compatibility don't have the implicit modifier anymore, they won't be found when building implicit instances of Intersection or Union. Only code compiled against older versions of shapeless should use them.

I don't mind about adding @deprecated. These methods are seldomly directly called anyway...

@aryairani
Copy link
Contributor Author

@alexarchambault Sure, I understand that — I just wished there was a way to warn the person linking against the old version; but I guess there isn't, and they wouldn't discover a bincompat issue until runtime.

I pushed the update, so fingers crossed! 😄

@milessabin
Copy link
Owner

LGTM. Many thanks ... merging :-)

milessabin added a commit that referenced this pull request Mar 20, 2016
fix bug in hlist.{Intersection, Union}
@milessabin milessabin merged commit 17fb693 into milessabin:master Mar 20, 2016
@aryairani aryairani deleted the fix-intersect branch March 20, 2016 14:07
@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.

None yet

4 participants