Remove the deriving-aeson dependency - #7871
Conversation
31dd5c7 to
6e995d2
Compare
zliu41
left a comment
There was a problem hiding this comment.
What tests exist that confirm that the behavior didn't change?
9bf6ad6 to
3cb9eae
Compare
|
Added them in the first commit, so they run against the old instances and again unchanged against the new ones. The serialized cost models are compared against the checked-in Everything is compared as One thing worth being explicit about: the models under test are loaded from those same files by TH, so this is a fixed-point check on the encoder rather than on the cost model numbers, and anything that breaks decoding already fails at the splice. What it adds on top is the case where The rest was already covered elsewhere: |
|
Also, |
5255399 to
461ecfb
Compare
Indeed, this is the reason - |
|
@kwxm please review |
Compare the serialized form of BuiltinCostModelBase and CekMachineCostsBase against the checked-in cost-model/data files for every semantics variant, and cover the field-omitting instances used by generate-cost-model. Differences are reported as a JSON patch so that a failure names the offending entries instead of printing both cost models in full.
Replace the CustomJSON deriving-via JSON instances of the cost model types with plain aeson generic instances (genericToJSON, genericToEncoding, genericParseJSON with explicit Options). The JSON format is unchanged. Drops the allow-newer exception for deriving-aeson:aeson from cabal.project. Issue: #7870
461ecfb to
63512e4
Compare
kwxm
left a comment
There was a problem hiding this comment.
This looks fine: it simplifies the dependencies and doesn't make things significantly more complicated: if fact making things a bit more explicit may be clearer.
I had a couple of comments where I wondered if things could be improved/simplified, but they're not urgent; I think @Unisay is off at the moment and I don't want to delay merging until he gets back. I made a ticket to look at those later.
| constructor name and the remaining `MinSize` is converted to | ||
| `min_size` by the Aeson library's | ||
| [`CamelToSnake`](https://hackage.haskell.org/package/deriving-aeson-0.2.8/docs/Deriving-Aeson.html#t:CamelToSnake) | ||
| [`camelTo2`](https://hackage.haskell.org/package/aeson/docs/Data-Aeson.html#v:camelTo2) |
There was a problem hiding this comment.
I was a bit worried that these might give different results, but if you look at the implementation in deriving-aeson it has
data CamelTo (separator :: Symbol)
...
type CamelToSnake = CamelTo "_"
...
instance (KnownSymbol separator, NonEmptyString separator) => StringModifier (CamelTo separator) where
getStringModifier = camelTo2 char
where
char = case symbolVal (Proxy @separator) of
c : _ -> c
_ -> error "Impossible"
so the previous code was secretly using camelTo2 anyway.
It does look as if you have to be a bit careful about what conversion functions like this do in general. For example we have a constructor called LinearInMaxYZ and there's a question of what happens if you have a sequence of multiple uppercase letters. In this case camelTo2 '_' gives linear_in_max_yz, but linear_in_max_y_z might be justifiable as well (and it looks as if there are functions in other libraries that would do that).
| ModelJSON "modelConstantOrTwoArguments" ModelConstantOrTwoArguments | ||
| instance | ||
| ToJSON ModelConstantOrTwoArguments | ||
| instance FromJSON ModelOneArgument where |
There was a problem hiding this comment.
The code here at least is at about the same level of complexity as the previous version, but maybe a bit more explicit, so it seems like a reasonable replacement. It's still very repetitive though (and that maybe makes it tempting to cut and paste when we introduce something new, which carries a risk of failing to update some of the pasted code). I wonder if we can simplify it by introducing a typeclass of our own to do some of the work? We probably want to merge this quite soon though, so we could come back to that later instead of trying to fix it now. I've made a ticket to think about that.
| The field names, constructor tags and structure of 'BuiltinCostModelBase' and | ||
| 'CekMachineCostsBase' are a frozen format: the serialized form has to match the checked-in | ||
| @cost-model/data/*.json@ files, and the flattened keys become the ledger's cost model | ||
| parameter names. Renaming any of them is an observable breakage rather than an implementation |
There was a problem hiding this comment.
Is this comment true? The ledger currently just uses an array of untagged numbers, although I think it maybe needed the tags in the past. It's probably best not to change anything though, especially if there's a risk of changing the ordering.
| -- | Every semantics variant, with the data files holding its frozen cost model. | ||
| variants :: [(String, BuiltinSemanticsVariant DefaultFun, FilePath, FilePath)] | ||
| variants = | ||
| [ ("A", DefaultFunSemanticsVariantA, DFP.builtinCostModelFileA, DFP.cekMachineCostsFileA) |
There was a problem hiding this comment.
We'll have to remember to update this if we ever add a new variant. Can we add a check that we've covered all of the variants here? It's possible that we need stuff like this elsewhere, so maybe we could implement a single function somewhere that returns all of the variants and their associated files and use that here.
| import PlutusCore.Evaluation.Machine.CostingFun.JSON () | ||
| import PlutusCore.Evaluation.Machine.ExBudget | ||
|
|
||
| import Barbies |
There was a problem hiding this comment.
Maybe qualify this so we can see what we're using?
I tried import Barbies (ConstraintsB, TraversableB, FunctorB) but that mysteriously gave me several errors like this:
plutus-core/src/PlutusCore/Evaluation/Machine/BuiltinCostModel.hs:202:46: error: [GHC-18872]
• Couldn't match representation of type: GHC.Generics.K1
GHC.Generics.R (f ModelFourArguments)
with that of: barbies-2.1.1.0:Data.Generics.GenericN.Rec
(barbies-2.1.1.0:Data.Generics.GenericN.Param
0 f ModelFourArguments)
(f ModelFourArguments)
arising from the 'deriving' clause of a data type declaration
The data constructor ‘GHC.Generics.K1’
of newtype ‘GHC.Generics.K1’ is not in scope
• When deriving the instance for (ConstraintsB
BuiltinCostModelBase)
|
202 | deriving anyclass (FunctorB, TraversableB, ConstraintsB)
| ^^^^^^^^^^^^
Not sure what's going on there.
plutus-coreusedderiving-aesononly for theCustomJSONderiving-via instances of the cost model types. This PR rewrites those as plainaesongeneric instances (genericToJSON/genericToEncoding/genericParseJSONwith explicitOptions) and drops both the dependency and theallow-newer: deriving-aeson:aesonexception that #7820 had to add becausederiving-aesoncapsaeson < 2.3on Hackage. The only public API change:PlutusCore.Evaluation.Machine.ExBudgetno longer exportsLowerInitialCharacter, which existed solely to supportderiving-aeson.The serialized JSON is unchanged, and the two commits are ordered so that this is checkable. The first adds tests comparing the serialized cost models against the checked-in
cost-model/datafiles for every semantics variant, using those files as the expected output rather than a copy of them. The tests pass against the existing instances, and the second commit replaces the instances without touching them.Closes #7870.