Remove braces from Rust 2015 panic macro

[nbdd0121]

I rediscovered #80846 myself when tweaking panic macros for const panics. I find this discrepancy between core panic, 2021 panic and 2015 std panic confusing and a little bit annoying.

Given that #80846 suggests that the braces is not there for purpose, and edition 2021 crater run does not seem to catch any crates depending on this behaviour (the difference is only triggered when using &panic!()), I think it shall be fine to remove this and align panic macros across editions.

Fix rust-lang/rust-clippy#7723

r? @m-ou-se

[bors]

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

[m-ou-se]

@rfcbot merge

This removes the extra {} from the expansion of std::panic!() in Rust 2015 and Rust 2018. (It is already removed in Rust 2021.)

This is a subtle breaking change:

fn main() {
    let a: &usize;
    a = &panic!();
}

used to compile, and will now fail with:

error[E0308]: mismatched types
 --> src/main.rs:4:9
  |
4 |     a = &panic!();
  |         ^^^^^^^^^ expected `usize`, found `!`
  |
  = note: expected reference `&usize`
             found reference `&!`

We didn't see any case of this during the 2021 crater run when switching to the 2021 panic macro, which also doesn't have this extra {}.

Grepping for &(panic|todo|unimplemented|unreachable)! through all *.rs on crates.io gives zero hits.

cc @rust-lang/lang

[rfcbot]

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[Mark-Simulacrum]

I don't think we should be landing breakage without strong motivation, particularly when we are already using the edition mechanism to transition users to a space where they'll never encounter this inconsistency.

Can we elaborate on the reasoning to do this? The PR description says that the discrepancy is annoying, but it's not clear to me which context that's in; I wouldn't expect any Rust user to encounter it, and I feel like std doesn't really get simpler if we omit a couple of braces on a macro that's unlikely to get touched much. I think we need stronger justification. If we had the opportunity to fully align the macros across all editions, I think that would be a different discussion, but this PR at least doesn't unify any macros in core/std as far as I can tell... and from past discussions my understanding is that isn't possible.

[nbdd0121]

I don't think we should be landing breakage without strong motivation, particularly when we are already using the edition mechanism to transition users to a space where they'll never encounter this inconsistency.

This inconsistency is not current migrated by cargo fix. If we don't fix the discrepancy there will be a perpetual gap for semantics in difference editions.

Can we elaborate on the reasoning to do this? The PR description says that the discrepancy is annoying, but it's not clear to me which context that's in; I wouldn't expect any Rust user to encounter it, and I feel like std doesn't really get simpler if we omit a couple of braces on a macro that's unlikely to get touched much.
I think we need stronger justification. If we had the opportunity to fully align the macros across all editions, I think that would be a different discussion, but this PR at least doesn't unify any macros in core/std as far as I can tell... and from past discussions my understanding is that isn't possible.

I actually discovered this subtle difference when I tried to align macros across all editions; I was experimenting doing thhe following in the rustc_builtin_macros for panic!:

• Check if edition is 2021 or the usage is compatible to 2021 panic:
  • Forward to 2021 macro if it is
• Otherwise it must be a panic!(sth), issue a lint, and forward it to the legacy panic (i.e. panic_any or panic_str).

The rationale behind the attempted change is to simplify the logic of current non_fmt_panic so that it will only need to check for legacy panic calls rather than having to inspect the arguments again.

Appearantly when I make the change the tests break, and I realized this inconsistency, and
a search leads me to #80846. I could change the code to forward 2015 panic to {panic_2021!(...)}, but I feel that we don't really need that discrepancy, and that discrepancy should be eliminated if we want to ensure an edition transistion is as smooth as possible.

[Mark-Simulacrum]

This inconsistency is not current migrated by cargo fix. If we don't fix the discrepancy there will be a perpetual gap for semantics in difference editions.

It's expected that there are gaps forever in between editions; I don't think whether cargo fix changes this really matters for that? Most users (particularly those not upgrading to new editions) are not running cargo fix regardless.

I could change the code to forward 2015 panic to {panic_2021!(...)}, but I feel that we don't really need that discrepancy, and that discrepancy should be eliminated if we want to ensure an edition transistion is as smooth as possible.

The transition would only be made more painful by breaking users on 2015, right? They'd not be able to run cargo fix, as it doesn't support by default running with a broken compilation in the baseline compiler. Edition migrations target the intersection of two editions, not immediately putting you into the new edition.

