Stream: rustdoc

Topic: jsondocck


view this post on Zulip CraftSpider (Jan 15 2021 at 18:36):

So, looking around, there already exists both a JSONPath idea, and an implementation of it in Python:
https://goessner.net/articles/JsonPath/
https://pypi.org/project/jsonpath-ng/

The licensing is Apache 2.0, I'm not sure what the rules are for pip dependencies / if the source would need to be copied in somewhere, but it would allow parsing JSON output with the same kind of directives as HTML has, and in a way where index only matters if we want it to

view this post on Zulip Joshua Nelson (Jan 15 2021 at 18:47):

I don't know the rules for dependencies either. I don't see any requirements.txt files which may mean no one has tried to do this before.

@simulacrum how do you feel about adding a python dependency to src/test/rustdoc-json?

view this post on Zulip simulacrum (Jan 15 2021 at 18:50):

I don't know anything about that folder, but I feel pretty unhappy I think. Python is an easier sell than node, because it's already required for LLVM and x.py, but both of those just need the standard installation.

I'm pretty sure I've seen rust crates providing a jq-like interface, though. Maybe we can use those?

view this post on Zulip Joshua Nelson (Jan 15 2021 at 18:51):

sounds good to me :) @Rune Tynan do you mind looking for ways to do this in rust instead?

view this post on Zulip simulacrum (Jan 15 2021 at 18:51):

I would also ask - do we really need this for testing? It might be worth talking to @matklad about what rust-analyzer does - I imagine there's a similar need there.

view this post on Zulip Joshua Nelson (Jan 15 2021 at 18:51):

the current testing situation is really painful

view this post on Zulip Joshua Nelson (Jan 15 2021 at 18:52):

it took me and Rune Tynan several hours to debug a recent failure: https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Removing.20doctree.20stuff/near/222780199

view this post on Zulip Joshua Nelson (Jan 15 2021 at 18:52):

and I would like to get away from contains.py if at all possible, I don't think it's really testing the right thing anyway

view this post on Zulip CraftSpider (Jan 15 2021 at 18:53):

It looks like there is a jsonpath Rust crate, but also that it's fairly young/limited, I'll see if it's enough for what we want

view this post on Zulip Joshua Nelson (Jan 15 2021 at 18:55):

hmm, that was last released 2 years ago which makes me a little nervous

view this post on Zulip Joshua Nelson (Jan 15 2021 at 18:55):

https://github.com/freestrings/jsonpath looks better maintained

view this post on Zulip CraftSpider (Jan 15 2021 at 18:57):

Yep, that's what I was just looking at

view this post on Zulip Joshua Nelson (Jan 15 2021 at 18:58):

ah ok, I was looking at https://docs.rs/jsonpath/0.1.1/jsonpath/ originally

view this post on Zulip CraftSpider (Jan 15 2021 at 18:58):

Oh, I was looking at that originally too. I had the same 'Hm, 2 years makes me nervous', one of the issues linked to a comparison page, that linked to the freestrings one

view this post on Zulip CraftSpider (Jan 15 2021 at 18:59):

So now I was looking at the freestrings one, and it looks both recently maintained, and to have much better feature support. It appears to accept either a serde Value or a string, so we have options too :P

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:00):

nice, that sounds good to me

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:00):

(weird that they chose jsonpath_lib as the crate name, but doesn't matter a lot)

view this post on Zulip simulacrum (Jan 15 2021 at 19:02):

To be clear, I'd be totally fine with rolling our own small thing, too - I imagine rustdoc's needs may be somewhat unique.

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:02):

rolling our own would be a lot more work than using the library I expect

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:02):

this is less "be a test suite" and more "I need an XPATH library" (but for JSON)

view this post on Zulip simulacrum (Jan 15 2021 at 19:02):

Yeah, if there's something, definitely let's use it.

view this post on Zulip CraftSpider (Jan 15 2021 at 19:03):

jsonpath was released first, so I imagine jsonpath_lib because the other was taken

view this post on Zulip simulacrum (Jan 15 2021 at 19:03):

FWIW I think asking people running this test suite to install jq (cli library providing this) would be fine

view this post on Zulip simulacrum (Jan 15 2021 at 19:04):

It also looks like there's several wrappers for libjq out there which may be even better

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:05):

libjq is a C library? that sounds more painful than either 'install jq' or a rust dependency

view this post on Zulip simulacrum (Jan 15 2021 at 19:05):

Well, I mean rust wrappers

view this post on Zulip simulacrum (Jan 15 2021 at 19:05):

So they probably just compile it internally

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:06):

sure - I think we'll only need a small subset of jq's features though

