-
Notifications
You must be signed in to change notification settings - Fork 718
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support dependent qualified types #1975
Conversation
Current state of play: need to fix a few tests which are failing:
|
I've fixed all known problems other than |
c829066
to
8bb23ad
Compare
Current situation: the tests fail on my machine even with bindgen master. I think this is because my machines have libclang 10 and 11, and I am not eager to try to downgrade. I'm therefore unlikely to finish up this PR especially soon. It may remain in draft state for a while. Next steps:
|
Sorry for the lag, I had exams these last few weeks. That's weird, tests pass here with clang 11. How do the failures look like? |
If you're on a mac, it might be #1948 or such? |
Thanks for the reply. Now I know it's supposed to work I will characterize my problems and if necessary file an issue to help get to the bottom of it. |
Here's a status update here. I got the tests working (on my Linux box) simply by updating to a more modern version of the This ( template <typename T>
struct Foo {
template <typename> using FirstAlias = typename T::Associated;
template <typename U> using SecondAlias = Foo<FirstAlias<U>>;
SecondAlias<int> member;
}; With the branch in this PR, this gives:
There are two problems with this.
error[E0277]: the trait bound `i32: __bindgen_has_inner_type_Associated` is not satisfied
--> tests/issue-544-stylo-creduce-2.rs:14:5
|
14 | pub member: Foo_SecondAlias<i32>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `__bindgen_has_inner_type_Associated` is not implemented for `i32` i.e. the compiler error would be a faithful representation of the C++ error message you'd get if you tried to instantiate the template, but this error would occur in Rust even if you didn't try to instantiate it. Once I've fixed the first problem, we will have to decide between these two policies:
|
I need to check the patch carefully, but I think that this:
Is totally acceptable (and I suspect it happens elsewhere as well). So I wouldn't oppose to tweak the test-case to do something less silly or what not. |
Thanks for the comment. That's good news. Honestly this test case is just making me go cross-eyed even in trying to fix the first bug. This patch may still be some time before it's reviewable :) That said, any initial thoughts about whether I'm along the right lines would be great. |
f660959
to
64ac52f
Compare
@emilio I am finally declaring this is ready for review. A few things to note:
|
Actually - I thought of some corner cases which I should add tests for (and in the absence of tests it's virtually guaranteed I've messed them up). |
☔ The latest upstream changes (presumably e6684dc) made this pull request unmergeable. Please resolve the merge conflicts. |
Will be used elsewhere in subsequent commits.
Will be used in subsequent commits, to avoid trying to resolve items which we've loaned and therefore can't be resolved.
For a situation like this: struct InnerType { typedef int related_type; int foo; int foo2; }; template <typename ContainedType> class Container { public: typedef typename ContainedType::related_type content_ty; content_ty contents_; }; typedef Container<InnerType> Concrete; struct LaterContainingType { Concrete outer_contents; }; previously bindgen would have generated an opaque type. It now correctly recognizes that the contents_ field is of a new TypeKind::DependentQualifiedName and correctly identifies the right type to use when instantiating Concrete. The generated code looks like this: #[repr(C)] #[derive(Debug, Default, Copy, Clone)] pub struct InnerType { pub foo: ::std::os::raw::c_int, pub foo2: ::std::os::raw::c_int, } pub type InnerType_related_type = ::std::os::raw::c_int; pub trait __bindgen_has_inner_type_related_type { type related_type: std::fmt::Debug + Default + Copy + Clone; } impl __bindgen_has_inner_type_related_type for InnerType { type related_type = InnerType_related_type; } #[repr(C)] #[derive(Debug, Default, Copy, Clone)] pub struct Container<ContainedType: __bindgen_has_inner_type_related_type> { pub contents_: Container_content_ty<ContainedType>, pub _phantom_0: ::std::marker::PhantomData<::std::cell::UnsafeCell<ContainedType>>, } pub type Container_content_ty<ContainedType> = <ContainedType as __bindgen_has_inner_type_related_type>::related_type; pub type Concrete = Container<InnerType>; #[repr(C)] #[derive(Debug, Default, Copy, Clone)] pub struct LaterContainingType { pub outer_contents: Concrete, } Note the trait constructed to mark types which have an inner type. This trait is then used in the Rust definition of Container_content_ty. Such a trait is emitted only if it's actually used, to avoid too much verbosity. This is useful for types which are parameterized with a traits type. For example Chromium's base::StringPiece which is parameterized by an STL string type (e.g. std::string) and looks up the correct size of character to use by using a parameterized type: typedef typename STRING_TYPE::value_type value_type; (Of course, std::string and other string types have other reasons why they're difficult to work with from Rust, such as self-referential pointers, but that's another story). This change assumes all types involved derive the same traits: in the above example note that __bindgen_has_inner_type_related_type requires all traits be derived. It would be possible to be more nuanced here and move those trait bounds to the places where the trait is used (e.g. pub type Concrete = ... in the above example).
When dependent qualified types are supported, this runs into a different bug, rust-lang#1913, where we generate an infinitely deep structure. The reduced template here doesn't really make any sense and wouldn't be encountered in the wild quite like this. Previously we generated opaque types for this case, so we didn't run into that bug. We now generate higher-fidelity types and therefore hit the problem. I can't see any way to salvage the fundamentals of this test case without the recursive problem, so I propose to remove it.
64ac52f
to
b53bf1f
Compare
☔ The latest upstream changes (presumably cf6edbd) made this pull request unmergeable. Please resolve the merge conflicts. |
For a situation like this:
previously
bindgen
would have generated an opaque type. It nowcorrectly recognizes that the
contents_
field is of a newTypeKind::DependentQualifiedType
and correctly identifies theright type to use when instantiating
Concrete
.The generated code looks like this:
Note the trait constructed to mark types which have an inner type.
This trait is then used in the Rust definition of
Container_content_ty
.Such a trait is emitted only if it's actually used, to avoid too much
verbosity.
This is useful for types which are parameterized with a traits type.
For example Chromium's
base::StringPiece
which is parameterized byan STL string type (e.g.
std::string
) and looks up the correct sizeof character to use by using a parameterized type:
(Of course,
std::string
and other string types have other reasonswhy they're difficult to work with from Rust, such as self-referential
pointers, but that's another story).
This change assumes all types involved derive the same traits:
in the above example note that
__bindgen_has_inner_type_related_type
requires all traits be derived. It would be possible to be more nuanced
here and move those trait bounds to the places where the trait is used
(e.g.
pub type Concrete = ...
in the above example).I have not read C++ specifications to confirm that "dependent qualified
type" is the correct terminology - this is the reference I used.
If you'd like me to split this into a series of commits for reviewability, that's fine! Let me know.
Fixes #1924.