[beta-1.91] Warn on future errors from temporary lifetimes shortening in Rust 1.92

[dianne]

Pursuant to discussion on Zulip, this implements a future-compatibility warning lint macro_extended_temporary_scopes for errors in Rust 1.92 caused by #145838:

warning: temporary lifetime shortening in Rust 1.92
  --> $DIR/macro-extended-temporary-scopes.rs:54:14
   |
LL |             &struct_temp().field
   |              ^^^^^^^^^^^^^ this expression creates a temporary value...
...
LL |         } else {
   |         - ...which will be dropped at the end of this block in Rust 1.92
   |
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see <https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#macro-extended-temporary-scopes>
   = note: consider using a `let` binding to create a longer lived value

Implementation-wise, this reuses the existing temporary scoping FCW machinery introduced for the tail_expr_drop_order edition lint: this adds BackwardIncompatibleDropHint statements to the MIR at the end of the shortened scopes for affected temporaries; these are then checked in borrowck to warn if the temporary is used after the future drop hint. There are trade-offs here: on one hand, I believe this gives some assurance over ad-hoc pattern-recognition that there are no false positives¹. On the other hand, this fails to lint on future dangling raw pointers and it complicates the potential addition of explanatory diagnostics or suggestions². I'm hopeful that the limitation around dangling pointers won't be relevant in real code, though; the only real instance we've seen of breakage so far is future errors in formatting macro invocations, which this should be able to catch.

Release logistics notes:

• This PR targets the beta branch directly, since the breakage it's a FCW for is landing in the next Rust version.
• Temporary lifetime extension for blocks #146098 undoes the breakage this is a FCW for. If that behavior is merged and stabilizes in Rust 1.92, this PR should be reverted (or shouldn't be merged) in order to avoid spurious warnings.

cc @traviscross

@rustbot label +T-lang

Footnotes

1. In particular, more syntactic approaches are complicated by having to avoid warning on promoted constants; they'd either be full of holes, they'd need a lot of extra logic, or they'd need to hack more MIR-to-HIR mapping into PromoteTemps. ↩

2. It's definitely possible to add more context and a suggestion, but the ways I've thought of to do so are either too hacky or too complex to feel appropriate for a last-minute direct-to-beta lint. ↩

[rustbot]

This PR changes MIR

cc @oli-obk, @RalfJung, @JakobDegen, @vakaras

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[dianne]

A general note: since this isn't targeting master, I've included a fair bit of single-purpose code. It's possible some of the generalizations to BackwardIncompatibleDropHint handling would be good to have for future FCWs, but everything new is made to be one-off and temporary.

Also, a note about the ping for changing the MIR: I believe that's from adding another case to BackwardIncompatibleDropReason.

And about changing MIR transforms: that's from making the significant-drop-reordering part of tail_expr_drop_order only fire on BackwardIncompatibleDropReason::Edition2024. It already checks the edition, so I figure it makes sense to check the BackwardIncompatibleDropReason too.

View changes since this review

[dianne]

I believe this should be avoidable. Let me know if it's worth trying to fix.

[dianne]

Ideally, the reference link shouldn't be tied to the lint's stabilization, but there's not currently a great reference that both includes a complete explanation of the breakage and how to work around it.

Also, I haven't included report_in_deps: true but since the breakage is hitting in the next version it might be warranted.

[dianne]

I've opted to use the "consider using a let binding" note from real borrowck diagnostics for simplicity, rather than providing a machine-applicable suggestion. To illustrate, consider real Rust 1.92 nightly diagnostics for the future breakage here:

error[E0716]: temporary value dropped while borrowed
  --> src/main.rs:18:13
   |
18 |     pin!({ &temp() });
   |     --------^^^^^----
   |     |       |    |
   |     |       |    temporary value is freed at the end of this statement
   |     |       creates a temporary value which is freed while still in use
   |     borrow later stored here
   |
   = note: consider using a `let` binding to create a longer lived value

error[E0716]: temporary value dropped while borrowed
  --> src/main.rs:19:17
   |
19 |     pin!({ &mut [()] });
   |            -----^^^-
   |            |    |  |
   |            |    |  temporary value is freed at the end of this statement
   |            |    creates a temporary value which is freed while still in use
   |            borrow later used here
   |
   = note: consider using a `let` binding to create a longer lived value

Note the differing spans. In many instances, the conflicting use reported in diagnostics is an implicit reborrow rather than the actual use in the macro expansion. I think getting more helpful diagnostics would either require:

• Improving the actual borrowck diagnostic, then using that machinery.
• Forcing maximal MIR-to-HIR mapping via source scopes when forward-incompatible drops are detected, then using bespoke diagnostic information stored earlier in compilation (e.g. the HIR source of forward-incompatible scope extension).

Another awkward aspect of that span imprecision is (without doing either of those things) it's hard to tell when adding a let statement actually can help. In non-pathological cases it should always either help or come with another note that the diagnostic originates in an external macro, so for simplicity, I've opted to always include it.

[PatchMixolydic]

This message seems like it'd be difficult to understand without context — to me, it almost seems like it's been cut off partway through. It might be better to be explicit here:

Suggested change

	diag.primary_message("temporary lifetime shortening in Rust 1.92");
	diag.primary_message("this temporary's lifetime will be shortened starting in Rust 1.92");

[dianne]

It is a bit difficult to understand as-is.. though I wonder if there's a way to write it without "this" in the primary message. My original take was "this temporary value's lifetime will shorten in Rust 1.92", but I moved away from it because something didn't feel right to me about the "this" and it felt redundant with the label. Maybe it would be fine?

[rustbot]

Some changes occurred in match lowering

cc @Nadrieril

[dianne]

I haven't adjusted the wording yet, but I did some renaming and I've added logic to also lint on shortening lifetimes for internal temporaries created by pin! and format_args!. These weren't initially caught since they're implemented in the compiler as local variables that follow temporary scoping rules (hence the match lowering change; that's where variables' drops are scheduled). This is much less likely to be relevant, but it's possible if someone was using format_args! with constant-promotable arguments in if branches in string formatting. e.g.

println!("{:?}{}",
    (),
    if true {
        format_args!("{:?}", Promotable)
    } else {
        format_args!("")
    }
);

It's a much more niche situation admittedly, but it compiles on stable/beta and not nightly (and it wasn't caught previously).

