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

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

Merged
merged 2 commits into from Aug 14, 2023

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Aug 14, 2023

Serde already supports a serde(skip) attribute to not serialize a particular variant, returning an error instead.

#[derive(Serialize)]
enum Enum {
    Foo,
    #[serde(skip)]
    Bar,
}

This PR gives equivalent behavior to #[non_exhaustive] remote enums: it's as if every other possible variant were listed with a serde(skip) on it.

#[derive(Serialize, Deserialize)]
#[serde(remote = "std::io::ErrorKind")]
#[non_exhaustive]
enum ErrorKindDef {
    NotFound,
    PermissionDenied,
    // ...
}
assert!(ErrorKindDef::serialize(&ErrorKind::OutOfMemory, serializer).is_err());

Closes #2360. Closes #2361.

@dtolnay dtolnay merged commit 5e56c9f into serde-rs:master Aug 14, 2023
16 checks passed
@dtolnay dtolnay deleted the remotenonexhaustive branch August 14, 2023 04:20
Comment on lines +1374 to +1376
// Error when Serialize for a non_exhaustive remote enum encounters a variant
// that is not recognized.
pub struct CannotSerializeVariant<T>(pub T);
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

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