Support spans when deserializing serde structures #239

udoprog · 2018-05-07T02:14:17Z

This PR introduces Spanned based on the previous POC by @dtolnay.

Internally we deserialize spans as "intermediate structs" with special naming conventions, which communicates to the Deserializer that it should emit a Map with similarly special names that we can pick up.

This PR contains a couple of changes:

Introduces a Spanned<T> type that can be used in structs to encapsulate spans.
Unified the naming convention of constants for datetime and spanned.
Added eat_spanned and expect_spanned to Tokenizer to correctly parse tables and arrays.

I'm unsure it is complete, the infrastructure needed to support Datetime permeates more of the deserialize infrastructure, but I haven't been able to decipher why yet. Any feedback is appreciated.

Future work

Ironically this doesn't help me with my original problem, but it's a fair step down the road. I use toml::Value in a similar way that Cargo uses it to permit more flexible decoding than is afforded through databinding. Introducing spans into the existing toml::Value would not be a backwards compatible change, so the only alternative I see is to have separate types (e.g. toml::SpannedValue) which contains the necessary spans. Suggestions are welcome.

udoprog · 2018-05-07T02:16:41Z

Note: I'm also pushing it through the build system to check for 1.15 compatibility. Initial builds are probably gonna break.

dtolnay · 2018-05-07T02:26:17Z

Would it be possible to support spans in a map key? BTreeMap<Spanned<String>, Spanned<String>>

udoprog · 2018-05-07T02:36:02Z

@dtolnay I was fiddling with some tests for cases which currently does not work. Pushed them out in dffb952.

dtolnay · 2018-05-07T02:27:44Z

Cargo.toml

@@ -20,7 +20,7 @@ travis-ci = { repository = "alexcrichton/toml-rs" }

 [dependencies]
 serde = "1.0"
+serde_derive = "1.0"


Looks like you wrote out the Deserialize impl so this is no longer used.

Fixed, thanks!

dtolnay · 2018-05-07T02:41:19Z

src/spanned.rs

