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.

Sign up for GitHub

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

Jump to bottom

Treat unmatched non-exhaustive remote variant as serde(skip) #2570

Merged
merged 2 commits into from Aug 14, 2023
Merged

Treat unmatched non-exhaustive remote variant as serde(skip) #2570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb490ec
Add test of remote enum with non_exhaustive
dtolnay Aug 14, 2023
8d3a032
Treat unmatched non-exhaustive remote variant as serde(skip)
dtolnay Aug 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions serde/src/private/ser.rs
View file
Open in desktop
Expand Up @@ -1370,3 +1370,16 @@ impl Serialize for AdjacentlyTaggedEnumVariant {
serializer.serialize_unit_variant(self.enum_name, self.variant_index, self.variant_name)
}
}

// Error when Serialize for a non_exhaustive remote enum encounters a variant
// that is not recognized.
pub struct CannotSerializeVariant<T>(pub T);
Comment on lines +1374 to +1376
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not doc comment? Even if this type is internal this is not the reason to keep internals undocumented, of course if your aim is not to make life of other developers harder

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Rust doc comments have significant syntactic ceremony that makes them less readable than non-doc comments. For example a normal, readable comment like:

    // Cow<str> and Cow<[u8]> never borrow by default:
//
//     impl<'de, 'a, T: ?Sized> Deserialize<'de> for Cow<'a, T>
//
// A #[serde(borrow)] attribute enables borrowing that corresponds
// roughly to these impls:
//
//     impl<'de: 'a, 'a> Deserialize<'de> for Cow<'a, str>
//     impl<'de: 'a, 'a> Deserialize<'de> for Cow<'a, [u8]>

    would need to be made into something like the following, which has added a bevy of backticks and other noise that does not contribute any value to this as documentation.

    /// `Cow<str>` and `Cow<[u8]>` never borrow by default:
///
/// ```ignore
/// impl<'de, 'a, T: ?Sized> Deserialize<'de> for Cow<'a, T>
/// ```
///
/// A `#[serde(borrow)]` attribute enables borrowing that corresponds
/// roughly to these impls:
///
/// ```ignore
/// impl<'de: 'a, 'a> Deserialize<'de> for Cow<'a, str>
/// impl<'de: 'a, 'a> Deserialize<'de> for Cow<'a, [u8]>
/// ```

  2. Most contributors touching an internal-facing "doc comment" on a private function will never bother figuring the flags for getting it rendering through rustdoc to validate it renders in the way they meant. So this adds pointless review overhead to audit whether their markdown is appropriate. For example in the snippet above, needing to know that unescaped <str> and [u8] would screw up rustdoc.

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Having doc comments helps to investigating internals, because editors is able to show documentation on items. They cannot show usual comments in the same way. I disagree that markdown syntax makes docs less readable, it's just a matter of habit
  2. You can use cargo doc --document-private-items and all syntax mistakes will be catch even in private documentation


impl<T> Display for CannotSerializeVariant<T>
where
T: Debug,
{
fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
write!(formatter, "enum variant cannot be serialized: {:?}", self.0)
}
}
9 changes: 9 additions & 0 deletions serde_derive/src/internals/attr.rs
View file
Open in desktop
Expand Up @@ -221,6 +221,7 @@ pub struct Container {
is_packed: bool,
/// Error message generated when type can't be deserialized
expecting: Option<String>,
non_exhaustive: bool,
}

/// Styles of representing an enum.
Expand Down Expand Up @@ -306,9 +307,12 @@ impl Container {
let mut variant_identifier = BoolAttr::none(cx, VARIANT_IDENTIFIER);
let mut serde_path = Attr::none(cx, CRATE);
let mut expecting = Attr::none(cx, EXPECTING);
let mut non_exhaustive = false;

for attr in &item.attrs {
if attr.path() != SERDE {
non_exhaustive |=
matches!(&attr.meta, syn::Meta::Path(path) if path == NON_EXHAUSTIVE);
continue;
}

Expand Down Expand Up @@ -587,6 +591,7 @@ impl Container {
serde_path: serde_path.get(),
is_packed,
expecting: expecting.get(),
non_exhaustive,
}
}

Expand Down Expand Up @@ -672,6 +677,10 @@ impl Container {
pub fn expecting(&self) -> Option<&str> {
self.expecting.as_ref().map(String::as_ref)
}

pub fn non_exhaustive(&self) -> bool {
self.non_exhaustive
}
}

fn decide_tag(
Expand Down
1 change: 1 addition & 0 deletions serde_derive/src/internals/symbol.rs
View file
Open in desktop
Expand Up @@ -19,6 +19,7 @@ pub const FLATTEN: Symbol = Symbol("flatten");
pub const FROM: Symbol = Symbol("from");
pub const GETTER: Symbol = Symbol("getter");
pub const INTO: Symbol = Symbol("into");
pub const NON_EXHAUSTIVE: Symbol = Symbol("non_exhaustive");
pub const OTHER: Symbol = Symbol("other");
pub const REMOTE: Symbol = Symbol("remote");
pub const RENAME: Symbol = Symbol("rename");
Expand Down
8 changes: 7 additions & 1 deletion serde_derive/src/ser.rs
View file
Open in desktop
Expand Up @@ -401,14 +401,20 @@ fn serialize_enum(params: &Parameters, variants: &[Variant], cattrs: &attr::Cont

let self_var = &params.self_var;

let arms: Vec<_> = variants
let mut arms: Vec<_> = variants
.iter()
.enumerate()
.map(|(variant_index, variant)| {
serialize_variant(params, variant, variant_index as u32, cattrs)
})
.collect();

if cattrs.non_exhaustive() {
arms.push(quote! {
unrecognized => _serde::__private::Err(_serde::ser::Error::custom(_serde::__private::ser::CannotSerializeVariant(unrecognized))),
});
}

quote_expr! {
match *#self_var {
#(#arms)*
Expand Down
12 changes: 12 additions & 0 deletions test_suite/tests/test_remote.rs
View file
Open in desktop
Expand Up @@ -125,6 +125,9 @@ struct Test {

#[serde(with = "EnumConcrete")]
enum_concrete: remote::EnumGeneric<u8>,

#[serde(with = "ErrorKindDef")]
io_error_kind: std::io::ErrorKind,
}

#[derive(Serialize, Deserialize)]
Expand Down Expand Up @@ -197,6 +200,15 @@ enum EnumConcrete {
Variant(u8),
}

#[derive(Serialize, Deserialize)]
#[serde(remote = "std::io::ErrorKind")]
#[non_exhaustive]
enum ErrorKindDef {
NotFound,
PermissionDenied,
// ...
}

impl From<PrimitivePrivDef> for remote::PrimitivePriv {
fn from(def: PrimitivePrivDef) -> Self {
remote::PrimitivePriv::new(def.0)
Expand Down