Manishearth / rust-gc

Simple tracing (mark and sweep) garbage collector for Rust
Mozilla Public License 2.0
962 stars 50 forks source link

`unsafe_ignore_trace` not respected on unnamed enum variant #142

Closed kaleidawave closed 2 years ago

kaleidawave commented 2 years ago

Kind of strange issue but the following does not compile as the Trace derive generates code that performs trace on a ignored field:

struct DoesNotImplementTrace;

#[derive(Trace, Finalize)]
pub enum Enum1 {
    A(#[unsafe_ignore_trace] DoesNotImplementTrace, bool),
    B(#[unsafe_ignore_trace] DoesNotImplementTrace, bool),
}

When building, I run into the following error:

error[E0277]: the trait bound `DoesNotImplementTrace: Trace` is not satisfied
 --> testing\src\main.rs:6:10
  |
6 | #[derive(Trace, Finalize)]
  |          ^^^^^
  |          |
  |          expected an implementor of trait `Trace`
  |          required by this bound in `_DERIVE_gc_Trace_FOR_Enum1::<impl Trace for Enum1>::root::mark`
  |
  = note: this error originates in the derive macro `Trace` (in Nightly builds, run with -Z macro-backtrace for more info)

Removing the boolean item from the enum variable it works...?

May be a issue with synstructure, or maybe something wrong with the following:

https://github.com/Manishearth/rust-gc/blob/939be1dcc94dc02d5e2a66e55dd546a8155c3ed1/gc_derive/src/lib.rs#L7-L12

Manishearth commented 2 years ago

Yeah synstructure generates bounds, so we should probably have it opt out of that particular bound.

Manishearth commented 2 years ago

https://github.com/Manishearth/rust-gc/blob/939be1dcc94dc02d5e2a66e55dd546a8155c3ed1/gc_derive/src/lib.rs#L15

kaleidawave commented 2 years ago

Hmm interesting. I am not familiar that familiar with synstructure and what and why s.add_bounds(AddBounds::Fields); does? Commenting it out and it still generates a impl which walks and trys to mark the ignored field ...?

use gc::Gc;
use gc_derive::{Trace, Finalize};
pub struct DoesNotImplementTrace;
pub enum Enum1 {
    A(#[unsafe_ignore_trace] DoesNotImplementTrace, bool),
    B(#[unsafe_ignore_trace] DoesNotImplementTrace, bool),
}
#[allow(non_upper_case_globals)]
#[doc(hidden)]
const _DERIVE_gc_Trace_FOR_Enum1: () = {
    unsafe impl ::gc::Trace for Enum1 {
        #[inline]
        unsafe fn trace(&self) {
            #[allow(dead_code)]
            #[inline]
            unsafe fn mark<T: ::gc::Trace + ?Sized>(it: &T) {
                ::gc::Trace::trace(it);
            }
            match *self {
                Enum1::A(ref __binding_1, ..) => mark(__binding_1),
                Enum1::B(ref __binding_1, ..) => mark(__binding_1),
            }
        }
        #[inline]
        unsafe fn root(&self) {
            #[allow(dead_code)]
            #[inline]
            unsafe fn mark<T: ::gc::Trace + ?Sized>(it: &T) {
                ::gc::Trace::root(it);
            }
            match *self {
                Enum1::A(ref __binding_1, ..) => mark(__binding_1),
                Enum1::B(ref __binding_1, ..) => mark(__binding_1),
            }
        }
        #[inline]
        unsafe fn unroot(&self) {
            #[allow(dead_code)]
            #[inline]
            unsafe fn mark<T: ::gc::Trace + ?Sized>(it: &T) {
                ::gc::Trace::unroot(it);
            }
            match *self {
                Enum1::A(ref __binding_1, ..) => mark(__binding_1),
                Enum1::B(ref __binding_1, ..) => mark(__binding_1),
            }
        }
        #[inline]
        fn finalize_glue(&self) {
            ::gc::Finalize::finalize(self);
            #[allow(dead_code)]
            #[inline]
            fn mark<T: ::gc::Trace + ?Sized>(it: &T) {
                ::gc::Trace::finalize_glue(it);
            }
            match *self {
                Enum1::A(ref __binding_1, ..) => mark(__binding_1),
                Enum1::B(ref __binding_1, ..) => mark(__binding_1),
            }
        }
    }
};
Manishearth commented 2 years ago

.add_bounds() controls what kinds of where bounds are added to the impl.

But come to think of it, the s.filter() should suffice here. Unsure what's going on. cc @mystor who knows synstructure better

Manishearth commented 2 years ago

Interestingly it depends on the order of parameters

Manishearth commented 2 years ago

https://github.com/mystor/synstructure/issues/48

Manishearth commented 2 years ago

Figured out how to fix it in synstructure: https://github.com/mystor/synstructure/pull/49

kaleidawave commented 2 years ago

Awesome, thanks for taking look and working on the fix!

kaleidawave commented 2 years ago

Have rearranged the order of my structures in the meantime, looks like the fix is in synstructure v0.12.6 now (https://github.com/mystor/synstructure/pull/49#issuecomment-939375075). Could we get a release with updated synstructure..?

Manishearth commented 2 years ago

I don't think this crate needs a new version; update the synstructure being pulled in on your end to get the fix -- our version spec is compatible

kaleidawave commented 2 years ago

Wha that's cool it works for procedural macros! one cargo update and its fixed, thanks!