derive(Copy) on tuple struct with non-Copy field uses unclear "<unnamed_field>"

[huonw]

struct Foo;
#[derive(Copy, Clone)]
struct Bar(Foo);

fn main() {}

<anon>:2:10: 2:14 error: the trait `Copy` may not be implemented for this type; field `<unnamed_field>` does not implement `Copy` [E0204]
<anon>:2 #[derive(Copy, Clone)]
                  ^~~~

It'd be nice to mention either the type, or the field index. (Or both.)

[abonander]

This one doesn't sound too hard. Mind if I try my hand at it?

[apasel422]

@cybergeek94 It might be worthwhile in the long run to fix this globally, so that things other than derive(Copy) avoid using <unnamed_field> as well, but fixing just this case should be relatively simple.

[apasel422]

Additionally, it would be nice to note the field in question.

[abonander]

@apasel422 Yeap, the core problem is that fields in a tuple struct aren't given names when added to the AST during parsing. I am unsure how to approach this without breaking a lot of difficult code.

[abonander]

The primary blocker to giving names to tuple struct fields is that rustc::middle::ty::ctxt::is_tuple_struct() assumes that a field must be part of a tuple struct if its name is "<unnamed_field>".

One solution I can see is to add a new field to struct FieldTy, pub is_unnamed: bool (bikeshedding welcome), which is set when the FieldTy is constructed from ast::StructField_ in rustc_typeck::convert_field, setting it to true if StructField_::kind == UnnamedField. Then, is_tuple_struct() can just check the value of that bool, which also might be a small performance win in eliminating a pointer indirection and a string-equality check in a linear algorithm (I just realized it's only comparing the interned string IDs, not the strings themselves).

With that refactored, we could create a name based on the field's index and/or type, and print that to point users towards the culprit in their code.

However, a less intrusive change would be to leave the naming semantics as-is, but change "<unnamed_field>" to something less debug-looking, like "(unnamed field)", then add @apasel422's suggestion of pointing to the field's span in a note: printout. The NodeId for the field (to look up the span), or the span itself, can simply be returned fromctxt::can_type_implement_copy() in the CopyImplementationError::FieldDoesNotImplementCopy error value.

This approach would change less of the compiler's internals while being more informative for Copy impl errors on both regular and tuple-structs.

However, the latter approach would not fix other occurrences of the uninformative "<unnamed_field>" besides making it look a little more presentable. I'm not exactly sure where else it comes up, so maybe those other occurrences should also have note: spanned printouts directing the user to the problem field instead of just telling them its index (which isn't as intuitive).

@huonw, what would you recommend?

[huonw]

One solution I can see is to add a new field to struct FieldTy, pub is_unnamed: bool (bikeshedding welcome), which is set when the FieldTy is constructed from ast::StructField_ in rustc_typeck::convert_field, setting it to true if StructField_::kind == UnnamedField. Then, is_tuple_struct() can just check the value of that bool, which also might be a small performance win in eliminating a pointer indirection and a string-equality check in a linear algorithm (I just realized it's only comparing the interned string IDs, not the strings themselves).

From a correctness point of view, I think this would make more sense as making name: Option<Name>, however this may be an annoying change.

At a high level doing this in a way that ensures anywhere that might print <unnamed_field> automatically gets the right piece of info would be good.

In any case, I'm not the best mentor. Maybe someone from @rust-lang/compiler can provide more precise guidance.

[abonander]

The name field wouldn't be optional for tuple structs; when the AST gets converted to trans' types, we'd generate a unique name based on the field index and type, e.g. (0)Vec<u8>.

Having an is_unnamed field would eliminate dependence on the assumption that tuple-struct fields all have the same name, <unnamed_field>.

[nrc]

It seems to me that using Option is tonnes nicer than using <unnamed_field>, but this is sort of orthogonal to the issue here. When making the error message in derive (or wherever that happens), can we detect that the field is unnamed, then make up a name on the spot, rather than using the name field?

[eddyb]

@cybergeek94 Returning more information in FieldDoesNotImplementCopy is indeed the easiest route, however... you have the index!
Simply change the iteration in ParameterEnvironment::can_type_implement_copy from for field in &fields to for (index, field) in fields.iter().enumerate() and you can return the index alongside the name, letting rustc_typeck::coherence::CoherenceChecker::check_implementations_of_copy print it in case the name is <unnamed_field>.

[eddyb]

Searching for unnamed_field reveals one such printing logic in the same file (for CoerceUnsized, the other trait being checked there) and another in debuginfo.

[nikomatsakis]

It makes sense to wait until @arielb1's PR #27551 lands, I suspect.

[sanxiyn]

#27551 is merged.