Auto merge of #52564 - pnkfelix:issue-52126-lhs-of-assign-op-is-invar…

…iant, r=eddyb

LHS of assign op is invariant

This addresses a bug injected by #45435. That PR changed the way we type-check `LHS <op> RHS` to coerce the LHS to the expected supertype in much the same way that we coerce the RHS.

The problem is that when we have a case of `LHS <op>= RHS`, we do not want to coerce to a supertype; we need the type to remain invariant. Otherwise we risk leaking a value with short-lifetimes into a expression context that needs to satisfy a long lifetime.

Fix #52126

src/librustc_typeck/check/op.rs

@@ -165,18 +165,25 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
                op,
                is_assign);
 
-        let lhs_needs = match is_assign {
-            IsAssign::Yes => Needs::MutPlace,
-            IsAssign::No => Needs::None
+        let lhs_ty = match is_assign {
+            IsAssign::No => {
+                // Find a suitable supertype of the LHS expression's type, by coercing to
+                // a type variable, to pass as the `Self` to the trait, avoiding invariant
+                // trait matching creating lifetime constraints that are too strict.
+                // E.g. adding `&'a T` and `&'b T`, given `&'x T: Add<&'x T>`, will result
+                // in `&'a T <: &'x T` and `&'b T <: &'x T`, instead of `'a = 'b = 'x`.
+                let lhs_ty = self.check_expr_with_needs(lhs_expr, Needs::None);
+                let fresh_var = self.next_ty_var(TypeVariableOrigin::MiscVariable(lhs_expr.span));
+                self.demand_coerce(lhs_expr, lhs_ty, fresh_var,  AllowTwoPhase::No)
+            }
+            IsAssign::Yes => {
+                // rust-lang/rust#52126: We have to use strict
+                // equivalence on the LHS of an assign-op like `+=`;
+                // overwritten or mutably-borrowed places cannot be
+                // coerced to a supertype.
+                self.check_expr_with_needs(lhs_expr, Needs::MutPlace)
+            }
         };
-        // Find a suitable supertype of the LHS expression's type, by coercing to
-        // a type variable, to pass as the `Self` to the trait, avoiding invariant
-        // trait matching creating lifetime constraints that are too strict.
-        // E.g. adding `&'a T` and `&'b T`, given `&'x T: Add<&'x T>`, will result
-        // in `&'a T <: &'x T` and `&'b T <: &'x T`, instead of `'a = 'b = 'x`.
-        let lhs_ty = self.check_expr_with_needs(lhs_expr, lhs_needs);
-        let fresh_var = self.next_ty_var(TypeVariableOrigin::MiscVariable(lhs_expr.span));
-        let lhs_ty = self.demand_coerce(lhs_expr, lhs_ty, fresh_var,  AllowTwoPhase::No);
         let lhs_ty = self.resolve_type_vars_with_obligations(lhs_ty);
 
         // NB: As we have not yet type-checked the RHS, we don't have the

src/test/ui/issue-52126-assign-op-invariance.nll.stderr

@@ -0,0 +1,15 @@
+error[E0597]: `line` does not live long enough
+  --> $DIR/issue-52126-assign-op-invariance.rs:44:28
+   |
+LL |         let v: Vec<&str> = line.split_whitespace().collect();
+   |                            ^^^^ borrowed value does not live long enough
+LL |         //~^ ERROR `line` does not live long enough
+LL |         println!("accumulator before add_assign {:?}", acc.map);
+   |                                                        ------- borrow later used here
+...
+LL |     }
+   |     - borrowed value only lives until here
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0597`.

src/test/ui/issue-52126-assign-op-invariance.rs

@@ -0,0 +1,59 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Issue 52126: With respect to variance, the assign-op's like += were
+// accidentally lumped together with other binary op's. In both cases
+// we were coercing the LHS of the op to the expected supertype.
+//
+// The problem is that since the LHS of += is modified, we need the
+// parameter to be invariant with respect to the overall type, not
+// covariant.
+
+use std::collections::HashMap;
+use std::ops::AddAssign;
+
+pub fn main() {
+    panics();
+}
+
+pub struct Counter<'l> {
+    map: HashMap<&'l str, usize>,
+}
+
+impl<'l> AddAssign for Counter<'l>
+{
+    fn add_assign(&mut self, rhs: Counter<'l>) {
+        rhs.map.into_iter().for_each(|(key, val)| {
+            let count = self.map.entry(key).or_insert(0);
+            *count += val;
+        });
+    }
+}
+
+/// often times crashes, if not prints invalid strings
+pub fn panics() {
+    let mut acc = Counter{map: HashMap::new()};
+    for line in vec!["123456789".to_string(), "12345678".to_string()] {
+        let v: Vec<&str> = line.split_whitespace().collect();
+        //~^ ERROR `line` does not live long enough
+        println!("accumulator before add_assign {:?}", acc.map);
+        let mut map = HashMap::new();
+        for str_ref in v {
+            let e = map.entry(str_ref);
+            println!("entry: {:?}", e);
+            let count = e.or_insert(0);
+            *count += 1;
+        }
+        let cnt2 = Counter{map};
+        acc += cnt2;
+        println!("accumulator after add_assign {:?}", acc.map);
+        // line gets dropped here but references are kept in acc.map
+    }
+}

src/test/ui/issue-52126-assign-op-invariance.stderr

@@ -0,0 +1,14 @@
+error[E0597]: `line` does not live long enough
+  --> $DIR/issue-52126-assign-op-invariance.rs:44:28
+   |
+LL |         let v: Vec<&str> = line.split_whitespace().collect();
+   |                            ^^^^ borrowed value does not live long enough
+...
+LL |     }
+   |     - `line` dropped here while still borrowed
+LL | }
+   | - borrowed value needs to live until here
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0597`.

src/test/ui/nll/constant.rs

@@ -11,7 +11,7 @@
 // Test that MIR borrowck and NLL analysis can handle constants of
 // arbitrary types without ICEs.
 
-// compile-flags:-Zborrowck=mir -Zverbose
+// compile-flags:-Zborrowck=mir
 // compile-pass
 
 const HI: &str = "hi";