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.

Sign up for GitHub

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

Jump to bottom

rustdoc: Yet more intra-doc links cleanup #92635

Merged
merged 10 commits into from
Jan 16, 2022
Merged

rustdoc: Yet more intra-doc links cleanup #92635

merged 10 commits into from
Jan 16, 2022

Conversation

camelid
Copy link
Member

@camelid camelid commented Jan 7, 2022 •
edited
Loading

@camelid 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
Copy link
Collaborator

Some changes occurred in clean/types.rs.

cc @camelid

@rustbot 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 rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 7, 2022
@camelid
Copy link
Member Author

camelid commented Jan 7, 2022

Blocked on #92601.

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

@camelid camelid marked this pull request as draft January 7, 2022 01:19
@camelid 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
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

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
Copy link
Member Author

camelid commented Jan 7, 2022

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
Copy link
Member Author

camelid commented Jan 7, 2022

And thanks for offering to help with review load :)

@camelid camelid mentioned this pull request Jan 8, 2022
Merged
@petrochenkov petrochenkov self-assigned this Jan 9, 2022
@petrochenkov
Copy link
Contributor

@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
Copy link
Member Author

camelid commented Jan 9, 2022

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.

@camelid 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
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.
@camelid
Move two intra-doc-link tests into the intra-doc folder
e18b23b
@camelid
Enable ignored part of test
ca20d64 
Inherent associated types *are* supported, just unstable.
@camelid
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.
@camelid
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.
@camelid
Remove unnecessary conditional for suggesting disambiguator
591ec49 
Now that `res` is used directly, it seems the conditional is
unnecessary.
@camelid
Update comment and make code clearer
a5f09f7 
I'm still not sure why this hack works so seemingly well.
@camelid
Extract functions for two closures
895fa9c 
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.
@camelid
Update some comments post the side channel removal
28d2353
@petrochenkov
Copy link
Contributor

r=me unless @Manishearth has more comments.

@petrochenkov petrochenkov removed their assignment Jan 12, 2022
@Manishearth
Copy link
Member

gonna have a look later today probably

Manishearth
Manishearth approved these changes Jan 13, 2022
View reviewed changes
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work!

@Manishearth
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 13, 2022

📌 Commit 28d2353 has been approved by Manishearth

@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 Jan 13, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022
@matthiaskrgr
Rollup merge of rust-lang#92635 - camelid:yet-more-cleanup, r=Manishe…
29adca6 
…arth

rustdoc: Yet more intra-doc links cleanup

r? `@Manishearth`
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 15, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022
@matthiaskrgr
Rollup merge of rust-lang#92635 - camelid:yet-more-cleanup, r=Manishe…
878c0c4 
…arth

rustdoc: Yet more intra-doc links cleanup

r? ``@Manishearth``
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 15, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 15, 2022
@matthiaskrgr
Rollup merge of rust-lang#92635 - camelid:yet-more-cleanup, r=Manishe…
bcd2ad5 
…arth

rustdoc: Yet more intra-doc links cleanup

r? ```@Manishearth```
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 15, 2022
Closed
@matthiaskrgr
Copy link
Member

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

@bors 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
@camelid
Copy link
Member Author

camelid commented Jan 15, 2022

@bors r=Manishearth

@bors
Copy link
Contributor

bors commented Jan 15, 2022

📌 Commit 554c765 has been approved by Manishearth

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 16, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2022
@bors
Auto merge of rust-lang#92970 - matthiaskrgr:rollup-tcx7cfb, r=matthi…
bd3cb52 
…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
@bors bors merged commit e1b9439 into rust-lang:master Jan 16, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 16, 2022
@camelid camelid deleted the yet-more-cleanup branch January 17, 2022 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov approved these changes

@Manishearth Manishearth Manishearth approved these changes

Assignees

@Manishearth Manishearth

Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
8 participants
@camelid @rust-highfive @rust-log-analyzer @petrochenkov @Manishearth @bors @matthiaskrgr @rustbot