Handle read_chan_signer in bindings (761 bindings updates)
#767
Conversation
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
Codecov Report
@@ Coverage Diff @@
## main #767 +/- ##
==========================================
- Coverage 91.23% 91.23% -0.01%
==========================================
Files 37 37
Lines 22921 22921
==========================================
- Hits 20913 20911 -2
- Misses 2008 2010 +2
Continue to review full report at Codecov.
|
TheBlueMatt
force-pushed
the
2020-12-chansigner-read-bindings
branch
2 times, most recently
from
d067833 to
a3b46a7
Compare
|
Rebased onto new upstream changes. |
Our bindings generator is braindead with respect to the idents used in a trait definition - it treats them as if they were used where the trait is being used, instead of where the trait is defined. Thus, if the idents used in a trait definition are not also imported the same in the files where the traits are used, we will claim the idents are bogus. I spent some time trying to track the TypeResolvers globally through the entire conversion run so that we could use the original file's TypeResolver later when using the trait, but it is somewhat of a lifetime mess. While likely possible, import consistency is generally the case anyway, so unless it becomes more of an issue in the future, it likely makes the most sense to just keep imports consistent. This commit keeps imports consistent across trait definition files around `MessageSendEvent` and `MessageSendEventsProvider`.
jkczyz
reviewed
Feb 2, 2021
| assert_simple_bound(&tr); | ||
| if let Some(mut path) = types.maybe_resolve_path(&tr.path, None) { | ||
| if types.skip_path(&path) { continue; } | ||
| let new_ident = if path != "std::ops::Deref" { |
There was a problem hiding this comment.
Unclear why std::ops::Deref has special handling.
There was a problem hiding this comment.
I added a comment, but in general we handle Deref<Target=X> as X because they're mostly used for traits which we handle by impl Deref<Target=Self> for our traits.
Instead of handling associated types separately, we can just shove them into the same generics resolution logic we use for template types. While we should probably have some precedence logic, aliasing type names seems like a bad idea anyway so no effort is made to handle it. This removes a good chunk of code and, more importantly, tees us up for supporting `Type<Self::AssociatedType>`-style generics.
In the case that we return an associated type to C (ie when implementing a trait which returns an associated type, we had to convert the Rust-returned concrete Rust type to the C trait struct), we had code to manually create the neccessary trait struct at the return site. This was special-cased in the method-body-writing function instead of letting the type conversion logic handle it. As a result, we are unable to do the same conversion when it appears in a different context, for example inside of a generic like `Result<Self::AssocType, ErrorType>`. To solve this, we do the actual work in a `impl From<nativeType> for CTraitStruct` implementation and then call `into()` from within the type conversion logic.
With this we support types like `Result<Self::AssociatedType, ()>`.
This is a rather trivial cleanup to ensure we always have the full path when we walk supertraits even if the supertrait is specified with only a single ident.
Instead of having manually-written lightning-specific code in a supertrait walk in the middle of a large function, move it to a utility function up next to the other manually-written-impl-block functions.
`CommitmentTransaction::new_with_auxiliary_htlc_data()` includes a unbounded generic parameter which we can't concretize and it's of limited immediate use for users in any case. We should eventually add a non-generic version which uses `()` for the generic but that can come later. `CommitmentTransaction::htlcs()` returns a reference to a Vec, which we cannot currently map. It should, however, be exposed to users, so in the future we'll need to have a duplication function which returns Vec of references or a cloned Vec.
Previously, types which were declared and used in the same file would fail if the use was before the declaration. This makes sense in a few cases where a "parent" class returns a reference to a "child" class and there's no reason we shouldn't support it. This change adds a second pass to our file processing which gathers the structs and enums whicha re declared in the file and adds them to the type resolver first, before doing the real conversion.
It is confusing to have two utility methods on different classes of types which do two different, but related, things with the same name.
If you try to call take_ptr on a pointer to an object which implements Deref, rustc hits the deref recursion limit. To avoid this, we can explicitly tell rustc that we want to treat the pointer as a pointer and call take_ptr on it directly.
Newer rustc complains that "attribute should be applied to a function or static"
We no longer have any public `Option<Signatures>` in our code, and thus get warnings that the two functions which support it are unused. Instead of removing support for them (which we may need in the future), we add `#[allow(unused)]`.
TheBlueMatt
force-pushed
the
2020-12-chansigner-read-bindings
branch
from
a3b46a7 to
93e562f
Compare
|
Addressed review comments and rebased on latest upstream (without any changes except pulling the bindings update commit out to its own commit and redoing it for upstream changes). |
jkczyz
approved these changes
Feb 3, 2021
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.
Based on #761, this handles the new associated-type-returning trait method in the bindings. The bindings bits are decently large, so its going to stand as its own PR here. Between binidngs-update commits which build+run, a bindings diff commit exists to ease review, though those can be squashed before merge.