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

Add deserialization helpers for map and sequence types #749

Closed
wants to merge 5 commits into from

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Feb 4, 2017

The motivating case for this is that https://github.com/gluon-lang/languageserver-types has a type which contains a HashMap<Url, SomeType> but Url can't be deserialized directly with serde 0.9, it is instead provided by functions and wrappers in a separate crate.

#553 mentions that this should perhaps be in another crate but since this PR simply reuses the normal sequence and map deserializers I'd argue it is a good idea to just provide them from serde directly.

gluon-lang/lsp-types@88e2565#diff-b4aea3e418ccdb71239b96952d9cddb6R335
#553

@oli-obk
Copy link
Member

oli-obk commented Feb 4, 2017

can you add some test cases which use the functions with something other than |v| v (and a type that doesn't impl Deserialize)

@Marwes
Copy link
Contributor Author

Marwes commented Feb 4, 2017

@oli-obk Done. Also noticed that I made a copy-paste mistake in e250e21 which wasn't caught by the tests.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and I completely agree with the idea of making collections with non-Deserialize contents easier to deserialize.

I think I would prefer to handle this using attributes:

#[serde(deserialize_keys_with = "url_serde::deserialize")]
HashMap<Url, Vec<TextEdit>>

Some of the reasons are:

  • Better symmetry between ser and de.
  • I expect non-Deserialize types in both key and value at the same time to be relatively rare.
  • Significantly simpler code for your use case - a single line instead of an entire function.
  • Easier to reason about.
  • User is not responsible for handling with_capacity correctly.
  • Better reusability of helper libraries like url_serde, hyper_serde, and Helper library of useful de/serialize_with functions #553.
  • Assuming you have used deserialize_with, you immediately intuitively know how to use deserialize_values_with.
  • If you are using a non-Deserialize type inside a map, you are probably also using it outside a map so you already have a deserialize_with function lying around.
  • The signature of your deserialize_seq and especially deserialize_map is a hell of a thing to grok; attributes accomplish the same thing but without any increase in complexity over the existing deserialize_with.

) => {
/// A visitor that produces a sequence.
pub struct $visitor_ty<$($typaram),*> {
Copy link
Member

Choose a reason for hiding this comment

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

$with_capacity:expr
) => {
/// A visitor that produces a map.
pub struct $visitor_ty<$($typaram),*> {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't realize that these types were public :/.

@dtolnay
Copy link
Member

dtolnay commented Feb 4, 2017

@Marwes let me know what you think of the alternative plan and whether you would like to try implementing it.

@Marwes
Copy link
Contributor Author

Marwes commented Feb 5, 2017

I think I would prefer to handle this using attributes:

That was actually my original idea but I rejected it because I thought it would incur to much complexity for a fairly niche use case. I think I have an idea how to implement it using an attribute but I am unsure how to debug the generated code (generated from test_annotations.rs). I remembered reading this post https://quodlibetor.github.io/posts/debugging-rusts-new-custom-derive-system/ but trying to compile the code generated by cargo expand gives a ton of name resolution errors. I assume there is a better way of debugging the generated code?

@dtolnay
Copy link
Member

dtolnay commented Feb 5, 2017

What I have been doing is handwrite what I would like the generated code to look like in a separate tiny crate that I can compile, then iterate on on the macro code until the output from cargo expand matches my handwritten one. As you saw, the generated code that gets printed is a lossy form of what is actually generated, because of hygiene and other issues, so compiling it typically doesn't work.

@dtolnay
Copy link
Member

dtolnay commented Feb 8, 2017

I opened #753 to track the preferred approach.

@dtolnay dtolnay closed this Feb 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants