Allow serialization to implementers of fmt::Write

[dilr]

This PR creates a ser::Write trait which is used by ser::Serializer rather than io::Write. The ser::Write trait has a blanket implementation for io::Write implementers, and can be implemented through the FmtWrite transparent wrapper for fmt::Write implementers. This has been done in an API compatible manner (machine checked by cargo-semver-checks, though this is not conclusive)

Structure

Most of the code is just mechanically changing out which Write implementation is being used, and making sure the input to the Write implementation is a string rather than a byte slice. Thus, for ease of review, this is split into 3 commits.

The first commit contains the new ser::Write and FmtWrite implementations and all of the functions which had some non-mechanical refactoring involved. This commit is small and is easiest to review first. The second commit contains the mechanical refactoring. The third commit contains a new test that confirms that the io::Write and fmt::Write implementations produce the same output for escaped characters.

Of Note

This PR does introduce one behavior change to existing code. If PrettyFormatter::with_indent is fed a byte slice that is not valid utf-8, then it will revert to using two spaces as an indent instead of using the non utf-8 string as an indent.

Motivation

I had wanted to clean up the unsafe code in to_string and to_string_pretty, because the safety comment only holds if all other serialization code in this module actually produces utf-8. (which is a safety promise that relies on a large amount of code) In attempting to make the change, I discovered that ser::Serializer operating on an io::Write implementation causes the fact that the JSON being serialized to utf-8 to be lost to the type system.

This could be an issue for users of the library as well. For instance, right now there is no way to choose the level of indentation when serializing directly to a String. (to_string_pretty does not expose an indent parameter) Similarly, a user might want to create JSON in an encoding other than utf-8 without serializing to a utf-8 String buffer first. This would be quickest with a fmt::Write implementation that translated the utf-8 from serde_json directly to the target encoding as serde_json is producing it.

Note that despite this motivation, this PR does not touch to_string and to_string_pretty, as this PR is already large enough on its own.

Regarding API design

I exposed new public API surface in this PR because I think it would be useful to users of the library for reasons stated in the previous section. That being said, I'm happy to change this surface. Possible changes include; only implementing ser::Write on String rather than having a FmtWrite wrapper, or hiding the existence of the ser::Write trait from users, and only using it internally.

[dilr]

Some PR checks are not passing. The complaints seems to be that FmtWrite is not constructed, which should be OK because FmtWrite has public visibility, and that error_other is unused, which is untrue since it is used in the ser::Write implementations of FmtWrite.

I think these are due to a spurrious bug in stable/beta editions of rustc, because both the nightly and older rust versions pass the CI tests. If you have any idea about what is going on though, I'd appreciate it.

[dilr]

Figured out the CI issues, which were due to the ser module always existing but only being public if "std" was enabled.

Since this PR enables core::fmt::Write to be used with ser::Serializer, it should now be possible to use ser::Serializer even if the "std" feature flag is not enabled. I have not made any such changes yet though.

[dilr]

I found out that there is a very similar de::Read trait and de::StrRead wrapper. I would like the implementation details of ser::Write and ser::FmtWrite to mirror these, so I have converted this into a draft until I find the time to do so.

[jbis9051]

@dilr I came across this PR looking into solutions to #1281. Given your recent comment

it should now be possible to use ser::Serializer even if the "std" feature flag is not enabled.

if I understand correctly, this PR would largely solve that issue as well. , I've linked this PR to #1281. If possible, consider that issue as well while you work through this PR.

You also might want to consider this comment #1122 (comment).

[dilr]

I'll make sure the updated version exposes Serializer in the alloc configuration then. Thanks for linking #1122. I don't think that applies to this PR because I would only be adding features with the std feature gate rather than changing features, but it is good to know about that policy. I'll probably get around to changing the implementation this weekend.

[jbis9051]

Hey @dilr , just following up on this. Happy to help if I can.

[dilr]

The primary issue I ran into is that the Formatter trait has lots of methods which take io::Write. Since external crates can implement Formatter, I can't just switch the methods to use serde::Write instead.

I could create a new trait (say WipFormat) which duplicates the methods of Formatter but uses my new serde::Write instead of io::Write, and change the Serializer to be able to use either. The main issue is that Formatter is a very large trait, so I could duplicate each method for Formatter, WipFormat, and the binding code for Serializer, but now both traits' method bodies need to be kept in sync. Alternatively, I could use a declarative macro to generate both traits, but that makes it less obvious that the macro generated code is equivalent to the original.

I was hoping that I could come up with something more clever, but so far I am drawing a blank. If you have any suggestions, I am all ears. If not, I can try finishing up my prototype of the declarative macro idea this weekend. I think it is the better of the two ideas, but still likely to be rejected because it is a major change and this is my first PR to this project. Perhaps it could at least spark some discussion about how this could be accomplished in the future though.

[jbis9051]

Thank you for the update. I can take a closer look soon, but to avoid breaking changes can we create an identical trait to Formatter, deprecate Formatter, then impl <T: Formatter> WipFormat for T gated behind std? Similar to how you handled io::Write. Formatter can be removed in a future version.

This doesn't solve the code duplication but if the change isn't breaking, maintainer might be more open to accepting the PR.