-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Suggest !
for bitwise negation when encountering a ~
#41722
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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. |
You should add a test case (probably in |
Thank you @kennytm . Test case added. |
src/libsyntax/parse/parser.rs
Outdated
let (span, e) = self.interpolated_or_expr_span(e)?; | ||
let mut err = self.diagnostic().struct_span_err(lo.to(span), | ||
"`~` can not be used as an unary operator"); | ||
err.help("use `!` instead of `~` if you meant to bitwise negation"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add err.span_label(span_of_tilda, "did you mean
!?")
and use span_of_tilda
for the primary error as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meant to bitwise negation
Redundant "to"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meant to bitwise negation
Alternatively meant to perform bitwise negation
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By span_of_tilda
I actually meant the span of ~
itself (let span_of_tilda = self.span; self.bump();
)
src/libsyntax/parse/parser.rs
Outdated
@@ -2700,6 +2700,17 @@ impl<'a> Parser<'a> { | |||
let (span, e) = self.interpolated_or_expr_span(e)?; | |||
(span, self.mk_unary(UnOp::Not, e)) | |||
} | |||
// fix issue #41679: Suggest `!` for bitwise negation when encountering a `~` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could you remove "fix issue #41679:"? (Not useful for a reader and the history is available from git if necessary.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
src/libsyntax/parse/token.rs
Outdated
@@ -201,6 +201,7 @@ impl Token { | |||
OpenDelim(..) => true, // tuple, array or block | |||
Literal(..) => true, // literal | |||
Not => true, // operator not | |||
Tilde => true, // operator not (incorrect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is slightly risky, can_begin_expr
can affect syntax accepted by parser in general, not only diagnostics.
~
is not used anywhere right now, so it's ok for now, but it may be used in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree. I have deleted this line, and the test works fine. At least for this case, it is not necessary to modify can_begin_expr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will remove the suggestion from something like return ~0
, but that's more or less ok.
I had hoped we could avoid adding a token here. @petrochenkov I did use |
@nagisa |
@aidanhs Comments are resolved. Do you have other concerns? |
@nagisa macro_rules! m {
($x: expr) => {};
(~ $x: expr) => {};
}
fn main() {
m!(~x); // ERROR expected expression, found `~`
} but it doesn't seem to work already. |
r=me modulo #41722 (comment) |
r? @petrochenkov |
@bors r+ |
📌 Commit a9d3b34 has been approved by |
let (span, e) = self.interpolated_or_expr_span(e)?; | ||
let span_of_tilde = lo; | ||
let mut err = self.diagnostic().struct_span_err(span_of_tilde, | ||
"`~` can not be used as an unary operator"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bit of a late review, but won't hurt. Anyway, this should be using "a" because unary doesn't start with a vowel sound (As in, it starts with "you").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@acdenisSK Looks like this PR has already been merged, would you like to submit a minor PR to fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, i guess, gimme a sec.
Suggest `!` for bitwise negation when encountering a `~` Fix rust-lang#41679 Here is a program ```rust fn main() { let x = ~1; } ``` It's output: ``` error: `~` can not be used as an unary operator --> /home/fcc/temp/test.rs:4:13 | 4 | let x = ~1; | ^^ | = help: use `!` instead of `~` if you meant to bitwise negation ``` cc @bstrie
⌛ Testing commit a9d3b34 with merge 42a4f37... |
@petrochenkov I believe that's covered by #27832 |
Fix #41679
Here is a program
It's output:
cc @bstrie