Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update intra-doc link documentation to match the implementation #80874

Merged
merged 7 commits into from
Mar 2, 2021

Conversation

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Jan 10, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2021
@rust-log-analyzer

This comment has been minimized.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 10, 2021

Nice!

Maybe it's good to also mention that both [...](Bar) and [..][Bar] works.

@jyn514
Copy link
Member Author

jyn514 commented Jan 10, 2021

Good idea, done.

@camelid
Copy link
Member

camelid commented Jan 10, 2021

Maybe it's good to also mention that both [...](Bar) and [..][Bar] works.

That's weird, I've found that [...](Bar) doesn't always work for me. Is it supposed to work?

src/doc/rustdoc/src/linking-to-items-by-name.md Outdated Show resolved Hide resolved
@@ -24,11 +24,29 @@ pub struct Foo4;
pub struct Bar;
```

Unlike normal markdown, `[bar][Bar]` syntax is also supported without needing a
`[Bar]: ...` reference link, and links are case-sensitive.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably note somewhere that non-intra-doc links are case-insensitive; see #80882.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, #80882 is a mess. I think I'd prefer to see what the plan is there before updating the documentation.

src/doc/rustdoc/src/linking-to-items-by-name.md Outdated Show resolved Hide resolved
namespace. In case of ambiguity, rustdoc will warn about the ambiguity and ask you to disambiguate, which can be done by using a prefix like `struct@`, `enum@`, `type@`, `trait@`, `union@`, `const@`, `static@`, `value@`, `fn@`, `function@`, `mod@`, `module@`, `method@`, `prim@`, `primitive@`, `macro@`, or `derive@`:
## Namespaces and Disambiguators

Paths in Rust have three namespaces: type, value, and macro. Item names must be unique within
Copy link
Member

Choose a reason for hiding this comment

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

It may not be necessary, but do you want to note that the type namespace includes modules?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would prefer not to; if you get it wrong it will be clear from the error message. If we mention modules we should also say functions are in the value namespace, though.

src/doc/rustdoc/src/linking-to-items-by-name.md Outdated Show resolved Hide resolved
src/doc/rustdoc/src/linking-to-items-by-name.md Outdated Show resolved Hide resolved
src/doc/rustdoc/src/linking-to-items-by-name.md Outdated Show resolved Hide resolved
Comment on lines 141 to 142
characters will be ignored. You can see the full criteria for 'sufficiently like' in [the source
code].
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if it's a good idea to link to the source code here. For one, it will very easily become out-of-date because it's tied to a particular commit. I think we should either skip this or include the list directly in the documentation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I kind of prefer not to tie this down so we can update it in the future. Maybe I could change the commit hash to master instead? That might have wrong line numbers though.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I could change the commit hash to master instead? That might have wrong line numbers though.

Yeah, I think that would be very confusing.

Hmm, I kind of prefer not to tie this down so we can update it in the future.

What do you mean? Documenting it isn't "tying it down".

Copy link
Member

Choose a reason for hiding this comment

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

So, this is a not a reference, this is a book, and I don't think it should go into that much detail. We do not need to document every last corner of the implementation here. Let's just drop this section.

src/doc/rustdoc/src/linking-to-items-by-name.md Outdated Show resolved Hide resolved
- Improve wording
- Use relative links
- Use a proper list instead of a wall of text
- Improve examples
@jyn514
Copy link
Member Author

jyn514 commented Jan 10, 2021

That's weird, I've found that ... doesn't always work for me. Is it supposed to work?

It should, if not that's a bug. Feel free to open an issue if you run into it again.

Comment on lines 133 to 140
Links are resolved in the scope of the module where the item is defined, even
when the item is re-exported. If a link from another crate fails to resolve, no
warning is given.

When re-exporting an item, rustdoc allows adding additional documentation to it.
That additional documentation will be resolved in scope of the re-export, not
the original, allowing you to link to items in the new crate. The new links
will still give a warning if they fail to resolve.
Copy link
Member

Choose a reason for hiding this comment

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

These two paragraph seem contradictory. The first one says that links are resolved in the module where the item is defined, even when it's re-exported, while the second says that documentation you add on a re-export will be resolved in the re-export crate. Which one is it? :P

Copy link
Member Author

Choose a reason for hiding this comment

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

The original links will be resolved in the original scope. The new links will be resolved in the new scope. The idea is for this to work:

// crate 1
/// Link to [f()]
pub struct S;
pub fn f() {}

// crate2
/// Link to [g()]
pub use crate1::S;
pub fn g() {}

Copy link
Member

Choose a reason for hiding this comment

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

I think you should say something like that in the docs, because I couldn't tell what you meant from the way it is right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't think of a way to explain this better, but I did add an example of both scenarios.

Co-authored-by: Camelid <camelidcamel@gmail.com>
@rust-log-analyzer

This comment has been minimized.

Co-authored-by: Camelid <camelidcamel@gmail.com>
@@ -76,19 +96,66 @@ struct Foo {}
fn Foo() {}
```

The following prefixes can be used:
Copy link
Member