view this post on Zulip simulacrum (Jan 15 2021 at 19:06):

But the CLI tool should be fine for us

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:06):

jq is a whole programming language really lol

view this post on Zulip simulacrum (Jan 15 2021 at 19:07):

https://github.com/dflemstr/rq

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:07):

I think we have enough tools :laughing: but I appreciate the help

view this post on Zulip simulacrum (Jan 15 2021 at 19:07):

That feels like the thing to use

view this post on Zulip CraftSpider (Jan 15 2021 at 19:08):

I think jsonpath works fine for our use cases, and has slightly less 'overhead' to it, but I'm fine either way. jq has at least as many features, I just went with the first option that fit the need :P

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:08):

let's go with jsonpath for now

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:08):

it uses the same syntax as jq, right?

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:08):

that would make it pretty easy to switch later if we need to

view this post on Zulip CraftSpider (Jan 15 2021 at 19:09):

Not quite the same, but similar. The jq wiki has a page on conversion, it looks easy to do

view this post on Zulip CraftSpider (Jan 15 2021 at 19:14):

Alright, so the actual test is invoked by compiletest. Would I add stuff there, or somewhere else?

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:15):

compiletest is just running compare.py, right? I would look how this works for tidy, which is also written in rust

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:16):

the idea is you want rustbuild to compile the checker before calling compiletest

view this post on Zulip CraftSpider (Jan 15 2021 at 19:47):

I'm playing the 'add things and see what breaks' method of learning what needs to change to make this work :P

view this post on Zulip Joshua Nelson (Jan 15 2021 at 19:47):

me every time I try to change bootstrap lol

view this post on Zulip CraftSpider (Jan 15 2021 at 19:59):

One issue is that there are currently two tests with a whole bunch of expected JSON, but the smart thing would be many smaller tests that assert specific parts of it. EG

pub struct PlainEmpty {}

pub struct Tuple(u32, String);

pub struct Unit;

pub struct WithPrimitives<'a> {
    num: u32,
    s: &'a str,
}

pub struct WithGenerics<T, U> {
    stuff: Vec<T>,
    things: HashMap<U, U>,
}

This should be at least one file per struct, as the implication (No comments + short name = I have to guess the intent) is that it's just ensuring the structs document correctly.

view this post on Zulip Joshua Nelson (Jan 15 2021 at 20:00):

sounds good to me - I think @pineapple wrote up his reasoning on the original PR if I can find it

view this post on Zulip Joshua Nelson (Jan 15 2021 at 20:00):

https://github.com/rust-lang/rust/pull/75114#discussion_r469426829

view this post on Zulip CraftSpider (Jan 15 2021 at 20:04):

Awesome, thank you

view this post on Zulip CraftSpider (Jan 15 2021 at 20:24):

The sudden temptation to merge htmldocck into this tool, to reduce duplication of rewriting parts of it in Rust

view this post on Zulip CraftSpider (Jan 15 2021 at 20:31):

thread 'main' panicked at 'not yet implemented', src\tools\jsondocck\src\main.rs

Yay! Now to actually implement the commands

view this post on Zulip Joshua Nelson (Jan 15 2021 at 20:31):

Rune Tynan said:

The sudden temptation to merge htmldocck into this tool, to reduce duplication of rewriting parts of it in Rust

FWIW once this is working I would be ok with extending it to replace htmldocck

view this post on Zulip Joshua Nelson (Jan 15 2021 at 20:32):

that seems low-priority though, htmldocck is pretty reliable

view this post on Zulip CraftSpider (Jan 15 2021 at 20:39):

Yeah, the only reason I said that is just that this basically has the exact same get_commands code, inputs, etc. So it hit the DRY part of my brain. But it's not what I'm focused on right now, I'd rather just get the json part done, then change it in the future.

view this post on Zulip CraftSpider (Jan 15 2021 at 20:48):

Question: Is using unstable features allowed? I ask solely for Option::contains, so I'm fine if the answer is no, I'll just handle it inline / make a helper

view this post on Zulip Joshua Nelson (Jan 15 2021 at 20:49):

@Rune Tynan preferably not, bootstrap tools are compiled with beta and I'd like not to add more RUSTC_BOOTSTRAP special casing

view this post on Zulip Joshua Nelson (Jan 15 2021 at 20:52):

see also https://github.com/rust-lang/rust/pull/76423

view this post on Zulip CraftSpider (Jan 15 2021 at 21:00):

Makes sense. I'll just use it inline, or make a helper if it looks cleaner that way.

view this post on Zulip CraftSpider (Jan 15 2021 at 21:37):

