-
Notifications
You must be signed in to change notification settings - Fork 484
[Builtin] Casing on Data.Constr
#7315
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
Execution Budget Golden Diffoutputplutus-benchmark/coop/test/9.6/authMpBurning.eval.golden
plutus-benchmark/coop/test/9.6/authMpMinting.eval.golden
plutus-benchmark/coop/test/9.6/certMpBurning.eval.golden
plutus-benchmark/coop/test/9.6/certMpMinting.eval.golden
plutus-benchmark/coop/test/9.6/fsMpBurning.eval.golden
plutus-benchmark/coop/test/9.6/fsMpMinting.eval.golden
plutus-benchmark/coop/test/9.6/mustBurnOwnSingleton.eval.golden
plutus-benchmark/linear-vesting/test/9.6/main.eval.golden
plutus-benchmark/script-contexts/test/V1/9.6/checkScriptContext1-20.eval.golden
plutus-benchmark/script-contexts/test/V1/9.6/checkScriptContext1-4.eval.golden
plutus-benchmark/script-contexts/test/V1/9.6/checkScriptContext2-20.eval.golden
plutus-benchmark/script-contexts/test/V1/9.6/checkScriptContext2-4.eval.golden
plutus-benchmark/script-contexts/test/V2/9.6/checkScriptContext1-20.eval.golden
plutus-benchmark/script-contexts/test/V2/9.6/checkScriptContext1-4.eval.golden
plutus-benchmark/script-contexts/test/V2/9.6/checkScriptContext2-20.eval.golden
plutus-benchmark/script-contexts/test/V2/9.6/checkScriptContext2-4.eval.golden
plutus-benchmark/script-contexts/test/V2/9.6/sopFwdStakeTrick.eval.golden
plutus-benchmark/script-contexts/test/V3/9.6/checkScriptContext1-20.eval.golden
plutus-benchmark/script-contexts/test/V3/9.6/checkScriptContext1-4.eval.golden
plutus-benchmark/script-contexts/test/V3/9.6/checkScriptContext2-20.eval.golden
plutus-benchmark/script-contexts/test/V3/9.6/checkScriptContext2-4.eval.golden
plutus-benchmark/script-contexts/test/V3/Data/9.6/purposeIsWellFormed-4.eval.golden
plutus-ledger-api/test-plugin/Spec/Data/Budget/9.6/geq1.eval.golden
plutus-ledger-api/test-plugin/Spec/Data/Budget/9.6/geq2.eval.golden
plutus-ledger-api/test-plugin/Spec/Data/Budget/9.6/geq3.eval.golden
plutus-ledger-api/test-plugin/Spec/Data/Budget/9.6/geq4.eval.golden
plutus-ledger-api/test-plugin/Spec/Data/Budget/9.6/geq5.eval.golden
plutus-ledger-api/test-plugin/Spec/Data/Budget/9.6/gt1.eval.golden
plutus-ledger-api/test-plugin/Spec/Data/Budget/9.6/gt2.eval.golden
plutus-ledger-api/test-plugin/Spec/Data/Budget/9.6/gt3.eval.golden
plutus-ledger-api/test-plugin/Spec/Data/Budget/9.6/gt4.eval.golden
plutus-ledger-api/test-plugin/Spec/Data/Budget/9.6/gt5.eval.golden
plutus-tx-plugin/test/Budget/9.6/map3.eval.golden
plutus-tx-plugin/test/Budget/9.6/toFromData.eval.golden
This comment will get updated when changes are made. |
|
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.
CEK part is more or less set, but plinth part needs some rework. Feel free to just review plutus-core part only.
vArgTypes <- traverse (fmap unNormalized . normalizeTypeM) argTypes | ||
-- made of sub-parts of a normalized type, so normalized | ||
checkTypeM ann c (Normalized $ mkIterTyFun () vArgTypes (unNormalized vResTy)) | ||
VariableArityBranch c argType -> do |
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 really happy here, but don't think there's any better way to go about without bigger changes. The fact that each branch can have arbitrary number of arguments introduces this dynamic behavior. So, it is only possible to check the return type and to check if are inputs(of arbitrary number) have correct type.
data CaseDataConstrBranch = forall a. CaseDataConstrBranch a | ||
|
||
caseDataConstr :: forall r. BuiltinData -> [CaseDataConstrBranch] -> r | ||
caseDataConstr d branches = | ||
let | ||
constr = unsafeDataAsConstr d | ||
idx = PlutusTx.Builtins.Internal.fst constr | ||
ds = PlutusTx.Builtins.Internal.snd constr | ||
unwrap (CaseDataConstrBranch x) = unsafeCoerce x | ||
applyToBranch :: r -> BuiltinList BuiltinData -> r | ||
applyToBranch body = | ||
caseList' body (\arg -> applyToBranch (unsafeCoerce body $ arg)) | ||
in applyToBranch (unwrap $ branches !! fromIntegral idx) ds | ||
{-# OPAQUE caseDataConstr #-} |
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.
This is horrendous; needed that existential type since number of argument each branch handler takes is arbitrary. This works, but it's impossible to make it also work for when we disable builtin-casing compilation option.
I'm going to try other ways. Namely, using \[a, b, c] -> ...
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.
Made some changes. It works good now without this atrocity
Memory: 574_864 | ||
Term Size: 954 | ||
Flat Size: 1_499 | ||
CPU: 46_154_228 |
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.
Nice
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 don't like this design. See the "What exactly breaks if we add the unsafe constrTermFromConstrData?" section of #5777 for why.
If the team thinks this is what we should do, I won't fight it. But I believe it is not what we should do.
Implements
where each branches get each elements of the "field" part of
Data.Constr
as arguments. This allows deconstruction ofData.Constr
value in single CEK step.