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

Improve error reporting about mismatched signature in with and default attributes #2558

Merged
merged 3 commits into from
Oct 21, 2024

Conversation

Mingun
Copy link
Contributor

@Mingun Mingun commented Aug 7, 2023

This PR changes the errors reported by rustc when signature of function specified in

  • #[serde(with = "...")]
  • #[serde(deserialize_with = "...")]
  • #[serde(serialize_with = "...")]
  • #[serde(default = "...")]

is wrong. With this PR error will point to the "...".

@Mingun
Copy link
Contributor Author

Mingun commented Aug 7, 2023

Strange, but order of implementations shown in failed run is different from my (my removed, GitHub runner added):

    = help: the following other types implement trait `Serializer`:
-             FlatMapSerializer<'a, M>
-             &'a mut Formatter<'b>
              _::_serde::__private::ser::content::ContentSerializer<E>
+             FlatMapSerializer<'a, M>
+             &'a mut Formatter<'b>

This can create difficulties in local testing.

@Mingun
Copy link
Contributor Author

Mingun commented Aug 4, 2024

Probably the order of suggested implementations is heavy dependent on rustc version. With rustc 1.82.0-nightly (fbccf5053 2024-07-27) the local output is the same as at GitHub.

Copy link
Member

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

These are great improvements. It seems like there is still space for rustc to provide more diagnostics attributes, but nothing that we can fix ok this end

@oli-obk oli-obk merged commit 04bb76b into serde-rs:master Oct 21, 2024
15 checks passed
@Mingun Mingun deleted the correct-span branch October 21, 2024 20:16
@Veetaha
Copy link

Veetaha commented Oct 21, 2024

@oli-obk looks like the merge of this PR broke master CI. The CI on this PR was green some time ago, and it broke with time

@dtolnay
Copy link
Member

dtolnay commented Oct 22, 2024

The failing ui test was from a non-textual conflict with 8e1ae68. Fixed in #2839. (Also #2840).

It is unfortunate that:

   = help: the following other types implement trait `Serializer`:
             &mut Formatter<'a>
             FlatMapSerializer<'a, M>
             _::_serde::__private::ser::content::ContentSerializer<E>

is the set of impls chosen to be suggested by rustc. Is there something we can do with doc(hidden) to exclude nonpublic APIs from this error message? If not, this needs to be reported as a rustc bug.

@dtolnay
Copy link
Member

dtolnay commented Oct 22, 2024

Reported rust-lang/rust#132024.

@oli-obk
Copy link
Member

oli-obk commented Oct 22, 2024

is the set of impls chosen to be suggested by rustc. Is there something we can do with doc(hidden) to exclude nonpublic APIs from this error message? If not, this needs to be reported as a rustc bug.

We'll get diagnostics::do_not_suggest or sth. Iirc it's being worked on or works on nightly already

@Turbo87
Copy link

Turbo87 commented Oct 22, 2024

it looks like this change might have unintended side effects. rust-lang/crates.io#9720 upgrades crates.io from v1.0.210 to v1.0.211 and we are now seeing various "not found in this scope" errors that were not there before.

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.

5 participants