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

Mark newtype wrappers as #[serde(transparent)] #724

Merged
merged 2 commits into from
Dec 24, 2024
Merged

Conversation

ahl
Copy link
Collaborator

@ahl ahl commented Dec 24, 2024

In JSON, the difference between a newtype that's transparent or not is unobservable--they both render to the same serialized output. This is not true of other serializations and--more critically--not true of other implementations of serde::Serializer.

In particular, I encountered this in oxidecomputer/progenitor#1017 where I observed that the serde_urlencoded::Serializer freaks out about our newtype wrappers. Perhaps this freak out is reasonable because the intended representation matches with the intention of transparent.

Consider the case where I noticed this: struct Name(String). Name wraps a String and imposes constraints on it. It deserializes from a string and its intended serialization is a string--the newtype wrapper is purely a type for enforcing constraints when creating or manipulating the type. This hasn't bit us before, but marking these newtypes as transparent seems like the right thing to do.

@ahl ahl requested review from sunshowers and augustuswm December 24, 2024 04:40
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This seems like the right thing to do to me!

@ahl ahl merged commit fa65d74 into main Dec 24, 2024
4 checks passed
@ahl ahl deleted the transparent-newtypes branch December 24, 2024 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants