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: Cleanup doc link resolution #96345

Merged
merged 9 commits into from
May 14, 2022
Merged

rustdoc: Cleanup doc link resolution #96345

merged 9 commits into from
May 14, 2022

Conversation

petrochenkov
Copy link
Contributor

See individual commits for specific changes

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Apr 23, 2022
@rust-highfive
Copy link
Collaborator

r? @notriddle

(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 Apr 23, 2022
@petrochenkov petrochenkov mentioned this pull request Apr 23, 2022
Merged
jyn514
jyn514 approved these changes Apr 23, 2022
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment •
edited

Choose a reason for hiding this comment

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

This is really awesome ❤️ thanks!

src/librustdoc/passes/collect_intra_doc_links.rs Show resolved Hide resolved
src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
notriddle
notriddle reviewed Apr 23, 2022
View reviewed changes
Copy link
Contributor

@notriddle notriddle left a comment

Choose a reason for hiding this comment

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

Overall, these changes seem pretty good, though, admittedly, I'm not an expert on most of this code.

r? jyn514

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
src/librustdoc/passes/collect_intra_doc_links.rs Show resolved Hide resolved
@notriddle
Copy link
Contributor

Try rolling the dice for a different reviewer who knows this code better?

r? rust-lang/rustdoc

@rust-highfive rust-highfive assigned jsha and unassigned notriddle Apr 25, 2022
jyn514
jyn514 approved these changes Apr 25, 2022
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

r? jyn514

This change looks good to me.

src/librustdoc/passes/collect_intra_doc_links.rs Outdated Show resolved Hide resolved
@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Apr 25, 2022
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 25, 2022
@bors

This comment was marked as resolved.

Sign in to view
@bors

This comment was marked as resolved.

Sign in to view
@petrochenkov petrochenkov mentioned this pull request May 13, 2022
Merged
@petrochenkov
rustdoc: Remove ResolutionFailure::NoParentItem
51eda74 
It's a bug and not an error
@petrochenkov
rustdoc: Remove ResolutionFailure::MalformedGenerics
c979ef5 
in favor of `PreprocessingError::MalformedGenerics`
@petrochenkov
rustdoc: Remove ResolutionFailure::Dummy
95fb05d 
The variant resolution check didn't make sense, and derive trait collision could be processed in a different way
@petrochenkov
rustdoc: Remove fn resolve_macro
7256c6f 
and otherwise unify resolution in macro namespace and other namespaces
@petrochenkov
rustdoc: Do not create UrlFragments until they are necessary
7c2283d 
This simplifies error types and allows to remove `fn resolve_inner` and `fn check_full_res`

`visited_links` caching is not touched for now
@petrochenkov
rustdoc: Do not resolve associated item paths unnecessarily
7689857
@petrochenkov
rustdoc: Use Visibility::is_public more
a9c14dc
@petrochenkov
Fix rebase
6c5c7f5
@petrochenkov
resolve: Move collection of all macro_rules in the crate to rustdoc
9ba5281
@jyn514
Copy link
Member

jyn514 commented May 13, 2022

@notriddle I don't think any current team member knows this code better than you, except maybe @camelid.

@notriddle
Copy link
Contributor

This all seems correct to me. I especially like that it reduces the number of LoC overall!

I tried building some of the crates that regressed on crater to make sure the macro_rules fallback still works. It seemed to work.

@bors r+

@bors
Copy link
Contributor

bors commented May 13, 2022

📌 Commit 9ba5281 has been approved by notriddle

@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 May 13, 2022
@bors
Copy link
Contributor

bors commented May 14, 2022

⌛ Testing commit 9ba5281 with merge 2d69117...

@bors
Copy link
Contributor

bors commented May 14, 2022

☀️ Test successful - checks-actions
Approved by: notriddle
Pushing 2d69117 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 14, 2022
@bors bors merged commit 2d69117 into rust-lang:master May 14, 2022
@rustbot rustbot added this to the 1.62.0 milestone May 14, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (2d69117): comparison url.

Summary:

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regression found
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 1 0 0 0
mean2 N/A 0.4% N/A N/A N/A
max N/A 0.4% N/A N/A N/A

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notriddle notriddle notriddle left review comments

@jyn514 jyn514 jyn514 approved these changes

Assignees

@jsha jsha

Labels
merged-by-bors This PR was explicitly merged by bors. 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.62.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@petrochenkov @rust-highfive @notriddle @bors @jyn514 @rust-timer @jsha @rustbot