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

Use intra-doc links in str and BTreeSet #74453

Closed
wants to merge 2 commits into from

Conversation

Manishearth
Copy link
Member

Fixes #32129, fixes #32130

A slight degradation in quality is that the #method.foo links would previously link to the same page on String's documentation, and now they will navigate to str. Not a big deal IMO, and we can also try to improve that.

@rust-highfive
Copy link
Collaborator

r? @LukasKalbertodt

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 17, 2020
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This reminds me why I love intra-doc links! This looks great :) There are a lot of broken links though. I guess the link checker won't check those until #71670 lands, can we wait for that?

@@ -97,8 +95,7 @@ impl<T: fmt::Debug> fmt::Debug for Iter<'_, T> {
/// This `struct` is created by the [`into_iter`] method on [`BTreeSet`]
/// (provided by the `IntoIterator` trait). See its documentation for more.
///
/// [`BTreeSet`]: struct.BTreeSet.html
/// [`into_iter`]: struct.BTreeSet.html#method.into_iter
/// [`into_iter`]: BTreeSet#method.into_iter
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 this should work:

Suggested change
/// [`into_iter`]: BTreeSet#method.into_iter
/// [`into_iter`]: BTreeSet::into_iter

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, it's a trait method

@@ -143,7 +142,7 @@ impl FromStr for bool {

/// An error returned when parsing a `bool` using [`from_str`] fails
///
/// [`from_str`]: ../../std/primitive.bool.html#method.from_str
/// [`from_str`]: FromStr::from_str
Copy link
Member

Choose a reason for hiding this comment

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

Do we support <bool as FromStr>::from_str? If not that seems like a good addition, we could also overload <bool as FromStr> to link to the trait implementation. No need to do anything for this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not

@@ -266,8 +265,7 @@ impl Utf8Error {
/// that it is valid UTF-8. `from_utf8()` checks to ensure that the bytes are valid
/// UTF-8, and then does the conversion.
///
/// [`&str`]: ../../std/primitive.str.html
/// [`u8`]: ../../std/primitive.u8.html
/// [`&str`]: str
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should strip references automatically? Not sure how that would play with slices eventually though.

/// into a string slice, use the [`str::from_utf8`] function.
///
/// [`str::from_utf8`]: ./str/fn.from_utf8.html
/// into a string slice, use the [`from_utf8`] function.
Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the same link text.

Suggested change
/// into a string slice, use the [`from_utf8`] function.
/// into a string slice, use the [`str::from_utf8`](from_utf8) function.

@@ -2429,8 +2416,7 @@ impl str {
/// The caller must ensure that the returned pointer is never written to.
/// If you need to mutate the contents of the string slice, use [`as_mut_ptr`].
///
/// [`u8`]: primitive.u8.html
/// [`as_mut_ptr`]: #method.as_mut_ptr
/// [`as_mut_ptr`]: str::method.as_mut_ptr
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// [`as_mut_ptr`]: str::method.as_mut_ptr
/// [`as_mut_ptr`]: str::as_mut_ptr

@@ -2954,7 +2932,7 @@ impl str {
///
/// To split by Unicode `Whitespace` instead, use [`split_whitespace`].
///
/// [`split_whitespace`]: #method.split_whitespace
/// [`split_whitespace`]: str::method.split_whitespace
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// [`split_whitespace`]: str::method.split_whitespace
/// [`split_whitespace`]: str::split_whitespace

@@ -3068,8 +3046,7 @@ impl str {
/// The [pattern] can be a `&str`, [`char`], a slice of [`char`]s, or a
/// function or closure that determines if a character matches.
///
/// [`char`]: primitive.char.html
/// [pattern]: str/pattern/index.html
/// [pattern]: self::pattern
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this is not very clear that it's the module and not an associated function ... but I guess as long as the link works it's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, associated functions are Self

@jyn514
Copy link
Member

jyn514 commented Jul 17, 2020

Hmm, well it caught at least some of them.

 std/primitive.str.html:128: broken link - std/str::method.as_mut_ptr
std/primitive.str.html:434: broken link - std/str::method.split_whitespace
std/primitive.str.html:1212: broken link - std/str::method.to_ascii_lowercase
thread 'main' panicked at 'found some broken links', src/tools/linkchecker/main.rs:41:9

but I would still prefer to wait for #71670.

@Manishearth
Copy link
Member Author

Fixed the links. The dot made us treat it as a URL and skip the lint (which is expected).

@jyn514
Copy link
Member

jyn514 commented Jul 17, 2020

Oh huh, I forgot that @lzutao turned on deny(resolution_failure) (thank you ❤️). So we should be good merge once you fix the broken links.

 error: `[str::to_ascii_lowercase]` cannot be resolved, ignoring it.
    --> src/libcore/str/mod.rs:4346:33
     |
4346 |     /// [`to_ascii_lowercase`]: str::to_ascii_lowercase
     |                                 ^^^^^^^^^^^^^^^^^^^^^^^ cannot be resolved, ignoring
     |
note: the lint level is defined here
    --> src/libcore/lib.rs:64:9
     |
64   | #![deny(intra_doc_link_resolution_failure)] // rustdoc is run without -D warnings
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`

error: aborting due to previous error

@Manishearth
Copy link
Member Author

Fixed. We can't link to any libstd things from libcore, so I reverted those.

@jyn514
Copy link
Member

jyn514 commented Jul 17, 2020

r=me once CI is passing

@Manishearth
Copy link
Member Author

@bors r=jyn514

that's what bors is for 😄 (and when making rollups we typically make sure CI is passing)

@bors
Copy link
Contributor

bors commented Jul 17, 2020

📌 Commit 748634e 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 Jul 17, 2020
@ehuss
Copy link
Contributor

ehuss commented Jul 18, 2020

This PR introduces quite a few regressions (as also noted in #74061) which I think would be unfortunate to add. In particular, many of the links are switched to remote links to https://doc.rust-lang.org/nightly/. Also, though not as severe, many of the primitive links now go to the module of the same name.

@Manishearth
Copy link
Member Author

@ehuss I don't consider it a regression for the links to link to doc.rlo. The primitive links can be fixed if we want.

@Manishearth
Copy link
Member Author

FWIW in my local build the links do not link to doc.rlo, they link locally. I think what you're seeing is #74458

Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
Use intra-doc links in `str` and `BTreeSet`

Fixes rust-lang#32129, fixes  rust-lang#32130

A _slight_ degradation in quality is that the `#method.foo` links would previously link to the same page on `String`'s documentation, and now they will navigate to `str`. Not a big deal IMO, and we can also try to improve that.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
Use intra-doc links in `str` and `BTreeSet`

Fixes rust-lang#32129, fixes  rust-lang#32130

A _slight_ degradation in quality is that the `#method.foo` links would previously link to the same page on `String`'s documentation, and now they will navigate to `str`. Not a big deal IMO, and we can also try to improve that.
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
Use intra-doc links in `str` and `BTreeSet`

Fixes rust-lang#32129, fixes  rust-lang#32130

A _slight_ degradation in quality is that the `#method.foo` links would previously link to the same page on `String`'s documentation, and now they will navigate to `str`. Not a big deal IMO, and we can also try to improve that.
@Manishearth Manishearth deleted the intra-doc-std branch July 18, 2020 01:14
@ehuss
Copy link
Contributor

ehuss commented Jul 18, 2020

I don't consider it a regression for the links to link to doc.rlo.

Why not? A fair bit of effort goes into making sure the documentation is usable offline. I also find it confusing when browsing the stable docs, then ending up (without warning) in the nightly docs, using something that appears to be stable, only to learn much later that it is not stable (or doesn't exist at all on stable).

To be clear, it is better to fix the broken links on the String page, but I consider it a backstep in other regards that should (eventually) be fixed.

FWIW in my local build the links do not link to doc.rlo, they link locally. I think what you're seeing is #74458

An example is core/str/index.html. Almost all of the links in the descriptions there now go to the remote site. I'm guessing that core and alloc are unable to link to the primitive types? Perhaps that will need to be special cased?

@Manishearth
Copy link
Member Author

@ehuss That's only on the libcore docs, it's because libcore is weird, yes. We might be able to fix that, I'll file an issue.

@Manishearth
Copy link
Member Author

Shit, closed the PR by mistake

@Manishearth
Copy link
Member Author

Opened as #74453

@Manishearth
Copy link
Member Author

@ehuss oh, right, this is because primitives only exist in std, and when documenting core we don't have std yet.

@jyn514
Copy link
Member

jyn514 commented Jul 18, 2020

primitives only exist in std

That seems weird, shouldn't they be in libcore?

bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 18, 2020
…arth

Rollup of 11 pull requests

Successful merges:

 - rust-lang#72414 ( Add lazy initialization primitives to std)
 - rust-lang#74069 (Compare tagged/niche-filling layout and pick the best one)
 - rust-lang#74418 (ci: Set `shell: bash` as a default, remove duplicates)
 - rust-lang#74441 (bootstrap.py: patch RPATH on NixOS to handle the new zlib dependency.)
 - rust-lang#74444 (Add regression test for rust-lang#69414)
 - rust-lang#74448 (improper_ctypes_definitions: allow `Box`)
 - rust-lang#74449 (Test codegen of compare_exchange operations)
 - rust-lang#74450 (Fix `Safety` docs for `from_raw_parts_mut`)
 - rust-lang#74453 (Use intra-doc links in `str` and `BTreeSet`)
 - rust-lang#74457 (rustbuild: drop tool::should_install)
 - rust-lang#74464 (Use pathdiff crate)

Failed merges:

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken links in BTreeSet struct documentation Broken links in String documentation from Deref impl
6 participants