Make compile error message point to field type instead of call site for "not implemented Serialize trait"

[Veetaha]

Before

The error points to the call site of the Serialize derive:

After

The error points to the type of the field directly, which makes it easier to find where exactly in the struct the type that doesn't implement Serialize is located.

---

The trick is to use the type of the field in the type hint for the serialize_field/entry method call. Previously that call used a quote_spanned inheriting the span of the field itself (I'm not sure that actually did anything). Now, instead of that we pass serialize_field::<#ty>, and thus the compiler knows to point to #ty in the error message.

Interestingly, the code generated by Deserialize already follows this pattern and thus doesn't have this problem. So this only fixes Serialize codegen.

[oli-obk]

how does this affect errors when using serialize_with?

[Veetaha]

I've checked the difference in error messages locally with serde(with) and it doesn't change (e.g. when the signature of the given function is wrong). I can add a ui snapshot test for that today in the evening.

Also, the error for serde(with) still points to the macro call site (not to the function in the serde(with) attribute) as it was before my changes and after. That can also be improved by wrapping the function itself in a ::core::convert::identity<fn(#(#field_types,)*) -> __S> which gives the type hint for the function signature and the compiler starts complaing that the function is wrong, so it points the error to the function directly. However, this unfortunatelly coerces the function to a function pointer...

As a workaround that hint could be inside of an if false:

if false {
    let _: fn( #(field_types,)*) -> __S = #fn_path;
}

// ... actual usage of `#fn_path` is unchanged

But this will just produce more errors (on inside of the if and one at the site where #fn_path is actually used.

One more alternative would be to generate an identity function like this:

fn type_hint<F: Fn(...) -> __S>(f: F) -> F { f }

that would give a type hint without an fn pointer coersion. But it would need to be a method on the internal SerdeWith wrapper struct, because it needs to inherit the generics and field types from it.

Also.. it will just increase the compile times because rustc will need to typecheck that type_hint function additionally.

[Mingun]

with attributes fixed by me in #2558

[Veetaha]

Interesting... So I suppose it's enough to wrap the expression of the function invocation with quote_spanned for this to work?

[Veetaha]

I wonder what if I attach the span of the type itself to the parameter of serialize_field/entry... Maybe it'll yield the same result. I'll experiment with this later today.

Otherwise I suppose #2558 should land first, because it looks like it may conflict with this PR?

[Mingun]

Not sure. I'm ready to rebase all my PRs once their fall into conflict state

[Veetaha]

I've checked how it works. The similar behavior can be achieved if even with the expression serialize_field::<_> where the span of _ is assigned to the span of the field type.

This is because with the explicit turbofish compiler emits an error pointing to it:

error[E0277]: the trait bound `NoImpls: _::_serde::Serialize` is not satisfied
    --> crates/sandbox/src/main.rs:34:61
     |
34   |             _serde::ser::SerializeStruct::serialize_field::<_>(&mut __serde_state, "x2", &self.x2)?;
     |                                                             ^ the trait `_::_serde::Serialize` is not implemented for `NoImpls`
     |

So if its span points to smth else (like the field type), the compiler will point to the field type instead.

[Veetaha]

When there is no turbofish syntax, it's important that all parts of the field expression have the span of the field, because only then will the compiler point to the original field.

error[E0277]: the trait bound `NoImpls: _::_serde::Serialize` is not satisfied
    --> crates/sandbox/src/main.rs:34:85
     |
34   |             _serde::ser::SerializeStruct::serialize_field(&mut __serde_state, "x2", &self.x2)?;
     |             ---------------------------------------------                           ^^^^^^^^ the trait `_::_serde::Serialize` is not implemented for `NoImpls`
     |             |
     |             required by a bound introduced by this call

For example, if all tokens &, self, . and x2 have the span of the field, you'll get an error pointing to the field in this case:

error[E0277]: the trait bound `NoImpls: _::_serde::Serialize` is not satisfied
    --> crates/sandbox/src/main.rs:8:5
     |
8    |     x2: NoImpls,
     |     ^^ the trait `_::_serde::Serialize` is not implemented for `NoImpls`

Alothough.. it would be better if the error pointed to the type itself. What makes this harder to achieve is that quote_spanned doesn't change the span of the interpolated tokens. So for example, today the self token in the field expression uses the callsite span when it's interpolated:

serde/serde_derive/src/ser.rs

Lines 100 to 104 in 3415619

	let self_var = if is_remote {
	Ident::new("__self", Span::call_site())
	} else {
	Ident::new("self", Span::call_site())
	};

But instead each usage of this token should change the span to the relevant field.

[Veetaha]

I've updated the impl a bit and moved the logic if skipping the type hint if serialize_with is enabled into a function with some comments explaining how it helps with hinting the correct error span.

Unrelated observations

I've also noticed that if the user passes invalid function to skip_serializing_if, the error also points to the callsite of the derive.

To fight with that we need to re-span the field expression to assign the span of the function in skip_serializing_if to all the tokens in #field_expr. Right now this code looks like this:

serde/serde_derive/src/ser.rs

Lines 1075 to 1078 in 3415619

	let skip = field
	.attrs
	.skip_serializing_if()
	.map(|path| quote!(#path(#field_expr)));

However, just using quote_spanned here won't help. We need to re-span the #field_expr for this to work.