Stream: rustdoc

Topic: Removing doctree stuff


view this post on Zulip CraftSpider (Jan 14 2021 at 18:48):

Is anyone currently working on removing doctree::Import?

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

@Rune Tynan not that I know of :)

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

it might be tricky though - I think Import is used for inlining

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

It looks like it is, but also that the inlining part of the clean implementation doesn't use anything not easily available from the Item or its Kind

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

gotcha - you can remove doctree::Import without changing much code in visit_ast or clean

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

sounds perfect :)

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

Yeah. I'm running tests now (I actually started doing the work, then thought 'wait I should ask if this is necessary'), I basically just ripped out Import, replaced impl Clean with clean_use_statement, and made Item {...} into Item::from_def_id_and_parts

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

I got what looks like an unrelated failure from issue-80893.rs, about 'failed to spawn rustc process: The system cannot find the file specified'

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

Other than that, rustdoc-ui and rustdoc suites pass

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

Rune Tynan said:

I got what looks like an unrelated failure from issue-80893.rs, about 'failed to spawn rustc process: The system cannot find the file specified'

hmm, is that a doctest or something?

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

// compile-flags: --test -Z unstable-options --test-builder true

/// ```no_run
/// This tests that `--test-builder` is accepted as a flag by rustdoc.
/// ```
pub struct Foo;

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

oh lol I don't think true exists on windows

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

I would just ignore that error, or if you have time find some innocuous command that's on both windows and linux

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

https://github.com/rust-lang/rust/issues/81021 opened

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

doctree::Variant looks like the easiest thing to remove: 1 use in all places, impl Clean for Variant, never constructed or referenced

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:22):

I really wish the compiler would warn about unused impls

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:22):

I have a feeling there's a ton floating around rustdoc

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

Staples: that was easy
https://github.com/rust-lang/rust/pull/81023

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:25):

I got in trouble on a final project once for saying that lol

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:25):

teacher called me a smartass but gave me a 100 anyway :laughing:

view this post on Zulip CraftSpider (Jan 14 2021 at 20:25):

Hahaha

view this post on Zulip CraftSpider (Jan 14 2021 at 20:30):

The StructType enum is used in clean types, it and its associated function look like it might be worth to just move them into clean/types.rs. And maybe remove the duplicate type in the json rendering module

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:30):

I would prefer to keep the JSON stuff for now

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:30):

it's buggy enough without tying it to the rest of rustdoc

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

but yeah, +1 to moving things from doctree to clean

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

Alright, so all the JSON stuff getting cleaned up is its own whole project

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

well, getting it to work at all is a project :laughing:

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

it ICEs half the time right now

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

https://github.com/rust-lang/rust/issues/80664

view this post on Zulip CraftSpider (Jan 14 2021 at 20:32):

Sounds like a Good Time™

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:36):

Joshua Nelson said:

it ICEs half the time right now

speaking of which https://github.com/rust-lang/rust/pull/81021#issuecomment-760456663

view this post on Zulip CraftSpider (Jan 14 2021 at 20:37):

Looking at that error, and the code it errored on, it looks like that code expects python3 but was run with python2

view this post on Zulip CraftSpider (Jan 14 2021 at 20:38):

super() is a python3 construct, for python2 you use super(ClsName, instance)

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:38):

... why would it only fail now?

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:38):

maybe it's never hit this code before

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:38):

dynamic languages are so annoying

view this post on Zulip CraftSpider (Jan 14 2021 at 20:40):

Yeah, it looks like this code is only hit in the error path

view this post on Zulip CraftSpider (Jan 14 2021 at 20:41):

So there was a legitimate error, but we can't see it because it triggered an unrelated error

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:41):

@Rune Tynan if you have python3 locally you should be able to replicate the error

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:41):

x.py test src/test/rustdoc-json

view this post on Zulip CraftSpider (Jan 14 2021 at 20:42):

Yep, trying it now

view this post on Zulip CraftSpider (Jan 14 2021 at 20:42):

Well. It just ran and both tests passed

view this post on Zulip CraftSpider (Jan 14 2021 at 20:42):

Wait! Wrong branch

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:42):

try with python2?

view this post on Zulip CraftSpider (Jan 14 2021 at 20:44):

Running on the right branch with python3.6, and behind door number one is... (waiting for librustdoc to build again)

view this post on Zulip CraftSpider (Jan 14 2021 at 20:44):

__main__.SubsetException: ['inner', 'items', '0:3', 'inner', 'items', '0:7', 'inner']: Key glob not found in output

