Add `#[empty_trace]` attribute for `Trace` derive macro #185

s9ferech · 2025-04-01T18:27:44Z

As suggested in issue #109, I have added a marker trait, EmptyTrace, for types that don't require tracing, a derive macro for this trait and an attribute ,#[empty_trace], for the Trace derive macro that allows users to safely skip fields which implement EmptyTrace. All of this is demonstrated in gc/tests/empty_trace.rs. I believe that something like this would be very nice to have, especially since the Trace implementation for Rc does not exist anymore.

Would this implementation be safe? I am not familiar with the exact safety assumptions that the library makes about Trace implementations. Or would there be any other issues with this approach?

Manishearth

This should work. The safety requirement is that all fields must implement EmptyTrace.

I'd actually go as far as generating calls to an empty function assert_empty_trace<T: EmptyTrace>(&T) in these bodies so that even if we change how the bounds get generated it will get verified.

s9ferech · 2025-04-02T18:04:45Z

Do you mean instead of generating the where predicates in both, the Trace and EmptyTrace macros? I believe that would produce different behavior. Currently, the following code would be accepted.

#[derive(Trace, Finalize)]
struct Foo<T>(T);

fn test() {
    Gc::new(Foo(0));
    
    // This would fail:
    // Gc::new(Foo(Rc::new(0)));
}

This Foo can satisfy Trace conditionally, depending on the parameter, and I think that is a good thing. It seems to be what built-in derives like Clone do. If the Trace macro omitted the where predicates for EmptyTrace and replaced them with such assertions, as I understand it, the generated code would fail to compile in cases like this one. But I am not sure if I really understood you correctly.

And what would be the precise safety requirements for custom implementations of EmptyTrace? The README and the documentation of Trace speak about "contained Gcs". Are those precisely the Gc instances that would be dropped with a value of the type? And implementing EmptyTrace for a type T is safe if and only if dropping a value of T will never drop a Gc?

Manishearth · 2025-04-02T19:00:30Z

No, you've misunderstood the proposition: it's not "instead of" , it's "in addition to", and it doesn't involve asserting things about type parameters, it's asserting things about fields.

So

#[derive(EmptyTrace)]
struct Foo<T, U> {
  x: u8,
  y: SomeGeneric<T>,
  z: U
}

would produce an assertion for u8, SomeGeneric<T>, and U.

Are those precisely the Gc instances that would be dropped with a value of the type? And implementing EmptyTrace for a type T is safe if and only if dropping a value of T will never drop a Gc?

Yes. A Gc or a GcCell.

andersk · 2025-04-02T19:32:16Z

gc_derive/src/lib.rs

