From c83602a70239cf3d8c7d8e07e5041de6838e0cd9 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Thu, 19 Jul 2018 15:10:14 +0200 Subject: [PATCH 1/3] When type-checking binops, LHS of assign-op like `+=` is invariant. Therefore we cannot coerce it to a supertype the same way that we can the LHS of `+`. Addresses issue 52126. --- src/librustc_typeck/check/op.rs | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/librustc_typeck/check/op.rs b/src/librustc_typeck/check/op.rs index 46746d4bd29..fb153464dff 100644 --- a/src/librustc_typeck/check/op.rs +++ b/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 From 1185798680395fbda2a99075462b482b9ac95217 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 20 Jul 2018 13:12:03 +0200 Subject: [PATCH 2/3] Regression test for issue. --- ...ssue-52126-assign-op-invariance.nll.stderr | 15 +++++ .../ui/issue-52126-assign-op-invariance.rs | 59 +++++++++++++++++++ .../issue-52126-assign-op-invariance.stderr | 14 +++++ 3 files changed, 88 insertions(+) create mode 100644 src/test/ui/issue-52126-assign-op-invariance.nll.stderr create mode 100644 src/test/ui/issue-52126-assign-op-invariance.rs create mode 100644 src/test/ui/issue-52126-assign-op-invariance.stderr diff --git a/src/test/ui/issue-52126-assign-op-invariance.nll.stderr b/src/test/ui/issue-52126-assign-op-invariance.nll.stderr new file mode 100644 index 00000000000..dcca491a87b --- /dev/null +++ b/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`. diff --git a/src/test/ui/issue-52126-assign-op-invariance.rs b/src/test/ui/issue-52126-assign-op-invariance.rs new file mode 100644 index 00000000000..b26ad9bc37d --- /dev/null +++ b/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 or the MIT license +// , 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 + } +} diff --git a/src/test/ui/issue-52126-assign-op-invariance.stderr b/src/test/ui/issue-52126-assign-op-invariance.stderr new file mode 100644 index 00000000000..a4ea8085c12 --- /dev/null +++ b/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`. From f153be6258f24155756298b3a79e9b14df7afe00 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Fri, 20 Jul 2018 14:18:52 +0200 Subject: [PATCH 3/3] For some reason, on my linux box, using `-Zverbose` here is causing a linker failure. Rather than try to work out what was happening, I just removed the flag because I see no reason for it to be on this test. --- src/test/ui/nll/constant.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/ui/nll/constant.rs b/src/test/ui/nll/constant.rs index 10ce0652d43..6e2c0ae7bba 100644 --- a/src/test/ui/nll/constant.rs +++ b/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";