view this post on Zulip CraftSpider (Jan 14 2021 at 20:44):

Hmmm, guess it's time for me to look through the json code to understand what changed

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:45):

hmm that's weird, you didn't mess with the JSON at all

view this post on Zulip CraftSpider (Jan 14 2021 at 20:46):

I changed Item {...} into Item::from_def_id_and_parts(...), I wonder if one of the values changed. You'd think that would change the HTML tests somehow, but maybe it's subtle / only the JSON cares??

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:47):

I would see what the JSON output was before and after your change

view this post on Zulip Joshua Nelson (Jan 14 2021 at 20:48):

(shouldn't need to recompile btw - I would expect latest nightly to have the same output)

view this post on Zulip CraftSpider (Jan 14 2021 at 20:52):

If I'm reading this trace right, the key missing is "glob": false on line 137 of nested.expected

view this post on Zulip CraftSpider (Jan 14 2021 at 21:02):

Yeah, I can't see why this would have changed, except that I somehow changed the kind of the item

view this post on Zulip Joshua Nelson (Jan 14 2021 at 21:03):

seems weird that the kind would have changed

view this post on Zulip Joshua Nelson (Jan 14 2021 at 21:03):

if you revert the from_def_id... call, does it fix it?

view this post on Zulip Joshua Nelson (Jan 14 2021 at 21:03):

if so you can run assert_eq! to see exactly what changed

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

That didn't seem to fix it, so I'm stumped. I also tried playing with using renamed.unwrap_or(item.ident.name) instead of what the generic impl does, and that didn't fix it either

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

hmm, let me look at the chnages again

view this post on Zulip CraftSpider (Jan 14 2021 at 21:18):

Please do, I think I've hit the 'I wrote it and I can't see the error' part :P

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

oh don't worry, this happens every time I try to fix up clean lol

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

@Rune Tynan can you revert the from_def_id change and push it to a branch somewhere so the diff is smaller?

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

even if it doesn't fix the error it will be easier to read

view this post on Zulip CraftSpider (Jan 14 2021 at 21:24):

https://github.com/CraftSpider/rust/tree/rustdoc-temp
Here's the branch, minus the from_def_id_and_parts changes

view this post on Zulip CraftSpider (Jan 14 2021 at 21:24):

Still errors, but may be easier to debug :P

view this post on Zulip Joshua Nelson (Jan 14 2021 at 21:24):

thanks :)

view this post on Zulip Joshua Nelson (Jan 14 2021 at 22:25):

@Rune Tynan so the main difference I saw is that the new generated JSON has way more items than the old one

> jq . /home/joshua/rustc/src/test/rustdoc-json/nested.expected | diff - <(jq . /home/joshua/rustc/build/x86_64-unknown-linux-gnu/test/rustdoc-json/nested/nested.json) | grep '^>' | wc -l
20880

view this post on Zulip Joshua Nelson (Jan 14 2021 at 22:25):

so it's possible that the tests are just broken

view this post on Zulip CraftSpider (Jan 14 2021 at 22:52):

Interesting. Also, looking at the actual json output, I notice that ["0:7"]["inner"]["glob"] exists, and is false

view this post on Zulip CraftSpider (Jan 14 2021 at 22:56):

Actually @Joshua Nelson From what I can tell, the .expected is not a full copy of the expected output, but a strict subset

view this post on Zulip CraftSpider (Jan 14 2021 at 22:56):

As the test method is called 'check_subset'

view this post on Zulip CraftSpider (Jan 14 2021 at 22:56):

And appears to only check that for item in expected, item matches in actual, not the other way around

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

I ran old vs new through a checker, and there is exactly one diff found:
image.png

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

(That image is backwards, new is on left)

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

I literally swap that line in expected, and... it works! Yay for error messages that didn't convey the change that occured

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

I think the position change is just because use items are now added to the final vec in the same order they are fed to rustdoc, not processed before other items

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

ok, so just a bug in the test suite then

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

maybe sort the items before checking them?

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

oh I see you updated the .expected file, that works too. Can you add a comment to the test suite for the next poor soul?

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

Does this sound good?

# WARNING: The error messages produced by this may be misleading, in the case of list re-ordering it may
#          point to apparently unrelated keys.

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

looks great, thanks :)

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

All this JSON stuff is making me wonder about how it could be improved, either the tests or the JSON rendering itself. I wonder if there could be something like jsondocck.py, which would interpret @has ... comments into JSON paths, and allow things like 'in this list, but I don't care where'

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

that would be pretty cool :)


Last updated: Oct 11 2021 at 22:34 UTC