Choose a reason for hiding this comment

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

I'd collapse this into a single sentence. Most people don't need to care about this, and rustdoc helps with diagnostics when they do.

trait implementations][#79682]. Rustdoc also supports linking to the following primitives, which
have no path and cannot be imported:

- [`slice`](../std/primitive.slice.html)
Copy link
Member

Choose a reason for hiding this comment

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

I'd collapse this into a single sentence. Most people don't need to care about this.

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.

overall this is written a little like a full reference; we don't need to cover everything, we should largely give space to things that are important to most users.

@Havvy
Copy link
Contributor

Havvy commented Jan 11, 2021

@Manishearth So....where does reference material for rustdoc go?

@Manishearth
Copy link
Member

I don't think there needs to be such in depth reference material for rustdoc. Like, so far in this pull request there's only one thing that I feel doesn't belong, and it's in the "rustdoc will ignore things that look like URLs" bit, which tbh i feel doesn't need to be documented at all, but either way, it's an extremely minor thing, and there aren't enough of these to warrant a separate book.

The rest is just a matter of style: A reference book can get away with being as precise as it can, but this book should be concise when talking about things that don't matter as much, like the disambiguators -- nobody needs to learn about them, in fact I'd argue they don't really belong in this book either, but it's fine to just have a sentence.

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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 Jan 29, 2021
@jyn514
Copy link
Member Author

jyn514 commented Feb 11, 2021

I don't think there needs to be such in depth reference material for rustdoc. Like, so far in this pull request there's only one thing that I feel doesn't belong, and it's in the "rustdoc will ignore things that look like URLs" bit, which tbh i feel doesn't need to be documented at all, but either way, it's an extremely minor thing, and there aren't enough of these to warrant a separate book.

I feel like there should be a reference somewhere. It doesn't need to be the rustdoc book, but right now it's hard to find how things work and why. Really this is an ongoing problem I've seen with rustdoc where things are underspecified and changes happen without much discussion. Here are some concrete examples:

All of these individually are small, but they build up. I feel like "whatever the implementation does" is really not a good way to build software, especially when there's no much discussion about these changes and they just happen.

@Manishearth
Copy link
Member

I think for a first pass this stuff should be long comments in the source, or just extra documentation in a docs folder. We could have a reference that strongly specifies rustdoc, but there's a lot we'd have to specify, and I don't think we have the bandwidth to do that. You're right that "whatever the implementation does" isn't great for software, but that doesn't mean that everything needs to be in design docs outside of the implementation. Using long comments or doc/foo.md folders is fine.

@Manishearth
Copy link
Member

But I am strongly against putting something that basically only implementors need to care about in the book. My opposition to a separate in-depth reference is mostly that it's a lot of work for very little benefit, and we can get something much smaller done better, but I'm not strongly against that.

Having a doc/ folder for random markdown files seems like a decent first step for pinning down things for implementors.

@jyn514
Copy link
Member Author

jyn514 commented Feb 11, 2021

But I am strongly against putting something that basically only implementors need to care about in the book.

I agree it probably doesn't fit in the book, but I don't think this is something only implementors need to worry about. These are all user-facing things that affect behavior, even if they're edge cases.

Anyway, I'll update this to not have quite so much detail and maybe open a tracking issue for the other things.

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

I'll point out that subtle edge cases of a lot of the things in the compiler are not user-facing-documented either. Things like "we try to figure out if something is supposed to be an intra-doc link with magic" and "type inference does magic" should be documented, but the precise details of what the magic is doesn't really need to be user-facing. It's sufficient for people to know that magic is going on, and if there are problems they can root cause it to the magic, and do something to appease the magic. Understanding the magic can be useful here, but really what you just need to know is how to bypass it; and for intra-doc links that's basically using the discriminants, and for inference it's using specific types.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@jyn514
Copy link
Member Author

jyn514 commented Feb 23, 2021

This is ready for review (I finally figured out the CI error).

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.

@bors r+ rollup

@Manishearth
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Mar 2, 2021

📌 Commit c2694f1 has been approved by Manishearth

@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 Mar 2, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 2, 2021
Rollup of 10 pull requests

Successful merges:

 - rust-lang#80189 (Convert primitives in the standard library to intra-doc links)
 - rust-lang#80874 (Update intra-doc link documentation to match the implementation)
 - rust-lang#82376 (Add option to enable MIR inlining independently of mir-opt-level)
 - rust-lang#82516 (Add incomplete feature gate for inherent associate types.)
 - rust-lang#82579 (Fix turbofish recovery with multiple generic args)
 - rust-lang#82593 (Teach rustdoc how to display WASI.)
 - rust-lang#82597 (Get TyCtxt from self instead of passing as argument in AutoTraitFinder)
 - rust-lang#82627 (Erase late bound regions to avoid ICE)
 - rust-lang#82661 (:arrow_up: rust-analyzer)
 - rust-lang#82691 (Update books)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a9339ed into rust-lang:master Mar 2, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 2, 2021
@jyn514 jyn514 deleted the intra-doc-docs branch March 2, 2021 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name 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.

9 participants