[rustbot]

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

[davidtwco]

r? @jackh726 who reviewed the original PR

[jackh726]

I haven't looked at this too deeply yet, but my general initial thought is this is a lot of code changes for something that is only going to hit beta with no prior testing on nightly. It really concerns me, I'm not sure about other compiler folks.

[samueltardieu]

Clippy changes review: no concerns about the mechanical changes.

View changes since this review

[jackh726]

Still haven't gotten to review this properly, but going to go ahead and add the nomination label to get a feel for what others think.

[traviscross]

We've decided to break this; we want to accept this one-version FCW, and we're waiving our 10-day final comment period.

Obviously it's compiler's call whether to accept the beta backport.

@rfcbot fcp merge

[rust-rfcbot]

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

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

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!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[tmandry]

@rfcbot reviewed

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[nikomatsakis]

@rfcbot reviewed

[joshtriplett]

Approving this, but let's keep a careful eye on the pre-release crater runs since this is a bunch of new code that isn't on top of tree.

[theemathas]

The crater run for 1.91 is already ongoing. Will we need another crater run for specifically this PR?

[apiraino]

Beta backport accepted as per compiler team on Zulip, though it wasn't an easy and clear decision. Normally we'd suggest reverting breakage changes and give us time to bake a fix properly, this time we feel there was not an easy way forward so this backport is not the ideal solution.

@rustbot label +beta-accepted

[cuviper]

Note: since this PR is directly against beta, I'm not going to handle the "backport" as I usually would from T-release. Whoever is reviewing this from the compiler side, please go ahead with r+ when you're ready.

@bors rollup=never