CoreConstructible: remove impl for Arrow, add arrow() accessor - #377
Open
apoelstra wants to merge 7 commits into
Open
CoreConstructible: remove impl for Arrow, add arrow() accessor#377apoelstra wants to merge 7 commits into
CoreConstructible: remove impl for Arrow, add arrow() accessor#377apoelstra wants to merge 7 commits into
Conversation
Despite being `pub` this is not actually exported in the public API. It is only used by the policy module, which is gated on the `elements` feature. So when compiling with `--no-default-features` we get warnings about it being unused.
In the next commits we will change the Constructible trait to represent something like "can be constructed given an Arrow". It will then be a bit hard to implement the Constructible traits for Arrow itself. (Well, it's not *that* hard, but you have to tiptoe around the possibility that your methods call each other in a cycle causing infinite recursion.) These traits are already not a great fit for Arrow itself: the witness, fail, assertl and assertr combinators all take auxiliary data that Arrow does not care about and which we can remove. The word combinator takes a whole word by value, but Arrow only cares about its length. In some sense ConstructData is already the "Arrow, but wrapped to impl the Constructible traits" type, so having Arrow implement these traits too is wasteful. We do lose the derived methods that we get from Constructible -- in particular scribe() and from_inner() are pretty useful. But scribe() is actually not that useful for Arrow (the Arrow for a scribe always has unit source and target equal to the value's type) and is never called. And from_inner is only called once, and requires a bunch of contortions with the Inner mapping combinators to make this call work. The code is actually clearer if I just inline the call (which I did, in src/node/commit.rs in the `unfinalize_types` method).
It will become very annoying to type arrow::Arrow all the time. types::Arrow would read much better (assuming you don't want to just type Arrow). The arrow module only exports two symbols, Arrow and FinalArrow, so just reexport both of these from the types module. Leave the module `pub`, though we could now make it private, to avoid unnecessarily breaking existing code.
This was just kinda bugging me. It's better to use `Self` than explicitly writing `Arrow` both because it makes refactorings easier (e.g. if we copy/paste or macroize the code, it will require no changes to apply to a different type), and because `Self` is more specific than `Arrow`. That is, `Self` means `Arrow<'brand>` while just `Arrow` means `Arrow<'_>`. This improves compiler error messages when there are lifetime problems.
In two commits we are going to refactor the `CoreConstructible` type to add an `arrow()` accessor. That is, we are redefining a "constructible node" to be one that has an unfinalized type arrow associated with it, rather than just something that shares the recursive constructor structure. This was already implicitly the case, since CoreConstructible::pair and others return a `Result<(), types::Error>`, but this change will make things explicit. To make construction of hidden nodes and "unknown programs" easier, introduce Arrow constructors that directly provide new free arrows and new 1-1 arrows. We already have two "free arrow" constructors -- Arrow::fail and Arrow::witness -- but this one is more clearly for an "unknown node" rather than a particular node that happens to have a free arrow.
stringhandler
approved these changes
Aug 25, 2026
stringhandler
left a comment
stringhandler
left a comment
Contributor
There was a problem hiding this comment.
utACK 8896e78
Maybe we should add a CI job with --no-default-features to catch this in future?
| // for `Hiding<N>` require `N: CoreConstructible`. | ||
| impl<'brand, N: CoreConstructible<'brand>> From<N> for Hiding<'brand, N> { | ||
| // We implement From<N> for arbitrary N, to avoid but the expectation is that this is | ||
| // FIXME revisit this. does it compile? |
Contributor
There was a problem hiding this comment.
Is this still relevant?
Collaborator
Author
There was a problem hiding this comment.
Oops, no, this is several iterations old. Removed.
Contributor
Added in #380 |
…ontext In the next commit we will change CoreConstructible to add an arrow() accessor. The `Hiding` type in particular requires significant refactoring to hold an arrow rather than directly holding an inference context. In particular, when creating hidden nodes, we should be creating fresh hidden arrows for them, since each hidden node is independent from all others. (Even if they have the same CMR, they may infer to different type arrows.) This is something of an annoying change -- hidden nodes don't really have type errors; there are no constraints on their source/target types so they can be used literally anywhere. This is arguably refrected best by the existing code, which does not track any type arrow for hidden nodes. However, when we change CoreConstructible to use the type arrow, we need to have something there, so everywhere that we construct a new hidden node, we call Arrow::hidden to get a new hidden arrow.
We are going to move type inference out of the `pair`/`comp`/`case` combinator functions. This will let us bypass type inference in the new "type promise" API, which in turn will eliminate the PairBuilder API in SimplicityHL as well as a ton of unwraps/expect in "constant program constructors".
apoelstra
force-pushed
the
2026-08/constructible
branch
from
August 25, 2026 13:39
1fef62f to
617c697
Compare
Contributor
|
utACK 617c697 |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 PR changes the
CoreConstructibletrait from "a node type that can be constructed from child node(s)" to "a node type that can be constructed from atypes::Arrowand child node(s)".This is a conceptually important shift but hopefully not one that requires many code changes. There are a couple goals here:
CoreConstructibletrait to represent many different kinds of things (nodes, type arrows, "hidden nodes" within policies). This enabled some code reuse, which we didn't even take advantage of, at the cost of some API friction. Lots of unused parameters and unused result return valuse.unit/iden/etc constructors are infallible while all thecomp/case/etc ones return aResult<Self, types::Error>. So you can't implement some sort of custom node type that imposes additonal restrictions. This is fine, but we could take better advantage of this. Specifically, I want to introduce infallible constructors where the caller provides atypes::Arrowand promises that the arrow is correct. (If the caller lies here they will create an ill-typed program which will then fail down the line when they attempt to finalize it or otherwise convert from aConstructNodeto a useful kind of node. You can't even serialize aConstructNodeso there's not much you can do here.)Then next PR will introduce these new constructors along with a new
TypePromiseconstruction which takes advantage of it to do infallible node construction in cases where the Rust compiler knows that the types all match.