Skip to content
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.

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

Handle more string addition cases with appropriate suggestions #60901

Merged
merged 4 commits into from
May 17, 2019

Conversation

estebank
Copy link
Contributor

No description provided.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2019
| | |
| | `+` 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

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.

src/librustc_typeck/check/op.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/op.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/op.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/op.rs Outdated Show resolved Hide resolved
@estebank
Copy link
Contributor Author

r? @varkor

@rust-highfive rust-highfive assigned varkor and unassigned pnkfelix May 17, 2019
@estebank
Copy link
Contributor Author

r? @Centril

@rust-highfive rust-highfive assigned Centril and unassigned varkor May 17, 2019
src/librustc_typeck/check/op.rs Outdated Show resolved Hide resolved
src/librustc_typeck/check/op.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

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

r=me assuming you are happy with how things are in #60901 (comment).

We can always adjust later if people complain...

@estebank
Copy link
Contributor Author

@bors r=Centril

@bors
Copy link
Contributor

bors commented May 17, 2019

📌 Commit ee0bf5e has been approved by Centril

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 17, 2019
@estebank
Copy link
Contributor Author

@bors r=Centril

@Centril added an extra check for correct handling of edge case.

@bors
Copy link
Contributor

bors commented May 17, 2019

📌 Commit 8895fb9 has been approved by Centril

Manishearth added a commit to Manishearth/rust that referenced this pull request May 17, 2019
Handle more string addition cases with appropriate suggestions
bors added a commit that referenced this pull request May 17, 2019
Rollup of 4 pull requests

Successful merges:

 - #60791 (Update books)
 - #60891 (Allow claiming issues with triagebot)
 - #60901 (Handle more string addition cases with appropriate suggestions)
 - #60902 (Prevent Error::type_id overrides)

Failed merges:

r? @ghost
@bors bors merged commit 8895fb9 into rust-lang:master May 17, 2019
@estebank estebank deleted the str-str-str branch November 9, 2023 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants