Skip to content

Commit

Permalink
Rollup merge of rust-lang#52605 - estebank:str-plus-eq, r=oli-obk
Browse files Browse the repository at this point in the history 
Do not suggest using `to_owned()` on `&str += &str`

 - Don't provide incorrect suggestion for `&str += &str` (fix rust-lang#52410)
 - On `&str + String` suggest `&str.to_owned() + &String` as a single suggestion
  • Loading branch information
@kennytm
kennytm authored Jul 24, 2018
2 parents a98c19e + 9369b52 commit 5b7e3a1
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 42 deletions.
67 changes: 36 additions & 31 deletions src/librustc_typeck/check/op.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -307,9 +307,9 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
if let Some(missing_trait) = missing_trait {
if op.node == hir::BinOpKind::Add &&
self.check_str_addition(expr, lhs_expr, rhs_expr, lhs_ty,
rhs_ty, &mut err) {
rhs_ty, &mut err, true) {
// This has nothing here because it means we did string
// concatenation (e.g. "Hello " + "World!"). This means
// concatenation (e.g. "Hello " += "World!"). This means
// we don't want the note in the else clause to be emitted
} else if let ty::TyParam(_) = lhs_ty.sty {
// FIXME: point to span of param
Expand Down Expand Up @@ -381,7 +381,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
if let Some(missing_trait) = missing_trait {
if op.node == hir::BinOpKind::Add &&
self.check_str_addition(expr, lhs_expr, rhs_expr, lhs_ty,
rhs_ty, &mut err) {
rhs_ty, &mut err, false) {
// This has nothing here because it means we did string
// concatenation (e.g. "Hello " + "World!"). This means
// we don't want the note in the else clause to be emitted
Expand Down Expand Up @@ -410,13 +410,16 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
(lhs_ty, rhs_ty, return_ty)
}

fn check_str_addition(&self,
expr: &'gcx hir::Expr,
lhs_expr: &'gcx hir::Expr,
rhs_expr: &'gcx hir::Expr,
lhs_ty: Ty<'tcx>,
rhs_ty: Ty<'tcx>,
err: &mut errors::DiagnosticBuilder) -> bool {
fn check_str_addition(
&self,
expr: &'gcx hir::Expr,
lhs_expr: &'gcx hir::Expr,
rhs_expr: &'gcx hir::Expr,
lhs_ty: Ty<'tcx>,
rhs_ty: Ty<'tcx>,
err: &mut errors::DiagnosticBuilder,
is_assign: bool,
) -> bool {
let codemap = self.tcx.sess.codemap();
let msg = "`to_owned()` can be used to create an owned `String` \
from a string reference. String concatenation \
Expand All @@ -428,34 +431,36 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
match (&lhs_ty.sty, &rhs_ty.sty) {
(&TyRef(_, l_ty, _), &TyRef(_, r_ty, _))
if l_ty.sty == TyStr && r_ty.sty == TyStr => {
err.span_label(expr.span,
"`+` can't be used to concatenate two `&str` strings");
match codemap.span_to_snippet(lhs_expr.span) {
Ok(lstring) => err.span_suggestion(lhs_expr.span,
msg,
format!("{}.to_owned()", lstring)),
_ => err.help(msg),
};
if !is_assign {
err.span_label(expr.span,
"`+` can't be used to concatenate two `&str` strings");
match codemap.span_to_snippet(lhs_expr.span) {
Ok(lstring) => err.span_suggestion(lhs_expr.span,
msg,
format!("{}.to_owned()", lstring)),
_ => err.help(msg),
};
}
true
}
(&TyRef(_, l_ty, _), &TyAdt(..))
if l_ty.sty == TyStr && &format!("{:?}", rhs_ty) == "std::string::String" => {
err.span_label(expr.span,
"`+` can't be used to concatenate a `&str` with a `String`");
match codemap.span_to_snippet(lhs_expr.span) {
Ok(lstring) => err.span_suggestion(lhs_expr.span,
msg,
format!("{}.to_owned()", lstring)),
_ => err.help(msg),
};
match codemap.span_to_snippet(rhs_expr.span) {
Ok(rstring) => {
err.span_suggestion(rhs_expr.span,
"you also need to borrow the `String` on the right to \
get a `&str`",
format!("&{}", rstring));
match (
codemap.span_to_snippet(lhs_expr.span),
codemap.span_to_snippet(rhs_expr.span),
is_assign,
) {
(Ok(l), Ok(r), false) => {
err.multipart_suggestion(msg, vec![
(lhs_expr.span, format!("{}.to_owned()", l)),
(rhs_expr.span, format!("&{}", r)),
]);
}
_ => {
err.help(msg);
}
_ => {}
};
true
}
Expand Down
5 changes: 0 additions & 5 deletions src/test/ui/issue-10401.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,6 @@ LL | a += { "b" };
| -^^^^^^^^^^^
| |
| cannot use `+=` on type `&str`
| `+` can't be used to concatenate two `&str` strings
help: `to_owned()` can be used to create an owned `String` from a string reference. String concatenation appends the string on the right to the string on the left and may require reallocation. This requires ownership of the string on the left
|
LL | a.to_owned() += { "b" };
| ^^^^^^^^^^^^

error: aborting due to previous error

Expand Down
8 changes: 2 additions & 6 deletions src/test/ui/span/issue-39018.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,8 @@ LL | let x = "Hello " + "World!".to_owned();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `+` can't be used to concatenate a `&str` with a `String`
help: `to_owned()` can be used to create an owned `String` from a string reference. String concatenation appends the string on the right to the string on the left and may require reallocation. This requires ownership of the string on the left
|
LL | let x = "Hello ".to_owned() + "World!".to_owned();
| ^^^^^^^^^^^^^^^^^^^
help: you also need to borrow the `String` on the right to get a `&str`
|
LL | let x = "Hello " + &"World!".to_owned();
| ^^^^^^^^^^^^^^^^^^^^
LL | let x = "Hello ".to_owned() + &"World!".to_owned();
| ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^

error: aborting due to 3 previous errors

Expand Down

0 comments on commit 5b7e3a1

Please sign in to comment.