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

Fix rendering of reexported macros 2.0 and fix visibility of reexported items #86841

Merged
merged 7 commits into from
Jul 12, 2021
Merged

Fix rendering of reexported macros 2.0 and fix visibility of reexported items #86841

merged 7 commits into from
Jul 12, 2021

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jul 3, 2021 •
edited
Loading

So, this PR grew a bit out of focus, it does the following things:

  • Fixes rustdoc: Re-exported macros 2.0 are rendered as macro_rules! #86276.
  • Fixes visibility display for reexported items: it now takes the visibility of the "use" statement rather than the visibility of the reexported item itself).
  • Fixes the display of reexported items if "--document-private-items" option is used. Before, they were simply skipped.
  • Fixes inconsistency on typedef items: they didn't display their visibility contrary to other items.

I added tests to check everything listed above.

cc @camelid @ollie27 (in case one of you want to review?)

r? @jyn514

@GuillaumeGomez GuillaumeGomez added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jul 3, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2021
@jyn514 jyn514 changed the title Reexported macro 2 render Rustdoc: Fix rendering of reexported macros 2.0 Jul 3, 2021
@jyn514
Copy link
Member

jyn514 commented Jul 3, 2021

Both this and #86282 basically rewrite the macro rendering, I would rather not review both at once. I plan to review the other first since it's a more serious bug.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jyn514 jyn514 added S-blocked Status: Marked as 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 Jul 3, 2021
@GuillaumeGomez
Copy link
Member Author

Both this and #86282 basically rewrite the macro rendering, I would rather not review both at once. I plan to review the other first since it's a more serious bug.

It's fine, let's wait until #86282 is merged! I'll mark the PR as blocked for the time being and give a try to improve the code by removing the duplication.

@GuillaumeGomez
Copy link
Member Author

I unified it into a function in the end. :)

@bors

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

Rebased on #86282.

@GuillaumeGomez GuillaumeGomez added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Jul 5, 2021
@jyn514 jyn514 mentioned this pull request Jul 5, 2021
Merged
jyn514
jyn514 approved these changes Jul 5, 2021
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.

LGTM with the nit about display_macro_source fixed

src/librustdoc/clean/utils.rs Show resolved Hide resolved
src/librustdoc/clean/mod.rs Outdated
Visibility::Restricted(krate)
}
ast::VisibilityKind::Restricted { id, .. } => {
let did = cx.enter_resolver(|r| r.local_def_id(id)).to_def_id();
Copy link
Member

Choose a reason for hiding this comment

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

It's unfortunate that this has to call into the resolver :/ I'm hoping to get rid of it at some point: #83761. But we already call load_macro_untracked above, so hopefully fixing that will allow removing this impl at the same time.

@jyn514
Copy link
Member

jyn514 commented Jul 5, 2021

Oh I forgot - can you add a test for pub(crate) and pub(self) macros? Right now impl Clean for ast::Visibility isn't tested at all.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 5, 2021
@JohnTitor
Rollup merge of rust-lang#86886 - jyn514:no-clean-symbol, r=Guillaume…
952deae 
…Gomez

Remove `impl Clean for {Ident, Symbol}`

These were only used once, in a place where it was trivial to replace.
Also, it's unclear what 'clean' would mean for these, so it seems better
to be explicit.

Found while reviewing rust-lang#86841, which makes the same change to `build_macro`, so the two will conflict.

r? `@GuillaumeGomez`
@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2021
@GuillaumeGomez
Copy link
Member Author

Adding the requested tests then.

@bors

This comment has been minimized.

Sign in to view
@GuillaumeGomez GuillaumeGomez changed the title Rustdoc: Fix rendering of reexported macros 2.0 Fix rendering of reexported macros 2.0 and fix visibility of reexported items Jul 6, 2021
@GuillaumeGomez
Copy link
Member Author

I updated the PR and the first comment as well to explain what I did here. I don't know why I expected less things to be broken around reexports... Anyway, that should clean things up quite a lot.

Stupremee
Stupremee approved these changes Jul 7, 2021
View reviewed changes
src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved
src/test/rustdoc/reexports-priv.rs Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

@Stupremee Fixed the issue you spotted and added a test to prevent a regression.

@GuillaumeGomez
Copy link
Member Author

@bors: r=Stupremee

@bors
Copy link
Contributor

bors commented Jul 12, 2021

📌 Commit 74d71c2 has been approved by Stupremee

@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 Jul 12, 2021
@bors
Copy link
Contributor

bors commented Jul 12, 2021

⌛ Testing commit 74d71c2 with merge 3a24abd...

@bors
Copy link
Contributor

bors commented Jul 12, 2021

☀️ Test successful - checks-actions
Approved by: Stupremee
Pushing 3a24abd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 12, 2021
@bors bors merged commit 3a24abd into rust-lang:master Jul 12, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jul 12, 2021
@GuillaumeGomez GuillaumeGomez deleted the reexported-macro-2-render branch July 12, 2021 14:39
This was referenced Jul 12, 2021
Merged
Merged
@jyn514 jyn514 mentioned this pull request Nov 8, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 approved these changes

@camelid camelid camelid left review comments

@Stupremee Stupremee Stupremee approved these changes

Assignees

@jyn514 jyn514

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.55.0
Development

Successfully merging this pull request may close these issues.

rustdoc: Re-exported macros 2.0 are rendered as macro_rules!
8 participants
@GuillaumeGomez @jyn514 @rust-log-analyzer @bors @camelid @Stupremee @rust-highfive @rustbot