-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Conversation
r? @nagisa (rust-highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
98791ad
to
a2ecc13
Compare
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 “ r? @estebank may have stronger opinions either way. |
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 |
a2ecc13
to
1075d8b
Compare
@bors r=estebank |
📌 Commit 1075d8b7c27ae83b5e484f1b0cd0caf5858e2948 has been approved by |
@bors r- |
1075d8b
to
5b3f391
Compare
@bors r+ |
📌 Commit 5b3f391 has been approved by |
…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.
@bors r- |
5b3f391
to
4c9b1bf
Compare
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
= 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😅
4c9b1bf
to
2d7d40d
Compare
Maybe this new wording is more reasonable? It's a bit long though. |
@bors r+ |
📌 Commit 2d7d40d6320c0867190462328dba93515447e180 has been approved by It is now in the queue for this repository. |
We can iterate on the wording later :) |
2d7d40d
to
01b2379
Compare
@bors r=estebank |
…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.
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
Signed-off-by: Yuki Okushi <[email protected]>
Signed-off-by: Yuki Okushi <[email protected]>
Rustup to rust-lang/rust#98320 Fixes #332 Signed-off-by: Yuki Okushi <[email protected]>
Slight improvement to diagnostic mentioning what macro an error originates from. Not sure if it's worthwhile.