Treat macros as HIR items

[inquisitivecrystal]

Macros have historically been treated differently from other items at the HIR level. This PR makes them just like normal items. There are a few special cases left over, which I've attempted to lay out below. By normalizing the treatment of macro items, this PR simplifies a fair bit of code and fixes some bugs at the same time. For more information, see #87406.

r? @cjgillot

Backwards incompatibility

This is backwards incompatible in one small way. Due to a mistake, it was previously possible to apply stability attributes to an exported macro, even without enabling the staged_api feature. This never should have worked. After this PR, it will error, as it should. We could make it a warning instead, but that would require a special case for a feature that shouldn't ever have worked and is likely used by no or very few crates, so I'm not thrilled about the idea.

Notes for reviewers

Commit seperation

I'd recommend reviewing this PR commit by commit. The commit chunking wasn't perfect, but it's better than looking at the combined diff, which is quite overwhelming. The compiler and standard library build after each commit, although tests do not necessarily pass and tools do not necessarily build till the end of the series.

Special cases

There are a few special cases that remain after this change. Here are the notable ones I remember:

1. Visibility works a bit differently for macro_rules! macros than other items, since they aren't generally marked with pub but instead with #[macro_export].
2. Since #[macro_export] macros always have paths at the top level of the crate, some additional handling needs to be done on the reexport to top level.

Performance impact

I don't know for sure, but theses changes may slightly hurt performance. They create more work for the compiler in a few places. For instance, some operations that were previously run only for exported macros are now run for all macros. A perf run is probably advisable. For all I know we'll see performance improvements instead. :)

Issues resolved

This resolves #87406 (the tracking issue for this change). It also fixes several bugs:

Fixes #59306.
Fixes #73754.
Fixes #87257.

[inquisitivecrystal]

I'm consulting with the rustdoc team about what the ideal behavior is here. Link to the Zulip thread.

[cjgillot]

Thanks @inquisitivecrystal. I will take a look when I get back.
@bors try @rust-timer queue

[bors]

⌛ Trying commit 19b208c with merge 2a825dbd5f82b9c9b0213aa4be81e09172d8122c...

[bors]

☀️ Try build successful - checks-actions
Build commit: 2a825dbd5f82b9c9b0213aa4be81e09172d8122c (2a825dbd5f82b9c9b0213aa4be81e09172d8122c)

[LeSeulArtichaut]

Looks like rust-timer didn't queue this PR: @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[petrochenkov]

Two general comments:

• I'm not sure literally moving #[macro_export] macros to the root in def collector and AST lowering is the best alternative.
  For all purposes a #[macro_export] macro_rules! item exists at the point at which it's written in source code (for #[allow] attributes, for example) and behaves like non-exported macro_rules! items, except that it inserts some kind of "reexport" of itself to the root module during metadata encoding.
• The is_exported property as it is defined now doesn't really makes sense and shouldn't be kept as a flag in HIR (unless perhaps temporary, for preserving bug-compatibility). Visibility/reachability properties are already correctly provided for all macros with visibility and privacy_access_levels queries and otherwise exported macro_rules should be treated identically to non-exported macro_rules (modulo the metadata encoding bit mentioned above).

[petrochenkov]

...and behaves like non-exported macro_rules! items, except that it inserts some kind of "reexport" of itself to the root module during metadata encoding.

This would be similar to AST-level treatment of #[macro_export] macro_rules! where it's treated as a regular macro_rules! item, except that it adds an additional NameBinding to the root during name resolution (see fn define_macro in resolve).

[cjgillot]

@rust-timer build 2a825dbd5f82b9c9b0213aa4be81e09172d8122c

[rust-timer]

Queued 2a825dbd5f82b9c9b0213aa4be81e09172d8122c with parent c6094fc, future comparison URL.

[inquisitivecrystal]

Two general comments:

• I'm not sure literally moving #[macro_export] macros to the root in def collector and AST lowering is the best alternative.
  For all purposes a #[macro_export] macro_rules! item exists at the point at which it's written in source code (for #[allow] attributes, for example) and behaves like non-exported macro_rules! items, except that it inserts some kind of "reexport" of itself to the root module during metadata encoding.

