-
-
Notifications
You must be signed in to change notification settings - Fork 777
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
Veetaha
wants to merge
5
commits into
serde-rs:master
Choose a base branch
from
Veetaha:feat/improve-no-serialize-impl-error-span
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
9144a88
Make compile error message point to field type instead of call site f…
Veetaha 6cac755
Return back `quote_spanned` for the rest of `serialize_entry/field/el…
Veetaha 02b2f86
Move span variable closer to usage
Veetaha cecb90a
Fix grammar
Veetaha 5a445e6
Fix span variable location
Veetaha File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
166
test_suite/tests/ui/on-unimplemented/struct_field.stderr
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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() {} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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 theserde(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
: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:
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 thattype_hint
function additionally.There was a problem hiding this comment.
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 #2558There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
So if its span points to smth else (like the field type), the compiler will point to the field type instead.
There was a problem hiding this comment.
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.
For example, if all tokens
&
,self
,.
andx2
have the span of the field, you'll get an error pointing to the field in this case: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 theself
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
But instead each usage of this token should change the span to the relevant field.