Skip to content
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

Closed

Conversation

adetaylor
Copy link
Contributor

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::DependentQualifiedType 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).

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.

@highfive
Copy link

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

@adetaylor adetaylor marked this pull request as draft January 28, 2021 23:15
@adetaylor
Copy link
Contributor Author

Current state of play: need to fix a few tests which are failing:

  • Already fixed a problem where we didn't create the trait unless it was used
  • issue-544-stylo-creduce-2 is just horrid and involves nearly self-referential template types
  • I'm going to have to do the work to move trait bounds (e.g. Debug, Clone) to consumers of the trait

@adetaylor adetaylor marked this pull request as ready for review February 5, 2021 01:40
@adetaylor
Copy link
Contributor Author

I've fixed all known problems other than issue-544-stylo-creduce-2 which remains just horrid. I can't actually get that to produce valid output in a C++ compiler; unless I'm missing something it appears to be an infinitely deep recursive structure whenever it's instantiated. If that is the only remaining failure then I might ruthlessly add a commit to amend or remove that test...

@adetaylor adetaylor marked this pull request as draft February 5, 2021 02:13
@adetaylor
Copy link
Contributor Author

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:

  • Set up a VM/container with old libclang, or wait for bindgen to upgrade to 10 or 11 :)
  • Get all tests passing on master
  • Ensure the only failure is issue-544-stylo-creduce-2.rs
  • If we can't make that work, at least add code to identify the precise situation that causes failure and instead output an opaque type, like bindgen did prior to this PR.

@emilio
Copy link
Contributor

emilio commented Feb 7, 2021

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?

@emilio
Copy link
Contributor

emilio commented Feb 7, 2021

If you're on a mac, it might be #1948 or such?

@adetaylor
Copy link
Contributor Author

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.

emilio added a commit that referenced this pull request Feb 13, 2021
Should fix the test failures described in #1991 and #1975 on modern Mac.
emilio added a commit that referenced this pull request Feb 13, 2021
Should fix the test failures described in #1991 and #1975 on modern Mac.
emilio added a commit that referenced this pull request Feb 15, 2021
Should fix the test failures described in #1991 and #1975 on modern Mac.
@adetaylor
Copy link
Contributor Author

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 bindgen code.

This (issue-544-stylo-creduce-2.rs) is the only failing test now. It includes this nonsensical template which can never be instantiated because (amongst other reasons) it relies on the existence of int::Associated:

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:

error[E0107]: missing generics for type alias `Foo_FirstAlias`
  --> tests/issue-544-stylo-creduce-2.rs:19:32
   |
19 | pub type Foo_SecondAlias = Foo<Foo_FirstAlias>;
   |                                ^^^^^^^^^^^^^^ expected 1 type argument

There are two problems with this.

  • bindgen should be generating pub type Foo_SecondAlias<T> = Foo<Foo_FirstAlias<T>> rather than pub type Foo_SecondAlias = Foo<Foo_FirstAlias>. This is just a bug. I need to fix this.
  • Even after that's fixed, the resulting code won't compile. This is a fundamental problem: in C++, errors are generated only when this type is instantiated. In Rust, the mere existence of this code causes the compiler to get upset:
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:

  • bindgen is allowed to generate nonsensical Rust code when given nonsensical C++ code. Users would need to work around it with the blocklist.
  • bindgen must detect this specific type of insanity, and generate opaque types instead.

@emilio
Copy link
Contributor

emilio commented Apr 20, 2021

I need to check the patch carefully, but I think that this:

bindgen is allowed to generate nonsensical Rust code when given nonsensical C++ code. Users would need to work around it with the blocklist.

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.

@adetaylor
Copy link
Contributor Author

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.

@adetaylor
Copy link
Contributor Author

@emilio I am finally declaring this is ready for review.

A few things to note:

@adetaylor adetaylor marked this pull request as ready for review April 23, 2021 22:15
@adetaylor adetaylor marked this pull request as draft April 24, 2021 02:03
@adetaylor
Copy link
Contributor Author

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).

@bors-servo
Copy link

☔ 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.
@bors-servo
Copy link

☔ The latest upstream changes (presumably cf6edbd) made this pull request unmergeable. Please resolve the merge conflicts.

@adetaylor
Copy link
Contributor Author

Merging this with master requires deciding whether to zap the fix for #2085. In theory it presumably shouldn't be necessary if this change does what it's supposed to do: but in practice the test case in #2102 gets into an infinite loop without the #2085 fix, so there's still work to do.

@pvdrz pvdrz deleted the branch rust-lang:master November 2, 2023 17:50
@pvdrz pvdrz closed this Nov 2, 2023
LoganBarnett pushed a commit to LoganBarnett/rust-bindgen that referenced this pull request Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Associated types not resolved to concrete type
5 participants