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

Hygiene for macro-generated newtype struct deserialization with with attr #2847

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Oct 22, 2024

Fixes #2846.

    error[E0425]: cannot find value `__e` in this scope
      --> test_suite/tests/regression/issue2846.rs:12:19
       |
    12 | declare_in_macro!("with");
       |                   ^^^^^^ not found in this scope
    warning: field `0` is never read
      --> test_suite/tests/regression/issue2846.rs:8:45
       |
    8  |         pub struct S(#[serde(with = $with)] i32);
       |                    - field in this struct   ^^^
    ...
    12 | declare_in_macro!("with");
       | ------------------------- in this macro invocation
       |
       = help: consider removing this field
       = note: `#[warn(dead_code)]` on by default
       = note: this warning originates in the macro `declare_in_macro` (in Nightly builds, run with -Z macro-backtrace for more info)
@dtolnay dtolnay merged commit ef0ed22 into serde-rs:master Oct 22, 2024
15 checks passed
@dtolnay dtolnay deleted the newtypewith branch October 22, 2024 18:14
@oli-obk
Copy link
Member

oli-obk commented Oct 22, 2024

This seems very footgunny, do you know if there is an issue or discussion about making this less painful? It feels to me like a wrong-defaults issue. I'd expect it to always use .resolved_at(Span::call_site()) unless opted out of.

That said, wouldn't it be cleaner to do that manually here and thus also avoid clippy issues?

@dtolnay
Copy link
Member Author

dtolnay commented Oct 22, 2024

.resolved_at(Span::call_site()) is almost never a good idea because various code in rustc and clippy looks at "resolved_at == call_site" to decide whether to disregard the "located_at" of a token when reporting an error or suggestion involving it. For example, on top of 04bb76b, this code change:

-    quote_spanned!(serialize_with.span()=> {
+    quote_spanned!(serialize_with.span().resolved_at(Span::call_site())=> {
         #[doc(hidden)]
         struct __SerializeWith #wrapper_impl_generics #where_clause {

has these unwanted effects, among others:

 error[E0277]: the trait bound `&u8: Serializer` is not satisfied
   --> tests/ui/with/incorrect_type.rs:14:10
    |
-14 | #[derive(Serialize, Deserialize)]
-   |          ^^^^^^^^^ the trait `Serializer` is not implemented for `&u8`
-15 | struct W(#[serde(with = "w")] u8, u8);
-   |                         --- required by a bound introduced by this call
+14 |   #[derive(Serialize, Deserialize)]
+   |            ^--------
+   |            |
+   |  __________in this derive macro expansion
+   | |
+15 | | struct W(#[serde(with = "w")] u8, u8);
+   | |                         --^
+   | |_________________________|_|
+   |                           | the trait `Serializer` is not implemented for `&u8`
+   |                           required by a bound introduced by this call
    |
    = help: the following other types implement trait `Serializer`:
              &mut Formatter<'a>
 error[E0061]: this function takes 1 argument but 2 arguments were supplied
   --> tests/ui/with/incorrect_type.rs:15:25
    |
 15 | struct W(#[serde(with = "w")] u8, u8);
    |                         ^^^ unexpected argument #2 of type `__S`
    |
 note: function defined here
   --> tests/ui/with/incorrect_type.rs:9:12
    |
 9  |     pub fn serialize<T, S: Serializer>(_: S) -> Result<S::Ok, S::Error> {
    |            ^^^^^^^^^                   ----
+help: remove the extra argument
+   |
+15 - struct W(#[serde(with = "w")] u8, u8);
+15 + struct W(#[serde(with = )] u8, u8);
+   |

@oli-obk
Copy link
Member

oli-obk commented Oct 23, 2024

Hmm... if we can make the world of proc macros better by stopping to do that, that seems great. I was working on some span stuff just a few months ago. I'll see if we can remove the PartialEq impl from spans entirely, it's already a footgun just in the compiler, seems best to remove it if its "benign" uses are problematic, for proc macros

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Serde still causing errors even after 1.0.212
2 participants