Refactor out type_check #649
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Oops, needs (another) rebase. |
type_check and related functions don't need to be blanket implementations of the Property trait, they can be simple impls. This simplify things and allows to remove 2 unreachable!()
|
Rebased! |
apoelstra
added a commit
that referenced
this pull request
Mar 6, 2024
e971e77 types: remove error path from sub-fragment parameter to threshold() (Andrew Poelstra) d4dc226 types: make a ton of constructors const (Andrew Poelstra) fdea7f6 types: drop `Property` trait entirely (Andrew Poelstra) b25023d compiler: remove a bunch of unused error paths (Andrew Poelstra) eb854aa compiler: stop using Property trait in CompilerExtData (Andrew Poelstra) 602fd29 types: remove all Result returns for ExtData (Andrew Poelstra) b6c3ca8 types: don't implement Property for ExtData. (Andrew Poelstra) 87294ff types: don't implement Property for Malleability (Andrew Poelstra) dd0978d types: don't implement Property for Correctness (Andrew Poelstra) 9620380 clippy: fix a couple nits (Andrew Poelstra) 3c1c896 blanket_traits: remove crate:: in a bunch of places (Andrew Poelstra) 9280609 blanket_traits: add some more bounds (Andrew Poelstra) Pull request description: Something of a followup to #649, which specifically removes the `type_check` method from `Property`. Basically, this whole trait is confused. Originally it was supposed to express "a recursively defined property of a Miniscript" which could be defined by a couple constructors and then automatically computed for any Miniscript. In fact, the trait required a separate constructor for every single Miniscript fragment so nothing was "automatic" except sometimes reusing code between the different hash fragments and between `older`/`after`. Furthermore, every implementor of this trait had slightly different requirements, meaning that there were `unreachable!()`s throughout the code, functions that were never called (e.g. `Type::from_sha256` passing through to `Correctness::from_sha256` and `Malleability::from_sha256` when all three of those types implemented the function by calling `from_hash`, which *also* was defined by `Type::from_hash` calling `Correctness::from_hash` and `Malleability::from_hash`. So much boilerplate) and many instances of methods being implemented but ignoring arguments, or (worse) being generic over `Pk`/`Ctx` and ignoring those types entirely, or returning `Result` but always returing the `Ok` variant. So basically, there was no value in being generic over the trait and the trait wasn't even a generalization of any of the types that implemented it. **Adding to this** having the trait prevents us from having constfns in our type checking code, which sucks because we have a lot of common fixed fragments. It also prevents us from returning different error types from different parts of the typechecker, which is my specific goal in removing the trait. This PR has a lot of commits but most of them are mechanical and/or small. ACKs for top commit: sanket1729: ACK e971e77. Looks a lot cleaner. Thanks for these changes. It's nice that we no longer have the artificial error paths because the parent trait had the definition to encompass all possible implementations. Tree-SHA512: 38c254caf938e5c3f84c1f0007e0bfc93180e678aef3968cb8f16ad1544722a0f7cfda2c014056446f0091b198edb12c91c0424df64f68c31f5d316fac28425c
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This apply the same logic of ElementsProject/elements-miniscript#74
removing a couple of unreachable, simplify things and very likely reduce binary bloat.
However, in doing so, I don't think this MR is equivalent to master for the logic used forget_child.I suspect current master is broken and this fix it (or viceversa?), but I have to suspend this work for now.