Skip to content

Conversation

@Mingun
Copy link
Contributor

@Mingun Mingun commented May 29, 2025

This PR removes one FIXME in the code and probably slightly improves compilation time in some cases

@Mingun Mingun force-pushed the remove-fixme branch 2 times, most recently from e75903c to ade1691 Compare September 21, 2025 11:30
Comment on lines 66 to 68
_serde::#private::Result::map(
_serde::de::EnumAccess::variant::<__Field>(__data),
|(__impossible, _)| match __impossible {})
Copy link
Member

Choose a reason for hiding this comment

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

This codepath for the zero variant case is here in order for the generated code to compile with rust older than 1.82. After this PR it would not compile:

error[E0004]: non-exhaustive patterns: `Ok(_)` not covered
 --> src/main.rs:1:10
  |
1 | #[derive(serde::Deserialize)] pub enum Void {}
  |          ^^^^^^^^^^^^^^^^^^ pattern `Ok(_)` not covered
  |
note: `Result<(__Field, <__A as EnumAccess<'_>>::Variant), <__A as EnumAccess<'_>>::Error>` defined here
 --> /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/result.rs:527:1
 ::: /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/result.rs:531:5
  |
  = note: not covered
  = note: the matched value is of type `Result<(__Field, <__A as EnumAccess<'_>>::Variant), <__A as EnumAccess<'_>>::Error>`
  = note: this error originates in the derive macro `serde::Deserialize` (in Nightly builds, run with -Z macro-backtrace for more info)

Serde_derive presently aims to support rust 1.61. We can probably raise that but I am not sure if 1.82 is too recent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I missed that. I removed that commit. However, it is strange why CI does not check the actual build on the minimum version (or even run tests). It only takes half a minute, is it really worth saving that time at the risk of missing breaking changes like these?

This may slightly improve compilation time and costs nothing
@Mingun Mingun changed the title Remove FIXME for externally tagged enum implementation Remove some ? from generated code Sep 28, 2025
@dtolnay dtolnay merged commit 4e27870 into serde-rs:master Sep 28, 2025
14 checks passed
@Mingun Mingun deleted the remove-fixme branch September 29, 2025 07:09
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.

2 participants