WIP: Capture dep info with Spanned

[gilescope]

Here we add Spanned info into dependencies. Nothing is done with the information, but this seems like a good first step towards line numbers.

[rust-highfive]

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

[gilescope]

@est31 should I be using toml-spanned-value crate rather than the toml crate or should the toml Spanned type be enough?

I'm getting invalid type: string "registry", expected a borrowed string for key errors which I don't quite understand.

[est31]

toml-spanned-value helps when you were dealing with Value. Otherwise the Spanned<> type would be enough. In fact, the SpannedValue type from toml-spanned-value is just a type alias for Spanned<Value>. However, that Value is not the Value from the toml crate, but a copy with SpannedValue being used instead of Value in e.g. arrays, so that you have spans available deeply nested in the datastructure.

From what I can recall, cargo mainly does its own types and doesn't use Value, so utility of toml-spanned-value might be limited. It's been some time since I last worked with toml-spanned-value, but I remember that there had to be some hacks to get spans through serde's limited data model. Maybe your error is related to those?

[bors]

☔ The latest upstream changes (presumably #9793) made this pull request unmergeable. Please resolve the merge conflicts.

[est31]

Mhh I wonder if it is good to cover everything in Spanned<_>. Maybe instead do use <path>::Spanned as S; and then do S<Thing>? Covering everything in that seems better to me.

[gilescope]

use <path>::Spanned? Sorry I don’t quite get it. I agree I think keys should stay as str. Just values being spanned is enough. Though serde + serde_ignored is all quite tricky figuring out what needs changing to allow it to parse. There’s a bit where we pass a string visitor rather than a spanned visitor and the wheels fall off.

…

On Thu, 19 Aug 2021 at 23:11, est31 ***@***.***> wrote: Mhh I wonder if it is good to spam everything in Spanned<_>. Maybe instead do use <path>::Spanned as S; and then do Spanned<Thing>? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9752 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGEJCC6ZAKO2DU7E7TDPFDT5VXPLANCNFSM5BK7DHQA> .

[est31]

@gilescope I meant use toml::Spanned as S;. In Rust you can rename imports. Generally I'm not a fan of it but when it greatly simplifies code, I think it's a good idea.

[gilescope]

May as well go the whole hog and define something like this:
type DepMap = BTreeMap<String, Spanned<TomlDependency>>;

My main problem is that when Spanned is added, serde for some reason tries to use Serde's default &str deserialiser which is hardcoded to Err:

fn visit_str<E>(self, v: &str) -> Result<Self::Value, E> { Err(...) }

and that seems to be because of this in serde:

impl<'de: 'a, 'a> Deserialize<'de> for &'a str {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        deserializer.deserialize_str(StrVisitor) // <-- This visitor seems hardcoded to Err on &str.
    }
}

The interactions between serde, serde-ignored and toml-rs deserialisers are non-trivial.

[est31]

Oh yeah serde is really nice for the simple use cases but it has its limits. Getting spanned information through serde happens through a bunch of hacks. Maybe those fail if serde-ignored is used?

[gilescope]

Yeah. Not sure what to do here. I think @dtolnay is probably the only one who could possibly pinpoint how to solve it. I've tried but my brain just can't grok what in the 3 tiers of serdes I need to change in order to fix it.

I could try taking out serde-ignored to try and simplify the problem but I'm going to have to put it back again...

[ehuss]

I'm going to close this for now since cargo has switched to toml_edit.
Pending toml-rs/toml#302, this might be something we could revisit in the future.

[epage]

FYI toml-rs/toml#435 adds span support in to toml_edit