Skip to content

Handle more string addition cases with appropriate suggestions #60901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 17, 2019
Merged

Handle more string addition cases with appropriate suggestions #60901

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
234adf8
Handle more string addition cases with appropriate suggestions
estebank May 17, 2019
2cb9181
Fix binop span
estebank May 17, 2019
ee0bf5e
review comments
estebank May 17, 2019
8895fb9
Account for &String + String
estebank May 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 60 additions & 23 deletions src/librustc_typeck/check/op.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,8 @@ 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, true, op) {
self.check_str_addition(
lhs_expr, rhs_expr, lhs_ty, rhs_ty, &mut err, true, op) {
// 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 @@ -400,8 +400,8 @@ 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, false, op) {
self.check_str_addition(
lhs_expr, rhs_expr, lhs_ty, rhs_ty, &mut err, false, op) {
// 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 @@ -502,9 +502,13 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
false
}

/// Provide actionable suggestions when trying to add two strings with incorrect types,
/// like `&str + &str`, `String + String` and `&str + &String`.
///
/// If this function returns `true` it means a note was printed, so we don't need
/// to print the normal "implementation of `std::ops::Add` might be missing" note
fn check_str_addition(
&self,
expr: &'gcx hir::Expr,
lhs_expr: &'gcx hir::Expr,
rhs_expr: &'gcx hir::Expr,
lhs_ty: Ty<'tcx>,
Expand All @@ -514,45 +518,78 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
op: hir::BinOp,
) -> bool {
let source_map = self.tcx.sess.source_map();
let remove_borrow_msg = "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";

let msg = "`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";
// If this function returns true it means a note was printed, so we don't need
// to print the normal "implementation of `std::ops::Add` might be missing" note

let is_std_string = |ty| &format!("{:?}", ty) == "std::string::String";

match (&lhs_ty.sty, &rhs_ty.sty) {
(&Ref(_, l_ty, _), &Ref(_, r_ty, _))
if l_ty.sty == Str && r_ty.sty == Str => {
if !is_assign {
err.span_label(op.span,
"`+` can't be used to concatenate two `&str` strings");
(&Ref(_, l_ty, _), &Ref(_, r_ty, _)) // &str or &String + &str, &String or &&str
if (l_ty.sty == Str || is_std_string(l_ty)) && (
r_ty.sty == Str || is_std_string(r_ty) ||
&format!("{:?}", rhs_ty) == "&&str"
) =>
{
if !is_assign { // Do not supply this message if `&str += &str`
err.span_label(
op.span,
"`+` cannot be used to concatenate two `&str` strings",
);
match source_map.span_to_snippet(lhs_expr.span) {
Ok(lstring) => err.span_suggestion(
lhs_expr.span,
msg,
format!("{}.to_owned()", lstring),
Applicability::MachineApplicable,
),
Ok(lstring) => {
err.span_suggestion(
lhs_expr.span,
if lstring.starts_with("&") {
remove_borrow_msg
} else {
msg
},
if lstring.starts_with("&") {
// let a = String::new();
// let _ = &a + "bar";
format!("{}", &lstring[1..])
} else {
format!("{}.to_owned()", lstring)
},
Applicability::MachineApplicable,
)
}
_ => err.help(msg),
};
}
true
}
(&Ref(_, l_ty, _), &Adt(..))
if l_ty.sty == Str && &format!("{:?}", rhs_ty) == "std::string::String" => {
err.span_label(expr.span,
"`+` can't be used to concatenate a `&str` with a `String`");
(&Ref(_, l_ty, _), &Adt(..)) // Handle `&str` & `&String` + `String`
if (l_ty.sty == Str || is_std_string(l_ty)) && is_std_string(rhs_ty) =>
{
err.span_label(
op.span,
"`+` cannot be used to concatenate a `&str` with a `String`",
);
match (
source_map.span_to_snippet(lhs_expr.span),
source_map.span_to_snippet(rhs_expr.span),
is_assign,
) {
(Ok(l), Ok(r), false) => {
let to_string = if l.starts_with("&") {
// let a = String::new(); let b = String::new();
// let _ = &a + b;
format!("{}", &l[1..])
} else {
format!("{}.to_owned()", l)
};
err.multipart_suggestion(
msg,
vec![
(lhs_expr.span, format!("{}.to_owned()", l)),
(lhs_expr.span, to_string),
(rhs_expr.span, format!("&{}", r)),
],
Applicability::MachineApplicable,
Expand Down
3 changes: 1 addition & 2 deletions src/libsyntax/parse/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3540,8 +3540,7 @@ impl<'a> Parser<'a> {
let binary = self.mk_binary(source_map::respan(cur_op_span, ast_op), lhs, rhs);
self.mk_expr(span, binary, ThinVec::new())
}
AssocOp::Assign =>
self.mk_expr(span, ExprKind::Assign(lhs, rhs), ThinVec::new()),
AssocOp::Assign => self.mk_expr(span, ExprKind::Assign(lhs, rhs), ThinVec::new()),
AssocOp::ObsoleteInPlace =>
self.mk_expr(span, ExprKind::ObsoleteInPlace(lhs, rhs), ThinVec::new()),
AssocOp::AssignOp(k) => {
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-47377.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ error[E0369]: binary operation `+` cannot be applied to type `&str`
LL | let _a = b + ", World!";
| - ^ ---------- &str
| | |
| | `+` can't be used to concatenate two `&str` strings
| | `+` cannot be used to concatenate two `&str` strings
| &str
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
|
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-47380.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ error[E0369]: binary operation `+` cannot be applied to type `&str`
LL | println!("🦀🦀🦀🦀🦀"); let _a = b + ", World!";
| - ^ ---------- &str
| | |
| | `+` can't be used to concatenate two `&str` strings
| | `+` cannot be used to concatenate two `&str` strings
| &str
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
|
Expand Down
20 changes: 20 additions & 0 deletions src/test/ui/span/issue-39018.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,23 @@ enum World {
Hello,
Goodbye,
}

fn foo() {
let a = String::new();
let b = String::new();
let c = "";
let d = "";
let e = &a;
let _ = &a + &b; //~ ERROR binary operation
let _ = &a + b; //~ ERROR binary operation
let _ = a + &b; // ok
let _ = a + b; //~ ERROR mismatched types
let _ = e + b; //~ ERROR binary operation
let _ = e + &b; //~ ERROR binary operation
let _ = e + d; //~ ERROR binary operation
let _ = e + &d; //~ ERROR binary operation
let _ = &c + &d; //~ ERROR binary operation
let _ = &c + d; //~ ERROR binary operation
let _ = c + &d; //~ ERROR binary operation
let _ = c + d; //~ ERROR binary operation
}
150 changes: 143 additions & 7 deletions src/test/ui/span/issue-39018.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ error[E0369]: binary operation `+` cannot be applied to type `&str`
LL | let x = "Hello " + "World!";
| -------- ^ -------- &str
| | |
| | `+` can't be used to concatenate two `&str` strings
| | `+` cannot be used to concatenate two `&str` strings
| &str
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
|
Expand All @@ -25,16 +25,152 @@ error[E0369]: binary operation `+` cannot be applied to type `&str`
--> $DIR/issue-39018.rs:11:22
|
LL | let x = "Hello " + "World!".to_owned();
| ---------^--------------------
| | |
| | std::string::String
| -------- ^ ------------------- std::string::String
| | |
| | `+` cannot be used to concatenate a `&str` with a `String`
| &str
| `+` 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();
| ^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^^

error: aborting due to 3 previous errors
error[E0369]: binary operation `+` cannot be applied to type `&std::string::String`
--> $DIR/issue-39018.rs:26:16
|
LL | let _ = &a + &b;
| -- ^ -- &std::string::String
| | |
| | `+` cannot be used to concatenate two `&str` strings
| &std::string::String
help: 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel a bit of information overload here; there's too much information packed in too little space. I'd consider giving things more "air" by e.g. moving help: down a line.

Copy link
Contributor Author

@estebank estebank May 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is handled elsewhere in the emitter code. I am ambivalent on wether making this case take more vertical by adding a \n| would actually improve things in general or not. I feel we should be fairly conservative when it comes to vertical space, and with colors it doesn't look that bad:

Screen Shot 2019-05-16 at 8 02 23 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it would have been nice to have a method on DiagnosticBuilder that forces a newline... but since it happens elsewhere... bummer! and let's move on.

I do agree colors make it less of a problem, but that still makes me somewhat dizzy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, these look much better when the text is much shorter than this. What we could do is make the help text short and only say what it should do (fix the types of the binop), and have a separate note talk about this, which would introduce a newline and look like this


error[E0369]: binary operation `+` cannot be applied to type `&std::string::String`
  --> $DIR/issue-39018.rs:33:15
   |
LL |     let _ = e + &d;
   |             - ^ -- &&str
   |             | |
   |             | `+` cannot be used to concatenate two `&str` strings
   |             &std::string::String
   = note: string concatenation appends the string on the right to the string on the left and may require reallocation, and this requires ownership of the string on the left
help: turn the string in the lift into a `String`
   |
LL |     let _ = e.to_owned() + &d;
   |             ^^^^^^^^^^^^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems better, yeah.

|
LL | let _ = a + &b;
| ^

error[E0369]: binary operation `+` cannot be applied to type `&std::string::String`
--> $DIR/issue-39018.rs:27:16
|
LL | let _ = &a + b;
| -- ^ - std::string::String
| | |
| | `+` cannot be used to concatenate a `&str` with a `String`
| &std::string::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 _ = a + &b;
| ^ ^^

error[E0308]: mismatched types
--> $DIR/issue-39018.rs:29:17
|
LL | let _ = a + b;
| ^
| |
| expected &str, found struct `std::string::String`
| help: consider borrowing here: `&b`
|
= note: expected type `&str`
found type `std::string::String`

error[E0369]: binary operation `+` cannot be applied to type `&std::string::String`
--> $DIR/issue-39018.rs:30:15
|
LL | let _ = e + b;
| - ^ - std::string::String
| | |
| | `+` cannot be used to concatenate a `&str` with a `String`
| &std::string::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 _ = e.to_owned() + &b;
| ^^^^^^^^^^^^ ^^

error[E0369]: binary operation `+` cannot be applied to type `&std::string::String`
--> $DIR/issue-39018.rs:31:15
|
LL | let _ = e + &b;
| - ^ -- &std::string::String
| | |
| | `+` cannot be used to concatenate two `&str` strings
| &std::string::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 _ = e.to_owned() + &b;
| ^^^^^^^^^^^^

error[E0369]: binary operation `+` cannot be applied to type `&std::string::String`
--> $DIR/issue-39018.rs:32:15
|
LL | let _ = e + d;
| - ^ - &str
| | |
| | `+` cannot be used to concatenate two `&str` strings
| &std::string::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 _ = e.to_owned() + d;
| ^^^^^^^^^^^^

error[E0369]: binary operation `+` cannot be applied to type `&std::string::String`
--> $DIR/issue-39018.rs:33:15
|
LL | let _ = e + &d;
| - ^ -- &&str
| | |
| | `+` cannot be used to concatenate two `&str` strings
| &std::string::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 _ = e.to_owned() + &d;
| ^^^^^^^^^^^^

error[E0369]: binary operation `+` cannot be applied to type `&&str`
--> $DIR/issue-39018.rs:34:16
|
LL | let _ = &c + &d;
| -- ^ -- &&str
| |
| &&str
|
= note: an implementation of `std::ops::Add` might be missing for `&&str`

error[E0369]: binary operation `+` cannot be applied to type `&&str`
--> $DIR/issue-39018.rs:35:16
|
LL | let _ = &c + d;
| -- ^ - &str
| |
| &&str
|
= note: an implementation of `std::ops::Add` might be missing for `&&str`

error[E0369]: binary operation `+` cannot be applied to type `&str`
--> $DIR/issue-39018.rs:36:15
|
LL | let _ = c + &d;
| - ^ -- &&str
| | |
| | `+` cannot be used to concatenate two `&str` strings
| &str
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 _ = c.to_owned() + &d;
| ^^^^^^^^^^^^

error[E0369]: binary operation `+` cannot be applied to type `&str`
--> $DIR/issue-39018.rs:37:15
|
LL | let _ = c + d;
| - ^ - &str
| | |
| | `+` cannot be used to concatenate two `&str` strings
| &str
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 _ = c.to_owned() + d;
| ^^^^^^^^^^^^

error: aborting due to 14 previous errors

For more information about this error, try `rustc --explain E0369`.
Some errors have detailed explanations: E0308, E0369.
For more information about an error, try `rustc --explain E0308`.
7 changes: 5 additions & 2 deletions src/test/ui/str/str-concat-on-double-ref.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,13 @@ error[E0369]: binary operation `+` cannot be applied to type `&std::string::Stri
|
LL | let c = a + b;
| - ^ - &str
| |
| | |
| | `+` cannot be used to concatenate two `&str` strings
| &std::string::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
|
= note: an implementation of `std::ops::Add` might be missing for `&std::string::String`
LL | let c = a.to_owned() + b;
| ^^^^^^^^^^^^

error: aborting due to previous error

Expand Down