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

Add tests for the relationship between leq and compare #79

Closed
wants to merge 2 commits into from

Conversation

mitchellwrosen
Copy link
Contributor

@mitchellwrosen mitchellwrosen commented Nov 28, 2018

This patch adds a test for the relationship between leq and compare:

if x leq y,
 if y leq x,
    x compare y should be EQ
  else
    x compare y should be LT
else
  x compare y should be GT

The test suite is failing because the derived instances of Ord for the types Dropped, etc. violate this property (the constructors are backwards)

@phadej
Copy link
Collaborator

phadej commented Nov 28, 2018

adding PartialOrd instances is 👍

Imposing law for Ord is 👎.

Library has:

instance (PartialOrd a, PartialOrd b) => PartialOrd (a, b) where
    -- NB: *not* a lexical ordering. This is because for some component partial orders, lexical
    -- ordering is incompatible with the transitivity axiom we require for the derived partial order
    (x1, y1) `leq` (x2, y2) = x1 `leq` x2 && y1 `leq` y2

and it's correct

@mitchellwrosen
Copy link
Contributor Author

Oh... interesting! Is it a special case? Are there any other types besides tuples for which the PartialOrd instance may not agree with the Ord instance?

Or asked differently, if PartialOrd was in base, would it be a superclass of Ord?

@mitchellwrosen
Copy link
Contributor Author

And separately: is it not a bug then that

Drop 5 `leq` Top

but

Drop 5 > Top

?

@phadej
Copy link
Collaborator

phadej commented Nov 29, 2018

I think having leq and <= agree were sensibly possible is good goal (so I'm 👍 on changing order of constructors in Dropped).

Yet I don't want expose the requirement leq = <=. I have usecases where leq is interesting in the "buisness domain"; where Ord is used to put things into Data.Set. (EDIT: I guess I have to explain this in the docs, but if you are willing to write as stub, that would be great!)

@mitchellwrosen
Copy link
Contributor Author

@phadej Sounds good. I've removed the tests, and made leq = (<=) for Dropped, Levitated, Lifted, and Op.

It would be awfully nice if leq = (<=) was a law but that discussion can be moved to a new ticket :)

@phadej
Copy link
Collaborator

phadej commented Feb 2, 2019

Merged in 787b093

@phadej phadej closed this Feb 2, 2019
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.

2 participants