Keep cfg_attr cfg predicates when expanding proc-macros so it can be used by the doc_cfg feature

[GuillaumeGomez]

View all comments

Fixes #103300.

After long last, this PR finally allows the derive proc-macro cfg_attr cfg predicates information to be kept so rustdoc can use it for its doc_cfg feature (cc #43781).

We currently convert cfg_attr into a cfg_attr_trace which doesn't contain the cfg predicates information anymore. This PR saves the cfg predicates into the CfgAttrTrace (like we do for CfgTrace) so it can be used in other locations. Then, when we expand the derive proc-macros, it adds the cfg_attr_trace attributes to the items generated by the expansion.

I think this could be improved further by converting the cfg_attr into a cfg on the generated items when we're doing the expansion, but I think the current PR is already big enough so it can be done in a follow-up.

Thanks a lot for the pointer @petrochenkov!

@bors r? @petrochenkov

[rust-bors]

Unknown command "r?". Run @bors help or go to https://bors.rust-lang.org/help to see available commands.

[rustbot]

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann, @JonathanBrouwer

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann, @JonathanBrouwer

[rustbot]

Error: Parsing assign command in comment failed: specify user to assign to (if you want to reroll without a specific reviewer in mind, use @rustbot reroll) when parsing: commands.
[!]

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #triagebot on Zulip.

[JonathanBrouwer]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: ef879b2 (ef879b23043f71c3be6a4e0eb4b6b184372653d8)
Base parent: 71c6416 (71c64160bd0f471bffdbdf7061fd62bab053d5b4)

[rust-timer]

Finished benchmarking commit (ef879b2): comparison URL.

Overall result: ❌✅ regressions and improvements - please read:

Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf.

Next, please: If you can, justify the regressions found in this try perf run in writing along with @rustbot label: +perf-regression-triaged. If not, fix the regressions and do another perf run. Neutral or positive results will clear the label automatically.

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

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	3
Regressions ❌ (secondary)	0.1%	[0.0%, 0.3%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.2%, -0.0%]	7
All ❌✅ (primary)	0.2%	[0.2%, 0.3%]	3

Max RSS (memory usage)

Results (primary -0.5%, secondary 2.1%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.3%	[2.3%, 2.3%]	1
Regressions ❌ (secondary)	2.1%	[1.2%, 3.7%]	3
Improvements ✅ (primary)	-1.9%	[-2.2%, -1.6%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.5%	[-2.2%, 2.3%]	3

Cycles

Results (primary -2.1%, secondary 0.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.2%	[1.5%, 7.7%]	9
Improvements ✅ (primary)	-2.1%	[-2.1%, -2.1%]	1
Improvements ✅ (secondary)	-4.3%	[-11.9%, -1.3%]	5
All ❌✅ (primary)	-2.1%	[-2.1%, -2.1%]	1

Binary size

Results (primary 0.1%, secondary 0.0%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	0.1%	[0.0%, 0.2%]	41
Regressions ❌ (secondary)	0.0%	[0.0%, 0.2%]	14
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[0.0%, 0.2%]	41

Bootstrap: 490.207s -> 489.844s (-0.07%)
Artifact size: 388.42 MiB -> 390.41 MiB (0.51%)

[rustbot]

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

[GuillaumeGomez]

CI is happy so PR ready for review.

[GuillaumeGomez]

Finally fixed CI. rustdoc-json-types was able to trigger all unreachable!(), added FIXME comments to come back to it later.

Going to run perf again to see if there is any change in perf impact.

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 1cb4141 (1cb4141ad54852d265476fea05ed1f82fbaaf368)
Base parent: 5503df8 (5503df87342a73d0c29126a7e08dc9c1255c46ad)

[rust-timer]

Finished benchmarking commit (1cb4141): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

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

Instruction count

This perf run didn't have relevant results for this metric.

Max RSS (memory usage)

Results (primary -2.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.3%	[-2.3%, -2.3%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.3%	[-2.3%, -2.3%]	1

Cycles

Results (primary 2.5%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	2.5%	[2.2%, 2.9%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.5%	[2.2%, 2.9%]	2

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 487.42s -> 489.273s (0.38%)
Artifact size: 389.91 MiB -> 389.87 MiB (-0.01%)

[GuillaumeGomez]

Fixed the performance regression, nice. This time it's ready @petrochenkov. :)

[GuillaumeGomez]

Applied suggestions and fixed the rustdoc-json-type issues. Added code comment about it.

[petrochenkov]

#158943 came up with a different solution for a similar problem - relying on expn_that_defined.

If we see an impl item, from its expn_that_defined we know both which derive invocation (if any) generated it, and which struct/enum item to inspect for the relevant cfg_attr trace. All of that can be done in rustdoc, if rustc preserves the necessary data in EarlyParsedAttribute::CfgAttrTrace.

(I don't want to merge this PR in its current form, unless absolutely necessary, it's 100% technical debt and if we continue doing this we'll end up in the same unsalvageable state as rustdoc.)

[GuillaumeGomez]

Sounds good to me! Gonna wait for it to be merged then.

[rust-bors]

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