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

Intra-rustdoc links do not work for all primitives #63351

Closed
nixpulvis opened this issue Aug 7, 2019 · 11 comments · Fixed by #80181
Closed

Intra-rustdoc links do not work for all primitives #63351

nixpulvis opened this issue Aug 7, 2019 · 11 comments · Fixed by #80181
Assignees
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.

Comments

@nixpulvis
Copy link

Coming from the tracking issue #43466.

I'm trying to link to slice::rotate_left and it doesn't seem to be supported I tested pointer::is_null as well, to test if it's just polymorphic primitives, and it seems that might be the issue, since it also doesn't work as a intra-link.

/cc @GuillaumeGomez

@jonas-schievink jonas-schievink added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-bug Category: This is a bug. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 7, 2019
@GuillaumeGomez GuillaumeGomez self-assigned this Aug 8, 2019
@GuillaumeGomez
Copy link
Member

I'll recheck it and see what's going on.

@GuillaumeGomez
Copy link
Member

The problem here is that, even if being primitive types, they're not considered as is in the compiler since they're generic. Therefore, the problem might be way more complicated to fix considering I have no idea how to link to a slice or a pointer...

@nixpulvis
Copy link
Author

nixpulvis commented Aug 12, 2019

Yea looking at the code, it seems things might stem from here:

impl PrimitiveTypeTable {
fn new() -> PrimitiveTypeTable {
let mut table = FxHashMap::default();
table.insert(sym::bool, Bool);
table.insert(sym::char, Char);
table.insert(sym::f32, Float(FloatTy::F32));
table.insert(sym::f64, Float(FloatTy::F64));
table.insert(sym::isize, Int(IntTy::Isize));
table.insert(sym::i8, Int(IntTy::I8));
table.insert(sym::i16, Int(IntTy::I16));
table.insert(sym::i32, Int(IntTy::I32));
table.insert(sym::i64, Int(IntTy::I64));
table.insert(sym::i128, Int(IntTy::I128));
table.insert(sym::str, Str);
table.insert(sym::usize, Uint(UintTy::Usize));
table.insert(sym::u8, Uint(UintTy::U8));
table.insert(sym::u16, Uint(UintTy::U16));
table.insert(sym::u32, Uint(UintTy::U32));
table.insert(sym::u64, Uint(UintTy::U64));
table.insert(sym::u128, Uint(UintTy::U128));
Self { primitive_types: table }
}
}

Based on this and the list of primatives in the STD docs, I'm guessing the following is a complete list of generic (or otherwise unimplemented) primitives:

  • array
  • fn
  • pointer
  • reference
  • slice
  • tuple
  • unit
  • never

This seems to be more broad than just generics however, since the () type isn't really generic. The issue seems to affect types with "names" different from their syntax, e.g. () vs unit.

Given the precedence set by the STD documentation for these primitives, I believe I'd expect to link to each with the English name and not the syntax, e.g. [pointer::is_null] and not [*::is_null] (or otherwise). This hasn't come up as an issue with codegen since as far as I can tell no primitives have methods defined on them without a self argument of some kind. This seems to be part of the need for the separate primitive modules.

I'm wondering if a solution to the need for separate primitive modules would also solve this. That's probably way more work than is needed for this specific issue however. Food for thought? Otherwise simply fixing this issue would be nice indeed, though does seem harder than I was hoping.

@GuillaumeGomez
Copy link
Member

The solution would be to "simply" try to check the current path and if not working, try to check if the first part of the path can be considered as primitive type (such as slice or pointer) and retry with this change. We already do something similar in some cases.

@jyn514
Copy link
Member

jyn514 commented Jul 6, 2020

The solution would be to "simply" try to check the current path and if not working, try to check if the first part of the path can be considered as primitive type (such as slice or pointer) and retry with this change.

I'm not sure it's that simple. If you look at https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/enum.PrimTy.html, none of the primitives you listed are present. Looking at https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/def/enum.Res.html, there isn't even a Res for all the types - ! and () are conspicuously absent.

The issue seems to affect types with "names" different from their syntax, e.g. () vs unit.

This isn't the issue, fn also doesn't work. The issue is that these are missing from PrimTy.

@jyn514

This comment has been minimized.

@jyn514
Copy link
Member

