rustdoc: Remove `GetDefId` #90154

camelid · 2021-10-22T03:22:31Z

See the individual commit messages for details.

r? @jyn514

Based on looking at the source code, it looks like the `did` needs to be for an impl, not functions in an impl.

jyn514

👍 making this not a trait is an excellent idea, thanks for spotting this :)

src/librustdoc/clean/types.rs

jyn514 · 2021-10-22T14:29:38Z

src/librustdoc/clean/inline.rs

@@ -374,7 +374,7 @@ crate fn build_impl(
    // Only inline impl if the implementing type is
    // reachable in rustdoc generated documentation
    if !did.is_local() {
-        if let Some(did) = for_.def_id() {
+        if let Some(did) = for_.def_id_full(&cx.cache) {


I think something in this commit broke the linkchecker, not sure exactly which change. Maybe try reverting them one at a time until you find what went wrong? If you could write a test for it that would be amazing ❤️ but I won't block on that.

Haha, this is so great. My commit "broke" the linkchecker — because some impls that should have been shown on str's page were not because def_id_full was not being used, and they have stuff like this, which are interpreted as links by the linkchecker:

I'll update those docs to use inline code. EDIT: Instead, I just escaped the brackets.

Should be fixed now :)

It was only used in one place, so it seems better to use ordinary functions.

Now that it's only implemented for `Type`, using inherent methods instead means that imports are no longer necessary. Also, `GetDefId` is only meant to be used with `Type`, so it shouldn't be a trait.

In general, it should be preferred over `Type::def_id()`. See each method's docs for more.

The old name was confusing because it's easy to assume that using `def_id()` is fine, but in some situations it's incorrect. In general, `def_id_full()` should be preferred, so `def_id_full()` should have a shorter name. That will happen in the next commit.

It should be preferred over `def_id_no_primitives()`, so it should have a shorter name. I also put it before `def_id_no_primitives()` so that it shows up first in the docs.

camelid · 2021-10-22T21:03:46Z

Hmm, I guess the linkchecker really doesn't like seeing brackets in the HTML. Why does it even check for broken intra-doc links; doesn't rustdoc do that?

camelid · 2021-10-22T21:08:30Z

I'll just add to the exceptions list for now since there are almost identical exceptions there already.

My change to use `Type::def_id()` (formerly `Type::def_id_full()`) in more places caused some docs to show up that used to be missed by rustdoc. Those docs contained unescaped square brackets, which triggered linkcheck errors. This commit escapes the square brackets and adds this particular instance to the linkcheck exception list.

jyn514 · 2021-10-27T01:54:42Z

Why does it even check for broken intra-doc links; doesn't rustdoc do that?

Rustdoc has been buggy in the past, this helped catch quite a lot of links when it was introduced. I don't have a link to the PR offhand.

jyn514 · 2021-10-27T01:55:38Z

library/core/src/str/traits.rs

@@ -234,7 +234,7 @@ unsafe impl SliceIndex<str> for ops::Range<usize> {
 /// Implements substring slicing with syntax `&self[.. end]` or `&mut
 /// self[.. end]`.
 ///
-/// Returns a slice of the given string from the byte range [`0`, `end`).
+/// Returns a slice of the given string from the byte range \[0, `end`).


Maybe instead just surround the whole range with backticks? That seems like it would look nicer when displayed too.

I initially was going to do that, but then I realized that closed ranges might look like arrays instead, which could be confusing: [0, end]. I was also leaning on the side of having the docs be displayed the same way as before. In addition, based on the pre-existing link-check exceptions, it seems that the [0, end] style is the "standard" way to write it.

All that said, I'm fine either way, so I can change them if you would prefer.

jyn514 · 2021-10-27T04:06:44Z

@bors r+

bors · 2021-10-27T04:06:46Z

📌 Commit 3ad0834 has been approved by jyn514

…askrgr Rollup of 3 pull requests Successful merges: - rust-lang#90154 (rustdoc: Remove `GetDefId`) - rust-lang#90232 (rustdoc: Use TTF based font instead of OTF for CJK glyphs to improve readability) - rust-lang#90278 (rustdoc: use better highlighting for *const, *mut, and &mut) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

camelid added 2 commits October 21, 2021 19:54

Fix docs for inline::build_impl

b987fd7

Based on looking at the source code, it looks like the `did` needs to be for an impl, not functions in an impl.

Remove unused impl of GetDefId for Typedef

1da8659

camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Oct 22, 2021

rust-highfive assigned jyn514 Oct 22, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2021

This comment has been minimized.

Sign in to view

camelid force-pushed the remove-getdefid branch from 3c270b9 to 55838a4 Compare October 22, 2021 03:42

This comment has been minimized.

Sign in to view

jyn514 reviewed Oct 22, 2021

View reviewed changes

camelid added 6 commits October 22, 2021 13:07

Remove GetDefId impl for FnRetTy

6d82ee8

It was only used in one place, so it seems better to use ordinary functions.

Remove unused impl of GetDefId for Option<T>

7fb1306

Replace GetDefId with inherent methods

bf0cc90

Now that it's only implemented for `Type`, using inherent methods instead means that imports are no longer necessary. Also, `GetDefId` is only meant to be used with `Type`, so it shouldn't be a trait.

Use def_id_full() where easily possible

0853c33

In general, it should be preferred over `Type::def_id()`. See each method's docs for more.

Rename Type::def_id_full() to Type::def_id()

f93cf66

It should be preferred over `def_id_no_primitives()`, so it should have a shorter name. I also put it before `def_id_no_primitives()` so that it shows up first in the docs.

camelid force-pushed the remove-getdefid branch 2 times, most recently from 8c6f07a to f801ac3 Compare October 22, 2021 20:29

camelid force-pushed the remove-getdefid branch from f801ac3 to 865d99f Compare October 22, 2021 21:09

Fix another place that used def_id_no_primitives()

3ad0834

camelid mentioned this pull request Oct 22, 2021

rustdoc: Replace all remaining uses of Type::def_id_no_primitives() with Type::def_id() #90187

Closed

jyn514 reviewed Oct 27, 2021

View reviewed changes

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 Oct 27, 2021

matthiaskrgr mentioned this pull request Oct 27, 2021

Rollup of 3 pull requests #90337

Merged

bors merged commit e3eebfe into rust-lang:master Oct 27, 2021

rustbot added this to the 1.58.0 milestone Oct 27, 2021

camelid deleted the remove-getdefid branch October 27, 2021 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: Remove `GetDefId` #90154

rustdoc: Remove `GetDefId` #90154

camelid commented Oct 22, 2021

This comment has been minimized.

This comment has been minimized.

jyn514 left a comment

jyn514 Oct 22, 2021

camelid Oct 22, 2021 •

edited

Loading

camelid Oct 22, 2021

camelid commented Oct 22, 2021

camelid commented Oct 22, 2021

jyn514 commented Oct 27, 2021

jyn514 Oct 27, 2021

camelid Oct 27, 2021

jyn514 commented Oct 27, 2021

bors commented Oct 27, 2021

rustdoc: Remove GetDefId #90154

rustdoc: Remove GetDefId #90154

Conversation

camelid commented Oct 22, 2021

This comment has been minimized.

This comment has been minimized.

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 Oct 22, 2021

Choose a reason for hiding this comment

camelid Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

camelid Oct 22, 2021

Choose a reason for hiding this comment

camelid commented Oct 22, 2021

camelid commented Oct 22, 2021

jyn514 commented Oct 27, 2021

jyn514 Oct 27, 2021

Choose a reason for hiding this comment

camelid Oct 27, 2021

Choose a reason for hiding this comment

jyn514 commented Oct 27, 2021

bors commented Oct 27, 2021

rustdoc: Remove `GetDefId` #90154

rustdoc: Remove `GetDefId` #90154

camelid Oct 22, 2021 •

edited

Loading