Skip to content

Conversation

thestinger
Copy link
Contributor

This provides a default implementation of Clone for all copyable types, which allows generic code to start using Clone instead of Copy.

I don't think there will be a need to expose Copy outside of this module once stuff is migrated.

@thestinger
Copy link
Contributor Author

The test_str test case currently causes a warning - any idea how to fix this?

warning: instantiating copy type parameter with a not implicitly copyable type

@catamorphism
Copy link
Contributor

The warning for that is strange -- I'll look at this more closely.

@catamorphism
Copy link
Contributor

Oh, this is interesting. I think what's going on is that your default implementation of Clone applies to both &str and ~str. For no particular reason, the typechecker is picking the &str impl first, because ~str can be autosliced to &str. That causes the warning, since the autoslicing requires an implicit copy.

I haven't proved this to myself yet, but the error does go away if I replace your generic implementation with specific ones for int and ~str.

@catamorphism
Copy link
Contributor

Actually, no, my previous comment is wrong. ~str isn't implicitly copyable, and only implicitly copyable types can instantiate T right now when there's a T: Copy. I'll change the test to be for @str instead.

@thestinger
Copy link
Contributor Author

@catamorphism: hmm, would be nice to be able to have this working on non-implicitly-copyable types though

@catamorphism
Copy link
Contributor

Sorry, I can't accept this as-is -- it breaks tests in std::sync (among other places) because there are existing impls of Clone (if you run make check you'll see where it goes wrong -- in rustdoc first, but that's easy to fix). That may be fixable, but it seems like this default impl has the potential to lead to overlapping instance city. I think this needs a little more thought.

RalfJung pushed a commit to RalfJung/rust that referenced this pull request Sep 3, 2025
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.

2 participants