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 Finite class #2858

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add Finite class #2858

wants to merge 2 commits into from

Conversation

kleinreact
Copy link
Member

@kleinreact kleinreact commented Dec 30, 2024

The PR adds the Finite class as well as supplemental instances for most of the standard types.

Finite is a class for types with only finitely many inhabitants and can be considered a more hardware-friendly alternative to Bounded and Enum, utilizing Index instead of Int and vectors instead of lists.

Have a look at the haddock documentation for further insights.

Requires

Still TODO:

  • Write a changelog entry (see changelog/README.md)
  • Check copyright notices are up to date in edited files

Copy link
Member

@martijnbastiaan martijnbastiaan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of questions! Mostly wondering whether it would make sense to provide an Index-based Enum instead.

Comment on lines 247 to 251
res = x :> prev'
prev' = case natVal (asNatProxy prev') of
0 -> repeat def
_ -> let next = x +>> prev
in registerB clk rst en (repeat def) next
_ -> let next' = x +>> prev'
in registerB clk rst en (repeat def) next'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that you have to do this is a pretty strong hint these names shouldn't be in Clash.Prelude. Grepping my projects for prev and next also reveals a number of uses.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's kind of unavoidable. Every new name we choose might already be picked up in existing user code and I doubt that it's in the interest of the user, if we choose artificially unintuitive names just because of that.

So yes, the user might experience some new warnings about "clashing" names after a Clash update, which are always easy to fix via renaming or hiding the new stuff from Clash.Prelude, in case they are not desired.

Please note that finding a good (and intuitive) naming scheme that doesn't clash with basic existing libraries (in particular base) is a challenge already, especially if the offered primitives are quire fundamental. I also tried to improve the situation here based on some prior experience I had with the finite library, which comes with a similar interface.

Copy link
Member

@DigitalBrains1 DigitalBrains1 Dec 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point is we write many state machines in Clash and there prev and next are very intuitive local names. Of course you can't avoid all name clashes, but there are some words that are very common in Clash code and prev and next are two examples of that specific category. So I agree with Martijn that trying to come up with an alternative is worthwhile in this specific case.

[edit]
Also, I believe we felt we wanted to move away from primes as a suffix in our code, instead using numeric suffixes to disambiguate. So I believe it'd be better to do that instead of introducing new uses of prime in our code base.
[/edit]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before/after?

prior/later?

back/forward?

Did not think them through much, just throwing it out there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fpred / fsucc for finite versions of Enum functions?

Copy link
Member Author

@kleinreact kleinreact Jan 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the function names to before and after now, as I just didn't feel comfortable with the options discussed so far (except before and after, as suggested by @DigitalBrains1).

Copy link
Member

@martijnbastiaan martijnbastiaan Jan 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback and all the suggestions. I like to change it to nextElement and previousElement then, which I still consider intuitive and recognizable.

This suffers from the same synonym (min vs minimum) issue I mentioned.

Edit: didn't see your message after. Though before and after still do. I don't get what's so bad about fpred/fsucc? At least 3 people in this thread seem to be fine with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking, there is precedent for this:

  • succ
  • countSucc
  • satSucc

I that list finSucc makes sense.

If I can poke a little bit in your thoughts: I think what you're rubbing against is the fact that you think names in Clash.Class.Finite shouldn't be influenced by names in other classes because it is its own namespace. This is typically true, but we're making one God-module that contains everything (Clash.Prelude), which makes the namespace mangled. The friction becomes especially annoying when you're trying to the RightThing(tm) and use qualified imports and then end up with:

Finite.finSucc

which succs!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This suffers from the same synonym (min vs minimum) issue I mentioned.

@martijnbastiaan Sry, I didn't saw your edit till now. I still don't get what the synonym issue is about? For me min is a shorthand for minimum and minimum is a proper English word you can find in a dictionary. So the name characterization looks quite clear.

Can you elaborate on your thoughts here a bit?

It's not that I don't like the names, but I think it's just not necessary to introduce new terminology, if we already have words that very well describe the functionality of these methods here. Just consider you'd live in universe that neither knows Clash nor Haskell (which certainly applies to a bunch of people in the world)? How would you interpret fpred and fsucc, just given the names?

Copy link
Member

@martijnbastiaan martijnbastiaan Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with synonyms is that they are words that refer to the same thing (modulo some culturally defined emphasis perhaps). So it makes it impossible to tell them apart -- you have to learn it by hard. As an example I used min vs minimum: is it:

min :: Ord a => a -> a -> a
minimum :: (Foldable t, Ord a) => t a -> a

or the other way around? You can't tell by the names! Similarly, if we were to do this for the various succs around Clash.Prelude you'd get (for example): succ, successor, next, and after. Sure, the identifiers are different, but you'd have to learn by hard which identifiers refer to which concept. That's why I think satSucc, finSucc, countSucc, and succ are much much better.

(To be clear: I think the actual solution is proper name spacing, but that ship has more or less sailed. Something we could consider is using succ for the classes and then prefixing them in Clash.Prelude.)

clash-prelude/src/Clash/Class/Finite/Internal.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Num/Zeroing.hs Show resolved Hide resolved
clash-prelude/src/Clash/Num/Wrapping.hs Show resolved Hide resolved
clash-prelude/src/Clash/Num/Saturating.hs Show resolved Hide resolved
clash-prelude/src/Clash/Class/Finite/Internal.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Class/Finite/Internal.hs Outdated Show resolved Hide resolved
@kleinreact kleinreact force-pushed the finite-class branch 8 times, most recently from 7a1399a to 1130232 Compare January 1, 2025 19:16
@martijnbastiaan
Copy link
Member

Another thing to consider: there is at least some overlap with Counter I believe. Perhaps Counter's superclass should be Finite?

@kleinreact kleinreact force-pushed the finite-class branch 2 times, most recently from 9564261 to 80dc978 Compare January 2, 2025 07:17
@kleinreact
Copy link
Member Author

Another thing to consider: there is at least some overlap with Counter I believe. Perhaps Counter's superclass should be Finite?

Good point. Making Finite a superclass would be a recognizable API change tough. Also, you technically can have a Counter instance for a type with infinitely many inhabitants, so adding the superclass requirement here would introduce a limitation.

How about adding another deriving strategy for Counter via FiniteDerive instead, like already present for Enum? Then the users can decide on their own, whether counting via the implicit index order of the Finite instance is exactly what they want.

@martijnbastiaan
Copy link
Member

Making Finite a superclass would be a recognizable API change tough.

I think however we slice it this PR will be an API change, unless we decide to not export it from Prelude. So that's fine with me (provided that there wouldn't be runtime changes).

How about adding another deriving strategy for Counter via FiniteDerive instead, like already present for Enum? Then the users can decide on their own, whether counting via the implicit index order of the Finite instance is exactly what they want.

I think Finite would count the same as Enum for sum-types right? In that case I don't see the benefit of offering both, but I'm also not opposed to it.

@kleinreact kleinreact force-pushed the finite-class branch 3 times, most recently from decaec3 to c8821c4 Compare January 7, 2025 07:55
clash-prelude/src/Clash/Class/Finite/Internal.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Class/Finite/Internal/TH.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Class/Finite/Internal/TH.hs Outdated Show resolved Hide resolved
elements = to <$> gElements

-- | Just the @0@ indexed element. Nothing if @ElementCount a = 0@.
lowest :: Maybe a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit conflicted about the Maybe on lowest and highest.
As in practice it seems a bit annoying.
And I'm unsure how useful Finite instances for empty types are.
But it is cool that elements @(Either (Maybe Bool) Void) is just a vector with only Lefts and no Rights.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change the type of lowest and highest to

lowest, highest :: 1 <= ElementCount a => a

and add some additional methods lowestMaybe and highestMaybe for user convenience. Only requires ConstrainedClassMethods, which shouldn't cause any issues.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmyeah, I'm also conflicted. I think we've got a couple of options:

  1. Maybe
  2. Constraint (1 <= ElementCount a)
  3. Producing bottoms (errorX "....")
  4. Define two classes: one that allows Void-like types, one that doesn't.
  5. Expose both functions

If precedent is worth anything, we've typically opted for (3). In cases where we did pick (2), we later moved to (3) because the constraints were annoying in practice. Producing bottoms isn't ideal either: I wish we had a non-partial unpack, for example.

So what about 5: having a lowestMightBeMaybe :: Maybe a (name to be bikeshedded) and a lowestMightProduceErrorX :: a?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the difference between lowestMightProduceErrorX and fromJust . lowestMightBeMaybe. Hence, if the user prefers to introduce partial functions in his codebase, then the Maybe version gives all freedom to do so.

This is why I prefer having lowest and lowestMaybe as suggested above. With that, the user has all options at hand:

  1. user prefers maybe: use lowestMaybe
  2. user prefers errors: use fromJust lowestMaybe (or fromMaybe (error "🙀") lowestMaybe if an individual error message is desired)
  3. user prefers constraints: use lowest

Also note that the constraint only appears for polymorphic instances. Thus, you really need to write code that needs to work for all Finite instances (using a Finite a => constraint) to encounter that constraint. In practice that's usually only the case when creating a new container type (like Vec) with a Finite instance derivation rule.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah makes sense to me. Just let the user choose! :)

Comment on lines +120 to +141
-- Any definition must satisfy the following laws (automatically
-- ensured when generic deriving the instance):
--
-- [Index Order]
-- @ index '<$>' elements = 'indicesI' @
-- [Forward Iterate]
-- @ 'iterateI' ('>>=' after) (lowest \@a) = 'Just' '<$>' (elements \@a) @
-- [Backward Iterate]
-- @ 'iterateI' ('>>=' before) (highest \@a)
-- = 'Just' '<$>' 'reverse' (elements \@a) @
-- [Index Isomorphism]
-- @ith (index x) = x@
-- [Minimum Predecessor]
-- @ lowest '>>=' before = 'Nothing' @
-- [Maximum Successor]
-- @ highest '>>=' after = 'Nothing' @
-- [No Uninhabited Extremes]
-- @ lowest \@a = 'Nothing' /and/ highest \@a = 'Nothing'
-- /if and only if/ ElementCount a = 0 @
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it makes sense to add something like:

-- If this type is also a `Bounded` instance, it should satisfy:
-- @ lowest = Just minBound @
-- @ highest = Just maxBound @

This would make the current instance of Down unlawful.

But maybe that's a good thing.
The whole thing of Down is that it flips things like Ord,Bounded,Enum, so it probably makes sense that it flips the Finite ordering too.
You could derive the Down instance via ReversedIndexOrder, or alternatively drop ReversedIndexOrder and rely on Down instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, the index order of Down should be reversed. I updated it accordingly.

ReversedIndexOrder originally was intended to reverse the index order of new data types without the need of wrapping them into a newtype. However, I missed to make it dependent on the generic class instead, which is fixed now.

To make that more clear, I renamed the newtype to GenericReverse and added some documentation that explains why we wanna have both: Down and GenericReverse.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a Bounded related law depends on whether we like Finite to be in relation with other base classes in the first place.

It might be desirable here to have a standalone class that can exist independently of Bounded and Enum. Just to make the differences between HW / SW and finite / infinite types more concrete.

I don't have a strong opinion on that though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants