Skip to content

[Builtins] Add the 'dropList' builtin#6468

Closed
effectfully wants to merge 45 commits intomasterfrom
effectfully/builtins/add-dropList
Closed

[Builtins] Add the 'dropList' builtin#6468
effectfully wants to merge 45 commits intomasterfrom
effectfully/builtins/add-dropList

Conversation

@effectfully
Copy link
Contributor

This experiment adds the dropList builtin so that folks can play with it. Not intended for merging.

@effectfully effectfully added Do not merge Builtins EXPERIMENT Experiments that we probably don't want to merge labels Sep 11, 2024
@effectfully effectfully force-pushed the effectfully/builtins/add-dropList branch from 017e9d4 to df4dea7 Compare September 11, 2024 18:10
@effectfully effectfully added the No Changelog Required Add this to skip the Changelog Check label Sep 11, 2024
@effectfully effectfully force-pushed the effectfully/builtins/add-dropList branch from df4dea7 to 7be8e1f Compare September 14, 2024 00:43
@effectfully effectfully force-pushed the effectfully/builtins/add-dropList branch from 7be8e1f to e22a57b Compare September 14, 2024 03:40
@effectfully effectfully mentioned this pull request Oct 23, 2024
3 tasks
@effectfully effectfully self-assigned this Oct 29, 2024
@effectfully effectfully force-pushed the effectfully/builtins/add-dropList branch from 16db02d to 41ea9b6 Compare October 29, 2024 23:43
Copy link
Contributor

@kwxm kwxm left a comment

Choose a reason for hiding this comment

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

I've added some comments based on a quick look. I may have another look later.

(runCostingFunOneArgument . paramNullList)

toBuiltinMeaning _semvar DropList =
let dropListDenotation :: Int -> SomeConstant uni [a] -> BuiltinResult (Opaque val [a])
Copy link
Contributor

@kwxm kwxm Oct 30, 2024

Choose a reason for hiding this comment

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

The time taken for this will probably be linear in the value (not the size!) of the first argument. That'll mean using some newtype wrapper, like this . We already have IntegerCostedLiterally, but this has an Int so that may need another wrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

^ @ramsay-t FYI

but this has an Int so that may need another wrapper.

Actually, why did I even make it Int, what's the point? Looks like a thinko.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The time taken for this will probably be linear in the value (not the size!) of the first argument.

No, I think it's gonna be a minimum of

  • linear in the value of the first argument
  • linear in the length of the second argument

since drop maxBound [] equals [].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, why did I even make it Int, what's the point? Looks like a thinko.

It's proper Integer in there now.

Copy link
Contributor

@kwxm kwxm Nov 5, 2024

Choose a reason for hiding this comment

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

No, I think it's gonna be a minimum of
linear in the value of the first argument
linear in the length of the second argument
since drop maxBound [] equals [].

I deliberately ignored that case! If we include the length of the list in the cost calculation then we'll have to traverse the entire list to measure the length when we're about to calculate the cost, and that'll slow things down. If we just base it on the number of things we want to drop then we'll still get a conservative estimate of the cost, and it'll be faster. Also, if you drop the whole of the list then you've probably done something wrong, and people probably won't submit scripts where that actually happens (or at least not often).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I see. OK, that's clever. Yeah, I think I agree, although I'd perhaps prefer to store lists together with their length, although that'd perhaps slow everything else down. Fair enough!

@ramsay-t ignore what I said and only consider the original comment in this thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd perhaps prefer to store lists together with their length

I've wondered about doing that in the past (and maybe something similar with Data for instance), but what happens if someone lies about the length of a list in their script? Maybe that could be checked during deserialisation ...

, headMaybe
, BI.head
, BI.tail
, BI.drop
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also go in PlutusTx.Prelude? I seem to remember that the contents of that file are kind of random though.

chooseList (BuiltinList (_:_)) _ b2 = b2

{-# OPAQUE drop #-}
drop :: Integer -> BuiltinList a -> BuiltinList a
Copy link
Contributor

@kwxm kwxm Oct 30, 2024

Choose a reason for hiding this comment

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

Why not dropList? So that it just replaces the Haskell version with minimal effort from the user?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For consistency, we also have head (and not headList) and tail (and not tailList) in this file.

@effectfully
Copy link
Contributor Author

/benchmark validation

@effectfully effectfully force-pushed the effectfully/builtins/add-dropList branch from f999164 to e8c0b9d Compare November 4, 2024 15:56
@effectfully
Copy link
Contributor Author

/benchmark validation

Adding `dropList` caused the `plutus-ledger-api` tests to fail.  This should fix it.
Adding `dropList` caused the `plutus-ledger-api` tests to fail.  This should fix it.
@kwxm
Copy link
Contributor

kwxm commented Feb 4, 2025

This had become very difficult to merge, so it's been closed in favour of #6817.

@kwxm kwxm closed this Feb 4, 2025
@kwxm kwxm deleted the effectfully/builtins/add-dropList branch September 13, 2025 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Builtins Do not merge EXPERIMENT Experiments that we probably don't want to merge No Changelog Required Add this to skip the Changelog Check

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants