rustdoc: Yet more intra-doc links cleanup by camelid · Pull Request #92635 · rust-lang/rust

camelid · 2022-01-07T01:19:10Z

r? @Manishearth

rust-highfive · 2022-01-07T01:19:13Z

Some changes occurred in clean/types.rs.

cc @camelid

camelid · 2022-01-07T01:19:49Z

Blocked on #92601.

This includes commits from that PR, so it's blocked on it.

petrochenkov · 2022-01-07T10:14:54Z

I have some meta comment.
To continue the work started in #88679 I'm interested in following the doc link work, and in particular reviewing PRs to rustdoc touching that area.

However, I'm only interested in what happens until the link path is resolved, but not in any of the following stuff, including rendering, etc.
Is it possible to structure the work in such a way that changes to resolution and post-resolution parts are submitted separately?
In that case I will be able to review the resolution changes (I know that rustdoc needs some reviewer workforce right now).

camelid · 2022-01-07T19:06:00Z

There actually isn't much intra-doc links code devoted to rendering; it's almost all resolution-related. I usually try to separate rendering cleanups from resolution cleanups anyway. Also, if a PR deals too much with rendering for you to review all of it, you could review just the resolution parts.

Unless I'm misunderstanding what you mean by rendering?

camelid · 2022-01-07T19:06:13Z

And thanks for offering to help with review load :)

petrochenkov · 2022-01-09T02:58:59Z

@camelid

Unless I'm misunderstanding what you mean by rendering?

I mean these PRs work with things like e.g. "url fragments" and "anchors", which seem unrelated to path resolution.

camelid · 2022-01-09T03:02:46Z

Actually, with my recent refactoring of fragments, they're very closely related to resolution. In most places in the intra-doc-links code, a Res is for a page (e.g., a type or free function), while an ItemFragment is a "projection" out of its parent Res. So, a variant would have the Res of its enum and an ItemFragment(FragmentKind::Variant, variant_did).

I want to refactor the code so it uses full Ress (rather than page Ress) in more places, but that's difficult.

This hack was added in 6ab1f05. I don't know what change allowed removing the hack, but that commit added a test (which I presume covered the hack's behavior), and all tests are passing with this change. So, I think it should be good.

Inherent associated types *are* supported, just unstable.

This currently calls `std` a "crate" in one part of the message and a "module" in another part. The next commits fix this so it says "crate" in both places.

This allows simplifying a lot of code. It also fixes a subtle bug, exemplified by the test output changes.

Now that `res` is used directly, it seems the conditional is unnecessary.

I'm still not sure why this hack works so seemingly well.

These closures were quite complex and part of a quite complex function. The fact that they are closures makes mistakes likely when refactoring. For example, earlier, I meant to use `resolved`, an argument of the closure, but I instead typed `res`, which captured a local variable and caused a subtle bug that led to a confusing test failure. Extracting them as functions makes the code easier to understand and refactor.

petrochenkov · 2022-01-12T08:28:42Z

r=me unless @Manishearth has more comments.

Manishearth · 2022-01-12T17:48:07Z

gonna have a look later today probably

Manishearth

nice work!

Manishearth · 2022-01-13T23:48:40Z

@bors r+

bors · 2022-01-13T23:48:41Z

📌 Commit 28d2353 has been approved by Manishearth

…arth rustdoc: Yet more intra-doc links cleanup r? `@Manishearth`

…arth rustdoc: Yet more intra-doc links cleanup r? ``@Manishearth``

…arth rustdoc: Yet more intra-doc links cleanup r? ```@Manishearth```

matthiaskrgr · 2022-01-15T14:51:15Z

@bors r-
failed in a rollup: #92931 (comment)
probably because of https://github.com/rust-lang/rust/pull/92635/files#diff-53e2bb31a1a4804925f22caf5a4844d0b6ffc9f7f307d93ef6098d62381a5085L9

camelid · 2022-01-15T18:20:48Z

@bors r=Manishearth

bors · 2022-01-15T18:20:49Z

📌 Commit 554c765 has been approved by Manishearth

…askrgr Rollup of 10 pull requests Successful merges: - rust-lang#92487 (Fix unclosed boxes in pretty printing of TraitAlias) - rust-lang#92581 (ARMv6K Horizon - Enable default libraries) - rust-lang#92619 (Add diagnostic items for macros) - rust-lang#92635 (rustdoc: Yet more intra-doc links cleanup) - rust-lang#92646 (feat: rustc_pass_by_value lint attribute) - rust-lang#92706 (Clarify explicitly that BTree{Map,Set} are ordered.) - rust-lang#92710 (Include Projections when elaborating TypeOutlives) - rust-lang#92746 (Parse `Ty?` as `Option<Ty>` and provide structured suggestion) - rust-lang#92792 (rustdoc: fix intra-link for generic trait impls) - rust-lang#92814 (remove unused FIXME) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

camelid added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Jan 7, 2022

rust-highfive assigned Manishearth Jan 7, 2022

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 7, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2022

camelid marked this pull request as draft January 7, 2022 01:19

camelid added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 7, 2022

This comment has been minimized.

Sign in to view

camelid force-pushed the yet-more-cleanup branch from e2bfb31 to dbff2da Compare January 7, 2022 02:49

camelid mentioned this pull request Jan 8, 2022

intra-doc: Use the impl's assoc item where possible #92680

Merged

petrochenkov self-assigned this Jan 9, 2022

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jan 11, 2022

camelid added 9 commits January 10, 2022 19:23

Remove hack that is no longer necessary

49553bb

This hack was added in 6ab1f05. I don't know what change allowed removing the hack, but that commit added a test (which I presume covered the hack's behavior), and all tests are passing with this change. So, I think it should be good.

Move two intra-doc-link tests into the intra-doc folder

e18b23b

Enable ignored part of test

ca20d64

Inherent associated types *are* supported, just unstable.

Add test for disambiguator mismatch with crate

977a7ca

This currently calls `std` a "crate" in one part of the message and a "module" in another part. The next commits fix this so it says "crate" in both places.

Use Res instead of Disambiguator for resolved in report_mismatch

9acd813

This allows simplifying a lot of code. It also fixes a subtle bug, exemplified by the test output changes.

Remove unnecessary conditional for suggesting disambiguator

591ec49

Now that `res` is used directly, it seems the conditional is unnecessary.

Update comment and make code clearer

a5f09f7

I'm still not sure why this hack works so seemingly well.

Update some comments post the side channel removal

28d2353

petrochenkov approved these changes Jan 12, 2022

View reviewed changes

petrochenkov removed their assignment Jan 12, 2022

Manishearth approved these changes Jan 13, 2022

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 Jan 13, 2022

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022

Rollup merge of rust-lang#92635 - camelid:yet-more-cleanup, r=Manishe…

29adca6

…arth rustdoc: Yet more intra-doc links cleanup r? `@Manishearth`

matthiaskrgr mentioned this pull request Jan 15, 2022

Rollup of 8 pull requests #92925

Closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022

Rollup merge of rust-lang#92635 - camelid:yet-more-cleanup, r=Manishe…

878c0c4

…arth rustdoc: Yet more intra-doc links cleanup r? ``@Manishearth``

matthiaskrgr mentioned this pull request Jan 15, 2022

Rollup of 8 pull requests #92926

Closed

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022

Rollup merge of rust-lang#92635 - camelid:yet-more-cleanup, r=Manishe…

bcd2ad5

…arth rustdoc: Yet more intra-doc links cleanup r? ```@Manishearth```

matthiaskrgr mentioned this pull request Jan 15, 2022

Rollup of 7 pull requests #92931

Closed

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 Jan 15, 2022

Fix broken link

554c765

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2022

matthiaskrgr mentioned this pull request Jan 16, 2022

Rollup of 10 pull requests #92970

Merged

bors merged commit e1b9439 into rust-lang:master Jan 16, 2022

rustbot added this to the 1.60.0 milestone Jan 16, 2022

camelid deleted the yet-more-cleanup branch January 17, 2022 18:46

Uh oh!

Conversation

camelid commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Jan 7, 2022

Uh oh!

camelid commented Jan 7, 2022

Uh oh!

This comment has been minimized.

petrochenkov commented Jan 7, 2022

Uh oh!

camelid commented Jan 7, 2022

Uh oh!

camelid commented Jan 7, 2022

Uh oh!

petrochenkov commented Jan 9, 2022

Uh oh!

camelid commented Jan 9, 2022

Uh oh!

petrochenkov commented Jan 12, 2022

Uh oh!

Manishearth commented Jan 12, 2022

Uh oh!

Manishearth left a comment

Choose a reason for hiding this comment

Uh oh!

Manishearth commented Jan 13, 2022

Uh oh!

bors commented Jan 13, 2022

Uh oh!

matthiaskrgr commented Jan 15, 2022

Uh oh!

camelid commented Jan 15, 2022

Uh oh!

bors commented Jan 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

camelid commented Jan 7, 2022 •

edited

Loading