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

Mention first and last macro in backtrace #98320

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

compiler-errors
Copy link
Member

Slight improvement to diagnostic mentioning what macro an error originates from. Not sure if it's worthwhile.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 21, 2022
@rust-highfive
Copy link
Collaborator

r? @nagisa

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2022
@rust-log-analyzer

This comment has been minimized.

@nagisa
Copy link
Member

nagisa commented Jun 25, 2022

Yeah, not entirely sure either. On one hand the first macro is going to be underlined already so naming it is probably not adding all that much in terms of new information.

But if we do go forward with it I would probably rephrase the message to “first macro! which eventually expands to last macro!”...

r? @estebank may have stronger opinions either way.

@rust-highfive rust-highfive assigned estebank and unassigned nagisa Jun 25, 2022
@estebank
Copy link
Contributor

I think it is worth it because when the text doesn't refer to the code as written, it confuses people. Having that extra redundancy makes it clearer what's going on. I agree that using "expands" instead of "calls" is better.

r=me after that change

@compiler-errors
Copy link
Member Author

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jun 28, 2022

📌 Commit 1075d8b7c27ae83b5e484f1b0cd0caf5858e2948 has been approved by estebank

@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 Jun 28, 2022
@compiler-errors
Copy link
Member Author

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 28, 2022
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 29, 2022

📌 Commit 5b3f391 has been approved by estebank

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 29, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 29, 2022
…estebank

Mention first and last macro in backtrace

Slight improvement to diagnostic mentioning what macro an error originates from. Not sure if it's worthwhile.
@matthiaskrgr
Copy link
Member

@bors r-
this needs clippy tests updated: #98689 (comment)

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 29, 2022
@@ -4,7 +4,7 @@ error[E0381]: borrow of possibly-uninitialized variable: `i`
LL | println!("{}", i);
| ^ use of possibly-uninitialized `i`
|
= note: this error originates in the macro `$crate::format_args_nl` (in Nightly builds, run with -Z macro-backtrace for more info)
= note: this error originates in the macro `println` which expands to macro `$crate::format_args_nl` (in Nightly builds, run with -Z macro-backtrace for more info)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
= note: this error originates in the macro `println` which expands to macro `$crate::format_args_nl` (in Nightly builds, run with -Z macro-backtrace for more info)
= note: this error originates in the macro `$crate::format_args_nl` which is expanded from the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

I think it would be better to name the inner macro first?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I prefer the outer macro first, because that's the one the user is actually familiar with.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is not always the case though, right? It might even be more common for errors to occur in user-written macros.

Copy link
Member Author

@compiler-errors compiler-errors Jul 6, 2022

Choose a reason for hiding this comment

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

I'm not sure if I understand. The first macro in the macro expansion is the one that's present in surface code -- presumably the user is familiar with it (or at least has seen it before) because they had to have written it for it to appear in code. The last macro in the expansion, at least for foreign macros, is less likely to be familiar because it can be some detail or inner macro. I don't think it's as useful to mention it first, since I'm worried the user's eyes will have glazed over when they read $crate::format_args_nl and before they see println.

Copy link
Contributor

@camsteffen camsteffen Jul 6, 2022

Choose a reason for hiding this comment

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

Here is an example.

macro_rules! foo {
    () => (1 + x);
}

macro_rules! bar {
    () => (1 + foo!());
}

fn main() {
    bar!();
}

Current error:

error[[E0425]](https://doc.rust-lang.org/stable/error-index.html#E0425): cannot find value `x` in this scope
  --> src/main.rs:2:16
   |
2  |     () => (1 + x);
   |                ^ not found in this scope
...
10 |     bar!();
   |     ------ in this macro invocation
   |
   = note: this error originates in the macro `foo` (in Nightly builds, run with -Z macro-backtrace for more info)

With this PR currently, the note would say

note: this error originates in the macro `bar` which expands to macro `foo`

This is misleading. The error is in foo, not bar.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about

note: the macro `bar` expands to macro `foo`, where the error originates from

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll just go with @camsteffen's ordering. I'm don't really want to refactor the code to make the new phrasing work 😅

@compiler-errors
Copy link
Member Author

Maybe this new wording is more reasonable? It's a bit long though.

@compiler-errors compiler-errors 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 Jul 8, 2022
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 18, 2022

📌 Commit 2d7d40d6320c0867190462328dba93515447e180 has been approved by estebank

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 Jul 18, 2022
@estebank
Copy link
Contributor

We can iterate on the wording later :)

@compiler-errors
Copy link
Member Author

compiler-errors commented Jul 18, 2022

Thanks @estebank ❤️! I fear there's like a 99% chance this no longer merges because of UI tests added since 11 days ago, so let me rebase this first after work today. I'd hate for this to mess up a rollup because of failing tests.

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 18, 2022
@compiler-errors
Copy link
Member Author

@bors r=estebank

@bors
Copy link
Contributor

bors commented Jul 19, 2022

📌 Commit 01b2379 has been approved by estebank

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 19, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 19, 2022
…estebank

Mention first and last macro in backtrace

Slight improvement to diagnostic mentioning what macro an error originates from. Not sure if it's worthwhile.
This was referenced Jul 19, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2022
Rollup of 8 pull requests

Successful merges:

 - rust-lang#97183 (wf-check generators)
 - rust-lang#98320 (Mention first and last macro in backtrace)
 - rust-lang#99335 (Use split_once in FromStr docs)
 - rust-lang#99347 (Use `LocalDefId` in `OpaqueTypeKey`)
 - rust-lang#99392 (Fix debuginfo tests.)
 - rust-lang#99404 (Use span_bug for unexpected field projection type)
 - rust-lang#99410 (Update invalid atomic ordering lint)
 - rust-lang#99434 (Fix `Skip::next` for non-fused inner iterators)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit af13e55 into rust-lang:master Jul 19, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 19, 2022
JohnTitor added a commit to JohnTitor/rust-semverver that referenced this pull request Jul 20, 2022
Signed-off-by: Yuki Okushi <jtitor@2k36.org>
JohnTitor added a commit to JohnTitor/rust-semverver that referenced this pull request Jul 20, 2022
Signed-off-by: Yuki Okushi <jtitor@2k36.org>
bors added a commit to rust-lang/rust-semverver that referenced this pull request Jul 20, 2022
Rustup to rust-lang/rust#98320

Fixes #332

Signed-off-by: Yuki Okushi <jtitor@2k36.org>
@compiler-errors compiler-errors deleted the macro-backtrace branch August 11, 2023 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants