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 MINIMAL pragma to ord #6657

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

Conversation

aleeusgr
Copy link

@aleeusgr aleeusgr commented Nov 13, 2024

addresses #6270

  • fix PlutusTx.Ord
  • check the existence of other minimal pragmas where such applies
    • get the list of all Type Classes in plinth
      git grep -e "^class .* where" -- **/*.hs
    • get the list of Type Classes where the pragma needs to be added.
  • fix commit signing

Background

Ord is a type class that defines a total ordering for types.

A pragma is a special instruction or directive that provides additional information to the compiler, allowing for optimizations or modifications in how the code is processed. Pragmas are denoted by the syntax {-# ... #-}, and they do not generally affect the semantic meaning of the program, but they can influence performance and behavior during compilation.

the MINIMAL pragma is used to specify the minimal complete definition of a type class, indicating which methods must be implemented for an instance of that class. This is particularly useful for classes that have methods with circular defaults, ensuring that instances adhere to a defined interface.

https://downloads.haskell.org/~ghc/7.8.1/docs/html/users_guide/pragmas.html

base

Checklist

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Changelog fragments have been written (if appropriate)
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • (For external contributions) Corresponding issue exists and is linked in the description
    • Targeting master unless this is a cherry-pick backport
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

aleeusgr

This comment was marked as resolved.

@aleeusgr

This comment was marked as resolved.

@aleeusgr aleeusgr force-pushed the 6270-add-minimal-pragma-to-ord branch 2 times, most recently from 00a2746 to 9a49bec Compare November 14, 2024 04:03
@aleeusgr
Copy link
Author

aleeusgr commented Nov 14, 2024

$ git grep -e "^class .* where" -- **/*.hs

output

src/PlutusTx/Applicative.hs:class Functor f => Applicative f where
src/PlutusTx/Blueprint/Class.hs:class HasBlueprintSchema (t :: Type) (referencedTypes :: [Type]) where
src/PlutusTx/Blueprint/Definition/Derive.hs:class DefinitionsFor' referencedTypes acc where
src/PlutusTx/Blueprint/Definition/Unroll.hs:class HasBlueprintDefinition (t :: Type) where
src/PlutusTx/Builtins/HasBuiltin.hs:class PLC.DefaultUni `PLC.Contains` a => HasToBuiltin a where
src/PlutusTx/Builtins/HasBuiltin.hs:class HasToBuiltin (FromBuiltin arep) => HasFromBuiltin arep where
src/PlutusTx/Builtins/HasOpaque.hs:class HasToOpaque a arep | a -> arep where
src/PlutusTx/Builtins/HasOpaque.hs:class HasFromOpaque arep a | arep -> a where
src/PlutusTx/Builtins/HasOpaque.hs:class MkNil arep where
src/PlutusTx/Enum.hs:class Enum a where
src/PlutusTx/Eq.hs:class Eq a where
src/PlutusTx/Foldable.hs:class Foldable t where
src/PlutusTx/Functor.hs:class Functor f where
src/PlutusTx/IsData/Class.hs:class ToData (a :: Type) where
src/PlutusTx/IsData/Class.hs:class FromData (a :: Type) where
src/PlutusTx/IsData/Class.hs:class UnsafeFromData (a :: Type) where
src/PlutusTx/Lattice.hs:class JoinSemiLattice a where
src/PlutusTx/Lattice.hs:class MeetSemiLattice a where
src/PlutusTx/Lattice.hs:class JoinSemiLattice a => BoundedJoinSemiLattice a where
src/PlutusTx/Lattice.hs:class MeetSemiLattice a => BoundedMeetSemiLattice a where
src/PlutusTx/Lift/Class.hs:class Typeable uni (a :: k) where
src/PlutusTx/Lift/Class.hs:class Lift uni a where
src/PlutusTx/Monoid.hs:class Semigroup a => Monoid a where
src/PlutusTx/Monoid.hs:class Monoid a => Group a where
src/PlutusTx/Numeric.hs:class AdditiveSemigroup a where
src/PlutusTx/Numeric.hs:class AdditiveSemigroup a => AdditiveMonoid a where
src/PlutusTx/Numeric.hs:class AdditiveMonoid a => AdditiveGroup a where
src/PlutusTx/Numeric.hs:class MultiplicativeSemigroup a where
src/PlutusTx/Numeric.hs:class MultiplicativeSemigroup a => MultiplicativeMonoid a where
src/PlutusTx/Numeric.hs:class (Ring s, AdditiveGroup v) => Module s v | v -> s where
src/PlutusTx/Ord.hs:class Eq a => Ord a where
src/PlutusTx/Semigroup.hs:class Semigroup a where
src/PlutusTx/Show/TH.hs:class Show a where
src/PlutusTx/Traversable.hs:class (Functor t, Foldable t) => Traversable t where

Copy link
Contributor

@effectfully effectfully left a comment

Choose a reason for hiding this comment

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

Looks good, let's wait for Kenneth to respond and then we'll either ignore the warning in the nofib file or keep your change.

@@ -203,6 +203,7 @@ instance TxPrelude.Eq Assign
where (a := b) == (a' := b') = a==a' && b==b'
instance TxPrelude.Ord Assign
where (a := b) < (a' := b') = (a<a') || (a==a' && b < b')
(a := b) <= (a' := b') = (a<a') || (a==a' && b <= b')
Copy link
Contributor

Choose a reason for hiding this comment

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

@kwxm do you remember anything about the nofib implementation? Is it fine to make this change here? Should we just disable the warning not allowing us to make this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, whatever, @aleeusgr could you please just add

{-# OPTIONS_GHC -Wno-missing-methods #-}

to the top of the file and that's it?

Copy link
Contributor

Choose a reason for hiding this comment

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

... and keep the original

(a := b) < (a' := b') = (a<a') || (a==a' && b < b')

definition.

@effectfully
Copy link
Contributor

Thanks for the list of the classes! I'll review it myself, that won't block the PR.

@aleeusgr aleeusgr force-pushed the 6270-add-minimal-pragma-to-ord branch from 9a49bec to 80b8a5a Compare November 18, 2024 04:50
Signed-off-by: Alex <alexeusgr@gmail.com>
@effectfully
Copy link
Contributor

Eek, I'm an idiot and forgot about this one. Sorry! Looking into it.

@effectfully effectfully marked this pull request as ready for review November 29, 2024 07:03
@effectfully
Copy link
Contributor

The only other place where we might need a MINIMAL pragma is PlutusTx.Enum.Enum, but it doesn't have the default implementations for some reason, so let's ignore it. This PR looks good to me, but please address the comment above and you also need to sign your commits, otherwise merging is blocked and I can't do anything about it.

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