Auto merge of #156425 - chenyukang:yukang-fix-156416-unused-assignments-diverging, r=<try>

Fix unused assignments in diverging branches

Author: bors

compiler/rustc_mir_transform/src/liveness.rs

@@ -40,6 +40,8 @@ enum CaptureKind {
 struct Access {
     /// Describe the current access.
     kind: AccessKind,
+    /// MIR location where this access happens.
+    location: Location,
     /// Is the accessed place is live at the current statement?
     /// When we encounter multiple statements at the same location, we only increase the liveness,
     /// in order to avoid false positives.
@@ -648,26 +650,30 @@ impl<'a, 'tcx> AssignmentResult<'a, 'tcx> {
             &checked_places.places,
         );
 
-        let mut check_place =
-            |place: Place<'tcx>, kind, source_info: SourceInfo, live: &DenseBitSet<PlaceIndex>| {
-                if let Some((index, extra_projections)) = checked_places.get(place.as_ref()) {
-                    if !is_indirect(extra_projections) {
-                        let is_direct = extra_projections.is_empty();
-                        match assignments[index].entry(source_info) {
-                            IndexEntry::Vacant(v) => {
-                                let access = Access { kind, live: live.contains(index), is_direct };
-                                v.insert(access);
-                            }
-                            IndexEntry::Occupied(mut o) => {
-                                // There were already a sighting. Mark this statement as live if it
-                                // was, to avoid false positives.
-                                o.get_mut().live |= live.contains(index);
-                                o.get_mut().is_direct &= is_direct;
-                            }
+        let mut check_place = |place: Place<'tcx>,
+                               kind,
+                               source_info: SourceInfo,
+                               location: Location,
+                               live: &DenseBitSet<PlaceIndex>| {
+            if let Some((index, extra_projections)) = checked_places.get(place.as_ref()) {
+                if !is_indirect(extra_projections) {
+                    let is_direct = extra_projections.is_empty();
+                    match assignments[index].entry(source_info) {
+                        IndexEntry::Vacant(v) => {
+                            let access =
+                                Access { kind, location, live: live.contains(index), is_direct };
+                            v.insert(access);
+                        }
+                        IndexEntry::Occupied(mut o) => {
+                            // There were already a sighting. Mark this statement as live if it
+                            // was, to avoid false positives.
+                            o.get_mut().live |= live.contains(index);
+                            o.get_mut().is_direct &= is_direct;
                         }
                     }
                 }
-            };
+            }
+        };
 
         let mut record_drop = |place: Place<'tcx>| {
             if let Some((index, &[])) = checked_places.get(place.as_ref()) {
@@ -684,7 +690,13 @@ impl<'a, 'tcx> AssignmentResult<'a, 'tcx> {
             match &terminator.kind {
                 TerminatorKind::Call { destination: place, .. }
                 | TerminatorKind::Yield { resume_arg: place, .. } => {
-                    check_place(*place, AccessKind::Assign, terminator.source_info, live);
+                    check_place(
+                        *place,
+                        AccessKind::Assign,
+                        terminator.source_info,
+                        body.terminator_loc(bb),
+                        live,
+                    );
                     record_drop(*place)
                 }
                 TerminatorKind::Drop { place, .. } => record_drop(*place),
@@ -693,21 +705,34 @@ impl<'a, 'tcx> AssignmentResult<'a, 'tcx> {
                         if let InlineAsmOperand::Out { place: Some(place), .. }
                         | InlineAsmOperand::InOut { out_place: Some(place), .. } = operand
                         {
-                            check_place(*place, AccessKind::Assign, terminator.source_info, live);
+                            check_place(
+                                *place,
+                                AccessKind::Assign,
+                                terminator.source_info,
+                                body.terminator_loc(bb),
+                                live,
+                            );
                         }
                     }
                 }
                 _ => {}
             }
 
             for (statement_index, statement) in bb_data.statements.iter().enumerate().rev() {
-                cursor.seek_before_primary_effect(Location { block: bb, statement_index });
+                let location = Location { block: bb, statement_index };
+                cursor.seek_before_primary_effect(location);
                 let live = cursor.get();
                 ever_live.union(live);
                 match &statement.kind {
                     StatementKind::Assign(box (place, _))
                     | StatementKind::SetDiscriminant { box place, .. } => {
-                        check_place(*place, AccessKind::Assign, statement.source_info, live);
+                        check_place(
+                            *place,
+                            AccessKind::Assign,
+                            statement.source_info,
+                            location,
+                            live,
+                        );
                     }
                     StatementKind::StorageLive(_)
                     | StatementKind::StorageDead(_)
@@ -745,7 +770,12 @@ impl<'a, 'tcx> AssignmentResult<'a, 'tcx> {
                     continue;
                 };
                 let source_info = body.local_decls[place.local].source_info;
-                let access = Access { kind, live: live.contains(index), is_direct: true };
+                let access = Access {
+                    kind,
+                    location: Location::START,
+                    live: live.contains(index),
+                    is_direct: true,
+                };
                 assignments[index].insert(source_info, access);
             }
         }
@@ -1098,23 +1128,34 @@ impl<'a, 'tcx> AssignmentResult<'a, 'tcx> {
                 continue;
             }
 
-            let mut next_direct_assign = None;
+            let mut next_direct_assignments: Vec<(Span, Location)> = Vec::new();
             let mut dead_statements = Vec::with_capacity(statements.len());
 
-            for (source_info, Access { live, kind, is_direct }) in statements.into_iter() {
-                let overwrite = match (kind, is_direct, next_direct_assign) {
-                    (AccessKind::Assign, true, Some(overwrite_span)) => {
-                        Some(errors::UnusedAssignOverwrite {
+            for (source_info, Access { live, kind, is_direct, location }) in statements.into_iter()
+            {
+                let overwrite = if kind == AccessKind::Assign && is_direct {
+                    next_direct_assignments
+                        .iter()
+                        .filter(|(_, overwrite_location)| {
+                            location.is_predecessor_of(*overwrite_location, self.body)
+                        })
+                        // MIR traversal order is not always source-nearest for assignments such
+                        // as compound assignment operations.
+                        .min_by_key(|(overwrite_span, _)| {
+                            let is_source_successor = source_info.span.lo() <= overwrite_span.lo();
+                            (!is_source_successor, overwrite_span.lo())
+                        })
+                        .map(|&(overwrite_span, _)| errors::UnusedAssignOverwrite {
                             assigned_span: source_info.span,
                             overwrite_span,
                             name,
                         })
-                    }
-                    _ => None,
+                } else {
+                    None
                 };
 
                 if kind == AccessKind::Assign && is_direct {
-                    next_direct_assign = Some(source_info.span);
+                    next_direct_assignments.push((source_info.span, location));
                 }
 
                 if live {

tests/ui/liveness/unused-assignments-diverging-branch-issue-156416.rs

@@ -0,0 +1,43 @@
+//@ run-pass
+#![allow(dead_code)]
+#![warn(unused_assignments)]
+
+fn diverging_else() {
+    let x;
+    if true {
+        x = 42;
+    } else {
+        x = 35;
+        //~^ WARN value assigned to `x` is never read
+        panic!();
+    }
+    println!("{x}");
+}
+
+fn diverging_match_arm() {
+    let x;
+    match true {
+        false => {
+            x = 35;
+            //~^ WARN value assigned to `x` is never read
+            panic!();
+        }
+        true => x = 42,
+    }
+    println!("{x}");
+}
+
+fn real_overwrite_after_if(cond: bool) {
+    let mut x;
+    if cond {
+        x = 35;
+        //~^ WARN value assigned to `x` is never read
+    } else {
+        x = 42;
+        //~^ WARN value assigned to `x` is never read
+    }
+    x = 99;
+    println!("{x}");
+}
+
+fn main() {}

tests/ui/liveness/unused-assignments-diverging-branch-issue-156416.stderr

@@ -0,0 +1,41 @@
+warning: value assigned to `x` is never read
+  --> $DIR/unused-assignments-diverging-branch-issue-156416.rs:10:9
+   |
+LL |         x = 35;
+   |         ^^^^^^
+   |
+   = help: maybe it is overwritten before being read?
+note: the lint level is defined here
+  --> $DIR/unused-assignments-diverging-branch-issue-156416.rs:3:9
+   |
+LL | #![warn(unused_assignments)]
+   |         ^^^^^^^^^^^^^^^^^^
+
+warning: value assigned to `x` is never read
+  --> $DIR/unused-assignments-diverging-branch-issue-156416.rs:21:13
+   |
+LL |             x = 35;
+   |             ^^^^^^
+   |
+   = help: maybe it is overwritten before being read?
+
+warning: value assigned to `x` is never read
+  --> $DIR/unused-assignments-diverging-branch-issue-156416.rs:36:9
+   |
+LL |         x = 42;
+   |         ^^^^^^ this value is reassigned later and never used
+...
+LL |     x = 99;
+   |     ------ `x` is overwritten here before the previous value is read
+
+warning: value assigned to `x` is never read
+  --> $DIR/unused-assignments-diverging-branch-issue-156416.rs:33:9
+   |
+LL |         x = 35;
+   |         ^^^^^^ this value is reassigned later and never used
+...
+LL |     x = 99;
+   |     ------ `x` is overwritten here before the previous value is read
+
+warning: 4 warnings emitted
+