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.

Sign up for GitHub

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

Jump to bottom

Cannot derive Trace for a Copy object. #87

Open
Razican opened this issue Apr 25, 2020 · 4 comments · May be fixed by #88
Open

Cannot derive Trace for a Copy object. #87

Razican opened this issue Apr 25, 2020 · 4 comments · May be fixed by #88
Labels
enhancement

Comments

@Razican
Copy link
Contributor

Razican commented Apr 25, 2020

It seems that if I try to derive both Copy and Trace in a simple structure, I get this error:

The Copy trait was implemented on a type with a Drop implementation.

Erroneous code example:

#[derive(Copy)]
struct Foo; // error!

impl Drop for Foo {
    fn drop(&mut self) {
    }
}

Explicitly implementing both Drop and Copy trait on a type is currently
disallowed. This feature can make some sense in theory, but the current
implementation is incorrect and can lead to memory unsafety (see
issue #20126), so it has been disabled for now.

But from my understanding, a simple Copy structure such as:

#[derive(Debug, Clone, Copy, Trace)]
struct Sym(usize);

should be easy to implement, and the Trace implementation could be empty, right? Is there a proper way to do this? It's currently blocking doing type-safe string interning in Boa.
@Manishearth
Copy link
Owner

Hmm, @mystor do you have an idea for how to have derive(Trace) work diffrntly here? Perhaps we need a derive(TraceCopy) that asserts that it's Copy.

@Manishearth
Copy link
Owner

But yeah, this should be easy, the reason it doesn't work here is because derive(Trace) shoves in its own Drop implementation since Drop impls on GCd objects are unsound

mystor added a commit that referenced this issue Apr 25, 2020
@mystor
Allow deriving 'Trace' on 'Copy' types
e025bc7 
This change means that the `Finalize::finalize` method is not automatically
called on drop. This might be a reasonable approach to take, and would avoid the
need for a separate derive, but would be a breaking change.

Fixes #87
@mystor mystor linked a pull request Apr 25, 2020 that will close this issue
Draft
@mystor
Copy link
Collaborator

mystor commented Apr 25, 2020

It's actually possible to implement this without requiring a separate derive (see #88), like derive(TraceCopy), but doing so would be a breaking change, due to the derived Drop impl calling Finalize::finalize currently.

The cleanest backwards compatible approach would probably be a derive(TraceCopy) which combines derive(Trace, Copy, Finalize) together, and doesn't generate the Drop impl.

@Razican
Copy link
Contributor Author

Razican commented Apr 26, 2020

From my point of view, even if it's not backwards compatible, the proposed solution is a cleaner approach, and this crate is still not 1.0, so breaking changes are expected.

This was referenced May 2, 2020
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow deriving 'Trace' on 'Copy' types Manishearth/rust-gc
4 participants
@andersk @Razican @mystor @Manishearth