Rollup merge of #99050 - JakobDegen:storage-docs, r=tmiasko

Clarify MIR semantics of storage statements

Seems worthwhile to start closing out some of the less controversial open questions about MIR semantics. Hopefully this is fairly non-controversial - it's what we implement already, and I see no reason to do anything more restrictive. cc ``@tmiasko`` who commented on this when it was discussed in the original PR that added these docs.

2 files changed, 20 insertions, 14 deletions

diff --git a/compiler/rustc_const_eval/src/transform/validate.rs b/compiler/rustc_const_eval/src/transform/validate.rs
index 2dfe226030a..d3bf6b49f12 100644
--- a/compiler/rustc_const_eval/src/transform/validate.rs
+++ b/compiler/rustc_const_eval/src/transform/validate.rs
@@ -206,7 +206,13 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> {
         }
 
         if self.reachable_blocks.contains(location.block) && context.is_use() {
-            // Uses of locals must occur while the local's storage is allocated.
+            // We check that the local is live whenever it is used. Technically, violating this
+            // restriction is only UB and not actually indicative of not well-formed MIR. This means
+            // that an optimization which turns MIR that already has UB into MIR that fails this
+            // check is not necessarily wrong. However, we have no such optimizations at the moment,
+            // and so we include this check anyway to help us catch bugs. If you happen to write an
+            // optimization that might cause this to incorrectly fire, feel free to remove this
+            // check.
             self.storage_liveness.seek_after_primary_effect(location);
             let locals_with_storage = self.storage_liveness.get();
             if !locals_with_storage.contains(local) {
diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs
index dcc37c565c9..45fc5f24a60 100644
--- a/compiler/rustc_middle/src/mir/syntax.rs
+++ b/compiler/rustc_middle/src/mir/syntax.rs
@@ -237,19 +237,19 @@ pub enum StatementKind<'tcx> {
 
     /// `StorageLive` and `StorageDead` statements mark the live range of a local.
     ///
-    /// Using a local before a `StorageLive` or after a `StorageDead` is not well-formed. These
-    /// statements are not required. If the entire MIR body contains no `StorageLive`/`StorageDead`
-    /// statements for a particular local, the local is always considered live.
-    ///
-    /// More precisely, the MIR validator currently does a `MaybeStorageLiveLocals` analysis to
-    /// check validity of each use of a local. I believe this is equivalent to requiring for every
-    /// use of a local, there exist at least one path from the root to that use that contains a
-    /// `StorageLive` more recently than a `StorageDead`.
-    ///
-    /// **Needs clarification**: Is it permitted to have two `StorageLive`s without an intervening
-    /// `StorageDead`? Two `StorageDead`s without an intervening `StorageLive`? LLVM says poison,
-    /// yes. If the answer to any of these is "no," is breaking that rule UB or is it an error to
-    /// have a path in the CFG that might do this?
+    /// At any point during the execution of a function, each local is either allocated or
+    /// unallocated. Except as noted below, all locals except function parameters are initially
+    /// unallocated. `StorageLive` statements cause memory to be allocated for the local while
+    /// `StorageDead` statements cause the memory to be freed. Using a local in any way (not only
+    /// reading/writing from it) while it is unallocated is UB.
+    ///
+    /// Some locals have no `StorageLive` or `StorageDead` statements within the entire MIR body.
+    /// These locals are implicitly allocated for the full duration of the function. There is a
+    /// convenience method at `rustc_mir_dataflow::storage::always_storage_live_locals` for
+    /// computing these locals.
+    ///
+    /// If the local is already allocated, calling `StorageLive` again is UB. However, for an
+    /// unallocated local an additional `StorageDead` all is simply a nop.
     StorageLive(Local),
 
     /// See `StorageLive` above.