+            {
+                let start = visitor.next_key::<StartKey>()?;
+
+                if start.is_none() {


I would recommend cutting out the key_deserialize macro and using:

if visitor.next_key()? != Some(START) {

Fixed, thanks!

dtolnay · 2018-05-07T02:42:34Z

src/de.rs

+        K: de::DeserializeSeed<'de>,
+    {
+        if self.start.is_some() {
+            seed.deserialize(spanned::START.into_deserializer()).map(Some)


This needs to be:

use serde::de::value::BorrowedStrDeserializer; seed.deserialize(BorrowedStrDeserializer::new(spanned::START)).map(Some)

in order for my other comment to work.

Fixed, thanks!

udoprog · 2018-05-07T02:52:10Z

Regarding BTreeMap<Spanned<String>, Spanned<String>>, what's the identity of Spanned<T>? If it's transitive to T, two differently spanned T's would override each other as expected. On the other hand, two differently spanned Ts aren't equal.

Generally I address this by distinguishing between the key of a structure and its span, like: BTreeMap<String, (Span, T)>. But that isn't really an option here.

dtolnay · 2018-05-07T02:58:28Z

I would expect Map<Spanned<K>, V> to behave exactly like Map<K, V> just with more information captured during deserialization.

dtolnay · 2018-05-07T03:11:14Z

test-suite/tests/spanned.rs

+        "foo = [0, 1, 2, 3, 4]",
+        "[0, 1, 2, 3, 4]"
+    );
+    // datetime


I tried an actual Datetime but it seems to get the wrong span. Is this a tokenizer issue?

good::<Datetime>("foo = 1997-09-09T09:09:09Z", "1997-09-09T09:09:09Z");

---- test_spanned_field stdout ---- thread 'test_spanned_field' panicked at 'assertion failed: `(left == right)` left: `26`, right: `19`', tests/spanned.rs:20:9

Good catch! Probably a case of the deserializer not combining the spans. will check it out.

Fixed in 2b40d54, thanks!

udoprog · 2018-05-07T10:40:14Z

@dtolnay

I would expect Map<Spanned, V> to behave exactly like Map<K, V> just with more information captured during deserialization.

I half-agree with this, but it becomes awkward when trying to implement and use: https://play.rust-lang.org/?gist=5a444c2ed6fd2322594658920b9eef12&version=stable&mode=debug (possibly my impl-skills are not strong enough, so please take a stab at this).

But if I have a map which includes spans, I'd want to be able to:

Have a way to get/remove elements by a reference of the contained value in the key (&K in Map<Spanned<K>, V>).
Have a way to access the span of a given key, by a reference of the contained value in the key (get Span from &K in Map<Spanned<K>, V>).

Note: One approach that satisfies this is to use Map<K, (Span, V)>, where the first element in the tuple is the span. We could introduce a third type specialized for map-serialization to support this: Map<K, MapSpanned<V>>.

alexcrichton

Looks good to me, thanks so much!

alexcrichton · 2018-05-09T18:22:31Z

src/lib.rs

@@ -169,3 +169,7 @@ mod tokens;

 #[doc(hidden)]
 pub mod macros;
+
+pub mod spanned;


Could this module start out private and only export the Spanned type below?

Will do. Thanks.

alexcrichton · 2018-05-09T18:23:35Z

src/spanned.rs

+    /// The end range (exclusive).
+    pub end: usize,
+    /// The spanned value.
+    pub value: T,


Instead of public fields could these be accessed through methods perhaps? That way we could tweak the representation here later in the future and/or do other fancy things with struct layout

These are the set of methods I'm fiddling with right now:

impl<T> Spanned<T> { fn start(&self) -> usize { .. } fn end(&self) -> usize { .. } fn span(&self) -> (usize, usize) { .. } fn borrow(&self) -> &T { .. } fn take(self) -> T { .. } }

Got any opinions on this?

alexcrichton · 2018-05-09T19:42:26Z

src/spanned.rs

+    }
+
+    /// Take the inner value.
+    pub fn take(self) -> T {


For consistency with libstd, mind naming this into_inner? I think we may want to go ahead and add methods like get_ref and get_mut here as well

Done, thanks!

Proof of concept: reporting span information to a Spanned<T> type

fd87ce3

udoprog added 2 commits May 7, 2018 04:28

Use custom struct/field naming to deserialize spans

6ff5c44

Adjust spans for more complex types

6c06720

dtolnay reviewed May 7, 2018

View reviewed changes

Use BorrowedStrDeserializer instead of generated key deserializers

579cf49

dtolnay reviewed May 7, 2018

View reviewed changes

udoprog added 2 commits May 7, 2018 05:36

Fix incorrectly reported spans for Datetime

2b40d54

Reintroduce serde_derive as dev-dependencies

87916e9

alexcrichton reviewed May 9, 2018

View reviewed changes

Make spanned module private and hide internals

0a5fe3f

alexcrichton merged commit cb49d59 into toml-rs:master May 9, 2018

udoprog deleted the spans branch May 9, 2018 21:57

alexcrichton mentioned this pull request Oct 6, 2018

lib usage: manifest errors with manifest path & source position rust-lang/cargo#6144

Closed

est31 mentioned this pull request Aug 23, 2019

Support emitting spans when deserializing structures #236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support spans when deserializing serde structures #239

Support spans when deserializing serde structures #239

udoprog commented May 7, 2018 •

edited

Loading

udoprog commented May 7, 2018

dtolnay commented May 7, 2018

udoprog commented May 7, 2018

dtolnay May 7, 2018

udoprog May 7, 2018

dtolnay May 7, 2018

udoprog May 7, 2018

dtolnay May 7, 2018

udoprog May 7, 2018

udoprog commented May 7, 2018

dtolnay commented May 7, 2018

dtolnay May 7, 2018

udoprog May 7, 2018

udoprog May 7, 2018

udoprog commented May 7, 2018 •

edited

Loading

alexcrichton left a comment

alexcrichton May 9, 2018

udoprog May 9, 2018

alexcrichton May 9, 2018

udoprog May 9, 2018 •

edited

Loading

alexcrichton May 9, 2018

udoprog May 9, 2018

Support spans when deserializing serde structures #239

Support spans when deserializing serde structures #239

Conversation

udoprog commented May 7, 2018 • edited Loading

Future work

udoprog commented May 7, 2018

dtolnay commented May 7, 2018

udoprog commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

udoprog commented May 7, 2018

dtolnay commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

udoprog commented May 7, 2018 • edited Loading

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

udoprog May 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

udoprog commented May 7, 2018 •

edited

Loading

udoprog commented May 7, 2018 •

edited

Loading

udoprog May 9, 2018 •

edited

Loading