Revert "Auto merge of #43690 - scalexm:issue-28229, r=nikomatsakis"

[arielb1]

I (and @nikomatsakis) think we needed an FCP for the original PR (#43690). Because it was already merged, let's have the FCP on the revert.

[rust-highfive]

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

[arielb1]

@Fcpbot close

[eddyb]

r? @nikomatsakis

[nikomatsakis]

@rfcbot fcp close

OK, so as I wrote on #43690, I kind of screwed up the process here. In particular, I accepted the PR, but @eddyb had some lingering objections. In particular, the PR has the compiler "auto-generate" clone impls. Some of these clone impls are sort of required -- in particular, those for Copy types. So, e.g., the compiler would auto-generate an impl (now) for [u8; 256] (which was not previously included) or fn(&u32) -> &u32 (same). This is required to avoid ICEs, since those types are Copy (which implies they must be Clone).

However, the PR also extends support for a few other things. Notably, [T; N] is now Clone so long as T: Clone, whereas this was not true before (the macro-generated impls required that T: Copy and only went up to N <= 32). It appears that there was not a good reason to limit things to T: Copy except for language limitations. (As a side benefit of the PR, for those cases where the array type actually is copy once monomorphized, we will generate an efficient impl that uses memcpy.)

I personally think this is all very desirable. I also do not think the changes are major enough to require a full RFC. But I do think we should use the consensus process to ensure we are all in agreement on this point. Therefore, @arielb1 prepared this PR to revert the change. Since I support the change, I am moving to close this RFC (and hence accept the change).

[rfcbot]

Team member @nikomatsakis has proposed to close this. The next step is review by the rest of the tagged teams:

• @aturon
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

Concerns:

• derive-consistency (Revert "Auto merge of #43690 - scalexm:issue-28229, r=nikomatsakis" #44045 (comment))

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[eddyb]

@rfcbot concern derive-consistency

Currently we have one primary mechanism (the builtin set of #[derive]-able traits) for generating impls without involving user-defined syntactic expansion (procedural derives / macros), and it's also syntactical. This means it can't access semantic type information, for example, to make decisions based on type properties (without relying on specialization, at least).

A more annoying consequence of syntactic deriving is it can't generate the right bounds (see #26925 - although it might not the best issue to explain the problem, let me know if there's another):

• if it places bounds on every field type, you can end up with mutually recursive bounds on the generated impls (from mutually data-recursive type definitions), which according to @nikomatsakis (correct me if I'm wrong) require a coinductive strategy to be soundly resolved - and the introduction of coinduction is posited as being complex/problematic and for now such impls produce trait bound recursion errors
• if it places bounds on type parameters, it may be wrong if the type parameter isn't used directly in the type, e.g. Rc<T>: Clone and &T: Copy always hold, and T::AssociatedType is a wild card

So far in the compiler, impls that are not syntactically available have very thin shims, with the only one that remotely looks like a #[derive] being "drop glue" (dropping all fields, recursively).

But after #43690, we have two ways to derive a Clone impl, the one being added using the same shim system "drop glue" uses nowadays, generating MIR from type information on the fly.
For T: Copy types, the shim is trivial (returning *self) and that by itself wouldn't bother me to have around - after all, we do claim these types are Copy, as @nikomatsakis has mentioned.

But deriving Clone for aggregates in two different ways in the same compiler is something that I would at least require a small technical RFC for. Preferably, we would implement all #[derive(Clone)] in this manner, which I'd be happy to have, if it can side-step the "correct bounds" issue and the generated MIR be properly verified (i.e. type & borrow-checked).

[nikomatsakis]

BTW, I think I missed something. Under #43690, if I'm not mistaken, one can now clone tuples of arbitrary arity (e.g., this compiled in nightly but not stable). Obviously I would want this to work, but it does introduce a kind of "incongruity" -- and, unlike [T; N] being Clone, we have no current plan to resolve it (we would need some sort of "varargs" tuples). Obviously we have "candidate plans". I feel potentially mildly uncomfortable with this.

[nikomatsakis]

@eddyb

But deriving Clone for aggregates in two different ways in the same compiler is something that I would at least require a small technical RFC for. Preferably, we would implement all #[derive(Clone)] in this manner, which I'd be happy to have, if it can side-step the "correct bounds" issue and the generated MIR be properly verified (i.e. type & borrow-checked).

This is an interesting comment -- I don't think of #[derive] as being "in the compiler" at all, even though I guess technically it is. To my mind, it's basically a regular "procedural macro" that we happen to ship by default. (In particular, does it have any sort of privileged access to compiler internals? I guess a bit, to handle the PartialEq and Eq interactions with matching, but that is hopefully temporary.)

In any case, if I can resummarize, you raised two points:

"Perfect deriving" -- in particular, the fact that when you have struct Foo<N>, #[derive(Clone)] will currently add an impl where N: Clone implies Foo<N>: Clone, regardless of the field types of Foo. But in some cases (e.g., struct Foo<N> { data: Rc<N> }) that is stricter than necessary. I agree this is annoying. I also think it's a fairly orthogonal concern to the existing PR. Resolving this seems to require us to gain a better understanding of how to combine co-inductive and inductive impls in a useful way that avoids the myriad sources of unsoundness we've encountered even in the existing auto traits and so forth. It also has basically nothing to do with how the code is generated (i.e., via MIR shim or what).

(I do have this sort of instinctual feeling that there is way to permit more co-inductive reasoning in the trait system, but I don't yet understand well enough how to think about it.)

"Two ways to have deriving." After #43690, for "user-defined" types (structs and enums), the #[derive] plugin generates a syntactic expansion, but for "builtin" types (tuples and arrays), the compiler generates the code. I agree this is something of a first, but it seems like a highly internal concern to me. If anything, the newer approach is better -- since it gets to use specialization-like features for greater efficiency -- so you could imagine us changing what #[derive] generates to use the "built-in" code more often in the future (though personally I'd rather target this through specialization, and move that code back out from the guts on the compiler, if we ever figure out how to make that work).

Put another way, it feels like you are sort of making the case that the change didn't go far enough -- i.e., it converted some clone impls, but not all, to a new system. I don't see why this is a problem. Unless we achieve the specialization goal, we will always have to contend with a mix of user-supplied and builtin clone impls. Whether the user-supplied impls are generated by #[derive] or not doesn't really matter to the rest of the compiler. So I don't see any added complexity from this approach.

[arielb1]

Generating MIR for user-defined types makes me concerned about edge-cases that would be detected in other parts of the compiler (e.g. #[repr(packed)]). OTOH, we basically have the same concern about drop glue (#[repr(packed)] + drop glue is UB today, so that's a sort of a "well-known" issue), where we have to use MIR inlining, so maybe that's a a concern we have to handle anyway.

The derived Clone impl isn't actually more complicated than drop glue, so I'm not much worried about that.

if it can side-step the "correct bounds" issue

I'm actually not sure whether there's a good solution to the "correct bounds" issue, other than making Clone coinductive and using the explicit fields as bounds. I think we might do some sort of "pretend Clone is coinductive, find out the requirements for this to be Clone, and go with them" pass, but that would be quite complicated.

and the generated MIR be properly verified (i.e. type & borrow-checked).

The generated MIR is already type-checked, but there's no free-region checking so dubious Clone impls might be able to do dubious things with lifetimes. That can however be fixed the same way we fixed compare_method. It should be manifestly borrow-correct - all the borrows are tied to the "anon" lifetime on Clone::clone.

Borrow-checking it in a "non-fake" sound way would require doing region inference on MIR, aka "NLL".

[eddyb]

I pretty much agree with everything @arielb1 said, which is why I think this requires an RFC.
#[repr(packed)] should probably solved by implementing as a MIR pass anyway.

[scalexm]

So about generating a Clone shim for aggregates: I did not make that change for the pure sake of fixing the arity limitation existing on stable. I made that change so that aggregate types which implement Copy also implement Clone, hence fixing the ICE described in issue #28229.

A side-effect of that change is that indeed tuples of Clone types are now Clone regardless of the arity (with the macro-generated impls on stable, this is only true up to tuples of 8 elements or something). I call that a "side-effect" because it would not have been possible to have both a built-in Clone impl for Copy tuples of any arity (in order to fix the ICE), and the existing macro-generated impls for Clone tuples: the built-in impl and the macro impls would have conflicted for the same reason that we cannot (currently) fix #28229 with specialization.

But I think this is a "good" side-effect because it indeed fixes an annoying limitation of the Rust language (not being able to write a trait impl for all array and tuple types). However, the fact that it was not the primary goal of PR #43690 makes me think that we should maybe not do the same for e.g. Eq impls and so on, but rather we should wait for a way to express such impls in the language (for example once we have const generics, we could remove the builtin Clone shim for arrays AND have a way to implement Eq and other traits as well).

So the way I see PR #43690 is that it is just a necessary (because of the ICE) workaround until we have a way to express trait impls for arrays and tuples in the language. The fact that is was necessary makes me believe that maybe we don't need an RFC for that change.

Also, I agree with @nikomatsakis and @arielb1 that the bounds problem in auto-derived impls seems orthogonal to PR #43690.

[eddyb]

For Copy types you can return *self without doing aggregate recursion. Do you mean that is not enough, caused issues or something else?

[scalexm]

@eddyb I mean that the trait selection algorithm still needs to express something like:

// This impl can be seen as a "variadic" impl generated by the compiler
impl<T> Clone for (T1, ...) where T1: Clone, ...

if you replace the Clone bound by a Copy bound, so that we indeed don't need to do aggregate recursion in the MIR shim:

impl<T> Clone for (T1, ...) where T1: Copy, ...

then this would conflict with the macro-generated impls that we have on stable:

impl<A, B> Clone for (A, B) where A: Clone, B: Clone { ... }
impl<A, B, C> Clone for (A, B, C) where A: Clone, B: Clone, C: Clone { ... }
// ... and so on up to tuples of 8 elements

because the compiler would not know which impl to select for e.g. (i32, i32) (the builtin one or the macro-generated one?)

[eddyb]

Ahhh this is the first time I ever see this said out loud by anyone. I doubt @nikomatsakis was even aware. The solution is obvious: treat the compiler implementation as a specialization or do not consider it if there are any library impls matching it.

[scalexm]

@eddyb For some reason I thought that specialization would not work in that case, but you're right it seems like it does, just changing to default fn clone(&self) in the macro-generated impls would work and this is good news. So now that the PR is merged, I guess the question is more like: do we want to artificially put back the arity restriction? I do not see any problem with that if you'd prefer to wait for a solution "in the language" for the arity limitation (and I'd prefer that IMO).

[nikomatsakis]

Let me be clear on what is being proposed:

Limit the built-in impls to the Copy case, and use specialization to ease the overlap between the existing impls (which cover T: Clone), and the rest? (Do this for both tuples and fixed-length arrays?)

I'd be fine with it. I considered suggesting it at some point (after all, I've been hoping to use specialization uniformly to cover this case...), except that I think it's not exactly a feature that you can't clone things. But I do think it'd be nice for things of arbitrary arity to work uniformly for all traits, not just Clone.

[arielb1]

We don't even need specialization. If we're ok with hacks, we can just not have the "complier-generated" impls on tuples with less than 8 elements and arrays with less than 32 elements.

However, I think that getting rid of the Clone situation once and for all is worth it.

[nikomatsakis]

We don't even need specialization. If we're ok with hacks, we can just not have the "complier-generated" impls on tuples with less than 8 elements and arrays with less than 32 elements.

Indeed, true.

However, I think that getting rid of the Clone situation once and for all is worth it.

This was roughly what I was thinking. But I don't have a very strong opinion here -- I'd be ok with either path.

[nikomatsakis]

@arielb1 quick question: I know you were depending on these changes for #43880, was the case that concerned you specific to tuples/arrays of large arity? I presume not.

[arielb1]

@nikomatsakis

No. Only the "function pointer with a binder" case.

[bors]

☔ The latest upstream changes (presumably #43076) made this pull request unmergeable. Please resolve the merge conflicts.

[nikomatsakis]

OK, so @eddyb and I were taking, and he said that he would be happy if we restricted the cases covered to precisely the ones needed to fix the ICE (and tried not to go further). I personally think this is OK. I just want the ICE fixed. We can argue about the rest later. I think we had agreed on a viable strategy for how to do that above.

@scalexm, would you be able to make such a patch? It may have to be backported to beta too.

[scalexm]

I can do that.

[nikomatsakis]

OK, let's close this PR in favor of #44196 (right?)