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

Enum instance of UFixed is not Haskell conform #2888

Open
kleinreact opened this issue Feb 13, 2025 · 3 comments
Open

Enum instance of UFixed is not Haskell conform #2888

kleinreact opened this issue Feb 13, 2025 · 3 comments

Comments

@kleinreact
Copy link
Member

As already noted here, the Enum instances of UFixed is not Haskell2010 conform:
(the arguments of the prior discussion are restated below for convenience)


I consider the successor of 1.5 :: UFixed 2 1 to be 2and not 2.5.

The current Enum (UFixed 2 1) instance implements a similar behavior as the Enum instance for Float, but the report makes an explicit exception for that particular type. It defines:

For all four numeric types (which are only Int, Integer, Float, and Double) , succ adds 1, and pred subtracts 1.

For any other type the semantics is:

Class Enum defines operations on sequentially ordered types. The functions succ and pred return the
successor and predecessor, respectively, of a value.

The sequential order of UFixed 2 1 is

0.0, 0.5, 1.0, 1.5, 2.0, 2.5, 3.0, 3.5

and, thus, the successor of 1.5 must be 2.0.

As UFixed 2 1 also has a Bounded instance, there even is proof that the current implementation is wrong, because in that case it is also required that

enumFromThen x y = enumFromThenTo x y bound
  where
    bound | fromEnum y >= fromEnum x = maxBound
          | otherwise = minBound

which does not hold for UFixed 2 1, x = 2.5 and y = 2.0, resulting in

[2.5,2.0,1.5,1.0,0.5,0.0] = []
@kleinreact kleinreact added the bug label Feb 13, 2025
@kleinreact kleinreact mentioned this issue Feb 13, 2025
2 tasks
@kleinreact kleinreact removed the bug label Feb 13, 2025
@martijnbastiaan
Copy link
Member

martijnbastiaan commented Feb 13, 2025

I get what you're saying, but I think a literal reading of the Enum class laws is pretty useless. These laws were written almost 30 years ago, back when there were very few numeric types to start with. From the mentioned four, only two extra existed: Ratio and Complex. The former doesn't have a Bounded instance (and its succ behaves like +1), and the latter doesn't have an Enum instance.

And so, if anything, we should read the carve out for Int..Double as a sign that the authors did realize just how silly the laws are for numeric types, that they didn't foresee Bounded numeric (fractional) types, or both. In fact you'll see that the documentation of Enum just considers numeric types not the four numeric types. E.g., for succ:

Successor of a value. For numeric types, succ adds 1.

This all leads me to the conclusion that in 2025 Enum should just be considered a way of getting [a, b ..] to do something non-silly. It's a hackjob of a class (cough fromEnum :: a -> Int, partial functions cough), but not quite terrible enough to warrant major breaking changes.

On top of that, as Peter mentioned in the other thread, we already changed the Enum a while ago to work with the interpretation of Enum I just laid out. This has caused at least some users a great deal of debug pain. So even if you think that reasoning is all wrong, it would be a great disservice to our users to change it again, only to potentially flip-flop back again years later.

To wrap it up: I don't think this is a bug on our end, nor would it be an improvement to abide by the mentioned laws. The only feasible way to improve on the situation would be to offer our own class, which I feel Finite was on its way of becoming.

@kleinreact
Copy link
Member Author

kleinreact commented Feb 14, 2025

clash-prelude is not alone in that it adds 1 to its Fixed number types:

...

On the other hand, we are deviating in behavior from the base implementation of Fixed. Why?

Image

we already changed the Enum a while ago to work with the interpretation of Enum I just laid out. This has caused at least some users a great deal of debug pain

I understood that there is a reason you don't like to implement similar behavior, but I don't understand the reason. Hence, it would really help if you could give some background on what's the actual issue here.

[Edit]
I can deduce from #1646 that you considered the Num instance of base's Fixed to break the rules, while breaking is a strong statement IMO, but it aligns with the semantics of sequential order. I cannot deduce the argument of why our implementation is desired to be different though.
[/Edit]

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

No branches or pull requests

3 participants