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

Move to intra doc links for ascii.rs and panic.rs #75464

Merged
merged 1 commit into from
Aug 18, 2020

Conversation

poliorcetics
Copy link
Contributor

Helps with #75080.

@rustbot modify labels: T-doc, A-intra-doc-links, T-rustdoc

I also updated the doc to fix the wording in AsciiExt since it is now deprecated.
The two file are small changes so I bundled them together.

Some links could not be changed to make them work, I believe those are known issues with primitive types.

@rustbot rustbot added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 12, 2020
@rust-highfive
Copy link
Collaborator

r? @shepmaster

(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 Aug 12, 2020
@jyn514
Copy link
Member

jyn514 commented Aug 17, 2020

Hmm, slices are known not to work but str should work fine. There were some changes around modules vs primitives (#74063, #58699) but the link should still work with --stage 0 even if it goes to a different place.

@poliorcetics
Copy link
Contributor Author

poliorcetics commented Aug 17, 2020

I tried the following with /// [`str::to_lowercase`]: ../primitive.str.html#method.to_lowercase and they both failed:

  • crate::str::to_lowercase
  • str::to_lowercase

EDIT: some precisions:

I rebased from a minutes-old pull of master before building and trying things.

@jyn514
Copy link
Member

jyn514 commented Aug 17, 2020

Well I'm still not sure why it's failing, but I figured out why to_lowercase fails when trim doesn't: to_lowercase is defined in alloc, not core, because it returns a String: https://doc.rust-lang.org/src/alloc/str.rs.html#360-403.

@jyn514
Copy link
Member

jyn514 commented Aug 17, 2020

Found the issue:

fn primitive_impl(cx: &DocContext<'_>, path_str: &str) -> Option<DefId> {
let tcx = cx.tcx;
match path_str {
"u8" => tcx.lang_items().u8_impl(),
"u16" => tcx.lang_items().u16_impl(),
"u32" => tcx.lang_items().u32_impl(),
"u64" => tcx.lang_items().u64_impl(),
"u128" => tcx.lang_items().u128_impl(),
"usize" => tcx.lang_items().usize_impl(),
"i8" => tcx.lang_items().i8_impl(),
"i16" => tcx.lang_items().i16_impl(),
"i32" => tcx.lang_items().i32_impl(),
"i64" => tcx.lang_items().i64_impl(),
"i128" => tcx.lang_items().i128_impl(),
"isize" => tcx.lang_items().isize_impl(),
"f32" => tcx.lang_items().f32_impl(),
"f64" => tcx.lang_items().f64_impl(),
"str" => tcx.lang_items().str_impl(),
"bool" => tcx.lang_items().bool_impl(),
"char" => tcx.lang_items().char_impl(),
_ => None,
}
}

Note it doesn't have everything from
let primitive_impls = [
lang_items.isize_impl(),
lang_items.i8_impl(),
lang_items.i16_impl(),
lang_items.i32_impl(),
lang_items.i64_impl(),
lang_items.i128_impl(),
lang_items.usize_impl(),
lang_items.u8_impl(),
lang_items.u16_impl(),
lang_items.u32_impl(),
lang_items.u64_impl(),
lang_items.u128_impl(),
lang_items.f32_impl(),
lang_items.f64_impl(),
lang_items.f32_runtime_impl(),
lang_items.f64_runtime_impl(),
lang_items.bool_impl(),
lang_items.char_impl(),
lang_items.str_impl(),
lang_items.slice_impl(),
lang_items.slice_u8_impl(),
lang_items.str_alloc_impl(),
lang_items.slice_alloc_impl(),
lang_items.slice_u8_alloc_impl(),
lang_items.const_ptr_impl(),
lang_items.mut_ptr_impl(),
lang_items.const_slice_ptr_impl(),
lang_items.mut_slice_ptr_impl(),
];

@jyn514
Copy link
Member

jyn514 commented Aug 17, 2020

and they're in a third place, ugh. Going to clean this up as part of the fix.

let did = match primitive {
Isize => tcx.lang_items().isize_impl(),
I8 => tcx.lang_items().i8_impl(),
I16 => tcx.lang_items().i16_impl(),
I32 => tcx.lang_items().i32_impl(),
I64 => tcx.lang_items().i64_impl(),
I128 => tcx.lang_items().i128_impl(),
Usize => tcx.lang_items().usize_impl(),
U8 => tcx.lang_items().u8_impl(),
U16 => tcx.lang_items().u16_impl(),
U32 => tcx.lang_items().u32_impl(),
U64 => tcx.lang_items().u64_impl(),
U128 => tcx.lang_items().u128_impl(),
F32 => tcx.lang_items().f32_impl(),
F64 => tcx.lang_items().f64_impl(),
Char => tcx.lang_items().char_impl(),
Bool => tcx.lang_items().bool_impl(),
Str => tcx.lang_items().str_impl(),
Slice => tcx.lang_items().slice_impl(),
Array => tcx.lang_items().slice_impl(),
Tuple => None,
Unit => None,
RawPointer => tcx.lang_items().const_ptr_impl(),
Reference => None,
Fn => None,
Never => None,
};

@jyn514
Copy link
Member

jyn514 commented Aug 17, 2020

@poliorcetics I have a fix for the issue in #75649. For now let's just keep it as a normal link.

@jyn514 jyn514 assigned jyn514 and unassigned shepmaster Aug 17, 2020
@jyn514
Copy link
Member

jyn514 commented Aug 17, 2020

@bors r+ rollup

And thanks for the bug report!

@bors
Copy link
Contributor

bors commented Aug 17, 2020

📌 Commit 0e010a6 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 17, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 18, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#75389 (attempt to improve span_label docs)
 - rust-lang#75392 (Add `as_uninit`-like methods to pointer types and unify documentation of `as_ref` methods)
 - rust-lang#75464 (Move to intra doc links for ascii.rs and panic.rs)
 - rust-lang#75578 (Allowing raw ptr dereference in const fn)
 - rust-lang#75613 (Add explanation for `&mut self` method call when expecting `-> Self`)
 - rust-lang#75626 (Clean up E0754 explanation)
 - rust-lang#75629 (Use intra-doc links in `std::env`, `std::alloc` and `std::error`)
 - rust-lang#75634 (Mark x86_64-linux-kernel as *)

Failed merges:

r? @ghost
@bors bors merged commit d18719b into rust-lang:master Aug 18, 2020
@poliorcetics poliorcetics deleted the intra-links-panic-and-ascii branch August 18, 2020 15:41
tmandry added a commit to tmandry/rust that referenced this pull request Aug 18, 2020
…aumeGomez

Fix intra-doc links for inherent impls that are both lang items and not the default impl

I found in rust-lang#75464 (comment) that `str::to_uppercase()` doesn't resolve while `str::trim()` does. The only real difference is that `to_uppercase` is defined in `alloc`, while trim is defined in `core`. It turns out that rustdoc was ignoring `lang_items.str_alloc_impl()` - it saw them in `collect_trait_impls`, but not for intra-doc links.

This uses the same `impls` for all parts of rustdoc, so that there can be no more inconsistency. It does have the slight downside that the matches are no longer exhaustive but it will be very clear if a new lang item is missed because it will panic when you try to document it (and if you don't document it, does rustdoc really need to know about it?).

~~This needs a test case (probably just `str::to_uppercase`).~~ Added.

This is best reviewed commit-by-commit.

r? @GuillaumeGomez
tmandry added a commit to tmandry/rust that referenced this pull request Aug 18, 2020
…aumeGomez

Fix intra-doc links for inherent impls that are both lang items and not the default impl

I found in rust-lang#75464 (comment) that `str::to_uppercase()` doesn't resolve while `str::trim()` does. The only real difference is that `to_uppercase` is defined in `alloc`, while trim is defined in `core`. It turns out that rustdoc was ignoring `lang_items.str_alloc_impl()` - it saw them in `collect_trait_impls`, but not for intra-doc links.

This uses the same `impls` for all parts of rustdoc, so that there can be no more inconsistency. It does have the slight downside that the matches are no longer exhaustive but it will be very clear if a new lang item is missed because it will panic when you try to document it (and if you don't document it, does rustdoc really need to know about it?).

~~This needs a test case (probably just `str::to_uppercase`).~~ Added.

This is best reviewed commit-by-commit.

r? @GuillaumeGomez
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
…aumeGomez

Fix intra-doc links for inherent impls that are both lang items and not the default impl

I found in rust-lang#75464 (comment) that `str::to_uppercase()` doesn't resolve while `str::trim()` does. The only real difference is that `to_uppercase` is defined in `alloc`, while trim is defined in `core`. It turns out that rustdoc was ignoring `lang_items.str_alloc_impl()` - it saw them in `collect_trait_impls`, but not for intra-doc links.

This uses the same `impls` for all parts of rustdoc, so that there can be no more inconsistency. It does have the slight downside that the matches are no longer exhaustive but it will be very clear if a new lang item is missed because it will panic when you try to document it (and if you don't document it, does rustdoc really need to know about it?).

~~This needs a test case (probably just `str::to_uppercase`).~~ Added.

This is best reviewed commit-by-commit.

r? @GuillaumeGomez
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
…aumegomez

Fix intra-doc links for inherent impls that are both lang items and not the default impl

I found in rust-lang#75464 (comment) that `str::to_uppercase()` doesn't resolve while `str::trim()` does. The only real difference is that `to_uppercase` is defined in `alloc`, while trim is defined in `core`. It turns out that rustdoc was ignoring `lang_items.str_alloc_impl()` - it saw them in `collect_trait_impls`, but not for intra-doc links.

This uses the same `impls` for all parts of rustdoc, so that there can be no more inconsistency. It does have the slight downside that the matches are no longer exhaustive but it will be very clear if a new lang item is missed because it will panic when you try to document it (and if you don't document it, does rustdoc really need to know about it?).

~~This needs a test case (probably just `str::to_uppercase`).~~ Added.

This is best reviewed commit-by-commit.

r? @GuillaumeGomez
tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020
…aumegomez

Fix intra-doc links for inherent impls that are both lang items and not the default impl

I found in rust-lang#75464 (comment) that `str::to_uppercase()` doesn't resolve while `str::trim()` does. The only real difference is that `to_uppercase` is defined in `alloc`, while trim is defined in `core`. It turns out that rustdoc was ignoring `lang_items.str_alloc_impl()` - it saw them in `collect_trait_impls`, but not for intra-doc links.

This uses the same `impls` for all parts of rustdoc, so that there can be no more inconsistency. It does have the slight downside that the matches are no longer exhaustive but it will be very clear if a new lang item is missed because it will panic when you try to document it (and if you don't document it, does rustdoc really need to know about it?).

~~This needs a test case (probably just `str::to_uppercase`).~~ Added.

This is best reviewed commit-by-commit.

r? @GuillaumeGomez
@jyn514 jyn514 removed the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 25, 2020
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants