Rename -Zexternal-macro-backtrace to -Zmacro-backtrace and clean up implementation. #67359

eddyb · 2019-12-16T20:22:10Z

This is my attempt at dealing with #66364 (comment), although I'm not sure it's the least disruptive one.

The behavior of -Zexternal-macro-backtrace was already to enable full macro backtraces for all macros, the only part of it that was specific to cross-crate macros was showing this when not used:

note: this error originates in a macro outside of the current crate
  (in Nightly builds, run with -Z external-macro-backtrace for more info)

After this PR:

the flag is renamed to -Zmacro-backtrace
- do we need to have a deprecation period? cc @rust-lang/compiler
the message informing you about the flag is always shown when an expansion of a bang macro/attribute/derive is involved, not just cross-crate ones
- this accounts for most of the changes in tests
- we could perhaps only show it for the bang macro case? feels odd for derives
fix_multispans_in_std_macros is split into fix_multispans_in_extern_macros and render_multispans_macro_backtrace
- this roughly reverts the non-behavioral parts of Use spans for -Z external-macro-backtrace #46605, which combined the two functionalities
- not sure where the old std_macros name came from, perhaps the <std macros> synthetic "file"? even then, odd that std specifically was mentioned
render_multispan_macro_backtrace, by default (i.e. without -Zmacro-backtrace), hides the in this macro invocation label specifically to avoid redundancy in the diagnostic
- that is, showing the macro use site is only useful when the diagnostic is inside the macro definition and the user can't otherwise tell which use site it applies to, not when the diagnostic is at/inside the use site already (which would make the label redundant)
- before, it was only checking for the situation in which a cross-crate macro definition span would be replaced with the invocation span, which both made the connection to redundancy unobvious, and didn't help with other redundancy (e.g. when the diagnostic was pointing to an argument inside the macro invocation)
- this accounts for the remaining test changes, which I've first noticed in Cleanup rmeta::MacroDef #66364 (comment) but only later understood as part of this PR (hence the "redundancy" descriptions)

This PR is not needed for #66364, but it would help, as after this PR there's only one .span_to_filename(...).is_macros() check (i.e. for <... macros> synthetic "files") left in rustc_errors, and it's much more self-contained.

r? @petrochenkov

src/bootstrap/builder.rs

petrochenkov · 2019-12-29T13:25:50Z

Meta: some mix between functional and formatting changes here, perhaps it will look better if rebased on master where everything is rustfmt-ed.

src/librustc_errors/emitter.rs

hdhoang · 2020-01-09T07:55:51Z

ping from triage, @eddyb can you rebase and apply reviewers' comment, thanks!

nikomatsakis · 2020-01-14T10:51:50Z

On this topic:

the flag is renamed to -Zmacro-backtrace
do we need to have a deprecation period? cc @rust-lang/compiler

I think this flag is used by folks? It seems like at minimum we should announce the change in a PSA on Inside Rust. A deprecation period would be polite, but it's probably not required.

I guess we don't really document -Z flags in the rustc book, so we don't have to update that (though I sometimes think that is not the best policy).

petrochenkov · 2020-01-22T18:39:48Z

Blocked on #68407.

@petrochenkov

rustc_span: return an impl Iterator instead of a Vec from macro_backtrace. Having `Span::macro_backtrace` produce an `impl Iterator<Item = ExpnData>` allows #67359 to use it instead of rolling its own similar functionality. The move from `MacroBacktrace` to `ExpnData` (which the first two commits are prerequisites for) both eliminates unnecessary allocations, and is strictly more flexible (exposes more information). r? @petrochenkov

…plicitly.

…macros.

eddyb · 2020-02-06T19:48:50Z

@bors r=petrochenkov

bors · 2020-02-06T19:48:52Z

📌 Commit 96af578 has been approved by petrochenkov

@petrochenkov

… r=petrochenkov Rename -Zexternal-macro-backtrace to -Zmacro-backtrace and clean up implementation. This is my attempt at dealing with rust-lang#66364 (comment), although I'm not sure it's the least disruptive one. The behavior of `-Zexternal-macro-backtrace` was already to enable full macro backtraces for *all* macros, the only part of it that was specific to cross-crate macros was showing this when *not used*: ``` note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) ``` After this PR: * the flag is renamed to `-Zmacro-backtrace` * do we need to have a deprecation period? cc @rust-lang/compiler * the message informing you about the flag is always shown when an expansion of a bang macro/attribute/derive is involved, not just cross-crate ones * this accounts for most of the changes in tests * we could perhaps only show it for the bang macro case? feels odd for derives * `fix_multispans_in_std_macros` is split into `fix_multispans_in_extern_macros` and `render_multispans_macro_backtrace` * this roughly reverts the non-behavioral parts of rust-lang#46605, which combined the two functionalities * not sure where the old `std_macros` name came from, perhaps the `<std macros>` synthetic "file"? even then, odd that `std` specifically was mentioned * `render_multispan_macro_backtrace`, by default (i.e. without `-Zmacro-backtrace`), hides the `in this macro invocation` label specifically to avoid redundancy in the diagnostic * that is, showing the macro use site is only useful when the diagnostic is inside the macro definition and the user can't otherwise tell which use site it applies to, not when the diagnostic is at/inside the use site already (which would make the label redundant) * before, it was only checking for the situation in which a cross-crate macro *definition* span would be replaced with the invocation span, which both made the connection to redundancy unobvious, and didn't help with other redundancy (e.g. when the diagnostic was pointing to an argument inside the macro invocation) * this accounts for the remaining test changes, which I've first noticed in rust-lang#66364 (comment) but only later understood as part of this PR (hence the "redundancy" descriptions) This PR is not needed for rust-lang#66364, but it would help, as after this PR there's only one `.span_to_filename(...).is_macros()` check (i.e. for `<... macros>` synthetic "files") left in `rustc_errors`, and it's much more self-contained. r? @petrochenkov

@ghost

Rollup of 6 pull requests Successful merges: - #67359 (Rename -Zexternal-macro-backtrace to -Zmacro-backtrace and clean up implementation.) - #68524 (Generator Resume Arguments) - #68791 (implement proper linkchecker hardening) - #68886 (Mark fn map_or() as eagerly evaluated.) - #68888 (error code examples: replace some more ignore with compile_fail) - #68894 (Update E0565 examples) Failed merges: r? @ghost

bors · 2020-02-07T03:09:58Z

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

Rustup rust-lang/rust#67359 cc rust-lang/rust#67359 changelog: none

rust-highfive assigned petrochenkov Dec 16, 2019

eddyb mentioned this pull request Dec 16, 2019

Cleanup rmeta::MacroDef #66364

Merged