Use more spans for error messages

[hcpl]

Basically, this PR makes errors that look like this:

error[E0277]: the trait bound `Foo: std::default::Default` is not satisfied
  --> src/lib.rs:11:21
   |
11 | #[derive(Serialize, Deserialize)]
   |                     ^^^^^^^^^^^ the trait `std::default::Default` is not implemented for `Foo`
   |
   = note: required by `std::default::Default::default`

to look like this instead:

error[E0277]: the trait bound `Foo: std::default::Default` is not satisfied
  --> src/lib.rs:17:5
   |
17 | /     #[serde(rename = "f")]
18 | |     #[serde(default)]
19 | |     c: Foo,
   | |__________^ the trait `std::default::Default` is not implemented for `Foo`
   |
   = note: required by `std::default::Default::default`

I believe I covered every possible invalid case, though I possibly missed some due to the sheer amount of supported derive configurations.

I am not sure if spans for attributes should be included in general --- on one hand, they consume vertical terminal estate; on the other hand, they may give a clue why the error happened at all (in the example above the std::default::Default requirement came from #[serde(default)]).

Also this needs compile-fail tests and I haven't figured out how to write those yet.

[dtolnay]

Thanks! This is important work.

Regarding compile-fail tests, please rebase on master to pick up 4821d09. After that you can add tests by adding a *.rs file under test_suite/tests/ui. To run those tests:

$ cd test_suite/deps
$ cargo clean
$ cargo update
$ cargo +nightly build
$ cd ..
$ cargo +nightly test --features unstable

To automatically generate the expected test outputs based on the most recent cargo test:

$ cd test_suite/tests/ui
$ ./update-references.sh /tmp */*.rs

Here is what the effect on the existing tests looks like: 184546e. Some of those changes seem suspicious to me, such as:

#[derive(Deserialize)]
struct Test<'a> {
    #[serde(borrow = "'b")]
    s: &'a str,
}

  error: field `s` does not have lifetime 'b
-  --> $DIR/wrong_lifetime.rs:4:10
+  --> $DIR/wrong_lifetime.rs:5:1
    |
- 4 | #[derive(Deserialize)]
-   |          ^^^^^^^^^^^
+ 5 | struct Test<'a> {
+   | ^^^^^^

Ideally the error would point to the borrow = "'b" attribute, but pointing to Deserialize seems better than pointing to struct in the case that the struct may have many different derives that could have triggered such an error.

I believe this line will need to return more information than just a String. You could use syn::Error which is basically a String + Span, then use to_compile_error() to turn it into a compile_error! with the right span.

[hcpl]

Thanks for the tips!

I implemented granular error reporting for compile_error!-based errors and updated existing UI tests as requested.

There were 5 remote tests that didn't get updated, from which missing_field and unknown_field were good already, whereas wrong_de, wrong_ser and wrong_getter deal with generated code that uses the remote type/values, so spans from the user type don't help. I plan to resolve this as well, preferably in this PR, though if there is a need to land this ASAP, I can organize a separate one.

Also a funny error message from remote/wrong_de has been reported: rust-lang/rust#56335.

Now addessing your points:

Regarding compile-fail tests, please rebase on master to pick up 4821d09.

I believe this line will need to return more information than just a String. You could use syn::Error which is basically a String + Span, then use to_compile_error() to turn it into a compile_error! with the right span.

Done.

Some of those changes seem suspicious to me, such as:

That's because previously errors pointed to the whole container while lacking multi-token span support unless --cfg procmacro2_semver_exempt is used. Now spans are more precise and rely on the syn::Error::new_spanned constructor which supports multi-token spans even on stable.

Ideally the error would point to the borrow = "'b" attribute

I chose to point to the field and attributes so that user could see the field type and quickly understand why Serde couldn't find the requested lifetime:

 error: field `s` does not have lifetime 'b
- --> $DIR/wrong_lifetime.rs:4:10
+ --> $DIR/wrong_lifetime.rs:6:5
   |
-4 | #[derive(Deserialize)]
-  |          ^^^^^^^^^^^
+6 | /     #[serde(borrow = "'b")]
+7 | |     s: &'a str,
+  | |______________^

but pointing to Deserialize seems better than pointing to struct in the case that the struct may have many different derives that could have triggered such an error.

I think the span that includes #[serde(borrow = "'b")] eliminates this problem, though there could be #[other_crate(borrow = "'b")]] with the same error message, however unlikely that may be. What do you think?

[hcpl]

I added missing test cases as UI tests.

Remote type-spanned errors still not finished yet, some internal restructuring needs to be made.

[dtolnay]

Wow -- I am very impressed by this pull request and this is a huge improvement. Thanks so much! I will go ahead and land this now so it doesn't start growing merge conflicts, and we can follow up on the remote error messages separately.