Stream: rustdoc

Topic: go to definition feature


view this post on Zulip GuillaumeGomez (Jun 25 2021 at 10:11):

Hi everyone,

https://github.com/rust-lang/rust/pull/84176 has been open for quite some time now and is at a dead point. The two arguments against it are:

(Please correct me if I'm wrong @Manish Goregaokar @Joshua Nelson).

With the (great!) help of @jsha, I've put together a document to try to have a more clear view on this feature, what it implies, how it works and where we should put limits on additions to this feature as well. This last point is however very open to debate because it seems very tricky. The goal is to try to sum up everything we said until now in order to (ideally) reach a consensus.

https://gist.github.com/GuillaumeGomez/66bc7c6424dfe8ffe70739d5634e7f97

So what do you think of this proposal?

cc @Oliver Middleton, @Nemo157, @Noah Lev, @CraftSpider, @jsha

view this post on Zulip Manish Goregaokar (Jun 25 2021 at 14:59):

So firstly , thanks for writing this. I think in general for big features like this I would prefer to have such proposals (though not necessarily _as_ in depth, just a plan in an issue is enough) _first_ before implementation work happens

view this post on Zulip GuillaumeGomez (Jun 25 2021 at 15:03):

Sorry about that. It was just so clear in my mind that I didn't take all the non-technical parts into consideration. Only after you and @Joshua Nelson strongly advising to look into non technical parts, I slowly started to wonder what I was missing. I'm ashamed to say so but @jsha helped me to fully understand what I was missing. Now I see better, and I understand why you were so careful about it, and i'm sorry for being so slow to see it.

The big issue now is to kinda say where we should put the limit. I'm not even sure if something like this is possible. It's hard to predict the future.

view this post on Zulip Manish Goregaokar (Jun 25 2021 at 15:05):

thanks

view this post on Zulip Manish Goregaokar (Jun 25 2021 at 15:07):

I think the thing @jsha brings up about JTD for variables is on point and is exactly the kind of scope expansion I was afraid of. I think many users will consider this feature incomplete without JTD for variables because they'll expect it to work. Which is fine, but it's more design work and could potentially significantly complicate the code. Personally I'd prefer to not do JTD for variables but otoh i do feel like if we're going to do a thing we should do it right.

Though honestly, my biggest concern remains maintainability (of course scope expansion makes maintainability harder :P). I had basically said that I'd be okay with it if others do the review and think it's maintainable, but so far that hasn't happened, and jyn has also stepped away from reviewing this. When I tried to review this it was definitely not easy to understand/review which means that I'm still pretty concerned about that. It's possible that there's a way to implement this feature that's easy to review, but I don't think we're there yet, and the fact that nobody is reviewing this is a signal to me.

view this post on Zulip GuillaumeGomez (Jun 25 2021 at 16:07):

Hum interesting, I'd personally not include JTD for variables but it's not much harder to add.

As for the maintenability, I'm surprised of your comment. @Oliver Middleton reviewed it a few times already after all, and so did @Joshua Nelson before being mostly not available (however he expressed concerns about the scope expansion).

If you take a look at the code of the PR: https://github.com/rust-lang/rust/pull/84176/files#diff-00b3b34ce2eb95c63cc6b1a4648ab3939af4b083e2fb0e8d199535fe239b576f , it's 200 lines and most of it is documentation.

In https://github.com/rust-lang/rust/pull/84176/files#diff-eaff3f0a46e1d30c3ec7dc4a9ca5cdad2eb6e27b9fe1463bac5a1c928c94140a we have ~230 lines in order to generate links into the source code files (and more than 40 lines of doc comments).

In https://github.com/rust-lang/rust/pull/84176/files#diff-04002bbcf90fc0a3356db4344dee8f1e03d868fa6c0f666713affa7fa2679b6f I created a source file collector because I use it in the new visitor (to create the span map) as well to prevent doing it more than necessary.

We have more than 100 lines of tests.

The rest is about passing down the parameter to the inside of rustdoc and extra documentation.

This is far from the first time where a big rust(doc) PR takes time to get reviewed. I don't see this as an issue but more as a precaution. However I proposed multiple times to help with the review and answer any questions that might appear. For me there is no big technical changes, but I'm the one who wrote it, so...

If anyone is willing to take a few hours to make a call so we can go through the PR together, I don't mind at all. It's a big PR with at least 300 lines of newly added code after all. So if I can do anything to make the review process simpler for you, please tell me.

view this post on Zulip Joshua Nelson (Jun 25 2021 at 17:14):

On the technical side, my concern is almost always about implicit state between different parts of rustdoc. This changes the following pieces of implicit state:

Overall it's a lot better than I remember and, ignoring the scope issue, I would be ok with merging it as-is.

view this post on Zulip Joshua Nelson (Jun 25 2021 at 17:19):

Also @Manish Goregaokar I haven't had time for any reviews, not just this one, I don't want that to be a point against it

view this post on Zulip GuillaumeGomez (Jun 25 2021 at 18:20):

The cleanup is paused for the moment: on my side, I need @Joshua Nelson to review these parts of rustdoc, even if not as "main" reviewer but I feel more confident having him take a look.

As for the technical issues listed, it's exactly as you said: that will require a huge refactor in any case. I hope we'll get to it at some point.

So now remains to agree on the scope issue or more precisely: how to put limits. I don't know how to word such a thing precisely or even how to categorize some features as "unwanted" and some others as "acceptable".

view this post on Zulip Manish Goregaokar (Jun 26 2021 at 01:04):

@GuillaumeGomez Sorry! I hadn't noticed that ollie had reviewed this. The new code does look better. Still a bit concerned but my main issue was that I'd be okay with it if the reviewer felt that maintainability was resolved and it seems like that's the case, so I'm fine with it.

A question is: putting aside whether or not we should be doing JTD for vars, how much more complicated do you forsee the implementation getting if we did? If the answer is "not much at all" then we're probably fine, otherwise we have to think about scoping.

@Joshua Nelson yeah I know you haven't had time for reviews, but the fact that it took so long to get a review for in general (and had multiple false starts where I started to review but it took too much time to even understand what was happening) was a signal for me

view this post on Zulip Joshua Nelson (Jun 26 2021 at 01:06):

I still feel like the cfg(doc) thing has not really been addressed; that's a fragile part of rustdoc and regularly gets special-cased in the compiler proper. Trying to type check bodies is tempting fate. (see also https://github.com/rust-lang/rfcs/pull/2963#issuecomment-669515931)

view this post on Zulip Manish Goregaokar (Jun 26 2021 at 08:12):

Yeah, that's a big reason to avoid looking at bodies. In general I am more comfy looking at signatures over bodies but this is the kind of scope expansion i was worried about

view this post on Zulip GuillaumeGomez (Jun 26 2021 at 08:49):

Manish Goregaokar said:

A question is: putting aside whether or not we should be doing JTD for vars, how much more complicated do you forsee the implementation getting if we did? If the answer is "not much at all" then we're probably fine, otherwise we have to think about scoping.

Not much at all. I simply didn't want to put more into this PR to keep it simpler to review. Most of the changes come from how we generate links into the source code pages, not from how we generate the span map. If we want to add this feature as well, I can add it into a follow-up PR.

Manish Goregaokar said:

Joshua Nelson yeah I know you haven't had time for reviews, but the fact that it took so long to get a review for in general (and had multiple false starts where I started to review but it took too much time to even understand what was happening) was a signal for me

At the time the PR was facing a bit more troubles because we came across this bug. I fixed it in this PR, making the PR lose a huge part of its code. Another thing to take into account is that this is a rustdoc part that not much rustdoc reviewers know. We are all "specialized", and I think only @Oliver Middleton and @Joshua Nelson are able to review this one "confidently". Also, you said yourselves that you were busy, and you didn't contribute on rustdoc source code since quite some time now. So maybe on your side, it'd take more time to remember the whole rustdoc structure, making the review more complicating, discouraging you?

Joshua Nelson said:

I still feel like the cfg(doc) thing has not really been addressed; that's a fragile part of rustdoc and regularly gets special-cased in the compiler proper. Trying to type check bodies is tempting fate. (see also https://github.com/rust-lang/rfcs/pull/2963#issuecomment-669515931)

I already started asking some compiler people about this. They didn't seem very worried about that though... So no idea if that's a big issue or not. I'd argue that the fact this feature it's nightly only and will remain until we're sure all issues are fixed and the scope is defined should put us more at ease but I understand your worries.

view this post on Zulip Joshua Nelson (Jun 26 2021 at 13:05):

:+1: I am ok with this if it's nightly only

view this post on Zulip GuillaumeGomez (Jun 26 2021 at 14:56):

It'll remain so for some time. I want at least you and @jsha to give approval on the code and on the front-end "look" before stabilization. So we have (at least/minimum) a few months ahead of us.


Last updated: Oct 11 2021 at 22:34 UTC