Open
Conversation
Collaborator
|
Some changes occurred in HTML/CSS/JS. |
rustbot
added
A-rustdoc-js
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
rustdoc-bang-attr-macro
branch
from
20f13be to
4b35c49
Compare
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
rustdoc-bang-attr-macro
branch
from
4b35c49 to
4416897
Compare
Member
Author
|
Oh and also cc @lolbinarycat as you reviewed the original PR too. :) |
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
rustdoc-bang-attr-macro
branch
from
4416897 to
be7815f
Compare
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
rustdoc-bang-attr-macro
branch
from
be7815f to
3e05767
Compare
This comment has been minimized.
This comment has been minimized.
Member
Author
|
Fixed merge conflicts. |
This comment has been minimized.
This comment has been minimized.
Contributor
|
blocking this on #154902, since that relies on |
GuillaumeGomez
force-pushed
the
rustdoc-bang-attr-macro
branch
from
3e05767 to
ce98d4e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
mejrs
reviewed
Apr 7, 2026
GuillaumeGomez
force-pushed
the
rustdoc-bang-attr-macro
branch
from
ce98d4e to
5ada01c
Compare
GuillaumeGomez
force-pushed
the
rustdoc-bang-attr-macro
branch
from
5ada01c to
26f32d9
Compare
Collaborator
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
GuillaumeGomez
force-pushed
the
rustdoc-bang-attr-macro
branch
from
26f32d9 to
eef2a58
Compare
GuillaumeGomez
force-pushed
the
rustdoc-bang-attr-macro
branch
from
eef2a58 to
cbe154c
Compare
Member
Author
|
Ready for review. |
lolbinarycat
requested changes
Apr 22, 2026
Contributor
There was a problem hiding this comment.
After reading through everything it seems there's still some level of conflation between declarative (macro_rules! and v2 macros) and procedural macros.
I still feel like our data model for macros is a bit of a mess, and there doesn't seem to be a clear vision of what the desired end state is, at least not one I have access to.
The search handling I'm also unsure about the design of. Remember we have itemParents now, that could be useful here.
I also worry about if cross-crate inlining breaks any of this stuff. In general, I think the test coverage is a bit lacking, there are only 7 potential cases1, but not a single test suite covers all of them.
View changes since this review
Footnotes
-
I guess doubled to 14 if you want to do all of them through cross-crate reexports too ↩
Comment on lines
+2909
to
+2910
| MacroKinds::ATTR => clean_proc_macro(item, &mut name, MacroKind::Attr, cx.tcx), | ||
| MacroKinds::DERIVE => clean_proc_macro(item, &mut name, MacroKind::Derive, cx.tcx), |
Contributor
There was a problem hiding this comment.
I think this is wrong, a macro_rules! macro should be able to go into this branch, which does not seem ideal.
Do we really not have a way of just... checking if something is a proc macro or not? can clean_proc_macro handle being given a non-proc macro?
Member
Author
There was a problem hiding this comment.
In this case, if macro_rules! containing only an attr or a derive, we treat them the same as their proc-macro equivalent.
Contributor
There was a problem hiding this comment.
is that the desired behavior? it seems somewhat odd.
Comment on lines
+2902
to
+2907
| MacroKinds::BANG => MacroItem( | ||
| Macro { | ||
| source: display_macro_source(cx.tcx, name, macro_def), | ||
| macro_rules: macro_def.macro_rules, | ||
| }, | ||
| MacroKinds::BANG, |
Contributor
There was a problem hiding this comment.
This branch is strictly redundant with the last branch, and can just be removed.
Comment on lines
+104
to
+105
| BangMacroAttribute = 28, | ||
| BangMacroDerive = 29, |
Contributor
There was a problem hiding this comment.
These feel like misnomers, a "bang macro" generally refers to a fn-like macro, called with !(), not just anything declared with macro_rules!. also could definitely benefit from doc comments.
Member
Author
There was a problem hiding this comment.
It's about how they're declared, not how they're used. Big +1 for the missing docs, adding that.
Contributor
There was a problem hiding this comment.
problem is that's not what "bang macro" conventionally means, and i would rather not introduce more instances of "this term is used completely differently within rustdoc".
i would prefer we just use "DeclMaro", as both of these can be grouped under "declarative macros"
Comment on lines
+772
to
+774
| pub(crate) fn is_macro_rules(&self) -> bool { | ||
| matches!(self.kind, ItemKind::MacroItem(..)) | ||
| } |
Contributor
There was a problem hiding this comment.
This seems incorrect, shouldn't we be checking the macro_rules field?
Member
Author
There was a problem hiding this comment.
I keep forgetting that we can declare bang macros like functions. I'll rename the function and add docs to clarify what it checks.
Comment on lines
+620
to
+622
| tokens.next(); | ||
| tokens.next(); | ||
| tokens.next(); |
Contributor
There was a problem hiding this comment.
maybe some assertions or debug assertions here so it doesn't silently break horribly if macro syntax changes again?
Comment on lines
-789
to
+797
| block("attr", "attributes", "Attribute Macros"); | ||
| block("attr", "attribute-macros", "Attribute Macros"); |
Contributor
There was a problem hiding this comment.
Once again, not a fan of bugfixes that break links being buried within a larger change like this.
Member
Author
There was a problem hiding this comment.
Needed for tests.
Contributor
There was a problem hiding this comment.
ah... it might be better to rename the other block, as that should break drastically less links
| * But the documentation lives in a single `macro.NAME.html` page, and | ||
| * this boolean flag is used for generating that HREF. | ||
| */ | ||
| isBangMacro: boolean, |
Contributor
There was a problem hiding this comment.
I really don't like this field name.
Member
Author
There was a problem hiding this comment.
Open to suggestions. :)
Contributor
There was a problem hiding this comment.
the problem is the current semantics are "is this a non-proc macro that is usable in multiple ways or is fn-like"
i think naming it based on purpose rather than on definition would be good, so forceMacroHref.
Comment on lines
+1663
to
+1669
| if (item.ty === 28 || item.ty === 29) { | ||
| // "proc attribute" is 23, "proc derive" is 24 whereas "bang macro attribute" is 28 and | ||
| // "bang macro derive" is 29, so 5 of difference to go from the latter to the former. | ||
| item.ty -= 5; | ||
| item.isBangMacro = true; | ||
| } | ||
| return item; |
Contributor
There was a problem hiding this comment.
ah, so this is the point of adding those two enum variants...
I do wonder if there's a better way to encode this...
Member
Author
There was a problem hiding this comment.
Likely not but very interested if you have ideas.
Contributor
There was a problem hiding this comment.
we could overload the last field even more x3
| let href; | ||
| let traitPath = null; | ||
| const type = itemTypesName[item.ty]; | ||
| const type = item.entry && item.entry.isBangMacro ? "macro" : itemTypesName[item.ty]; |
Contributor
There was a problem hiding this comment.
so, these hybrid macros will show up with attr: and derive: searches but will have macro in the url? seems confusing.
Member
Author
There was a problem hiding this comment.
That's the whole debate we had 2 rustdoc meetings ago. The alternative was to have a file for each kind of the macro but this approach was rejected.
Contributor
There was a problem hiding this comment.
derive and attr are children of macro, so it should be fine.
Comment on lines
+6
to
+25
| /// An attribute bang macro. | ||
| #[macro_export] | ||
| macro_rules! attr_macro { | ||
| attr() () => {}; | ||
| () => {}; | ||
| } | ||
|
|
||
| /// A derive bang macro. | ||
| #[macro_export] | ||
| macro_rules! derive_macro { | ||
| derive() () => {}; | ||
| () => {}; | ||
| } | ||
|
|
||
| #[macro_export] | ||
| macro_rules! one_for_all_macro { | ||
| attr() () => {}; | ||
| derive() () => {}; | ||
| () => {}; | ||
| } |
Contributor
There was a problem hiding this comment.
what about something of the shape of:
#[macro_export]
macro_rules! attr_macro {
attr() () => {};
}or
#[macro_export]
macro_rules! one_for_all_macro {
attr() () => {};
derive() () => {};
}I suspect the former may show up as a proc macro.
Member
Author
There was a problem hiding this comment.
I confirm it would. Adding a test to cover them.
Member
Author
There was a problem hiding this comment.
Ah it's already tested in tests/rustdoc-html/macro/macro-attr.rs actually. :)
rustbot
added
S-waiting-on-author
Member
Author
|
Went through all comments and pushed improvements and suggestions (and a new test). So if I didn't miss anything, only tests missing are for cross-crate, right? |
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
rustdoc-bang-attr-macro
branch
from
6058428 to
76a6846
Compare
Member
Author
|
The |
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Contributor
|
☔ The latest upstream changes (presumably #155745) made this pull request unmergeable. Please resolve the merge conflicts. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
Since it seems like I can't reopen #145458, opening this one. Although, it's the same PR minus the last new commit to handle a comment that was left unresolved in the original PR. All relevant details are still in the original PR though.
It's an alternative (and likely a take-over) of #148005 since lang-team rejected the idea to add documentation on macro branches, making the multiple files approach less suitable.
This implements #145153 in rustdoc. This PR voluntarily doesn't touch anything related to intra-doc links, I'll send a follow-up if needed.
So now about the implementation itself: this is a weird case where a macro can be different things at once but still only gets one file generated. I saw two ways to implement this:
ItemKind::Macrodifferently and iter through itsMacroKindsvalues.ItemKindenum, which means that when we encounter them in rendering, we need to ignore them. It also makes theItemKindenum bigger (and also needs more code to be handled). Another downside is that it needs to be handled in the JSON output.Now there was an interesting improvement that came with this PR in the
html::render::print_item::item_modulefunction: I simplified its implementation and split the different parts in aHashMapwhere the key is the item type. So then, we can just iterate through the keys and open/close the section at each iteration instead of keeping anOption<section>around.From RFCs:
macro_rules!) attribute macros (RFC 3697) #144579macro_rules!) derive macros (RFC 3698) #145208derive:
attr:
both:
r? @notriddle