If there is a major benefit to compiler internals, then we could consider breaking users, so perhaps seeing the benefits there would be a good motivation -- if it's a fairly local increase in complexity, the edition mechanism was an explicit choice in tradeoffs that made the call that we're OK with more complex internals in favor of avoiding breakage.

[m-ou-se]

Another option is changing the language/compiler to make the version without braces work as well in this situation.

[nikomatsakis]

I agree with @Mark-Simulacrum that I would want to see more discussion of the motivation here. Why was this change in Rust 2021, for starters?

[nbdd0121]

Why was this change in Rust 2021, for starters?

core::panic! never had the brace, and Rust 2021 aligns std::panic! towards core::panic!`.

[nbdd0121]

Another option is changing the language/compiler to make the version without braces work as well in this situation.

I think this is a case where brace introduces an opportunity for coercion. It's similar to e.g. this case:

let x: &&dyn Any = &&1; // This doesn't work
let x: &&dyn Any = &{&1}; // This works

[m-ou-se]

Why was this change in Rust 2021, for starters?

@nikomatsakis This is a difference between std::panic and core::panic in Rust 2015/2018, see #80846. In Rust 2021, std::panic and core::panic are the same, removing this difference.

[nikomatsakis]

OK. I see now. I'm still feeling a bit skeptical-- as Marc says, the point of editions is not permit permanent differences between editions, and to ensure that users have to "opt-in" to see them. We don't necessarily mandate cargo fix for changes that are not especially significant, but I am still wary of subtle changes in semantics that will affect people, at least without strong motivation. I have to admit I don't really understand the motivation here yet -- it sounds like it will make the implementation simpler, but I don't understand how. Maybe someone could explain that in more detail?

[yaahc]

Marking this issue as Nominated since it seems that this FCP may be stalled and it seems like it's probably due for discussion.

[nbdd0121]

(from #88860 (comment))

Clippy currently relies on the exact expansion of panic! macros to recognize it (https://github.com/rust-lang/rust-clippy/blob/389a74b31aabca4aac3289763eeb2eccedd1b988/clippy_utils/src/higher.rs#L721), and the brace difference causes clippy to not recognize core::panic! or Rust 2021 panic.

Although the long term goal should be for clippy to not depend on std's implementation details but I think this is another data point showing that the discrepency is bad and the brace should be removed.

[Manishearth]

(clippy team member here) No, we definitely should not fossilize the panic implementation because of clippy, we should fix the resugaring

[yaahc]

(clippy team member here) No, we definitely should not fossilize the panic implementation because of clippy, we should fix the resugaring

I might be misinterpreting @nbdd0121's comment but they seem to be advocating for not fossilizing the panic impl and removing the braces. Is that also what you mean by fixing the resugaring @Manishearth or am I misunderstanding your desired path to resolution?

[Manishearth]

Oh I misinterpreted stuff, sorry. I meant to say: rustc should not avoid updating this because of clippy; and in the long term we need to come up with a better way to do resugaring in rust/clippy so this is never a problem.

[bors]

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

[yaahc]

We discussed this PR during today's libs-api team meeting and our general consensus was that we would prefer to not modify the 2015 panic macro without a strong motivation, and it was not clear to us that this change is strongly motivated. Our reasoning was that this was already fixed as part of the 2021 edition boundary, and so anyone who is negatively affected by this inconsistency can already fix it by using the latest edition starting tomorrow.

It seems that the motivation in this PR is that the discrepancy is confusing and makes continued maintenance and development around panics more difficult. If this continues to be a recurring issue across the project we can revisit this decision, but for now we'd prefer to prioritize avoiding breakage, even if it's only theoretical breakage.

[nbdd0121]

We discussed this PR during today's libs-api team meeting

There isn't much detail in the meeting notes.

From the meeting notes:

Needs some motivation. Looks like there is none. Close it?

Is the clippy thing discussed?

[Mark-Simulacrum]

Clippy has basically a bug, which should be fixed, buy the appropriate fix is in clippy, not changing the standard library implementation in a way that includes at least theoretical breakage. So it's not really motivation for this patch.

[yaahc]

Is the clippy thing discussed?

Yes. Insofar as we noted that the clippy team had advised that we not focus on clippy maintenance difficulties when making decisions about std APIs. @Manishearth correct me if I'm wrong.

[crlf0710]

Closing according to #88919 (comment) .