Skip to content

Conversation

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Apr 13, 2021

Description

This PR adds an option (disabled by default) to add links in the source code page on ident. So for for example:

mod other_module;
struct Foo;
fn bar() {}

fn x<T: other_module::Trait>(f: Foo, g: other_module::Whatever, t: &T) {
    let f: Foo = Foo;
    bar();
    f.some_method();
}

In the example (mostly in the x function), other_module::Trait, Foo, other_module::Whatever, bar and some_method are now links (and other_module at the top too).

In case there is a type coming from another crate, it'll link to its documentation page and not its definition (but you can then click on [src] so I guess it's fine).

Another important detail: I voluntarily didn't add links for primitive types. I think we can discuss about adding links on them or not in a later PR (adding the support for them would require only a few lines).

Here is a video summing up everything I wrote above:

Peek.2021-04-13.23-15.mp4

Performance impact

So, on my computer, the performance remains more or less the same (which is quite surprising but that's a nice surprise). Here are the numbers:

Without the option:

  • core: 1m 21s
  • alloc: 26.78s
  • std: 27.30s
  • proc_macro: 4.50s

With source to definition links generation (I enabled by default the option):

  • core: 1m 25s
  • alloc: 25.76s
  • std: 27.07s
  • proc_macro: 4.66s

So no real change here (again, I'm very surprised by this fact).

For the size of the generated source files (only taking into account the src folder here since it's the only one impacted) by running du -shc . (when I am in the source folder).

Without the option: 11.939 MB
With the option: 12.611 MB

So not a big change here either. In all those docs, I ran grep -nR '<a class=' . | wc -l and got 43917. So there are quite a lot of links added. :)

cc @rust-lang/rustdoc
r? @jyn514

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Apr 13, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 13, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

I added a little improvement: when a source code link is hovered, the link is underlined (like what we have in the "path" of the items at the top of the documentation page).

@GuillaumeGomez
Copy link
Member Author

For the span panics, one theory to explain it could be because we leave the "compiler scope". Meaning that it's a potential bug that went uncovered for quite a long time... Thanks to @estebank for this! I'll move the source code pages generation into that scope and check if we still have the panic or not.

@jyn514
Copy link
Member

jyn514 commented Apr 16, 2021

I still feel like this is a major expansion of scope and would prefer it goes through an RFC first (https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/extend.20source.20code.20viewer). @Manishearth what do you think?

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Apr 16, 2021

I thought we agreed on having a disabled by default first implementation to be able to get feedback and potential new suggestions on the feature in order to then open an RFC, but maybe I completely misunderstood... In this case, an RFC would mean at least months before being able to get any feedback based on an experimental first implementation.

It wouldn't be the first time that a feature gets implemented and remains disabled by default to get feedback before opening an RFC to stabilize it and enable it by default.

@bjorn3
Copy link
Member

bjorn3 commented Apr 16, 2021

I added a little improvement: when a source code link is hovered, the link is underlined (like what we have in the "path" of the items at the top of the documentation page).

Could you make it always underlined? That matches what haddock does and makes it easier to see what is clickable and what is not. And maybe also highlight the background when hovering. This is what haddock does:

image

image

https://www.stackage.org/haddock/lts-17.9/abstract-deque-0.3/src/Data.Concurrent.Deque.Class.html#Deque

IMHO the contrast is a bit low though.

@GuillaumeGomez
Copy link
Member Author

@bjorn3 I'd rather not (personal opinion: it would attract the eye and make it more difficult to read the source code because of the distractions, up to debate), or at least not for the moment. The goal of this PR (once it's merged) is to set a base on which people can experiment and try things and then debate on what we want in the end (through a RFC).

@bors

This comment has been minimized.

@bors

This comment has been minimized.

Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see a lot more documentation. My main worry continues to be maintenance, this is smaller than I thought but I want to make sure that the team is well equipped to maintain it. Good documentation is a part of that

From the feature's side, should item definitions link to their own docs so that you can always jump back to the docs?

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 18, 2021
@bors

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

I keep failing my pushes for some reasons... All hail git reflog! 🙏

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Apr 19, 2021

I'd like to see a lot more documentation. My main worry continues to be maintenance, this is smaller than I thought but I want to make sure that the team is well equipped to maintain it. Good documentation is a part of that

This is an excellent point. I'll add documentation on all new functions/types I created.

From the feature's side, should item definitions link to their own docs so that you can always jump back to the docs?

Funny, I was wondering about that too. I have some ideas about it. For example adding a data-documentation attribute to idents which have available documentation and then it'd display a popup when you click on the link. But do we want to do it in this PR directly? It's already quite big. I'd prefer if possible to only handle the "basics" here and then iteratively implement more things on top. That will make the review process a lot less painful.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

@bors: r=jyn514

@bors
Copy link
Collaborator

bors commented Aug 5, 2021

📌 Commit ba11dc7 has been approved by jyn514

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2021
@bors
Copy link
Collaborator

bors commented Aug 5, 2021

⌛ Testing commit ba11dc7 with merge e2f7957...

@bors
Copy link
Collaborator

bors commented Aug 6, 2021

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing e2f7957 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-rustdoc-ui Area: Rustdoc UI (generated HTML) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.