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

Add clearer error message using `&str + &str` #39116

Merged
merged 1 commit into from Feb 2, 2017

Conversation

@mgattozzi
Copy link
Member

commented Jan 17, 2017

This is the first part of #39018. One of the common things for new users
coming from more dynamic languages like JavaScript, Python or Ruby is to
use + to concatenate strings. However, this doesn't work that way in
Rust unless the first type is a String. This commit adds a check for
this use case and outputs a new error as well as a suggestion to guide
the user towards the desired behavior. It also adds a new test case to
test the output of the error.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Jan 17, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

Mostly looks okay? Would like to see if @jonathandturner has thoughts on this.

Feel free to try and add handling for the other forms. String is a struct so you can search for it.

I've been considering having lang-item-like annotations for types in the stdlib, which let diagnostics check for their existence. We do path matching extensively in clippy, and it would be useful to do something less hacky in rustc.

};
err.span_suggestion(expr.span,
&format!("Your first argument should be a `String`, not `&str`. \
Try using `to_string()`"),

This comment has been minimized.

Copy link
@Manishearth

Manishearth Jan 17, 2017

Member

.to_string()

//~^ ERROR 12:18: 12:26: binary operation `+` cannot be applied to type `&'static str` [E0369]
//~| NOTE You can't use `+` between a `&str` and `&str`
//~| HELP Your first argument should be a `String`, not `&str`. Try using `to_string()`
//~| NOTE an implementation of `std::ops::Add` might be missing for `&'static str`

This comment has been minimized.

Copy link
@Manishearth

Manishearth Jan 17, 2017

Member

Have a thingy for the suggestion too

span_note!(&mut err, lhs_expr.span,
"an implementation of `{}` might be missing for `{}`",
"an implementation of `{}` might be missing for `{}`",

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Jan 17, 2017

Contributor

Please remove this extra space.

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2017

I've updated the code with the above suggestions. @Manishearth how would I go about searching for a String so that I could match against it here.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

This is what we do in clippy to match structs.

https://github.com/Manishearth/rust-clippy/blob/630e0ef96875e0958277674f81921bfaba30bc5f/clippy_lints/src/utils/mod.rs#L140-L179

I think we should just land as-is since that's a bit hacky. I'm filing an issue for the diagnostic-lang-item stuff.

r? @jonathandturner

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

Filed #39131 about the diagnostic item

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2017

In the meantime I think it might be good to get the diagnostic of things like String + String if possible but that could be a separate PR if that's better.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

Actually, you can make this work without the full framework of diagnostics items; just add a #[diagnostic_item = "String"] annotation and then in your code check that cx.tcx.get_attrs(adt_def.def_id) has that attribute.

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2017

@Manishearth using the code from clippy or just putting it in the code now as is?

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

No, we don't want to use the clippy code, it's hacky. Use an attribute as shown above.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

Maybe rustc_diagnostic_item

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2017

I used ripgrep and git grep to search the code and neither of those attributes exist in the code base so I have no basis of comparison to do it the way you've mentioned.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

I'm saying you invent a new attribute :)

Annotate String with #[rustc_diagnostic_item = "String"]. Then, using tcx.get_attrs(adt_def.def_id), you can check if your TyAdt has that attribute.

You will probably have to add it to libsyntax/feature_gate.rs as well, as a Gated attribute.

@Manishearth

This comment has been minimized.

Copy link
Member

commented Jan 17, 2017

Anyway we can land as-is if you'd prefer too.

@jonathandturner

This comment has been minimized.

Copy link
Contributor

commented Jan 17, 2017

I like the idea.

Can you use a src/test/ui test instead of a compile_fail test? Just so that we can see what the final output looks like a bit better in the PR.

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2017

@Manishearth ohhhhhh okay I think I see what you're talking about now. I can try when I'm off work.

@jonathandturner I'll switch it over to ui test when I'm off work!

if let TyRef(_, r_ty) = rhs_ty.sty {
if l_ty.ty.sty == TyStr && r_ty.ty.sty == TyStr {
span_note!(&mut err, lhs_expr.span,
"You can't use `+` between a `&str` and `&str`");

This comment has been minimized.

Copy link
@nrc

nrc Jan 18, 2017

Member

We usually use the passive voice for error messages, i.e., something like "+ can't be used to concatenate two &str strings" and similarly later on.

This comment has been minimized.

Copy link
@mgattozzi

mgattozzi Jan 18, 2017

Author Member

Consistency is good. This one is a quick fix.

_ => format!("<expression>")
};
err.span_suggestion(expr.span,
&format!("Your first argument should be a `String`, not `&str`. \

This comment has been minimized.

Copy link
@nrc

nrc Jan 18, 2017

Member

It would be nice to explain why a String is needed, otherwise this seems a little like boilerplate.

This comment has been minimized.

Copy link
@mgattozzi

mgattozzi Jan 18, 2017

Author Member

Yeah I can see why it's boilerplate like. I'll put a more descriptive message.

};
err.span_suggestion(expr.span,
&format!("Your first argument should be a `String`, not `&str`. \
Try using to_string()"),

This comment has been minimized.

Copy link
@nrc

nrc Jan 18, 2017

Member

I would prefer phrasing like "to_string can be used to create an owned string from a string reference", I actually prefer using to_owned rather than to_string because it better describes what is going on

This comment has been minimized.

Copy link
@mgattozzi

mgattozzi Jan 18, 2017

Author Member

to_owned or to_string is always a hot button topic. Now they're semantically equivalent at least. I can put to_owned. It's all the same to me and with the rest of the sentence you put it communicates the intent of the function call.

//~| NOTE You can't use `+` between a `&str` and `&str`
//~| HELP Your first argument should be a `String`, not `&str`. Try using to_string()
//~| SUGGESTION let string = "Hello ".to_string() + "World" + "!"
//~| NOTE an implementation of `std::ops::Add` might be missing for `&'static str`

This comment has been minimized.

Copy link
@nrc

nrc Jan 18, 2017

Member

Would be nice if we could skip the NOTE in this case.

This comment has been minimized.

Copy link
@jonathandturner

jonathandturner Jan 18, 2017

Contributor

@nrc - I think he's going to move to a UI test, so we won't need them (and we'll be able to see what the actual error will look like)

This comment has been minimized.

Copy link
@nrc

nrc Jan 18, 2017

Member

I meant remove it from the error message, not from the test

This comment has been minimized.

Copy link
@mgattozzi

mgattozzi Jan 18, 2017

Author Member

Shouldn't be too hard. I have an idea of how to bypass this output.

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2017

@nrc I've updated the wording, though I'm wondering if it could use some improvement. I didn't want to make it too verbose, but I wanted to get the point across as well. I've also added a way to bypass the NOTE you didn't want output here through some conditional logic short circuiting. It feels a little hackish and I was wondering if you had a better idea here as to what could be improved.

Also not since the test hasn't been updated yet this should fail testing.

// and it was an addition with &str then skip the
// span note below it. Otherwise just emit the span
// note
if missing_trait == "std::ops::Add" &&

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Jan 18, 2017

Contributor

This will also skip the note when the trait is not Add instead of only when Add + &str

This comment has been minimized.

Copy link
@mgattozzi

mgattozzi Jan 19, 2017

Author Member

You're right. I got the logic wrong here. I tested it just to make sure so this will need to get changed

err.span_suggestion(expr.span,
&format!("to_owned() can be used to create an owned `String` \
from a string reference. This allows concatenation \
since the `String` is owned."), suggestion);

This comment has been minimized.

Copy link
@nrc

nrc Jan 23, 2017

Member

The second sentence doesn't quite explain why that is necessary. How about, "String concatenation appends the string on the right to the string on the left. That modifies the string on the left and may require reallocation, this requires ownership of the string on the left." It's a bit wordy, but tries to explain why you can't use a reference.

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

@nrc and @jonathandturner these commits should cover both the ui test and message output that was brought up earlier.

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

Looking at the raw log one failed on my test due to the output being different but it passed for all the other ones. I'm not sure why this would happen though

@nrc

nrc approved these changes Jan 24, 2017

@nrc

This comment has been minimized.

Copy link
Member

commented Jan 24, 2017

r = me with the test fixed. I'm not sure why that is failing, but it looks legit to me.

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

@nrc looking at the logs further this seems to be the problem

expected stderr:
error[E0514]: found crate `std` compiled by an incompatible version of rustc
  |
  = help: please recompile that crate using this compiler (rustc 1.16.0-dev (b27d71cb1 2017-01-23))
  = note: crate `std` path #1: /home/michael/Code/rustc/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-86f66de14176b4c2.rlib compiled by "rustc 1.16.0-dev (5c73f42db 2017-01-23)"
  = note: crate `std` path #2: /home/michael/Code/rustc/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-86f66de14176b4c2.so compiled by "rustc 1.16.0-dev (5c73f42db 2017-01-23)"
  = note: crate `std` path #3: /home/michael/Code/rustc/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd_unicode-b421c85d3facafd6.rlib compiled by "rustc 1.16.0-dev (5c73f42db 2017-01-23)"
  = note: crate `std` path #4: /home/michael/Code/rustc/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd_shim-dac73ac8aa295bd7.rlib compiled by "rustc 1.16.0-dev (5c73f42db 2017-01-23)"

error: aborting due to previous error

I think I added in this file by accident. I think removing it should fix the build issue. If not I'm building the compiler again from scratch and reupdating the references

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

@nrc I fixed the build!

@nrc

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

@mgattozzi nice! Could you squash the commits please? Then r+

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

@nrc squash and force push?

@nrc

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

yes

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

@nrc got it. Wasn't sure policy regarding force pushing. Alright it's all rebased and good to go!

@nrc

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

@bors: r+

Thanks for sticking with it @mgattozzi , nice PR!

@bors

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2017

📌 Commit 4da2994 has been approved by nrc

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

@nrc I'm glad to have been able to contribute to the compiler itself finally! It was a joy and I'm hoping to tackle some more issues. Thanks for reviewing it!

@bors

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2017

⌛️ Testing commit 4da2994 with merge 27a80f7...

bors added a commit that referenced this pull request Jan 25, 2017

Auto merge of #39116 - mgattozzi:better-string-message, r=nrc
Add clearer error message using `&str + &str`

This is the first part of #39018. One of the common things for new users
coming from more dynamic languages like JavaScript, Python or Ruby is to
use `+` to concatenate strings. However, this doesn't work that way in
Rust unless the first type is a `String`. This commit adds a check for
this use case and outputs a new error as well as a suggestion to guide
the user towards the desired behavior. It also adds a new test case to
test the output of the error.
@bors

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2017

💔 Test failed - status-travis

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 25, 2017

@nrc looks like curl messed up trying to download the rustc binary to do compilation.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

@bors: retry

@bors

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2017

☔️ The latest upstream changes (presumably #39353) made this pull request unmergeable. Please resolve the merge conflicts.

@kevinmehall

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2017

Looks like this unintentionally added files under src/target/debug/ to git.

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 28, 2017

@kevinmehall thanks I'll remove it

Add clearer error message using `&str + &str`
This is the first part of #39018. One of the common things for new users
coming from more dynamic languages like JavaScript, Python or Ruby is to
use `+` to concatenate strings. However, this doesn't work that way in
Rust unless the first type is a `String`. This commit adds a check for
this use case and outputs a new error as well as a suggestion to guide
the user towards the desired behavior. It also adds a new test case to
test the output of the error.
@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Jan 29, 2017

@nrc it needs to get approved again so it can go through

@nrc

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2017

📌 Commit b54f593 has been approved by nrc

@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2017

⌛️ Testing commit b54f593 with merge 6abe648...

bors added a commit that referenced this pull request Feb 2, 2017

Auto merge of #39116 - mgattozzi:better-string-message, r=nrc
Add clearer error message using `&str + &str`

This is the first part of #39018. One of the common things for new users
coming from more dynamic languages like JavaScript, Python or Ruby is to
use `+` to concatenate strings. However, this doesn't work that way in
Rust unless the first type is a `String`. This commit adds a check for
this use case and outputs a new error as well as a suggestion to guide
the user towards the desired behavior. It also adds a new test case to
test the output of the error.
@bors

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 6abe648 to master...

@bors bors merged commit b54f593 into rust-lang:master Feb 2, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Manishearth

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

Finally!

@mgattozzi

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2017

@Manishearth and @nrc thanks a lot! This great that it's finally merged!

@mgattozzi mgattozzi deleted the mgattozzi:better-string-message branch Feb 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.