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

Associated types not resolved to concrete type #1924

Open
adetaylor opened this issue Nov 19, 2020 · 3 comments
Open

Associated types not resolved to concrete type #1924

adetaylor opened this issue Nov 19, 2020 · 3 comments

Comments

@adetaylor
Copy link
Contributor

Input C/C++ Header

struct InnerType {
    typedef int related_type;
};

template <typename ContainedType> class Container {
public:
    typedef typename ContainedType::related_type contents;
    contents contents_;
};

typedef Container<InnerType> Concrete;

struct LaterContainingType {
    Concrete contents;
};

Bindgen Invocation

    let bindings = bindgen::Builder::default()
        .header("wrapper.hpp")
        .clang_args(&["-x", "c++", "-std=c++14"])
        .layout_tests(false)
        .whitelist_type("LaterContainingType")
        .parse_callbacks(Box::new(bindgen::CargoCallbacks))
        .generate()
        .expect("Unable to generate bindings");

Rust code

#![allow(non_upper_case_globals)]
#![allow(non_camel_case_types)]
#![allow(non_snake_case)]

include!(concat!(env!("OUT_DIR"), "/bindings.rs"));

fn main() {
    Concrete { contents_: 32 };
}

Actual Results

$ cargo build
   Compiling bindgen-test-case v0.1.0
error[E0308]: mismatched types
 --> src/main.rs:8:27
  |
8 |     Concrete { contents_: 32 };
  |                           ^^ expected array `[u8; 0]`, found integer

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
error: could not compile `bindgen-test-case`

Expected Results

I'd expect the type of contents_ to be a c_int.

Other notes

Perhaps this counts as the "traits templates" situation listed under the recorded C++ limitations but, as it doesn't involve any nasty SFINAE magic I am hoping this may be possible.

Thanks for bindgen, it's amazing! Apologies if this is a duplicate.

@adetaylor
Copy link
Contributor Author

adetaylor commented Dec 10, 2020

I am looking into solving this.

Here is my plan. I would appreciate any guidance: since this is my first potential bindgen contribution, I feel this is quite ambitious and I could easily spend lots of time down blind alleys.

TL;DR: we generate a new trait for each associated type.

  1. Type gains a new bool (alongside is_const) called is_associated_type.
  2. In ClangItemParser for Item, in the function from_ty_with_id ("one of the trickiest methods you'll find", terrific) there is a check for ty.is_associated_type() where we currently generate an opaque type. Instead, we will call build_ty_wrapper with a new parameter that indicates that this is an associated type. This is stored in the bool.
  3. Instead of pushing these into the inner_types member of the comp, we will add these to a new list, associated_types.
  4. In impl CodeGenerator for CompInfo, codegen func, we already iterate through inner types. We will also iterate through associated_types, but instead of simply generating those types we will generate:
pub trait __bindgen_has_associated_type_ASSOCIATED_TYPE_NAME {
  type ASSOCIATED_TYPE_NAME;
};

(generated only once per ASSOCIATED_TYPE_NAME) and

impl __bindgen_has_associated_type_ASSOCIATED_TYPE_NAME for TYPE {
  type ASSOCIATED_TYPE_NAME = WHATEVER_THE_TYPEDEF_POINTED_TO;
}
  1. Where we generate any type that depends upon resolving such an associated type based on its generic parameters, we will need to constrain those generic parameters based on these generated traits. I have not yet figured out in the code where to do this.
  2. In BindgenContext::resolve_typerefs, adjust the resolution so it ends up outputting something like <TYPE as __bindgen_has_associated_type_ASSOCIATED_TYPE_NAME>::ASSOCIATED_TYPE_NAME
  3. Sometimes we'll require types to implement Debug + Copy + Clone + __bindgen_has_associated_type_ASSOCIATED_TYPE_NAME which is unpleasant but do-able.

I anticipate Much Doom in attempting to implement this plan. Any pointers for how to avoid the most egregious pits of lava would be much appreciated.

@adetaylor
Copy link
Contributor Author

OK, here's my progress so far (this is a branch 90% full of logging hacks, obviously much rebasing and cleaning would occur before PRing it).
master...adetaylor:associated-type-wip

A few differences from the above plan:

  • Steps 1-2 seemed achievable using a new TypeKind::TypeParamAssociatedType(String) rather than an extra bool, which is nicer.
  • In step 3 using the existing inner_type vec seemed fine.
  • Step 4: no changes
  • Step 5: this is what I'm working on right now. With this test code:
template <typename ContainedType> class Container {
public:
    typedef typename ContainedType::related_type content_ty;
...
}

the typedef becomes a plain old Alias (clang doesn't feed us CXCursor_TypeAliasTemplateDecl which we might have expected). That Alias currently does not have any way to squeeze out an EdgeKind::TemplateArgument and thus the UsedTemplateParameters analysis does not think the typedef refers to ContainedType. As such we generate:

pub type Container_content_ty = ContainedType::related_type

instead of

pub type Container_content_ty<ContainedType> = ContainedType::related_type

Once I can make those edges appear properly, there's further work even on this specific line because it actually needs to be

pub type Container_content_ty<ContainedType> = <ContainedType as __bindgen_has_inner_type_related_type>::related_type;

but that's for later.

@adetaylor
Copy link
Contributor Author

OK, I came back to this after a while and found I couldn't remember much :) Notes-to-self:

How to run & test

cargo build && ./tests/test-one.sh inner_type_simple

What works

I have managed to make a suitable edge exist to the TypeParameterAssociatedType. For the following header:

struct InnerType {
    typedef int related_type;
    long int foo;
};

template <typename ContainedType> class Container {
public:
    typename ContainedType::related_type contents_;
};

typedef Container<InnerType> Concrete;

we now generate the following bindings (test code removed for clarity):

#[repr(C)]
#[derive(Debug, Default, Copy, Clone)]
pub struct InnerType {
    pub foo: ::std::os::raw::c_long,
}
pub type InnerType_related_type = ::std::os::raw::c_int;
pub trait __bindgen_has_inner_type_related_type {
    type related_type: std::fmt::Debug + Copy + Clone + Default;
}
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_: ContainedType::related_type,
    pub _phantom_0: ::std::marker::PhantomData<::std::cell::UnsafeCell<ContainedType>>,
}
pub type Concrete = Container;

What doesn't work

This is all correct except:

  • The extra trait constraints which are being applied to __bindgen_has_inner_type_related_type::related_type should probably instead be applied to something later, per the example in tests/expectations/tests/inner_type.rs in my branch-o-hacks.
  • The last line is wrong. It should be pub type Concrete = Container<InnerType>. This is for just the reasons given in the previous comment - clang is giving us an Alias not a TemplateAlias.

Next steps

I'm hoping to:

  • Find some other way of referring to Container<InnerType> which doesn't run into the type alias bug above. e.g. as a function parameter or variable.
  • Get bindgen to generate working code for such a case.
  • Move the extra trait constraints as mentioned above
  • Tidy up all my hacks, remove logging, make a series of orthogonal commits which can be put into a PR.
  • PR!
  • Then, figure out what (if anything) we can do about the Alias scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant