Add support for "true"/"false" intra doc link #75101

GuillaumeGomez · 2020-08-03T12:41:07Z

Part of #74515.

Explanations of what I did here: instead of checking more globally in primitive types, I think that only true and false values should be transformed into a link to bool. Others are either numbers or strings and I don't think we want to link all of them "generally". So in here, in case true and false didn't match anything (maybe a module was here, we don't know?), we then link them to the bool type.

r? @jyn514

cc @rust-lang/rustdoc

rust-highfive · 2020-08-03T12:41:10Z

Some changes occurred in intra-doc-links.

cc @jyn514

jyn514 · 2020-08-03T12:43:36Z

How do we know these are the only primitive values that have keywords? I'm ok with this workaround but I want to be sure we didn't miss anything.

jyn514 · 2020-08-03T12:47:20Z

Ok, there is a list of keywords at https://doc.rust-lang.org/std/index.html#keywords.

Do we want to support all of those, like as and dyn? Or just the keywords that are types?

jyn514 · 2020-08-03T12:48:07Z

I think we should at least support enum, union, and trait.

GuillaumeGomez · 2020-08-03T12:56:43Z

keyword != primitive. In here we use values which are in fact of primitive type. enum, union and trait aren't. Or maybe I missed your point?

GuillaumeGomez · 2020-08-03T12:57:28Z

CI failure:

warning: spurious network error (1 tries remaining): failed to get 200 response from `https://crates.io/api/v1/crates/autocfg/1.0.0/download`, got 504
error: failed to download from `https://crates.io/api/v1/crates/libc/0.2.74/download`

Mark-Simulacrum · 2020-08-03T13:07:22Z

It seems a bit odd to link to the bool type for these rather than https://doc.rust-lang.org/nightly/std/keyword.true.html and https://doc.rust-lang.org/nightly/std/keyword.false.html -- if we're going to autolink keywords, we should link to the keyword documentation, I think.

GuillaumeGomez · 2020-08-03T13:17:53Z

Oh I see, we had two different things in mind. Then we could try to do this based on the doc aliases directly, don't you think? Or should we keep a list based on the official keywords and only link using those? The second solution would allow a much simpler fix.

jyn514 · 2020-08-03T14:28:30Z

I think linking by doc-aliases would be the best approach - that would link to all keywords, right?

The issue with special-casing keywords is that now we have to hard-code either stable or nightly; if we use resolve for it then it will always be the appropriate version.

GuillaumeGomez · 2020-08-03T14:30:42Z

Ok, I'll update the PR with this change.

pickfire · 2020-08-03T14:31:34Z

src/librustdoc/passes/collect_intra_doc_links.rs

+                                match PRIMITIVES.iter().find(|(ty, _)| *ty == "bool") {
+                                    Some((_, res)) => (*res, Some("bool".to_owned())),
+                                    None => {
+                                        resolution_failure(cx, &item, path_str, &dox, link_range);
+                                        continue;
+                                    }


Should this be always checking in a loop? Shouldn't we get bool only once or make this static?

src/test/rustdoc/intra-doc-link-true-false.rs

Manishearth · 2020-08-03T15:36:56Z

I think we should link to the keywords here.

But I'm kinda skeptical of doing this at all, these are keywords and not paths. Seems ok for true and false but maybe not so much for other keywords. I also see a very low utility for it, and because they're not paths, I don't expect people to reach for them aside from the bool literals (which behave kinda like paths, at least)

jyn514 · 2020-08-03T16:09:04Z

FWIW I'm ok with just not implementing this, this is definitely out of the scope of the original RFC. Up to @GuillaumeGomez if he wants to go through with the PR since he already wrote a lot of it.

If we do implement it though, I think it would be nice to make it work for all keywords.

GuillaumeGomez · 2020-08-03T16:10:54Z

Honestly I'm not sure it's worth it so let's just keep it simple. I'll update to link to the keyword pages though.

Manishearth · 2020-08-03T16:19:34Z

I'm very against linking to all keywords, the only reason I'm somewhat okay with this one is because true and false behave kind of like paths; they operate in path positions (unlike most other keywords).

GuillaumeGomez · 2020-08-03T16:20:15Z

Yes we agree, only keywords for true and false. ;)

GuillaumeGomez · 2020-08-03T16:35:54Z

Updated!

Manishearth · 2020-08-03T16:36:12Z

No that was a response to @jyn514 who wanted us to do all keywords if we do true/false.

GuillaumeGomez · 2020-08-03T16:37:04Z

Oh I see, well, we can talk about it at a later time if enough people are interested. But currently, not very convinced.

jyn514 · 2020-08-03T19:09:51Z

@bors r+

Manishearth · 2020-08-03T23:29:22Z

@bors r-

two team members are against it at this point. Maybe we should hold off unless there's strong motivation?

GuillaumeGomez · 2020-08-04T09:03:13Z

I don't have strong feelings here so we can close it if everyone agrees?

jyn514 · 2020-08-04T13:09:04Z

Sounds good to me.

jyn514 · 2020-08-15T18:02:10Z

This came up again in #75567, which has 38 different links to true and false ... what do you think about only linking true and false and no other keywords? I took a bit of a strong position last time 😆

cc @poliorcetics

GuillaumeGomez · 2020-08-15T21:26:46Z

Well, if enough people are interested, please reopen then. 😆

ollie27 · 2020-08-15T22:44:53Z

what do you think about only linking true and false and no other keywords?

What would they link to? I think linking to the bool page makes sense and would be more useful than the keyword pages which don't contain much.

Manishearth · 2020-08-16T01:01:27Z

I agree with Ollie here

jyn514 · 2020-08-16T03:12:37Z

Linking to bool sounds fine to me, that's actually where they go currently: https://github.com/rust-lang/rust/pull/75567/files#diff-3935db3bd98ce45bbc780286b8d11c78R212

poliorcetics · 2020-08-16T12:24:18Z

true and false would be very nice to have for documenting rust itself (I expected them to behave like Ok or Some) but if it needs a lot of special-casing code, don't bother. They shouldn't be used too much outside std, core or alloc (though that claim is completely unverified) so we should ensure the benefits outweigh the costs.

jyn514 · 2020-08-16T12:47:48Z

if it needs a lot of special-casing code, don't bother

If we're just linking to bool, the code can be simplified quite a bit, @GuillaumeGomez just needs to add an alias next to bool:

rust/src/librustdoc/passes/collect_intra_doc_links.rs

Line 1222 in cd53760

("bool", Res::PrimTy(hir::PrimTy::Bool)),

I expected them to behave like Ok or Some

Well, they can't really because Ok and Some are documented on the same page as Result, but all the keywords are on separate pages. Does linking to bool sound like the right behavior?

poliorcetics · 2020-08-16T13:09:40Z

Oh that's nice if it doesn't ask for a lot of code.

Yeah, linking to bool is better I think.

jyn514 · 2020-08-18T00:15:48Z

Opened #75652 which links true and false to bool. It's a little simpler than this one because it doesn't have to deal with keywords.

@GuillaumeGomez do you mind if I close this PR (again :P)?

GuillaumeGomez · 2020-08-18T07:25:31Z

Too late, I already did! :p

@Manishearth

Resolve true and false as booleans Successor to rust-lang#75101. r? @Manishearth cc @rust-lang/rustdoc

@Manishearth

Resolve true and false as booleans Successor to rust-lang#75101. r? @Manishearth cc @rust-lang/rustdoc

@Manishearth

Resolve true and false as booleans Successor to rust-lang#75101. r? @Manishearth cc @rust-lang/rustdoc

rust-highfive assigned jyn514 Aug 3, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 3, 2020

jyn514 added A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-enhancement Category: An issue proposing an enhancement or a PR with one. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Aug 3, 2020

pickfire reviewed Aug 3, 2020

View reviewed changes

src/test/rustdoc/intra-doc-link-true-false.rs Show resolved Hide resolved

GuillaumeGomez force-pushed the true-false-intra-doc-link branch from 14a9aca to 4d2a53f Compare August 3, 2020 16:35

GuillaumeGomez added 2 commits August 3, 2020 18:36

Link to bool primitive type if "true" or "false" is used

53e0357

Add test for true/false intra-doc link

66d998f

GuillaumeGomez force-pushed the true-false-intra-doc-link branch from 4d2a53f to 66d998f Compare August 3, 2020 16:36

bors added 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 3, 2020

jyn514 closed this Aug 4, 2020

jyn514 mentioned this pull request Aug 4, 2020

Intra-doc links do not resolve for value keywords #74515

Closed

GuillaumeGomez deleted the true-false-intra-doc-link branch August 4, 2020 13:11

jyn514 mentioned this pull request Aug 15, 2020

Move to intra doc links in std::net #75567

Merged

This comment has been minimized.

Sign in to view

GuillaumeGomez restored the true-false-intra-doc-link branch August 15, 2020 21:48

This comment has been minimized.

Sign in to view

jyn514 reopened this Aug 15, 2020

jyn514 mentioned this pull request Aug 18, 2020

Resolve true and false as booleans #75652

Merged

GuillaumeGomez closed this Aug 18, 2020

tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020

Rollup merge of rust-lang#75652 - jyn514:true-false-2, r=GuillaumeGomez

a35be3e

Resolve true and false as booleans Successor to rust-lang#75101. r? @Manishearth cc @rust-lang/rustdoc

tmandry added a commit to tmandry/rust that referenced this pull request Aug 19, 2020

Rollup merge of rust-lang#75652 - jyn514:true-false-2, r=GuillaumeGomez

7c6e38f

Resolve true and false as booleans Successor to rust-lang#75101. r? @Manishearth cc @rust-lang/rustdoc

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 19, 2020

Rollup merge of rust-lang#75652 - jyn514:true-false-2, r=GuillaumeGomez

e6e917e

Resolve true and false as booleans Successor to rust-lang#75101. r? @Manishearth cc @rust-lang/rustdoc

jyn514 mentioned this pull request Oct 9, 2020

Clean up in intra-doc link collector #77743

Merged

GuillaumeGomez deleted the true-false-intra-doc-link branch August 19, 2024 12:24

Add support for "true"/"false" intra doc link #75101

Add support for "true"/"false" intra doc link #75101

Conversation

GuillaumeGomez commented Aug 3, 2020

rust-highfive commented Aug 3, 2020

jyn514 commented Aug 3, 2020

jyn514 commented Aug 3, 2020

jyn514 commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

Mark-Simulacrum commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

jyn514 commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

pickfire Aug 3, 2020

Choose a reason for hiding this comment

Manishearth commented Aug 3, 2020

jyn514 commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

Manishearth commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

Manishearth commented Aug 3, 2020

GuillaumeGomez commented Aug 3, 2020

jyn514 commented Aug 3, 2020

Manishearth commented Aug 3, 2020

GuillaumeGomez commented Aug 4, 2020

jyn514 commented Aug 4, 2020

jyn514 commented Aug 15, 2020

GuillaumeGomez commented Aug 15, 2020

This comment has been minimized.

This comment has been minimized.

ollie27 commented Aug 15, 2020

Manishearth commented Aug 16, 2020

jyn514 commented Aug 16, 2020

poliorcetics commented Aug 16, 2020 • edited Loading

jyn514 commented Aug 16, 2020 • edited Loading

poliorcetics commented Aug 16, 2020

jyn514 commented Aug 18, 2020 • edited Loading

GuillaumeGomez commented Aug 18, 2020

poliorcetics commented Aug 16, 2020 •

edited

Loading

jyn514 commented Aug 16, 2020 •

edited

Loading

jyn514 commented Aug 18, 2020 •

edited

Loading