-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Stop compilation early if macro expansion failed #144409
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
Stop compilation early if macro expansion failed #144409
Conversation
b6bd61c
to
3ec0e1a
Compare
Removed split ui tests to trigger old errors but in separate files. |
3ec0e1a
to
faa28f9
Compare
r=me with nit (should allow reverting the changes to the |
This comment has been minimized.
This comment has been minimized.
faa28f9
to
72e4a6b
Compare
@bors r=oli rollup |
Got the handle wrong... @bors r=oli-obk |
💡 This pull request was already approved, no need to approve it again.
|
…ly-abort, r=oli-obk Stop compilation early if macro expansion failed Fixes rust-lang#116180. So there isn't really a type that is central for macro expansion and some errors are actually emitted (because the resolution happens after the expansion I suppose) after the expansion pass (like "not found macro"). Sometimes, errors are only emitted on the second "try" (to improve error output). So I couldn't reach a similar solution than what was done in rust-lang#133937 and suggested by `@estebank` in rust-lang#116180 (comment). But maybe I missed something? So in the end, I realized that there is method called every time (except one, described below) a macro error is actually emitted: `ExtCtxt::trace_macros_diag`. Considering I updated what it did, I renamed it into `macro_error_and_trace_macros_diag` to better reflect it. There is only one call of `trace_macros_diag` which isn't reporting an error but just used for `macro_trace` feature, so I kept it as is. r? `@oli-obk`
Rollup of 13 pull requests Successful merges: - #144356 (Add `ignore-backends` annotations in failing GCC backend ui tests) - #144359 (add codegen test for variadics) - #144376 (Suggest unwrapping when private method name is available in inner type) - #144379 (test using multiple c-variadic ABIs in the same program) - #144383 (disable cfg.has_reliable_f128 on amdgcn) - #144409 (Stop compilation early if macro expansion failed) - #144412 (Small cleanup: Use LocalKey<Cell> methods more) - #144421 (Call `is_parsed_attribute` rather than keeping track of a list of parsed attributes manually) - #144422 (library/windows_targets: Fix macro expansion error in 'link' macro) - #144424 (Allow setting `release-blog-post` label with rustbot) - #144430 (tests: aarch64-outline-atomics: Remove hardcoded target) - #144435 (rustc-dev-guide subtree update) - #144445 (Fix `./x check bootstrap` (again)) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 13 pull requests Successful merges: - #144356 (Add `ignore-backends` annotations in failing GCC backend ui tests) - #144359 (add codegen test for variadics) - #144376 (Suggest unwrapping when private method name is available in inner type) - #144379 (test using multiple c-variadic ABIs in the same program) - #144383 (disable cfg.has_reliable_f128 on amdgcn) - #144409 (Stop compilation early if macro expansion failed) - #144412 (Small cleanup: Use LocalKey<Cell> methods more) - #144421 (Call `is_parsed_attribute` rather than keeping track of a list of parsed attributes manually) - #144422 (library/windows_targets: Fix macro expansion error in 'link' macro) - #144424 (Allow setting `release-blog-post` label with rustbot) - #144430 (tests: aarch64-outline-atomics: Remove hardcoded target) - #144435 (rustc-dev-guide subtree update) - #144445 (Fix `./x check bootstrap` (again)) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 13 pull requests Successful merges: - #144356 (Add `ignore-backends` annotations in failing GCC backend ui tests) - #144359 (add codegen test for variadics) - #144376 (Suggest unwrapping when private method name is available in inner type) - #144379 (test using multiple c-variadic ABIs in the same program) - #144383 (disable cfg.has_reliable_f128 on amdgcn) - #144409 (Stop compilation early if macro expansion failed) - #144412 (Small cleanup: Use LocalKey<Cell> methods more) - #144421 (Call `is_parsed_attribute` rather than keeping track of a list of parsed attributes manually) - #144422 (library/windows_targets: Fix macro expansion error in 'link' macro) - #144424 (Allow setting `release-blog-post` label with rustbot) - #144430 (tests: aarch64-outline-atomics: Remove hardcoded target) - #144435 (rustc-dev-guide subtree update) - #144445 (Fix `./x check bootstrap` (again)) r? `@ghost` `@rustbot` modify labels: rollup
Failed in #144450. |
@bors r- |
72e4a6b
to
a0d6db3
Compare
…=<try> Stop compilation early if macro expansion failed
a0d6db3
to
2725138
Compare
@bors r=oli-obk |
…ly-abort, r=oli-obk Stop compilation early if macro expansion failed Fixes rust-lang#116180. So there isn't really a type that is central for macro expansion and some errors are actually emitted (because the resolution happens after the expansion I suppose) after the expansion pass (like "not found macro"). Sometimes, errors are only emitted on the second "try" (to improve error output). So I couldn't reach a similar solution than what was done in rust-lang#133937 and suggested by `@estebank` in rust-lang#116180 (comment). But maybe I missed something? So in the end, I realized that there is method called every time (except one, described below) a macro error is actually emitted: `ExtCtxt::trace_macros_diag`. Considering I updated what it did, I renamed it into `macro_error_and_trace_macros_diag` to better reflect it. There is only one call of `trace_macros_diag` which isn't reporting an error but just used for `macro_trace` feature, so I kept it as is. r? `@oli-obk`
Rollup of 10 pull requests Successful merges: - #144359 (add codegen test for variadics) - #144409 (Stop compilation early if macro expansion failed) - #144422 (library/windows_targets: Fix macro expansion error in 'link' macro) - #144430 (tests: aarch64-outline-atomics: Remove hardcoded target) - #144445 (Fix `./x check bootstrap` (again)) - #144453 (canonicalize build root in `tests/run-make/linker-warning`) - #144454 (move uefi test to run-make) - #144464 (Only run bootstrap tests in `x test` on CI) - #144495 (bump cargo_metadata) - #144500 (thread name in stack overflow message) r? `@ghost` `@rustbot` modify labels: rollup
…ly-abort, r=oli-obk Stop compilation early if macro expansion failed Fixes rust-lang#116180. So there isn't really a type that is central for macro expansion and some errors are actually emitted (because the resolution happens after the expansion I suppose) after the expansion pass (like "not found macro"). Sometimes, errors are only emitted on the second "try" (to improve error output). So I couldn't reach a similar solution than what was done in rust-lang#133937 and suggested by ``@estebank`` in rust-lang#116180 (comment). But maybe I missed something? So in the end, I realized that there is method called every time (except one, described below) a macro error is actually emitted: `ExtCtxt::trace_macros_diag`. Considering I updated what it did, I renamed it into `macro_error_and_trace_macros_diag` to better reflect it. There is only one call of `trace_macros_diag` which isn't reporting an error but just used for `macro_trace` feature, so I kept it as is. r? ``@oli-obk``
Rollup of 9 pull requests Successful merges: - #144359 (add codegen test for variadics) - #144409 (Stop compilation early if macro expansion failed) - #144422 (library/windows_targets: Fix macro expansion error in 'link' macro) - #144430 (tests: aarch64-outline-atomics: Remove hardcoded target) - #144445 (Fix `./x check bootstrap` (again)) - #144453 (canonicalize build root in `tests/run-make/linker-warning`) - #144454 (move uefi test to run-make) - #144464 (Only run bootstrap tests in `x test` on CI) - #144495 (bump cargo_metadata) r? `@ghost` `@rustbot` modify labels: rollup
…ly-abort, r=oli-obk Stop compilation early if macro expansion failed Fixes rust-lang#116180. So there isn't really a type that is central for macro expansion and some errors are actually emitted (because the resolution happens after the expansion I suppose) after the expansion pass (like "not found macro"). Sometimes, errors are only emitted on the second "try" (to improve error output). So I couldn't reach a similar solution than what was done in rust-lang#133937 and suggested by ```@estebank``` in rust-lang#116180 (comment). But maybe I missed something? So in the end, I realized that there is method called every time (except one, described below) a macro error is actually emitted: `ExtCtxt::trace_macros_diag`. Considering I updated what it did, I renamed it into `macro_error_and_trace_macros_diag` to better reflect it. There is only one call of `trace_macros_diag` which isn't reporting an error but just used for `macro_trace` feature, so I kept it as is. r? ```@oli-obk```
Rollup of 13 pull requests Successful merges: - #144359 (add codegen test for variadics) - #144379 (test using multiple c-variadic ABIs in the same program) - #144383 (disable cfg.has_reliable_f128 on amdgcn) - #144409 (Stop compilation early if macro expansion failed) - #144422 (library/windows_targets: Fix macro expansion error in 'link' macro) - #144429 (Enable outline-atomics for aarch64-unknown-linux-musl) - #144430 (tests: aarch64-outline-atomics: Remove hardcoded target) - #144445 (Fix `./x check bootstrap` (again)) - #144453 (canonicalize build root in `tests/run-make/linker-warning`) - #144464 (Only run bootstrap tests in `x test` on CI) - #144470 (clif: Don't set the `compiler-builtins-no-f16-f128` feature) - #144480 (Revert "coverage: Enlarge empty spans during MIR instrumentation, not codegen") - #144495 (bump cargo_metadata) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 13 pull requests Successful merges: - #141840 (If `HOME` is empty, use the fallback instead) - #144359 (add codegen test for variadics) - #144379 (test using multiple c-variadic ABIs in the same program) - #144383 (disable cfg.has_reliable_f128 on amdgcn) - #144409 (Stop compilation early if macro expansion failed) - #144422 (library/windows_targets: Fix macro expansion error in 'link' macro) - #144429 (Enable outline-atomics for aarch64-unknown-linux-musl) - #144430 (tests: aarch64-outline-atomics: Remove hardcoded target) - #144445 (Fix `./x check bootstrap` (again)) - #144453 (canonicalize build root in `tests/run-make/linker-warning`) - #144464 (Only run bootstrap tests in `x test` on CI) - #144470 (clif: Don't set the `compiler-builtins-no-f16-f128` feature) - #144480 (Revert "coverage: Enlarge empty spans during MIR instrumentation, not codegen") r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #144409 - GuillaumeGomez:macro-expansion-early-abort, r=oli-obk Stop compilation early if macro expansion failed Fixes #116180. So there isn't really a type that is central for macro expansion and some errors are actually emitted (because the resolution happens after the expansion I suppose) after the expansion pass (like "not found macro"). Sometimes, errors are only emitted on the second "try" (to improve error output). So I couldn't reach a similar solution than what was done in #133937 and suggested by ````@estebank```` in #116180 (comment). But maybe I missed something? So in the end, I realized that there is method called every time (except one, described below) a macro error is actually emitted: `ExtCtxt::trace_macros_diag`. Considering I updated what it did, I renamed it into `macro_error_and_trace_macros_diag` to better reflect it. There is only one call of `trace_macros_diag` which isn't reporting an error but just used for `macro_trace` feature, so I kept it as is. r? ````@oli-obk````
(impl $c:ident Trait) => { impl $c Trait {} }; | ||
//~^ ERROR inherent | ||
//~| WARN trait objects without an explicit `dyn` are deprecated | ||
//~| WARN this is accepted in the current edition | ||
(dyn $c:ident Trait) => { dyn $c Trait {} }; //~ ERROR macro expansion |
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.
Wait what?? Wait, why was this modification made? That doesn't make any sense. This destroyed the original test intention.
This is an extremely carefully crafted test case.
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.
Same as previously: to keep an output as close as possible to the original because otherwise the new "early exit if the expansion macro failed" truncated the output.
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 sorry but that's not how it should've been approached. These test cases are all about the macro matchers, it doesn't matter what the diagnostics look like, it's about MATCH vs. NO MATCH, so the ordering, amount of the matchers is extremely important. This changed the meaning of this test case.
For reference, tests like these were added (by me) to prevent (or demonstrate) regressions like the most recent #146417, so it's important keep the coverage 1:1.
I'll restore the test intention in a sec.
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.
Yeah I completely missed the point, my bad. Is there a way to add constraints on a test to prevent this situation from happening again in the future? Like move it into another testsuite maybe?
}; | ||
(const $Trait:path) => {}; | ||
([const] $Trait:path) => {}; | ||
([const] $Trait:path) => { [const] Trait }; |
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.
Wait what?? Wait, why was this modification made? That doesn't make any sense. This destroyed the original test intention.
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.
To keep the same error output, otherwise it was truncated. Sorry if I understood wrongly what the test was checking...
…nds, r=estebank Less greedily parse `[const]` bounds > [!IMPORTANT] > If you're coming here from any beta backport nomination thread on Zulip, only the last commit is truly relevant (the first commit doesn't need to be backported, it only contains test modifications)! Don't consider `[` to start a bound, only consider `[const]` in its entirety to do so. This drastically reduces (but doesn't eliminate!) the chance of *real* breakages. Like `const`, `~const` and `async` before, `[const]` unavoidably brings along theoretical breakages, see preexisting tests: `macro-const-trait-bound-theoretical-regression.rs` and `macro-async-trait-bound-theoretical-regression.rs`. Side note: It's unfortunate that we have to do this but apart from the known fact that MBE hurts forward compatibility, the `[const]` syntax is simply a bit scuffed (also CC'ing rust-lang#146122, section (3)). Fixes [after beta backport] rust-lang#146417. * 1st commit: Restore the original test intentions of several preexisting related tests that were unfortunately lost over time * I've added a bunch of SCREAMING comments to make it less likely to be lost again * CC PR rust-lang#119099 which added most of these tests * CC [rust-lang#144409 (comment)](rust-lang#144409 (comment)) for further context (NB: It's not the only PR that negatively affected the test intention) * 2nd commit: Actually address the regression r? `@oli-obk` or anyone
Rollup merge of #146422 - fmease:less-greedy-maybe-const-bounds, r=estebank Less greedily parse `[const]` bounds > [!IMPORTANT] > If you're coming here from any beta backport nomination thread on Zulip, only the last commit is truly relevant (the first commit doesn't need to be backported, it only contains test modifications)! Don't consider `[` to start a bound, only consider `[const]` in its entirety to do so. This drastically reduces (but doesn't eliminate!) the chance of *real* breakages. Like `const`, `~const` and `async` before, `[const]` unavoidably brings along theoretical breakages, see preexisting tests: `macro-const-trait-bound-theoretical-regression.rs` and `macro-async-trait-bound-theoretical-regression.rs`. Side note: It's unfortunate that we have to do this but apart from the known fact that MBE hurts forward compatibility, the `[const]` syntax is simply a bit scuffed (also CC'ing #146122, section (3)). Fixes [after beta backport] #146417. * 1st commit: Restore the original test intentions of several preexisting related tests that were unfortunately lost over time * I've added a bunch of SCREAMING comments to make it less likely to be lost again * CC PR #119099 which added most of these tests * CC [#144409 (comment)](#144409 (comment)) for further context (NB: It's not the only PR that negatively affected the test intention) * 2nd commit: Actually address the regression r? `@oli-obk` or anyone
Fixes #116180.
So there isn't really a type that is central for macro expansion and some errors are actually emitted (because the resolution happens after the expansion I suppose) after the expansion pass (like "not found macro"). Sometimes, errors are only emitted on the second "try" (to improve error output). So I couldn't reach a similar solution than what was done in #133937 and suggested by @estebank in #116180 (comment). But maybe I missed something?
So in the end, I realized that there is method called every time (except one, described below) a macro error is actually emitted:
ExtCtxt::trace_macros_diag
. Considering I updated what it did, I renamed it intomacro_error_and_trace_macros_diag
to better reflect it.There is only one call of
trace_macros_diag
which isn't reporting an error but just used formacro_trace
feature, so I kept it as is.r? @oli-obk