The python version of the regex uses lookbehind, which the regex crate doesn't support. It looks like it is basically equivalent to just including a single whitespace before the rest of the regex, so I'm doing that

view this post on Zulip CraftSpider (Jan 15 2021 at 21:39):

How do I force the test to re-run, don't ignore unchanged tests?

view this post on Zulip Joshua Nelson (Jan 15 2021 at 21:39):

touch test.rs should work

view this post on Zulip Joshua Nelson (Jan 15 2021 at 21:39):

there may be a flag but touch is usually easier

view this post on Zulip CraftSpider (Jan 15 2021 at 21:52):

[
Command { negated: false, cmd: Has, args: ["nested.json", "$.[0:3]"], lineno: 4 },
Command { negated: false, cmd: Has, args: ["nested.json", "$.[0:7].inner.glob", "false"], lineno: 7 }
]

We're getting there

view this post on Zulip CraftSpider (Jan 15 2021 at 23:14):

image.png

view this post on Zulip CraftSpider (Jan 15 2021 at 23:14):

This is running the new jsondocck, not the old compare, with two directives added, and it fails if I swap their polarity or such!

view this post on Zulip Joshua Nelson (Jan 15 2021 at 23:14):

that's awesome! :D

view this post on Zulip Joshua Nelson (Jan 15 2021 at 23:15):

thank you so much for working on this :heart:

view this post on Zulip CraftSpider (Jan 15 2021 at 23:16):

No problem! Its been fun to work on.

view this post on Zulip CraftSpider (Jan 15 2021 at 23:17):

What's left:
'count' command, maybe something like 'matches', and code cleanup

view this post on Zulip CraftSpider (Jan 15 2021 at 23:18):

The whole 0:3 system of identifiers is gonna be interesting, that's hard to not hardcode and if I understand DefId's right, will change any time an item is added

view this post on Zulip Joshua Nelson (Jan 15 2021 at 23:18):

yes, DefIds are not at all stable unfortunately

view this post on Zulip CraftSpider (Jan 15 2021 at 23:19):

That's more a JSON format thing, if I remember the RFC those identifiers are explicitly allowed to change at rustdoc's discretion, so maybe there's a better alternative in the future. But that's outside the scope of just better testing :P

view this post on Zulip Joshua Nelson (Jan 15 2021 at 23:20):

Rune Tynan said:

That's more a JSON format thing, if I remember the RFC those identifiers are explicitly allowed to change at rustdoc's discretion, so maybe there's a better alternative in the future. But that's outside the scope of just better testing :P

if you're interested in this, take a look at https://github.com/rust-lang/rust/issues/80664#issuecomment-753681341

view this post on Zulip CraftSpider (Jan 15 2021 at 23:44):

image.png
Thoughts on this output format? (with line numbers / context)

view this post on Zulip Joshua Nelson (Jan 15 2021 at 23:46):

seems fine - the context is the most important IMO

view this post on Zulip Joshua Nelson (Jan 15 2021 at 23:46):

it would be really nice if it could say where it failed

view this post on Zulip Joshua Nelson (Jan 15 2021 at 23:46):

like for .x.y.z say 'y' isn't found instead of '.x.y.z did not match'

view this post on Zulip CraftSpider (Jan 15 2021 at 23:50):

I'll see if the jsonpath lib gives me that info, if so it should be easy enough to add. If not I'll consider, may make the sad-path slow but informative by making it 'peel back' each subpath and try again?

view this post on Zulip Joshua Nelson (Jan 15 2021 at 23:55):

sounds great! yeah, that's how intra-doc links does error reporting

view this post on Zulip CraftSpider (Jan 16 2021 at 00:11):

Oh, also, would you know why I keep getting a panic 'need path to cargo' from tidy when I run tests? The test suite keeps going, but it fills up output

view this post on Zulip CraftSpider (Jan 16 2021 at 00:15):

Looking around, I think the has_tidy check in compiletest is doing it. It silences stdout but not stderr

view this post on Zulip CraftSpider (Jan 16 2021 at 01:25):

Working on splitting up the tests better now

view this post on Zulip CraftSpider (Jan 16 2021 at 01:39):

https://github.com/rust-lang/rust/pull/81063

view this post on Zulip CraftSpider (Jan 16 2021 at 01:39):

Ready for the first round of reviews :tada:

view this post on Zulip Joshua Nelson (Jan 16 2021 at 17:36):

left another review, it's on the right track :)

view this post on Zulip CraftSpider (Jan 16 2021 at 21:37):

I've gone through and left comments/reasoning on a couple


Last updated: Oct 11 2021 at 22:34 UTC