+    // Add where bounds for all bindings manually because synstructure only adds them if they depend on one of the parameters.
+    let mut where_predicates = Vec::new();
+    for v in s.variants() {
+        for bi in v.bindings() {
+            let ty = &bi.ast().ty;
+            let span = ty.span();
+            where_predicates.push(parse_quote_spanned! { span=> #ty: ::gc::EmptyTrace });
+        }
+    }
+    for p in where_predicates {
+        s.add_where_predicate(p);
+    }
+    s.add_bounds(AddBounds::None);


Isn’t this what AddBounds::Fields is for?

Suggested change

// Add where bounds for all bindings manually because synstructure only adds them if they depend on one of the parameters.

let mut where_predicates = Vec::new();

for v in s.variants() {

for bi in v.bindings() {

let ty = &bi.ast().ty;

let span = ty.span();

where_predicates.push(parse_quote_spanned! { span=> #ty: ::gc::EmptyTrace });

}

}

for p in where_predicates {

s.add_where_predicate(p);

}

s.add_bounds(AddBounds::None);

s.add_bounds(AddBounds::Fields);

Also, yeah, this. I found this strange but hadn't yet dug deeper.

It's what I tried to explain in the comment at the top. If you use AddBounds::Fields then it skips any field that does not mention a parameter. But we need those to implement EmptyTrace too.

Ah, yeah, I suggest generating assertions for them; the typical way that custom derives work is that the actual method code enforces that each field implements the trait. This trait doesn't have a method (we could add a hidden defaulted one).

Otherwise the field trait bounds escape into the docs.

andersk · 2025-04-02T19:48:35Z

gc/src/empty_trace.rs

+/// TODO: Safety conditions
+pub unsafe trait EmptyTrace {}
+
+unsafe impl EmptyTrace for String {}


We should unify this with simple_empty_finalize_trace! in trace.rs and get implementations for almost everything for which we provide Finalize and Trace.

Sort of a side note, but Box<String>/Rc<String> are unusual because of the double indirection; typically Box<str>/Rc<str>/some thin pointer equivalent should be preferred.

Manishearth · 2025-04-02T19:48:46Z

Also, to clarify, my request for generating assertions is more of a defense-in-depth mechanism. Adding the right where bounds works, this just provides an extra check for the same thing done differently. For complex custom derives I've found it useful when you start tweaking the actual bounds produced.

s9ferech · 2025-04-02T19:53:39Z

The the assertions seem redundant to me. I see that they would make sense if the trait contained any methods that actually handled values of the type and required that condition, but, in this situation, they would just literally repeat the types from the where clause. It would also some significant complexity since you can't put them inside the trait implementation because there are no methods to implement.

andersk

If we have an EmptyTrace trait, the #[empty_trace] attribute should be unnecessary. We can just provide Trace implementations:

impl<T: EmptyTrace + ?Sized> Finalize for Rc<T> {}
unsafe impl<T: EmptyTrace + ?Sized> EmptyTrace for Rc<T> {}
unsafe impl<T: EmptyTrace + ?Sized> Trace for Rc<T> {
    unsafe_empty_trace!();
}

and then users can put Rc in their structs without extra annotation.

andersk · 2025-04-02T19:57:05Z

gc/src/empty_trace.rs

+unsafe impl<T: EmptyTrace> EmptyTrace for Rc<T> {}
+
+unsafe impl<T: EmptyTrace> EmptyTrace for Box<T> {}


Should allow T: ?Sized.

Manishearth · 2025-04-02T19:59:43Z

The the assertions seem redundant to me. I see that they would make sense if the trait contained any methods that actually handled values of the type and required that condition, but, in this situation, they would just literally repeat the types from the where clause.

The ideal is to not have extraneous where bounds in the where clause; the impl gets wordy otherwise.

It would also some significant complexity since you can't put them inside the trait implementation because there are no methods to implement.

It's mild complexity: you add a doc(hidden) defaulted method, or you generate an additional hidden function.

I don't think this is strictly necessary, I just have found for more complex impls that this type of assertion has saved us from problems more often nthan not.

s9ferech · 2025-04-02T20:07:49Z

Keeping trivial where bounds out of the docs sounds like a good argument. I will see if it's easy to do. Instead of using a hidden method, one could also use a separate impl with all the same bounds to hold that method, or even just a function, right?

The only downside for the user would be that the error messages are slightly less consistent and helpful. I do find it a bit strange when Trace gives you four copies of the same error and refers to some local function.

s9ferech · 2025-04-02T20:15:29Z

The Trace implementation for Rc sounds like a good idea. Would implementations like that solve all cases or are there still scenarios where you might want #[empty_trace]?

Manishearth · 2025-04-02T20:16:30Z

@s9ferech yeah, you can just make a function, potentially stuffed inside a const _: () = {...} block to insulate it.

I'm still not sure if we should do this but I mostly wanted to offer it. We can at least start with just adding field bounds.

andersk · 2025-04-02T20:59:52Z

Looking back at #109 (comment), it seems the argument was that if we provide impl<T: EmptyTrace + ?Sized> Trace for Rc<T>, it would prevent the user from providing impl Trace for Rc<UserType> without specialization.

However, given #134, I don’t think it’s possible to have a correct impl Trace for Rc<UserType> for any UserType with a nonempty impl Trace. So I still say everything providing EmptyTrace can also provide Trace, and there’s no need for #[empty_trace].

s9ferech · 2025-04-03T21:53:09Z

I have just added some trait implementations and was wondering if there is a reason why Trace is not implemented for Ref, Weak or pointer types. I would use this opportunity to add them too if there isn't.

andersk · 2025-04-03T22:40:31Z

gc/src/trace.rs

+pub unsafe trait EmptyTrace {}
+
+// TODO: The README needs to be updated to explain when `Rc` and the other types here can be managed by GC.
+impl<T: EmptyTrace + ?Sized> Finalize for Rc<T> {}


Suggested change

impl<T: EmptyTrace + ?Sized> Finalize for Rc<T> {}

impl<T: ?Sized> Finalize for Rc<T> {}

since Finalize itself doesn’t recursively finalize inner members (we have Trace::finalize_glue for that).

Same for the rest.

andersk · 2025-04-03T22:51:11Z

gc/src/trace.rs

+impl<T: EmptyTrace + ?Sized> Finalize for RefCell<T> {}
+unsafe impl<T: EmptyTrace + ?Sized> Trace for RefCell<T> {
+    unsafe_empty_trace!();
+}
+unsafe impl<T: EmptyTrace + ?Sized> EmptyTrace for RefCell<T> {}
+
+impl<T: EmptyTrace + ?Sized> Finalize for Cell<T> {}
+unsafe impl<T: EmptyTrace + ?Sized> Trace for Cell<T> {
+    unsafe_empty_trace!();
+}
+unsafe impl<T: EmptyTrace + ?Sized> EmptyTrace for Cell<T> {}
+
+impl<T: EmptyTrace> Finalize for OnceCell<T> {}
+unsafe impl<T: EmptyTrace> Trace for OnceCell<T> {
    unsafe_empty_trace!();
 }
+unsafe impl<T: EmptyTrace> EmptyTrace for OnceCell<T> {}
+
+impl<T: EmptyTrace + ?Sized> Finalize for Mutex<T> {}
+unsafe impl<T: EmptyTrace + ?Sized> Trace for Mutex<T> {
+    unsafe_empty_trace!();
+}
+unsafe impl<T: EmptyTrace + ?Sized> EmptyTrace for Mutex<T> {}
+
+impl<T: EmptyTrace + ?Sized> Finalize for RwLock<T> {}
+unsafe impl<T: EmptyTrace + ?Sized> Trace for RwLock<T> {
+    unsafe_empty_trace!();
+}
+unsafe impl<T: EmptyTrace + ?Sized> EmptyTrace for RwLock<T> {}
+
+impl<T: EmptyTrace> Finalize for OnceLock<T> {}
+unsafe impl<T: EmptyTrace> Trace for OnceLock<T> {
+    unsafe_empty_trace!();
+}
+unsafe impl<T: EmptyTrace> EmptyTrace for OnceLock<T> {}


None of these other types have the sharing problem that Rc<T> and Arc<T> have. It should be possible to give them full Trace implementations with only a T: Trace constraint.

(There are some subtleties here—for example, we need to trace a Mutex with try_lock instead of lock in case safe code has leaked a MutexGuard. But I think they’re all solvable.)

Is the internal mutability not a problem too? I thought that was the reason why GcCell is required.

andersk · 2025-04-03T22:52:06Z

gc/src/trace.rs

@@ -286,6 +353,7 @@ unsafe impl<T: Trace, E: Trace> Trace for Result<T, E> {
        }
    });
 }
+unsafe impl<T: EmptyTrace, E: Trace> EmptyTrace for Result<T, E> {}


Suggested change

unsafe impl<T: EmptyTrace, E: Trace> EmptyTrace for Result<T, E> {}

unsafe impl<T: EmptyTrace, E: EmptyTrace> EmptyTrace for Result<T, E> {}

andersk · 2025-04-03T22:53:00Z

gc/src/trace.rs

@@ -325,6 +396,7 @@ unsafe impl<K: Trace, V: Trace, S: Trace> Trace for HashMap<K, V, S> {
        }
    });
 }
+unsafe impl<K: EmptyTrace, V: EmptyTrace> EmptyTrace for HashMap<K, V> {}


Suggested change

unsafe impl<K: EmptyTrace, V: EmptyTrace> EmptyTrace for HashMap<K, V> {}

unsafe impl<K: EmptyTrace, V: EmptyTrace, S: EmptyTrace> EmptyTrace for HashMap<K, V, S> {}

andersk · 2025-04-03T22:53:42Z

gc/src/trace.rs


 impl<T: ?Sized> Finalize for PhantomData<T> {}
 unsafe impl<T: ?Sized> Trace for PhantomData<T> {
    unsafe_empty_trace!();
 }
+unsafe impl<T: EmptyTrace> EmptyTrace for PhantomData<T> {}


Suggested change

unsafe impl<T: EmptyTrace> EmptyTrace for PhantomData<T> {}

unsafe impl<T> EmptyTrace for PhantomData<T> {}

By definition, PhantomData<T> does not contain a T.

s9ferech · 2025-04-04T11:36:33Z

I just realized that the derive macro for EmptyTrace would probably also need to implement Drop to be safe. Doesn't it?

Add #[empty_trace] attribute for Trace derive macro

fefec65

Manishearth reviewed Apr 1, 2025

View reviewed changes

andersk reviewed Apr 2, 2025

