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 1 commit
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
57 changes: 43 additions & 14 deletions src/librustc_typeck/check/op.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,11 @@ 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,
Expand All @@ -514,33 +519,57 @@ 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
debug!("check_str_addition: {:?} + {:?}", lhs_ty, rhs_ty);
match (&lhs_ty.sty, &rhs_ty.sty) {
(&Ref(_, l_ty, _), &Ref(_, r_ty, _))
if l_ty.sty == Str && r_ty.sty == Str => {
(&Ref(_, l_ty, _), &Ref(_, r_ty, _)) // &str or &String + &str, &String or &&str
if (l_ty.sty == Str || &format!("{:?}", l_ty) == "std::string::String") && (
r_ty.sty == Str ||
&format!("{:?}", r_ty) == "std::string::String" ||
&format!("{:?}", rhs_ty) == "&&str"
) =>
{
if !is_assign {
err.span_label(op.span,
"`+` can't be used to concatenate two `&str` strings");
err.span_label(
op.span,
"`+` can't 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" => {
(&Ref(_, l_ty, _), &Adt(..)) // Handle `&str` & `&String` + `String`
if (l_ty.sty == Str || &format!("{:?}", l_ty) == "std::string::String") &&
&format!("{:?}", rhs_ty) == "std::string::String" =>
{
err.span_label(expr.span,
"`+` can't be used to concatenate a `&str` with a `String`");
match (
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
}
143 changes: 141 additions & 2 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 @@ -35,6 +35,145 @@ help: `to_owned()` can be used to create an owned `String` from a string referen
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
| | |
| | `+` can't 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
| &std::string::String
| `+` 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 _ = &a.to_owned() + &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
| &std::string::String
| `+` 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 _ = 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
| | |
| | `+` can't 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
| | |
| | `+` can't 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
| | |
| | `+` can't 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
| | |
| | `+` can't 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
| | |
| | `+` can't 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
| |
| | |
| | `+` can't 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