jyn514 commented Aug 19, 2020

A possible solution is to move from Res to Ty as the canonical way to look up primitives, using TyCtxt::type_of. Everything except primitives could keep using Res, it's only primitives that would have to change.

Relevant code:

fn is_primitive(path_str: &str, ns: Namespace) -> Option<(&'static str, Res)> {

@jyn514
Copy link
Member

jyn514 commented Aug 19, 2020

Yeah no I take it back, resolve() immediately returns the Res and so you can no longer distinguish it from ADTs. So this really does need a full refactor of collect_intra_doc_links.

@jyn514
Copy link
Member

jyn514 commented Aug 29, 2020

Ok, the bits that are actually used from Res are:

  • For primitive types, just the URL fragment from is_primitive and the fact that it's a primitive
  • For ADTs, it calls register_res and stores that DefId. It also stores a URL fragment but I'm pretty sure that's just user defined and not tied to the Res.
    • register_res which mostly just calls
    inline::record_extern_fqn(cx, did, kind);
    if let TypeKind::Trait = kind {
        inline::record_extern_trait(cx, did);
    }

The URL fragment is independent of Res. The fact that it's a primitive is independent of Res. The Kind is somewhat tied to the Res, but you could get the same info from a Ty or TyKind. However, I don't see an easy way to go from an ADT to a TyKind or Ty.

So, this scheme might work:

enum Resolution {
    Def(DefKind, DefId),  // replaces `Res::Def`
    Primitive { fragment: String },
}

Hmm ... that doesn't seem so bad.

camelid added a commit to camelid/rust that referenced this issue Sep 8, 2020
The only link that I did not change is a link to a function on the
`pointer` primitive because intra-doc links for the `pointer` primitive
don't work yet (see rust-lang#63351).
tmandry added a commit to tmandry/rust that referenced this issue Sep 10, 2020
…r, r=jyn514

Use intra-doc links in `core::ptr`

Part of rust-lang#75080.

The only link that I did not change is a link to a function on the
`pointer` primitive because intra-doc links for the `pointer` primitive
don't work yet (see rust-lang#63351).

---

@rustbot modify labels: A-intra-doc-links T-doc
tmandry added a commit to tmandry/rust that referenced this issue Sep 10, 2020
…r, r=jyn514

Use intra-doc links in `core::ptr`

Part of rust-lang#75080.

The only link that I did not change is a link to a function on the
`pointer` primitive because intra-doc links for the `pointer` primitive
don't work yet (see rust-lang#63351).

---

@rustbot modify labels: A-intra-doc-links T-doc
@jyn514 jyn514 added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Oct 26, 2020
@clarfonthey
Copy link
Contributor

clarfonthey commented Nov 8, 2020

Not sure if this is 100% related, but if you auto-link [()] or [!] it doesn't go to tuple or never as expected.

@jyn514
Copy link
Member

jyn514 commented Nov 9, 2020

@clarfonthey correct, that's this issue.

@jyn514 jyn514 assigned jyn514 and unassigned GuillaumeGomez Dec 19, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Dec 23, 2020
…shearth

Fix intra-doc links for non-path primitives

This does *not* currently work for associated items that are
auto-implemented by the compiler (e.g. `never::eq`), because they aren't
present in the source code. I plan to fix this in a follow-up PR.

Fixes rust-lang#63351 using the approach mentioned in rust-lang#63351 (comment).

r? `@Manishearth`

cc `@petrochenkov` - this makes `rustc_resolve::Res` public, is that ok? I'd just add an identical type alias in rustdoc if not, which seems a waste.
@bors bors closed this as completed in 257becb Dec 27, 2020
notriddle added a commit to notriddle/rust that referenced this issue Feb 23, 2023
Now that rust-lang#63351 is fixed, there's no reason not to.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 24, 2023
…=thomcc

docs: use intra-doc links for `Vec::get(_mut)`

Now that rust-lang#63351 is fixed, there's no reason not to.

CC rust-lang#75672
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 25, 2023
…=thomcc

docs: use intra-doc links for `Vec::get(_mut)`

Now that rust-lang#63351 is fixed, there's no reason not to.

CC rust-lang#75672
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-bug Category: This is a bug. E-hard Call for participation: Hard difficulty. Experience needed to fix: A lot. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants