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

Reduce verbosity of errors involving builtin macros #106526

Closed
wants to merge 1 commit into from

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jan 6, 2023

Do not show note "this error originates in" when it is caused by one in an explicit block list of builtin macros. Most notably, println! and friends will stop doing so.

@rustbot
Copy link
Collaborator

rustbot commented Jan 6, 2023

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added 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. labels Jan 6, 2023
Do not show note "this error originates in" when it is caused by one in
an explicit block list of builtin macros. Most notably, `println!` and
friends will stop doing so.
@petrochenkov
Copy link
Contributor

I disagree with adding this kind of hacks to macro expansion infra, this PR does more harm than good.

Comment on lines +370 to +371
"assert" | "bench" | "cfg" | "concat" | "concat_idents" | "env" | "format_args"
| "format_args_nl" | "global_allocator" | "include_str" | "test",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't love the hardcoded list of macro names. It feels to me like what these have in common, and why we might want to suppress the subdiagnostic, is that these act more like a regular function call that returns a value than a macro that expands into multiple statements which you would potentially want a macro-backtrace expansion for.

Perhaps we should have an internal rustc attribute that controls this behavior which can be applied to the appropriate macros in the std? That feels like it could be a cleaner solution than hardcoding the macro names at this place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't love the hardcoded list of macro names.

That's fair, I would like to hide the note for all builtin macros at some point, just audit that the derive macros in particular don't have bad corner cases (didn't see any in the test suite, but they have occurred in the past). The idea behind the blocklist was to test this out in a controlled manner, and see if we have to back off from hiding this information.

Initially I tried using a solution based on DefIds, but doing any kind of querying in the Emitter was a non-starter. That being said, maybe we could inject a trait object callable to allow Emitter to have queries. If that were to work, then we can accomplish the same behavior with annotations on the macros. I do not know at this point how feasible it would be to do so.

@bors
Copy link
Collaborator

bors commented Jan 9, 2023

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

@apiraino
Copy link
Contributor

apiraino commented Feb 2, 2023

Hi, what's the status of this pull request? Needs more design work or is the idea to fold the proposal? thanks

@estebank
Copy link
Contributor Author

estebank commented Feb 2, 2023

@apiraino I need to change the approach. I can close and retry later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants