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

Filter out lookup_can_derive_copy for template type #2764

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ileixe
Copy link

@ileixe ileixe commented Feb 20, 2024

Template type in tagged union should not be copiable field as it requires ManuallyDrop. Filter out copy derive when the variable is resolved to template type.

Fix: #2157

Now the input.hpp in the issue will generate non rust union or correct ManuallyDrop for generic type.

rust-bindgen$ target/debug/bindgen input.hpp
/* automatically generated by rust-bindgen 0.69.4 */

#[repr(C)]
pub struct __BindgenUnionField<T>(::std::marker::PhantomData<T>);
impl<T> __BindgenUnionField<T> {
    #[inline]
    pub const fn new() -> Self {
        __BindgenUnionField(::std::marker::PhantomData)
    }
    #[inline]
    pub unsafe fn as_ref(&self) -> &T {
        ::std::mem::transmute(self)
    }
    #[inline]
    pub unsafe fn as_mut(&mut self) -> &mut T {
        ::std::mem::transmute(self)
    }
}
impl<T> ::std::default::Default for __BindgenUnionField<T> {
    #[inline]
    fn default() -> Self {
        Self::new()
    }
}
impl<T> ::std::clone::Clone for __BindgenUnionField<T> {
    #[inline]
    fn clone(&self) -> Self {
        *self
    }
}
impl<T> ::std::marker::Copy for __BindgenUnionField<T> {}
impl<T> ::std::fmt::Debug for __BindgenUnionField<T> {
    fn fmt(&self, fmt: &mut ::std::fmt::Formatter<'_>) -> ::std::fmt::Result {
        fmt.write_str("__BindgenUnionField")
    }
}
impl<T> ::std::hash::Hash for __BindgenUnionField<T> {
    fn hash<H: ::std::hash::Hasher>(&self, _state: &mut H) {}
}
impl<T> ::std::cmp::PartialEq for __BindgenUnionField<T> {
    fn eq(&self, _other: &__BindgenUnionField<T>) -> bool {
        true
    }
}
impl<T> ::std::cmp::Eq for __BindgenUnionField<T> {}
#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct _Vector_base {
    pub _address: u8,
}
#[repr(C)]
pub struct _Vector_base__Storage<_Tp> {
    pub _M_byte: __BindgenUnionField<::std::os::raw::c_uchar>,
    pub _M_val: __BindgenUnionField<_Tp>,
    pub bindgen_union_field: u8,
    pub _phantom_0: ::std::marker::PhantomData<::std::cell::UnsafeCell<_Tp>>,
}
rust-bindgen$ target/debug/bindgen input.hpp --default-non-copy-union-style manually_drop
/* automatically generated by rust-bindgen 0.69.4 */

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub struct _Vector_base {
    pub _address: u8,
}
#[repr(C)]
pub union _Vector_base__Storage<_Tp> {
    pub _M_byte: ::std::mem::ManuallyDrop<::std::os::raw::c_uchar>,
    pub _M_val: ::std::mem::ManuallyDrop<_Tp>,
    pub _phantom_0: ::std::marker::PhantomData<::std::cell::UnsafeCell<_Tp>>,
}

Template type in tagged union should not be copy fields as it
requires ManuallyDrop. Filter out copy derive when the variable is
resolved to template type.

Fix: 2157
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2024

Error: The feature assign is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@@ -2796,6 +2797,25 @@ If you encounter an error missing from this list, please file an issue or a PR!"
.contains(&id.into())
}

/// Look up whether the item with `id` is type parameter after resolve.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a test, but it seems to me this should happen in the can_derive_copy analysis.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add test to verify regression.

As for can_derive_copy analysis, it makes sense to me as this can be analyzed before actual codegen. But it's not clear to me how. I skimmed over CannotDerive.constrain_type() and it's a bit hard where the code required.

image

This is graph from the problematic code. What I was trying is to make Item(2)(_Tp) CanDerive::No. From my undesrtanding, the basis is TypeParam which Union refers need to be CanDevice::No. But I saw it's bit hard because it's back edge from TypeParam.

Am I correctly understanding basic strategy? If then, how can I make _Tp CanDevice::No?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emilio Gentle reminder.

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 this pull request may close these issues.

Tagged unions inside template fail to build.
3 participants