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

Stack overflow in Collect::needs_trace for enum with recursion in first variant #120

Open
lgoeldner opened this issue Dec 21, 2024 · 1 comment

Comments

@lgoeldner
Copy link

use std::marker::PhantomData;

use gc_arena::{rootless_arena, Collect, Gc};

#[derive(Collect)]
#[collect(no_drop)]
enum A<'gc> {
    V(Vec<A<'gc>>),
    Bar(PhantomData<&'gc ()>),
}

fn main() {
    rootless_arena(|mc| drop(Gc::new(mc, A::Bar(PhantomData))));
}

This causes a stack overflow for me. It seems to have something to do with the impl Collect for Collections, that seems to trigger an infinite loop. It works with every Collection that has indirection and the Collect::needs_trace is T::needs_trace().

@kyren
Copy link
Owner

kyren commented Dec 23, 2024

Yep, this definitely sounds like a bug in the released version of gc-arena. This can be worked around by manually implementing Collect for your type.

In the master branch right now, the bug still sort of exists but it is much less harmful because it fails at compile time, since Collect::NEEDS_TRACE is now an associated constant (it fails for me with something like cycle detected when elaborating drops for ...).

In the master branch (and also possibly the released version) this can also be worked around for types that actually contain Gc pointers by changing the ordering of fields / variants such that NEEDS_TRACE is set to true || <some other expression>, short-cutting the failure. If the type doesn't actually contain any Gc pointers then this will never work, but in that case usually you can get around the problem entirely with #[collect(require_static)] (not in your example because it is non-'static, but I assume this is artificial and only to demonstrate the problem).

What we could do to cover every case without requiring the user to re-order fields or variants is allow you to set #[collect(always_needs_trace)] which could replace the generated expression for NEEDS_TRACE with just true, which is always safe to do (but may be less efficient). This is still confusing and not entirely ideal, but I can't think of a better solution right now.

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

No branches or pull requests

2 participants