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.

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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 25 additions & 12 deletions serde_derive/src/ser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,7 @@ fn serialize_struct_tag_field(cattrs: &attr::Container, struct_trait: &StructTra
match cattrs.tag() {
attr::TagType::Internal { tag } => {
let type_name = cattrs.name().serialize_name();
let func = struct_trait.serialize_field(Span::call_site());
let func = struct_trait.serialize_field(None);
quote! {
#func(&mut __serde_state, #tag, #type_name)?;
}
Expand Down Expand Up @@ -1081,8 +1081,13 @@ fn serialize_tuple_struct_visitor(
field_expr = wrap_serialize_field_with(params, field.ty, path, &field_expr);
}

let span = field.original.span();
let func = tuple_trait.serialize_element(span);
let ty = if field.attrs.serialize_with().is_none() {
Some(field.ty)
} else {
None
};

let func = tuple_trait.serialize_element(ty);
let ser = quote! {
#func(&mut __serde_state, #field_expr)?;
};
Expand Down Expand Up @@ -1131,7 +1136,13 @@ fn serialize_struct_visitor(
#func(&#field_expr, _serde::__private::ser::FlatMapSerializer(&mut __serde_state))?;
}
} else {
let func = struct_trait.serialize_field(span);
let ty = if field.attrs.serialize_with().is_none() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this affect errors when using serialize_with?

Copy link
Author

@Veetaha Veetaha Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with attributes fixed by me in #2558

Copy link
Author

@Veetaha Veetaha Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@Veetaha Veetaha Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@Veetaha Veetaha Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@Veetaha Veetaha Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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.

Some(field.ty)
} else {
None
};

let func = struct_trait.serialize_field(ty);
quote! {
#func(&mut __serde_state, #key_expr, #field_expr)?;
}
Expand Down Expand Up @@ -1300,16 +1311,17 @@ enum StructTrait {
}

impl StructTrait {
fn serialize_field(&self, span: Span) -> TokenStream {
fn serialize_field(&self, ty: Option<&syn::Type>) -> TokenStream {
let ty = ty.into_iter();
match *self {
StructTrait::SerializeMap => {
quote_spanned!(span=> _serde::ser::SerializeMap::serialize_entry)
quote!(_serde::ser::SerializeMap::serialize_entry #( ::<_, #ty> )* )
}
StructTrait::SerializeStruct => {
quote_spanned!(span=> _serde::ser::SerializeStruct::serialize_field)
quote!(_serde::ser::SerializeStruct::serialize_field #( ::<#ty> )* )
}
StructTrait::SerializeStructVariant => {
quote_spanned!(span=> _serde::ser::SerializeStructVariant::serialize_field)
quote!(_serde::ser::SerializeStructVariant::serialize_field #( ::<#ty> )* )
}
}
}
Expand All @@ -1334,16 +1346,17 @@ enum TupleTrait {
}

impl TupleTrait {
fn serialize_element(&self, span: Span) -> TokenStream {
fn serialize_element(&self, ty: Option<&syn::Type>) -> TokenStream {
let ty = ty.into_iter();
match *self {
TupleTrait::SerializeTuple => {
quote_spanned!(span=> _serde::ser::SerializeTuple::serialize_element)
quote!(_serde::ser::SerializeTuple::serialize_element #( ::<#ty> )*)
}
TupleTrait::SerializeTupleStruct => {
quote_spanned!(span=> _serde::ser::SerializeTupleStruct::serialize_field)
quote!(_serde::ser::SerializeTupleStruct::serialize_field #( ::<#ty> )*)
}
TupleTrait::SerializeTupleVariant => {
quote_spanned!(span=> _serde::ser::SerializeTupleVariant::serialize_field)
quote!(_serde::ser::SerializeTupleVariant::serialize_field #( ::<#ty> )*)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error[E0277]: the trait bound `MyStruct: Serialize` is not satisfied
--> tests/ui/on_unimplemented.rs:21:15
--> tests/ui/on-unimplemented/fn_param.rs:21:15
|
21 | to_string(&MyStruct);
| --------- ^^^^^^^^^ the trait `Serialize` is not implemented for `MyStruct`
Expand All @@ -19,7 +19,7 @@ error[E0277]: the trait bound `MyStruct: Serialize` is not satisfied
(T0, T1, T2, T3, T4)
and $N others
note: required by a bound in `to_string`
--> tests/ui/on_unimplemented.rs:6:8
--> tests/ui/on-unimplemented/fn_param.rs:6:8
|
4 | fn to_string<T>(_: &T) -> String
| --------- required by a bound in this function
Expand All @@ -28,7 +28,7 @@ note: required by a bound in `to_string`
| ^^^^^^^^^ required by this bound in `to_string`

error[E0277]: the trait bound `MyStruct: Deserialize<'_>` is not satisfied
--> tests/ui/on_unimplemented.rs:22:23
--> tests/ui/on-unimplemented/fn_param.rs:22:23
|
22 | let _: MyStruct = from_str("");
| ^^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `MyStruct`
Expand All @@ -46,7 +46,7 @@ error[E0277]: the trait bound `MyStruct: Deserialize<'_>` is not satisfied
(T0, T1, T2, T3)
and $N others
note: required by a bound in `from_str`
--> tests/ui/on_unimplemented.rs:13:8
--> tests/ui/on-unimplemented/fn_param.rs:13:8
|
11 | fn from_str<'de, T>(_: &'de str) -> T
| -------- required by a bound in this function
Expand Down
11 changes: 11 additions & 0 deletions test_suite/tests/ui/on-unimplemented/struct_field.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
use serde_derive::{Deserialize, Serialize};

struct NoImpls;

#[derive(Serialize, Deserialize)]
struct S {
x1: u32,
x2: NoImpls,
}

fn main() {}
166 changes: 166 additions & 0 deletions test_suite/tests/ui/on-unimplemented/struct_field.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
error[E0277]: the trait bound `NoImpls: Serialize` is not satisfied
--> tests/ui/on-unimplemented/struct_field.rs:8:9
|
8 | x2: NoImpls,
| ^^^^^^^ the trait `Serialize` is not implemented for `NoImpls`
|
= note: for local types consider adding `#[derive(serde::Serialize)]` to your `NoImpls` type
= note: for types from other crates check whether the crate offers a `serde` feature flag
= help: the following other types implement trait `Serialize`:
&'a T
&'a mut T
()
(T,)
(T0, T1)
(T0, T1, T2)
(T0, T1, T2, T3)
(T0, T1, T2, T3, T4)
and $N others
note: required by a bound in `_::_serde::ser::SerializeStruct::serialize_field`
--> $WORKSPACE/serde/src/ser/mod.rs
|
| fn serialize_field<T>(&mut self, key: &'static str, value: &T) -> Result<(), Self::Error>
| --------------- required by a bound in this associated function
| where
| T: ?Sized + Serialize;
| ^^^^^^^^^ required by this bound in `SerializeStruct::serialize_field`

error[E0277]: the trait bound `NoImpls: Deserialize<'_>` is not satisfied
--> tests/ui/on-unimplemented/struct_field.rs:8:9
|
8 | x2: NoImpls,
| ^^^^^^^ the trait `Deserialize<'_>` is not implemented for `NoImpls`
|
= note: for local types consider adding `#[derive(serde::Deserialize)]` to your `NoImpls` type
= note: for types from other crates check whether the crate offers a `serde` feature flag
= help: the following other types implement trait `Deserialize<'de>`:
&'a Path
&'a [u8]
&'a str
()
(T,)
(T0, T1)
(T0, T1, T2)
(T0, T1, T2, T3)
and $N others
note: required by a bound in `next_element`
--> $WORKSPACE/serde/src/de/mod.rs
|
| fn next_element<T>(&mut self) -> Result<Option<T>, Self::Error>
| ------------ required by a bound in this associated function
| where
| T: Deserialize<'de>,
| ^^^^^^^^^^^^^^^^ required by this bound in `SeqAccess::next_element`

error[E0277]: the trait bound `NoImpls: Deserialize<'_>` is not satisfied
--> tests/ui/on-unimplemented/struct_field.rs:8:9
|
8 | x2: NoImpls,
| ^^^^^^^ the trait `Deserialize<'_>` is not implemented for `NoImpls`
|
= note: for local types consider adding `#[derive(serde::Deserialize)]` to your `NoImpls` type
= note: for types from other crates check whether the crate offers a `serde` feature flag
= help: the following other types implement trait `Deserialize<'de>`:
&'a Path
&'a [u8]
&'a str
()
(T,)
(T0, T1)
(T0, T1, T2)
(T0, T1, T2, T3)
and $N others
note: required by a bound in `next_value`
--> $WORKSPACE/serde/src/de/mod.rs
|
| fn next_value<V>(&mut self) -> Result<V, Self::Error>
| ---------- required by a bound in this associated function
| where
| V: Deserialize<'de>,
| ^^^^^^^^^^^^^^^^ required by this bound in `MapAccess::next_value`

error[E0277]: the trait bound `NoImpls: Deserialize<'_>` is not satisfied
--> tests/ui/on-unimplemented/struct_field.rs:5:21
|
5 | #[derive(Serialize, Deserialize)]
| ^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `NoImpls`
|
= note: for local types consider adding `#[derive(serde::Deserialize)]` to your `NoImpls` type
= note: for types from other crates check whether the crate offers a `serde` feature flag
= help: the following other types implement trait `Deserialize<'de>`:
&'a Path
&'a [u8]
&'a str
()
(T,)
(T0, T1)
(T0, T1, T2)
(T0, T1, T2, T3)
and $N others
note: required by a bound in `_::_serde::__private::de::missing_field`
--> $WORKSPACE/serde/src/private/de.rs
|
| pub fn missing_field<'de, V, E>(field: &'static str) -> Result<V, E>
| ------------- required by a bound in this function
| where
| V: Deserialize<'de>,
| ^^^^^^^^^^^^^^^^ required by this bound in `missing_field`
= note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `NoImpls: Deserialize<'_>` is not satisfied
--> tests/ui/on-unimplemented/struct_field.rs:5:21
|
5 | #[derive(Serialize, Deserialize)]
| ^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `NoImpls`, which is required by `InPlaceSeed<'_, NoImpls>: DeserializeSeed<'_>`
|
= note: for local types consider adding `#[derive(serde::Deserialize)]` to your `NoImpls` type
= note: for types from other crates check whether the crate offers a `serde` feature flag
= help: the following other types implement trait `Deserialize<'de>`:
&'a Path
&'a [u8]
&'a str
()
(T,)
(T0, T1)
(T0, T1, T2)
(T0, T1, T2, T3)
and $N others
= note: required for `InPlaceSeed<'_, NoImpls>` to implement `DeserializeSeed<'_>`
note: required by a bound in `next_element_seed`
--> $WORKSPACE/serde/src/de/mod.rs
|
| fn next_element_seed<T>(&mut self, seed: T) -> Result<Option<T::Value>, Self::Error>
| ----------------- required by a bound in this associated function
| where
| T: DeserializeSeed<'de>;
| ^^^^^^^^^^^^^^^^^^^^ required by this bound in `SeqAccess::next_element_seed`
= note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound `NoImpls: Deserialize<'_>` is not satisfied
--> tests/ui/on-unimplemented/struct_field.rs:5:21
|
5 | #[derive(Serialize, Deserialize)]
| ^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `NoImpls`, which is required by `InPlaceSeed<'_, NoImpls>: DeserializeSeed<'_>`
|
= note: for local types consider adding `#[derive(serde::Deserialize)]` to your `NoImpls` type
= note: for types from other crates check whether the crate offers a `serde` feature flag
= help: the following other types implement trait `Deserialize<'de>`:
&'a Path
&'a [u8]
&'a str
()
(T,)
(T0, T1)
(T0, T1, T2)
(T0, T1, T2, T3)
and $N others
= note: required for `InPlaceSeed<'_, NoImpls>` to implement `DeserializeSeed<'_>`
note: required by a bound in `next_value_seed`
--> $WORKSPACE/serde/src/de/mod.rs
|
| fn next_value_seed<V>(&mut self, seed: V) -> Result<V::Value, Self::Error>
| --------------- required by a bound in this associated function
| where
| V: DeserializeSeed<'de>;
| ^^^^^^^^^^^^^^^^^^^^ required by this bound in `MapAccess::next_value_seed`
= note: this error originates in the derive macro `Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)
10 changes: 10 additions & 0 deletions test_suite/tests/ui/on-unimplemented/struct_variant.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
use serde_derive::{Deserialize, Serialize};

struct NoImpls;

#[derive(Serialize, Deserialize)]
enum E {
S { x1: u32, x2: NoImpls },
}

fn main() {}
Loading