It was your idea in the first place! :) Seriously though, I'd be thrilled if I could stop hoisting #[macro_export] macro_rules! items to the top level. It's both the messiest and least maintainable part of my code.

That being said, I don't really know what to replace it with. The main reason this was necessary was that rustdoc needed to see exported macros at the top level rather than where they're defined. Rustdoc works solely from the HIR: it doesn't see anything we do to the metadata. How do I make rustdoc put #[macro_export] macros in the right place?

Also, I'm not sure that it's necessary to do anything during metadata encoding. As I recall, everything was building before I moved exported macros to the top level; the only reason I needed to move the items was to get rustdoc to work right. Am I missing something?

• The is_exported property as it is defined now doesn't really makes sense and shouldn't be kept as a flag in HIR (unless perhaps temporary, for preserving bug-compatibility). Visibility/reachability properties are already correctly provided for all macros with visibility and privacy_access_levels queries and otherwise exported macro_rules should be treated identically to non-exported macro_rules (modulo the metadata encoding bit mentioned above).

HIR normally contains the visibility of an item, through item.vis.node.is_pub(). This doesn't work correctly for macros, because #[macro_export] macros are always public regardless of their vis. It feels a bit weird to have less information available in the HIR for macros than other items. In practical terms though, I guess I could have a go of replacing the places I use is_exported with the queries and let you know if I bump into problems?

[petrochenkov]

@inquisitivecrystal

As I recall, everything was building before I moved exported macros to the top level; the only reason I needed to move the items was to get rustdoc to work right. Am I missing something?

Macros are implemented through hacks in one more place - they are actually encoded to metadata as reexports and not as regular items.
You can see in fn encode_info_for_mod how module_exports are encoded, and those module_exports table is filled in fn finalize_resolutions_in, where it's called export_map and where macro bindings (macro items and #[macro_export] macro_rules root bindings) are added to it in addition to regular use imports.

If rustdoc can document reexports (and it can, probably through the item_children query), then it can document exported macros as well.
I suspect that rustdoc simply needs to skip hir::Item macros unless --document-private-items is enabled, and continue documenting them through reexports.

In practical terms though, I guess I could have a go of replacing the places I use is_exported with the queries and let you know if I bump into problems?

Yeah, and if that ends up being non-trivial, it can be deferred to a separate PR.

[petrochenkov]

except that it inserts some kind of "reexport" of itself to the root module during metadata encoding

I suspect that this is the only place where we want to continue encoding macros with reexports, otherwise it would be good to remove the aforementioned hack and stop adding macros to the module_exports lists in resolve.

[petrochenkov]

HIR normally contains the visibility of an item, through item.vis.node.is_pub(). This doesn't work correctly for macros, because #[macro_export] macros are always public regardless of their vis.

If the "syntactic" visibility is used for something after lowering to HIR instead of tcx.visibility(), then it's likely an unnoticed bug.

[inquisitivecrystal]

HIR normally contains the visibility of an item, through item.vis.node.is_pub(). This doesn't work correctly for macros, because #[macro_export] macros are always public regardless of their vis.

If the "syntactic" visibility is used for something after lowering to HIR instead of tcx.visibility(), then it's likely an unnoticed bug.

That's interesting to hear. Should that be switched to tcx.visibility()?

[rust-timer]

Finished benchmarking try commit (2a825dbd5f82b9c9b0213aa4be81e09172d8122c): comparison url.

Summary: This change led to significant regressions 😿 in compiler performance.

• Large regression in instruction counts (up to 3.6% on incr-unchanged builds of unicode_normalization)

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[inquisitivecrystal]

Well, that was worse than I was really expecting. I'm hoping that yanking out the hoisting to top level stuff should improve things some, though I don't know how much. I guess we'll have to see.

[bors]

☀️ Test successful - checks-actions
Approved by: cjgillot
Pushing 05cccdc to master...

[petrochenkov]

The macro-document-private-duplicate.rs test introduced in this PR causes issues - #89228.