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

Ability to configure Deserializer to enable extensions when parsing #281

Closed
Boscop opened this issue Nov 9, 2020 · 7 comments
Closed

Ability to configure Deserializer to enable extensions when parsing #281

Boscop opened this issue Nov 9, 2020 · 7 comments

Comments

@Boscop
Copy link

Boscop commented Nov 9, 2020

Problem: I want #![enable(unwrap_newtypes, implicit_some)] to be the default, and don't want my users to require to write it in every RON file.

Current workaround: Inject that line before parsing. It's suboptimal because I can't use BufReader and if there's a parsing error, the reported line number will be wrong from the user's POV.

Proposed Solution: Ability to configure Deserializer to enable extensions before parsing.

@kvark
Copy link
Collaborator

kvark commented Nov 14, 2020

That seems plausible.

@toyboot4e
Copy link

toyboot4e commented Oct 19, 2021

I'd like this feature if #319 comes!

Just exposing Extensions might be enough:

impl<'de> Deserializer<'de> {
    pub fn exts(&self) -> &Extensions {
        &self.bytes.exts
    }

    pub fn exts_mut(&mut self) -> &mut Extensions {
        &mut self.bytes.exts
    }
}

@torkleyy
Copy link
Contributor

I do understand why this is needed, but at the same time I don't like that, with this implemented, RON files can't be read by themselves; you need to know what extensions they are parsed with.

@kvark
Copy link
Collaborator

kvark commented Oct 19, 2021

We have 2 models here: the original typed model, where you need to know the types being deserialized, and the value model, where RON is read opaquely. Having this feature seems to be in line with the typed model, but will block the value model.

@kvark
Copy link
Collaborator

kvark commented Oct 19, 2021

Maybe files that rely on some extensions by default should just be named differently as a convention? like .ronx

@torkleyy
Copy link
Contributor

torkleyy commented Nov 5, 2021

I've been thinking about this issue a lot and my own conclusion is:

What are RON extensions?

RON extensions, right now, are deserialization options that are configured together with the deserialized data (in the .ron file). They do not influence parsing of RON - the syntax always stays the same.

Considerations

Knowing a Rust type, one might have certain expectations about how to write correct RON for it.

However, that's not all there is to it: What RON is correct and what isn't is completely controlled by what deserialization code serde comes up with. If I were to use a different deserialization library I might have to write different RON for it (e.g. it might not accept a RON tuple when a fixed-size array is expected).

Now that means that we have two options to control deserialization:

  1. Configuring what the RON deserializer tells serde, in terms of serde's type model (RON -> serde, RON extensions)
  2. Configuring how serde transforms "serde data" to Rust structures (serde -> Rust, serde attributes)

As it turns out, there are many options in serde to control deserialization (https://serde.rs/attributes.html). Now, does that mean RON extensions are unnecessary because we can achieve the same thing with serde? Some examples:

implicit_some

Could be replaced by serde attributes by implementing a custom visitor which visits deserialize_any and forwards deserialization to the type wrapped in Option. I think it would be possible to implement a generic module which can be used like this:

#[derive(Deserialize)]
struct Foo {
    #[serde(with = "implicit_some")]
    x: Option<i32>
}

other extensions

I was gonna look at each extension, but it seems that the same applies all the other extensions; they could be replaced in a similar fashion.

Are serde attributes a true replacement?

They are not the same. Some differences:

  • Requires access to the code with the derive
  • Possibly tedious to add all those attributes
  • No longer can the author of the RON file choose for themselves how explicit they want to be in terms of options, newtypes, etc (the developer makes the decision for them)

Conclusion

With all of that in mind, and because it is not a change in syntax it does seem fine to me to configure the deserializer to behave differently (enable extensions from Rust).

However, contrary to serde attributes, there is one potential disadvantage: One must take care to configure the same extensions for deserialization and serialization, otherwise roundtripping will not work.

Unresolved questions

Should the extension syntax be deprecated if we choose to implement this? If yes, RON files can no longer use different deserialization options.


Please, share your opinions on this :)

@juntyr
Copy link
Member

juntyr commented Dec 3, 2021

This is now (#343) possible with Options::default().with_default_extensions(<Extensions>).from_str("RON")

@juntyr juntyr closed this as completed Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants