[rustdoc] Add support new bang macro kinds

[GuillaumeGomez]

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:

1. Handle ItemKind::Macro differently and iter through its MacroKinds values.
2. Add new placeholder variants in the ItemKind enum, which means that when we encounter them in rendering, we need to ignore them. It also makes the ItemKind enum bigger (and also needs more code to be handled). Another downside is that it needs to be handled in the JSON output.

I implemented 1. in the first commit and 2. in the third commit so if we don't to keep 2., I can simply remove it. I personally have no preference for one or the other since the first is "simpler" but it's easy to forget to go through the macro kinds whereas the second needs a lot more code.

Now there was an interesting improvement that came with this PR in the html::render::print_item::item_module function: I simplified its implementation and split the different parts in a HashMap where 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 an Option<section> around.

cc @joshtriplett

[rustbot]

r? @notriddle

rustbot has assigned @notriddle.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha, @lolbinarycat

[lolbinarycat]

I can't really say much about the actual logic correctness (it seems mostly fine?), but i do have a few nitpicks about naming, datatype structure and logic clarity.

It was also a bit weird reviewing this with the two alternate implementations, since as far as I can tell there's a few lines of code that are common between both impls, but actually have different semantics between them.

[lolbinarycat]

@rustbot author

[GuillaumeGomez]

@rustbot ready

[bors]

☔ The latest upstream changes (presumably #138907) made this pull request unmergeable. Please resolve the merge conflicts.

[GuillaumeGomez]

ping @lolbinarycat

[lolbinarycat]

Please add a test that demonstrates how these interact with search item kind filters, and also with kind disambiguators in intra-doc links. I'm not entirely sure what the ideal behavior is in either case, but we definitely should have tests for whatever it is.

[rustbot]

This PR was rebased onto a different master 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]

Very good idea. Should have thought about it. Added the new rustdoc-js test which checks that as well (and fixed the merge conflict while I was at it).

[GuillaumeGomez]

Fixed the search result href.

[lolbinarycat]

The test needs to be updated to include the href field.

[GuillaumeGomez]

Arf, the git checkout -- in the middle was not the right move. ^^'

Added back what I removed.

[lolbinarycat]

Few more things about test coverage:

1. macro_derive is not used in the test. I'd also like to see what happens when a single macro is all 3.
2. Intra-doc link disambiguators are still not tested. This would go in the rustdoc test suite.

[GuillaumeGomez]

Few more things about test coverage:

1. `macro_derive` is not used in the test.  I'd also like to see what happens when a single macro is all 3.

2. Intra-doc link disambiguators are still not tested.  This would go in the `rustdoc` test suite.

1. Damn I completely missed it. Added tests for it as well, good catch!

2. From the first comment:

   This PR voluntarily doesn't touch anything related to intra-doc links, I'll send a follow-up if needed.

   So considering how big this PR is already, I think it's better to wait for a follow-up. ;)

With this the PR should be finally ready.

[lolbinarycat]

Test still technically doesn't cover every combination of MacroKinds, but it's probably good enough.

And yeah, leaving intra-doc links for later seems reasonable, as long as its intentional.

[notriddle]

Now that binarycat points it out, I notice that it's not just the tests not covering every combination of MacroKinds. The code itself can't do it, even though the compiler can.

If it can't do it, then I think the design is flawed.

Instead of generating placeholders and trying to cram multiple macro kinds into a single clean::Item, maybe it should instead take the easy road and just generate duplicate pages? That way, you don't have to do any of this plumbing, and, anyway, this feature is quite niche and most crates will only want to support one mode per macro.

[lolbinarycat]

This is unreachable unless new macro kinds are added. a downside of the bitflags crate is it kinda ruins exhaustiveness checking for cases like this, due to having no distinction between a set of flags and a single flag. something like enumset on the other hand, doesn't have this limitation.

[notriddle]

There's no case here that covers this:

macro_rules! macro {
    attr() () => {};
    derive() () => {};
}

Such a macro has MacroKinds::DERIVE | MacroKinds::ATTR, but since it doesn't have MacroKinds::BANG, it falls through to the todo!() line.

[notriddle]

I implemented #148005, which should fix this problem. It also adds a bit less code.

[lolbinarycat]

are you misunderstanding what bitflags iter does?

...am i misunderstanding what bitflags iter does?

[notriddle]

I'm not sure? It says it does this:

https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir/def/struct.MacroKinds.html#method.iter

Yield a set of contained flags values.

Each yielded flags value will correspond to a defined named flag. Any unknown bits will be yielded together as a final flags value.

So, assuming there aren't any unknown bits (if there are, it should fall into the default match arm and panic), and assuming that "contained" means "if I call contains() it'll return true" (I don't know what else it would mean), this should do what I want, which is to iterate through each MacroKind that is turned on for that particular macro.

In any case, I copied over most of the same test cases, and they pass.

[GuillaumeGomez]

Instead of generating placeholders and trying to cram multiple macro kinds into a single clean::Item, maybe it should instead take the easy road and just generate duplicate pages? That way, you don't have to do any of this plumbing, and, anyway, this feature is quite niche and most crates will only want to support one mode per macro.

It's exactly what I wanted to avoid ^^' (hence all the extra code)

However if we add support for /// on the attr/derive branches, then this approach makes more sense.

[GuillaumeGomez]

As said in #148005, although I find the result of @notriddle's approach less good (ie, duplicated files generated for the same macro), considering how much easier the implementation is, I'd prefer for #148005 to be merged instead. Closing this one.