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.

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

Do not suggest using -Zmacro-backtrace for builtin macros #138379

Merged
merged 1 commit into from
Mar 15, 2025

Conversation

estebank
Copy link
Contributor

For macros that are implemented on the compiler, or that are annotated with rustc_diagnostic_item, which have arbitrary implementations from the point of view of the user and might as well be intrinsics, we do not mention the -Zmacro-backtrace flag. This includes derives and standard macros like panic! and format!.

This PR adds a field to every Span's ExpnData stating whether it comes from a builtin macro. This is determined by the macro being annotated with either #[rustc_builtin_macro] or #[rustc_diagnostic_item]. An alternative to using these attributes that already exist for other uses would be to introduce another attribute like #[rustc_no_backtrace] to have finer control on which macros are affected (for example, an error within vec![] now doesn't mention the backtrace, but one could make the case that it should). Ideally, instead of carrying this information in the ExpnData we'd instead try to query the DefId of the macro (that is already stored) to see if it is annotated in some way, but we do not have access to the TyCtxt from rustc_errors.

r? @petrochenkov

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver) labels Mar 11, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 11, 2025

This PR modifies run-make tests.

cc @jieyouxu

@estebank estebank force-pushed the macro-backtrace-note branch from 91abd6e to 70fc288 Compare March 11, 2025 23:48
@estebank estebank force-pushed the macro-backtrace-note branch 2 times, most recently from 9a97100 to 6a7ddb6 Compare March 11, 2025 23:52
@estebank
Copy link
Contributor Author

This is similar to #106526 where you stated "I disagree with adding this kind of hacks to macro expansion infra, this PR does more harm than good." without clarification on whether the concern is with modifying ExpnData to carry this information, or with the hardcoded list that the earlier PR had.

@rust-log-analyzer

This comment has been minimized.

@estebank estebank force-pushed the macro-backtrace-note branch from 6a7ddb6 to aaff81e Compare March 12, 2025 01:50
@rustbot
Copy link
Collaborator

rustbot commented Mar 12, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 12, 2025
@bors
Copy link
Contributor

bors commented Mar 12, 2025

⌛ Trying commit aaff81e with merge f3fd6c6...

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 12, 2025
Do not suggest using `-Zmacro-backtrace` for builtin macros

For macros that are implemented on the compiler, or that are annotated with `rustc_diagnostic_item`, which have arbitrary implementations from the point of view of the user and might as well be intrinsics, we do *not* mention the `-Zmacro-backtrace` flag. This includes `derive`s and standard macros like `panic!` and `format!`.

This PR adds a field to every `Span`'s `ExpnData` stating whether it comes from a builtin macro. This is determined by the macro being annotated with either `#[rustc_builtin_macro]` or `#[rustc_diagnostic_item]`. An alternative to using these attributes that already exist for other uses would be to introduce another attribute like `#[rustc_no_backtrace]` to have finer control on which macros are affected (for example, an error within `vec![]` now doesn't mention the backtrace, but one could make the case that it should). Ideally, instead of carrying this information in the `ExpnData` we'd instead try to query the `DefId` of the macro (that is already stored) to see if it is annotated in some way, but we do not have access to the `TyCtxt` from `rustc_errors`.

r? `@petrochenkov`
@bors
Copy link
Contributor

bors commented Mar 12, 2025

☀️ Try build successful - checks-actions
Build commit: f3fd6c6 (f3fd6c6e54616979f79b504602b55b06e0d96e0e)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (f3fd6c6): comparison URL.

Overall result: no relevant changes - no action needed

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 may lead to changes in compiler perf.

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

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary -1.6%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
1.4% [1.4%, 1.4%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-4.7% [-4.7%, -4.7%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.6% [-4.7%, 1.4%] 2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary 0.1%, secondary 0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Bootstrap: 779.667s -> 779.217s (-0.06%)
Artifact size: 365.24 MiB -> 365.26 MiB (0.01%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 12, 2025
@petrochenkov
Copy link
Contributor

I'm fine with doing this for compiler-implemented macros with no actual source code, we already have a flag for those - SyntaxExtension::builtin_name.

I still disagree with doing this for standard library macros though.
They are not a part of "just my code" for sure, but that's no different from any other third party macros, and we shouldn't invent any hacks to specifically recognize them.
-Zmacro-backtrace on them still shows additional details, still maybe be useful if you are interested in them, and is still worth recommending.

There can be some system for hiding the details, similar to #[collapse_debuginfo], but I don't think it is worth it in this case.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2025
@estebank estebank force-pushed the macro-backtrace-note branch from aaff81e to 197d2db Compare March 14, 2025 16:58
@rust-log-analyzer

This comment has been minimized.

@estebank estebank force-pushed the macro-backtrace-note branch from 197d2db to c37a10c Compare March 14, 2025 18:52
@rustbot
Copy link
Collaborator

rustbot commented Mar 14, 2025

The Miri subtree was changed

cc @rust-lang/miri

@estebank
Copy link
Contributor Author

Made it so that we only hide the note when it comes from an intrinsic.

@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 14, 2025
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2025
For macros that are implemented on the compiler, we do *not* mention the `-Zmacro-backtrace` flag. This includes `derive`s and standard macros.
@estebank estebank force-pushed the macro-backtrace-note branch from c37a10c to f0b8e13 Compare March 14, 2025 19:50
@estebank estebank added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 14, 2025
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 14, 2025

📌 Commit f0b8e13 has been approved by petrochenkov

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2025
@bors
Copy link
Contributor

bors commented Mar 15, 2025

⌛ Testing commit f0b8e13 with merge adea7cb...

@bors
Copy link
Contributor

bors commented Mar 15, 2025

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing adea7cb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 15, 2025
@bors bors merged commit adea7cb into rust-lang:master Mar 15, 2025
7 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 15, 2025
Copy link

This is an experimental post-merge analysis report. You can ignore it.

Post-merge report

Comparing 2828650 (base) -> adea7cb (this PR)

Test differences

No test diffs found

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (adea7cb): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results (primary 0.1%, secondary 0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Bootstrap: 772.05s -> 773.052s (0.13%)
Artifact size: 365.07 MiB -> 365.08 MiB (0.00%)

@RalfJung
Copy link
Member

It seems like the PR description does not reflect the fact that the PR changed course before being landed? Or maybe I just entirely misunderstood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants