haskell / core-libraries-committee

95 stars 16 forks source link

Add `Enum` instance to `Down`, flipping direction #51

Closed gergoerdi closed 2 years ago

gergoerdi commented 2 years ago

This is an idea that was prompted by my surprise that this behaviour is not already the case...

The ~Bounded and~ Enum instances of Data.Ord.Down should be changed such that

succ @(Down a) = fmap pred

and all other methods implemented similarly.

Implementation is in MR !8242.

gergoerdi commented 2 years ago

-1. Considering that this implementation causes errors for unsigned types (see the MR), I'm voting against this proposal.

See this comment, unsigned types work just fine, within the spelled-out constraints of Enum.

mixphix commented 2 years ago

unsigned types work just fine, within the spelled-out constraints of Enum.

This is a great point that clears it up for me. Thank you, I'm now comfortable to vote in favour.

+1

LSLeary commented 2 years ago

Rather than negate, we want a function f :: Int -> Int that is an order-reversing map—one with the property that for all x, y :: Int, x < y ==> f y < f x. Since Int is finite, it's straightforward to see that this function exists and is unique. Possible implementations include subtract 1 . negate and negate . (+1).

Order-reversing maps are to order-preserving maps as reflections are to translations; they're closed under composition, with an even number of the former giving you the latter. Since both Down and getDown are by definition order-reversing, it follows that using f as below preserves the property that fromEnum and toEnum are order-preserving at the underlying type, if indeed they are.

  fromEnum = f . fromEnum . getDown
  toEnum = Down . toEnum . f

Note that, while not explicitly a law, I do expect fromEnum to satisfy this property whenever Int is large enough.

tomjaguarpaw commented 2 years ago

@LSLeary Could you spell out your conclusion explicitly? Are you saying that the use of negate in this MR is wrong? It should be subtract 1 . negate?

LSLeary commented 2 years ago

@tomjaguarpaw "Wrong" in the loose sense that it may destroy a property we wish to keep, and yes.

Bodigrim commented 2 years ago

FWIW subtract 1 . negate is Data.Bits.complement.

When x = minBound :: Int and y /= minBound, we have x < y but at the same time somewhat counterintuitively negate x < negate y, because negate minBound = minBound. Subtracting 1 makes this peculiarity to disappear.

Back into Enum realm, @LSLeary's desired property is that if fromEnum x < fromEnum y then fromEnum (Down x) > fromEnum (Down y). For this purpose subtract 1 . negate indeed works better than just negate and does not seem to have other downsides, but I'm not terribly bullish on this: fromEnum is already broken for all means and purposes.

tomjaguarpaw commented 2 years ago

That's pretty convincing to me. If we are going to do this then it should be with subtract 1 . negate as @LSLeary says. I don't see the point of making a broken type class even worse.

-1

Bodigrim commented 2 years ago

More specifically, the argument goes like this. For instance Enum Int we enjoy fromEnum = toEnum = id, and fromEnum automatically preserves ordering. However if fromEnum = negate in instance Enum (Down Int), the mapping is not order-reversing because of paradoxical behaviour of negate minBound.

(@LSLeary I hope I'm not misrepresenting your point?)

@gergoerdi how do you feel about amending the proposal with Data.Bits.complement in place of negate?

gergoerdi commented 2 years ago

That's pretty convincing to me. If we are going to do this then it should be with subtract 1 . negate as @LSLeary says. I don't see the point of making a broken type class even worse.

-1

Wait are we discussing the specific MR here, or the proposal to add a backwards-listing Enum instance? Your "-1" seems to apply to the former only, not the latter.

gergoerdi commented 2 years ago

@gergoerdi how do you feel about amending the proposal with Data.Bits.complement in place of negate?

I think at this point the best would be for me to just change the MR to use complement, and then remove any specifics from the proposal, since the proposal's intention was never to propose any specific implementation strategy...

tomjaguarpaw commented 2 years ago

Yes, my "-1" was for the specific MR, not the proposal. My understanding is that the CLC votes on specific MRs, not general proposals (although I suppose the wording of @Bodigrim's message could be interpreted either way).

Bodigrim commented 2 years ago

The MR has been updated to use complement: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/8242/diffs

@mixphix @cgibbard could you please confirm that this does not affect your approval? @tomjaguarpaw is this change satisfactory? @chessai @emilypi just a gentle reminder to vote.

mixphix commented 2 years ago

LGTM

tomjaguarpaw commented 2 years ago

+1


Yes, that's satisfactory. I think the toEnum/fromEnum is a big strange, but I don't think it actually violates any known laws.

emilypi commented 2 years ago

+1 from me

Bodigrim commented 2 years ago

Altogether this gives us at least 4 votes in favor, which is sufficient to approve. Thanks all!

chshersh commented 1 year ago

I'm trying to summarise the state of this proposal as part of my volunteering effort to track the progress of all approved CLC proposals.

Field Value
Authors @gergoerdi
Status merged
base version 4.18.0.0
Merge Request (MR) https://gitlab.haskell.org/ghc/ghc/-/merge_requests/8242
Blocked by nothing
CHANGELOG entry present
Migration guide not needed

Please, let me know if you find any mistakes 🙂