View reviewed changes

Support self-referential types in EmptyTrace derive macro

36c57cc

andersk reviewed Apr 2, 2025

View reviewed changes

s9ferech added 2 commits April 3, 2025 23:43

Add basic trait implementations involving EmptyTrace

915446c

Remove #[empty_trace] attribute for Trace derive macro

d46f1ff

andersk reviewed Apr 3, 2025

View reviewed changes

		unsafe impl<T: EmptyTrace> EmptyTrace for Rc<T> {}

		unsafe impl<T: EmptyTrace> EmptyTrace for Box<T> {}

	impl<T: EmptyTrace + ?Sized> Finalize for Rc<T> {}
	impl<T: ?Sized> Finalize for Rc<T> {}

	unsafe impl<T: EmptyTrace, E: Trace> EmptyTrace for Result<T, E> {}
	unsafe impl<T: EmptyTrace, E: EmptyTrace> EmptyTrace for Result<T, E> {}

	unsafe impl<K: EmptyTrace, V: EmptyTrace> EmptyTrace for HashMap<K, V> {}
	unsafe impl<K: EmptyTrace, V: EmptyTrace, S: EmptyTrace> EmptyTrace for HashMap<K, V, S> {}

	unsafe impl<T: EmptyTrace> EmptyTrace for PhantomData<T> {}
	unsafe impl<T> EmptyTrace for PhantomData<T> {}

Add #[empty_trace] attribute for Trace derive macro #185

Are you sure you want to change the base?

Add #[empty_trace] attribute for Trace derive macro #185

Uh oh!

Conversation

s9ferech commented Apr 1, 2025

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

s9ferech commented Apr 2, 2025

Uh oh!

Manishearth commented Apr 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andersk Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Apr 2, 2025

Uh oh!

s9ferech commented Apr 2, 2025

Uh oh!

andersk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Apr 2, 2025

Uh oh!

s9ferech commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s9ferech commented Apr 2, 2025

Uh oh!

Manishearth commented Apr 2, 2025

Uh oh!

andersk commented Apr 2, 2025

Uh oh!

s9ferech commented Apr 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s9ferech commented Apr 4, 2025

Uh oh!

Uh oh!

Add `#[empty_trace]` attribute for `Trace` derive macro #185

Add `#[empty_trace]` attribute for `Trace` derive macro #185

andersk Apr 2, 2025 •

edited

Loading

s9ferech commented Apr 2, 2025 •

edited

Loading