refactor: introduce TemplateProgram between AST and CompiledProgram - #388
refactor: introduce TemplateProgram between AST and CompiledProgram#388apoelstra wants to merge 8 commits into
TemplateProgram between AST and CompiledProgram#388Conversation
Changes the `as_inner` method to return an `&Arc` rather than an `&str`, which makes more sense given the name, and is also useful in a couple of places. Then * Add `PartialEq` between the wrapper types and `str`, eliminating a ton of .as_inner calls entirely. * Add a `from_ident` method which converts an `Identifier` into the type, which is a common operation throughout the codebase to "strongly type" an identifier. * Add a `from_str` method to cover the remaining cases where we really do need an explicit &str
The existing code has an interesting sort of type confusion: we parse Arguments, UnresolvedValues, and WitnessValues by means of a ParserVisitor which calls into chumsky to parse the key. For each of these cases, we expect an identifier, which depending on the map, may reference either a parameter or a witness. In both cases, we put them into the WitnessName type, which covers both but does not distinguish between them. To achieve this, in src/parse.rs we implement ChumskyParse on WitnessName, by parsing an identifier and then interpreting it as a WitnessName. (We never use this codepath when parsing actual programs; in actual programs identifiers are just Identifiers until a later resolution stage; this ChumsyParse impl is only used as a helper for serde-deserialization of .args and .wit files.) (In programs, witnesses and parameters must be prefixed with `witness::` or `param::`, and these have their own parsing path.) *However*, in the following commits, we will update WitnessNames so that it distinguishes between parameters and witnesses, because we are going to delay parameter resolution until after compilation. Once we do this, the ChumskyParse impl for WitnessName will no longer be tenable, because it needs to construct a WitnessName but it doesn't know whether the identifier it's converting is supposed to be a parameter or a witness name. This commit simply refactors code and does not change any data structures or behavior.
Our "template program" is really a "templated SimplicityHL AST". I'd
like to introduce a new "templated Simplicity Program" for which the
name TemplateProgram would be a better fit. So rename this out of the
way.
This commit can be reproduced with
find bitcoind-tests external-jet-lib-example src fuzz tests \
-type f \
\( -name '*.rs' -o -name '*.md' \) \
-exec sed -i s/TemplateProgram/TemplateAst/g {} \;
Over the next couple commits I am going to generalize WitnessName to cover multiple possibilities (both named witnesses and parameters, in a new "template program" type). Start by moving stuff around, and replacing the macro-generated accessors with hand-written ones.
We are already using this type for both parameters and witnesses. Update the name to reflect that. This commit is just a search-and-replace. It is hopefully easy to review even though it's big, for that reason.
There are two purposes to this trait: * Moves a bunch of function definitions out of macro-generated code and into default trait methods; this is easier to read and improves LSP integration * Introduces the ident_to_key method, which will allow Arguments and WitnessValues to both be created from the UnresolvedValues map. Currently this is done using a From<HashMap> bound, but when we extend WitnessNames to hold parameters as well as witness names, this From bound won't be sufficient. Also, change UnresolvedValues to be keyed by Identifier rather than by WitnessName. Until we resolve the name, we don't know whether we have a parameter name or a witness name, so until then, Identifier is the more correct type.
4881f30 to
9942a5a
Compare
stringhandler
left a comment
There was a problem hiding this comment.
ACK 9942a5a Ran cargo test locally
|
Oops, this triggers a fuzz failure. Will track it down and add a unit test. |
After all our prep work this is fairly easy to do. The one tricky thing was that I had to adjust the Arbitrary impls for WitnessValues and Arguments so that their arbitrary values contain only the appropriate kind of TemplateProgramWitness.
9942a5a to
917c572
Compare
|
Ah the failure was just that my |
|
On 917c572 successfully ran local tests |
| /// | ||
| /// The supplied `arguments` are consistent with the program's parameters. | ||
| /// Call [`Arguments::is_consistent`] before calling this method! | ||
| pub fn instantiate(self, _: Arguments) -> Arc<named::CommitNode> { |
There was a problem hiding this comment.
Strange to have the unused variable here. Perhaps we need a comment explaining its reason?
There was a problem hiding this comment.
It will be used in the next PR.
| pub use crate::value::Value; | ||
| pub use crate::witness::{ | ||
| Arguments, Parameters, WitnessNameToValueMap as _, WitnessTypes, WitnessValues, | ||
| }; |
There was a problem hiding this comment.
nit: it looks like now we have two exports of WitnessNameToValueMap, one below that is gated by serde feature, and this one
There was a problem hiding this comment.
Oo, good catch. I'm not sure why I added this new export. It kinda looks like something my IDE would've done, but I don't think it'd ever add a pub use automatically.
There was a problem hiding this comment.
Oh, I think actually it was just a rebasing mistake. Anyway, fixed.
Currently this doesn't do anything. It just adds an extra step to compilation. The next commit will rearrange things such that instantiation happens -after- compilation.
917c572 to
a62a3ff
Compare
This PR does a number of refactors ahead of introducing quoting and canonical CMRs. The goal of these changes is to rearrange compilation.
Rather than the process being
Templated AST (
TemplateProgram) -> instantiated AST -> compiled programinstead it will be
Templated AST -> Templated compiled program -> compiled program
I tried to reduce churn but there are a couple renames that come with this:
TemplateProgrambecomesTemplateAst(and a newTemplateProgramtype is introduced)WitnessNames, which is the name of the witness type for our compiled programs, becomesTemplateProgramWitness, and is expanded to track both witness names and parametersIn fact,
WitnessNamesis already (ab)used to store both witness names and parameters, which makes these refactors a little easier. But this appears to be a form a code reuse rather than a deliberate API choice; the currentWitnessNamestruct is just a wrapper aroundArc<str>that does not track what kind of object it's holding, and we distinguish based on what context it appears in. Because we instatiate before compiling, any remainingWitnessNamesin a compiled program are definitely witness names and not parameter names. This PR changes that, so we need to extend the type.Once these refactors are done, the actual rearranging of the instantiation step is suprisingly easy. But it will have to wait for the next PR.
This PR has no behavior changes, does not introduce any new functionality, and does not use any features of the upcoming rust-simplicity.