-
Notifications
You must be signed in to change notification settings - Fork 486
PlutusTx.Data.List & PlutusTx.BuiltinList - Feature Parity #7055
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
base: master
Are you sure you want to change the base?
Conversation
import PlutusTx.Code (CompiledCode, unsafeApplyCode) | ||
import PlutusTx.Lift (liftCodeDef) | ||
import PlutusTx.Prelude qualified as P | ||
import PlutusTx.Test (goldenBundle) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please avoid wildcard imports except for the Prelude ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not asking to change this, but you're really brave introducing that symbolic name haha.
|
||
-- | Plutus Tx version of 'Data.List.zip' for 'BuiltinList'. | ||
_zip | ||
:: forall a b. (MkNil a, MkNil b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to make it MkNil (BuiltinPair a b)
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this was implied by
instance (MkNil a, MkNil b) => MkNil (BuiltinPair a b)
-> BuiltinPair (BuiltinList a) (BuiltinList b) | ||
_unzip = caseList' emptyPair | ||
( \p l -> do | ||
let BI.BuiltinPair (x, y) = p |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this isn't gonna work, you can't match on BuiltinPair
in Plinth.
|
||
-- | Plutus Tx version of 'Data.List.splitAt' for 'BuiltinList'. | ||
_splitAt | ||
:: forall a. (MkNil a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would have to be MkPair (BuiltinList a) (BuiltinList a)
, but we don't have MkPair
at all and so this function is hopeless right now, we can't define it without a MkPair
type class or a mkPair
builtin.
{-# INLINABLE replicate #-} | ||
|
||
-- | Plutus Tx version of 'Data.List.partition' for 'BuiltinList'. | ||
_partition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also hopeless without MkPair
.
|
||
-- | Plutus Tx version of 'Data.List.sortBy' for 'BuiltinList'. | ||
_sortBy :: MkNil a => (a -> a -> Ordering) -> BuiltinList a -> BuiltinList a | ||
_sortBy cmp = mergeAll . sequences |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like for this one you can perform the same trick of reusing the empty list as with dropWhile
to get rid of the MkNil
constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite unfortunately... It doesn't typecheck, and we need the call to singleton
anyway, which requires MkNil
-- They require changes to PlutusTx.Compiler.Expr.compileExpr | ||
-- See https://github.com/IntersectMBO/plutus-private/issues/1604 | ||
instance (MkNil a) => MkNil (BuiltinList a) | ||
instance (MkNil a, MkNil b) => MkNil (BuiltinPair a b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove them if they aren't gonna work anyway? If that requires removing broken code as well, I'm OK with that.
@zliu41 what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't remove them otherwise lots of the functions in BuiltinList.hs won't typecheck, they require the MkNil a
constraint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove those functions then too, they're broken anyway. Note that if you address the other comments such as introducing MkNil (BuiltinPair a b)
constraints, fewer of the functions will remain broken.
, ("PT21", "PlutusTx.BuiltinList.!!: negative index") | ||
, ("PT22", "PlutusTx.BuiltinList.!!: index too large") | ||
, ("PT23", "PlutusTx.BuiltinList.head: empty list") | ||
, ("PT24", "PlutusTx.BuiltinList.tail: empty list") | ||
, ("PT25", "PlutusTx.BuiltinList.last: empty list") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we don't need to, but it looks helpful, doesn't cost us much in terms of maintenance and it's already done. So I think let's keep them.
Co-authored-by: effectfully <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reformat all file with fourmolu
making sure that proper fourmolu.yaml config is applied. I tried to format the file and there were a lot of changes (like 50% of it).
And other new files too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a few concerns remain:
fourmolu
- dead code.
Co-authored-by: Yura <[email protected]>
let | ||
!x : Unit = trace {Unit} "PT21" Unit | ||
in | ||
error {integer} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What an optimisation!
However, you might want to switch optimisations off, as they go too far reducing this test to the form where its not testing what you want in runtime, instead eliminating the whole problem (test setup) in compile time.
(program 1.1.0 (\xs -> (\x -> error) (force trace "PT21" (constr 0 [])))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: the aspect you're trying to test (how are negative indexes handled) got optimised away.
!x : Unit = trace {Unit} "PT22" Unit | ||
in | ||
error {Unit -> integer}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like error here is unconditional too...
(constr 0 []) | ||
(constr 0 [])) | ||
(force dropList 10 xs))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ifThenElse
got eliminated...
PlutusTx.Data.List
andPlutusTx.BuiltinList
.mayMaybe
function toPlutusTx.List
"PT23"
,"PT24"
,"PT25"
error codes