Stream: rustdoc

Topic: Cache paths


view this post on Zulip Joshua Nelson (Jul 02 2021 at 03:00):

https://github.com/rust-lang/rust/issues/86798 is probably causing lots of bugs :/ if someone has time it would be nice to look into

view this post on Zulip Stu (Jul 02 2021 at 04:38):

I can take a look :+1:

view this post on Zulip Stu (Jul 03 2021 at 18:55):

Okay this is going to be annoying and hard to fix. I nailed it down to 4-5 tests that are failing, but I can't figure it out right now.

view this post on Zulip Joshua Nelson (Jul 03 2021 at 18:57):

yeah that was about what I expected :laughing:

view this post on Zulip Stu (Jul 03 2021 at 18:58):

If I fix one test, another one is failing, if I fix that one, the other one is failing again :sad:

view this post on Zulip Joshua Nelson (Jul 03 2021 at 18:59):

what approach are you taking? merge everything into paths?

view this post on Zulip Stu (Jul 03 2021 at 18:59):

Yes. Everything is in one map right now.

view this post on Zulip Joshua Nelson (Jul 03 2021 at 19:02):

ok. and what tests are failing?

view this post on Zulip Stu (Jul 03 2021 at 19:02):

I can send you later. I'm working on another PR right now :+1:

view this post on Zulip Joshua Nelson (Jul 03 2021 at 19:03):

haha https://github.com/rust-lang/rust/pull/86644 is super useful too :)

view this post on Zulip Joshua Nelson (Jul 03 2021 at 19:03):

have you tried worktrees btw?

view this post on Zulip Joshua Nelson (Jul 03 2021 at 19:03):

https://rustc-dev-guide.rust-lang.org/building/suggested.html#working-on-multiple-branches-at-the-same-time

view this post on Zulip Stu (Jul 03 2021 at 19:04):

I kinda use them. Right now I have two worktrees but I need more haha

view this post on Zulip Joshua Nelson (Jul 03 2021 at 19:04):

since https://github.com/rust-lang/rust/pull/82653 it should be super cheap to add more :)

view this post on Zulip Joshua Nelson (Jul 03 2021 at 19:04):

err I guess that isn't merged yet

view this post on Zulip Joshua Nelson (Jul 03 2021 at 19:04):

but you can cherry-pick it

view this post on Zulip Stu (Jul 03 2021 at 19:05):

Thats pretty cool

view this post on Zulip Joshua Nelson (Jul 03 2021 at 19:09):

/me currently has 4 worktrees :upside_down:

but I haven't used the 4th in a while

view this post on Zulip Stu (Jul 03 2021 at 19:10):

Now I have 3 tmux windows, in every one is a separate rustc setup to work on haha

view this post on Zulip Stu (Jul 03 2021 at 19:20):

failures:
    [rustdoc] rustdoc/elided-lifetime.rs
    [rustdoc] rustdoc/hidden-impls.rs
    [rustdoc] rustdoc/intra-doc/cross-crate/submodule-outer.rs
    [rustdoc] rustdoc/sidebar-links-to-foreign-impl.rs

These are the failing tests. If I try to fix hidden-impls.rs, rustdoc/issue-43701.rs is apparently failing, as were they mutually exclusive

view this post on Zulip Stu (Jul 03 2021 at 19:21):

Both of these have to do with this line:

view this post on Zulip Stu (Jul 03 2021 at 19:23):

elided-lifetime.rs is about a missing link in the re-exported methods return type

view this post on Zulip Stu (Jul 03 2021 at 19:25):

I can open a WIP PR so you can see yourself. But I have a hard time figuring out why these are failing. Maybe you can find out faster

view this post on Zulip Joshua Nelson (Jul 03 2021 at 19:26):

probably not :sweat_smile: rustdoc is way too complicated

view this post on Zulip Joshua Nelson (Jul 03 2021 at 19:26):

at a guess, I would expect the issue to be a missing is_local() check somewhere?

view this post on Zulip Joshua Nelson (Jul 03 2021 at 19:26):

but I've never seen these tests before

view this post on Zulip GuillaumeGomez (Jul 04 2021 at 10:50):

Like @Joshua Nelson said mostly. If it can help: maybe we should make it into an enum so then it would be enforced by the type directly wether it's local or not. It'd require more changes but it will be impossible to forget a check. :)

view this post on Zulip GuillaumeGomez (Jul 04 2021 at 10:51):

(I mean a map of enum where paths are variants of the enum)

view this post on Zulip Noah Lev (Jul 04 2021 at 22:37):

In general, it feels like enums are the secret to improving rustdoc's code quality :)

view this post on Zulip Stu (Jul 05 2021 at 04:41):

That’s a good idea. I will try that

view this post on Zulip GuillaumeGomez (Jul 05 2021 at 12:56):

Noah Lev said:

In general, it feels like enums are the secret to improving rustdoc's code quality :)

That's generally a good way to ensure that specific checks are not forgotten. :)

view this post on Zulip Stu (Jul 05 2021 at 18:00):

Already worked awesome with fake def ids ;)

view this post on Zulip Stu (Jul 06 2021 at 16:20):

Damn. Enums are really awesome. I converted all paths to use enums and only two tests failing and they were super easy to fix.

view this post on Zulip Stu (Jul 06 2021 at 16:25):

Pull Request is up

view this post on Zulip Stu (Jul 06 2021 at 17:32):

holy shit rustdoc is soo much fun. Gimme more issues like the DefId and the Cache paths ones :)

view this post on Zulip GuillaumeGomez (Jul 06 2021 at 21:19):

If you have time and motivation, you can review https://github.com/rust-lang/rust/pull/86841 if you want. ;)

view this post on Zulip GuillaumeGomez (Jul 06 2021 at 21:22):

Otherwise I approved your PR, nice job! Maybe @Joshua Nelson wants to take a look too so I'll approve in a few days if he doesn't have time. (ping me otherwise I'll forget)

view this post on Zulip Joshua Nelson (Jul 07 2021 at 03:05):

left a review

view this post on Zulip Joshua Nelson (Jul 07 2021 at 03:05):

sorry to burst your bubble :(

view this post on Zulip Joshua Nelson (Jul 07 2021 at 03:07):

Stu said:

holy shit rustdoc is soo much fun. Gimme more issues like the DefId and the Cache paths ones :)

https://github.com/rust-lang/rust/issues/84619 and https://github.com/rust-lang/rust/issues/84304, I would describe all three as "pretty hard"

view this post on Zulip Joshua Nelson (Jul 07 2021 at 03:07):

but not "give up on the whole thing and create an MCP to change the compiler to make rustdoc's life easier" hard which is currently the hardest I've found :sweat_smile: https://github.com/rust-lang/rust/issues/83761

view this post on Zulip GuillaumeGomez (Jul 07 2021 at 08:22):

Joshua Nelson said:

sorry to burst your bubble :(

It's fine, I'm glad you reviewed it (I was surprised there was no change in the tests but didn't think much of it). However I think the approch is the correct one: having an enum to prevent forgetting the def_id.is_local() is a good idea in my opinion. However it needs to be correctly filled as well

view this post on Zulip Joshua Nelson (Jul 07 2021 at 13:07):

@GuillaumeGomez the problem is they are not in sync. The code enforces a check on the value, not the key, and it's possible for the DefId to be local without the value being local and vice versa. So I think introducing a check on the value is the wrong approach.

view this post on Zulip Joshua Nelson (Jul 07 2021 at 13:08):

(and I'm dubious that it's so common to treat local paths differently we need to enforce it)

view this post on Zulip GuillaumeGomez (Jul 07 2021 at 14:06):

Thanks for the explanations. Then indeed, my approach was the wrong one

view this post on Zulip Stu (Jul 11 2021 at 08:02):

So I reworked my code and now I combined both solutions. The paths map is still a HashMap<DefId, CachedPath>, but on every insert I check if DefId and cached path match. This works really well and there are only 3 tests still failing, which are super annoying to fix because I think they relied on that there were external DefIds in the old paths map

view this post on Zulip Joshua Nelson (Jul 11 2021 at 13:13):

@Stu yeah that sounds annoying - you're allowed to update the usage sites btw, if checking extern_paths in addition or something works than that seems reasonable :)

view this post on Zulip Stu (Jul 11 2021 at 13:19):

Yea I try to, but it's hard because I have no idea how some of the problems worked before my changes.

view this post on Zulip Stu (Jul 11 2021 at 13:27):

So one of the problems is that the elided-lifetime.rs test is failing because the return types on the re-exported methods do not have an anchor to the struct. This is caused because the crate that is inlined has an Unknown external location which causes this branch to be taken so None is returned and there is no link.

view this post on Zulip Stu (Jul 11 2021 at 13:29):

Wait, I think I know why. Writing out a problem really seems to help :sweat_smile:

view this post on Zulip Stu (Jul 11 2021 at 13:44):

Okay nvm. I still have no idea

view this post on Zulip Joshua Nelson (Jul 11 2021 at 14:48):

@Stu if it's inlined, it shouldn't be in the external crate, rustdoc should copy the documentation into the current crate

view this post on Zulip Joshua Nelson (Jul 11 2021 at 14:49):

hmm, are you maybe checking the DefId of the original definition instead of the use?

view this post on Zulip Stu (Jul 11 2021 at 14:53):

It's not an actual use item. It's an extern crate one

#[doc(inline)]
extern crate bar;

Last updated: Oct 21 2021 at 20:33 UTC