Skip to content

Improve diagnostics for unary plus operators (#88276) #88553

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 7 commits into from
Sep 8, 2021
Merged

Improve diagnostics for unary plus operators (#88276) #88553

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6cfe98f
Improve error checking on unary plus
theo-lw Sep 1, 2021
e7fb98e
Apply formatting
theo-lw Sep 1, 2021
5a863d5
Add checks for a block before a unary plus. Fix failing tests
theo-lw Sep 1, 2021
ce9e765
Update formatting
theo-lw Sep 1, 2021
bc9877c
Undo debug statements
theo-lw Sep 1, 2021
65eb7e5
Use verbose suggestions and only match if the + is seen before a nume…
theo-lw Sep 5, 2021
20eba43
Fix formatting
theo-lw Sep 5, 2021
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
21 changes: 21 additions & 0 deletions compiler/rustc_parse/src/parser/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,27 @@ impl<'a> Parser<'a> {
token::BinOp(token::And) | token::AndAnd => {
make_it!(this, attrs, |this, _| this.parse_borrow_expr(lo))
}
token::BinOp(token::Plus) => {
Copy link
Contributor

@estebank estebank Sep 3, 2021

Choose a reason for hiding this comment

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

This should only be done if the following token is a numeric literal. As this stands now, if you write +foo or +"" this error will trigger as well. The suggestion won't be wrong per-se, but in those cases the problems are deeper than the +.

You can use Parser::look_ahead which takes a closure giving you a token for whatever N tokens you are advancing, so you can do something like token::BinOp(token::Plus) if this.look_ahead(1, |t| t.is_numeric_literal() => { or something. We need to decide whether an identifier like +foo is something where we want to also give this error or not. Personally I think we might not want to.

Copy link
Contributor Author

@theo-lw theo-lw Sep 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think it makes sense to limit the check to +'s that are followed by a numeric literal, made changes to reflect that.

let mut err = this.struct_span_err(lo, "leading `+` is not supported");
err.span_label(lo, "unexpected `+`");
Comment on lines +520 to +521
Copy link
Contributor

@estebank estebank Sep 3, 2021

Choose a reason for hiding this comment

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

We might want to replace this with

Suggested change
let mut err = this.struct_span_err(lo, "leading `+` is not supported");
err.span_label(lo, "unexpected `+`");
let mut err = this.unexpected().unwrap_err();

Copy link
Contributor Author

@theo-lw theo-lw Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I'm getting some unexpected results after changing this to unexpected::<P<Expr>>():

error: expected `#`, found `+`
 --> src/test/ui/parser/issue-88276-unary-plus.rs:4:13
  |
4 |     let _ = +1; //~ ERROR leading `+` is not supported
  |             ^ expected `#`
  |

I think this message is misleading because the programmer likely intended to type let _ = 1; and not let _ = #1. It might be better to keep these lines as is.


// a block on the LHS might have been intended to be an expression instead
let sp = this.sess.source_map().start_point(lo);
if let Some(sp) = this.sess.ambiguous_block_expr_parse.borrow().get(&sp) {
this.sess.expr_parentheses_needed(&mut err, *sp);
} else {
err.span_suggestion(
lo,
"try removing the `+`",
"".to_string(),
Applicability::MachineApplicable,
);
Copy link
Contributor

@estebank estebank Sep 3, 2021

Choose a reason for hiding this comment

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

Let's use span_suggestion_verbose, so it appears on its own subdiagnostic (the new patch-style makes removal a bit easier to see, IMO), but this is a minor thing. Alternatively we could also use tool_only_span_suggestion here, because the removal of the + is kind of obvious, and all we might want is the tooltip in an editor, the label in the output doesn't add that much.

Copy link
Contributor

@estebank estebank Sep 3, 2021

Choose a reason for hiding this comment

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

If we change the creation of err to be a call to unexpected(), then we can check here for the look_ahead and see if it is a literal or a binding identifier, and if so emit the suggestion and this.bump().

I don't know if we can also check if the next element would be a parenthesized expression (checking for OpenDelim(Paren)?) and have some confidence that there won't be other parse errors.

Copy link
Contributor Author

@theo-lw theo-lw Sep 5, 2021

Choose a reason for hiding this comment

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

Let's use span_suggestion_verbose, so it appears on its own subdiagnostic (the new patch-style makes removal a bit easier to see, IMO), but this is a minor thing. Alternatively we could also use tool_only_span_suggestion here, because the removal of the + is kind of obvious, and all we might want is the tooltip in an editor, the label in the output doesn't add that much.

span_suggestion_verbose makes the fix more obvious, changed.

I don't know if we can also check if the next element would be a parenthesized expression (checking for OpenDelim(Paren)?) and have some confidence that there won't be other parse errors.

I think there could be other parse errors if the next token is a parenthesis. For instance, the following expression has a parse error between the parentheses: +(+x).

}
err.emit();

this.bump();
this.parse_prefix_expr(None)
} // `+expr`
token::Ident(..) if this.token.is_keyword(kw::Box) => {
make_it!(this, attrs, |this, _| this.parse_box_expr(lo))
}
Expand Down
7 changes: 5 additions & 2 deletions src/test/ui/associated-types/issue-36499.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
error: expected expression, found `+`
error: leading `+` is not supported
--> $DIR/issue-36499.rs:4:9
|
LL | 2 + +2;
| ^ expected expression
| ^
| |
| unexpected `+`
| help: try removing the `+`

error: aborting due to previous error

6 changes: 3 additions & 3 deletions src/test/ui/parser/expr-as-stmt.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,18 @@
#![allow(unused_must_use)]

fn foo() -> i32 {
({2}) + {2} //~ ERROR expected expression, found `+`
({2}) + {2} //~ ERROR leading `+` is not supported
//~^ ERROR mismatched types
}

fn bar() -> i32 {
({2}) + 2 //~ ERROR expected expression, found `+`
({2}) + 2 //~ ERROR leading `+` is not supported
//~^ ERROR mismatched types
}

fn zul() -> u32 {
let foo = 3;
({ 42 }) + foo; //~ ERROR expected expression, found `+`
({ 42 }) + foo; //~ ERROR leading `+` is not supported
//~^ ERROR mismatched types
32
}
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/parser/expr-as-stmt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,18 @@
#![allow(unused_must_use)]

fn foo() -> i32 {
{2} + {2} //~ ERROR expected expression, found `+`
{2} + {2} //~ ERROR leading `+` is not supported
//~^ ERROR mismatched types
}

fn bar() -> i32 {
{2} + 2 //~ ERROR expected expression, found `+`
{2} + 2 //~ ERROR leading `+` is not supported
//~^ ERROR mismatched types
}

fn zul() -> u32 {
let foo = 3;
{ 42 } + foo; //~ ERROR expected expression, found `+`
{ 42 } + foo; //~ ERROR leading `+` is not supported
//~^ ERROR mismatched types
32
}
Expand Down
12 changes: 6 additions & 6 deletions src/test/ui/parser/expr-as-stmt.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,30 @@
error: expected expression, found `+`
error: leading `+` is not supported
--> $DIR/expr-as-stmt.rs:8:9
|
LL | {2} + {2}
| ^ expected expression
| ^ unexpected `+`
|
help: parentheses are required to parse this as an expression
|
LL | ({2}) + {2}
| + +

error: expected expression, found `+`
error: leading `+` is not supported
--> $DIR/expr-as-stmt.rs:13:9
|
LL | {2} + 2
| ^ expected expression
| ^ unexpected `+`
|
help: parentheses are required to parse this as an expression
|
LL | ({2}) + 2
| + +

error: expected expression, found `+`
error: leading `+` is not supported
--> $DIR/expr-as-stmt.rs:19:12
|
LL | { 42 } + foo;
| ^ expected expression
| ^ unexpected `+`
|
help: parentheses are required to parse this as an expression
|
Expand Down
13 changes: 13 additions & 0 deletions src/test/ui/parser/issue-88276-unary-plus.fixed
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// run-rustfix
#[allow(unused_parens)]
fn main() {
let _ = 1; //~ ERROR leading `+` is not supported
let _ = -(1+2)*3; //~ ERROR leading `+` is not supported
let _ = --(1+2)*3; //~ ERROR leading `+` is not supported
//~| ERROR leading `+` is not supported
let _ = (1 + 2) * 3; //~ ERROR leading `+` is not supported
//~| ERROR leading `+` is not supported
let _ = (&"hello"); //~ ERROR leading `+` is not supported
let _ = [3, 4+6]; //~ ERROR leading `+` is not supported
//~| ERROR leading `+` is not supported
}
13 changes: 13 additions & 0 deletions src/test/ui/parser/issue-88276-unary-plus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// run-rustfix
#[allow(unused_parens)]
fn main() {
let _ = +1; //~ ERROR leading `+` is not supported
let _ = -+(1+2)*3; //~ ERROR leading `+` is not supported
let _ = -+-+(1+2)*3; //~ ERROR leading `+` is not supported
//~| ERROR leading `+` is not supported
Copy link
Contributor

@estebank estebank Sep 3, 2021

Choose a reason for hiding this comment

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

This is an interesting case, because with the suggestions applied, this looks like C's prefix decrement --foo, which we don't have. We should have a lint against it 😅

Copy link
Contributor Author

@theo-lw theo-lw Sep 5, 2021

Choose a reason for hiding this comment

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

That's a good idea for a lint!

Copy link
Contributor

@estebank estebank Sep 5, 2021

Choose a reason for hiding this comment

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

Filed #88669 so we don't forget.

let _ = (1 + +2) * +3; //~ ERROR leading `+` is not supported
//~| ERROR leading `+` is not supported
let _ = (+&"hello"); //~ ERROR leading `+` is not supported
Copy link
Contributor

@estebank estebank Sep 3, 2021

Choose a reason for hiding this comment

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

So this is one case where removing the + might be obvious when written like this, but I don't know about the general case.

let _ = +[+3, 4+6]; //~ ERROR leading `+` is not supported
//~| ERROR leading `+` is not supported
}
83 changes: 83 additions & 0 deletions src/test/ui/parser/issue-88276-unary-plus.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
error: leading `+` is not supported
--> $DIR/issue-88276-unary-plus.rs:4:13
|
LL | let _ = +1;
| ^
| |
| unexpected `+`
| help: try removing the `+`

error: leading `+` is not supported
--> $DIR/issue-88276-unary-plus.rs:5:14
|
LL | let _ = -+(1+2)*3;
| ^
| |
| unexpected `+`
| help: try removing the `+`

error: leading `+` is not supported
--> $DIR/issue-88276-unary-plus.rs:6:14
|
LL | let _ = -+-+(1+2)*3;
| ^
| |
| unexpected `+`
| help: try removing the `+`

error: leading `+` is not supported
--> $DIR/issue-88276-unary-plus.rs:6:16
|
LL | let _ = -+-+(1+2)*3;
| ^
| |
| unexpected `+`
| help: try removing the `+`

error: leading `+` is not supported
--> $DIR/issue-88276-unary-plus.rs:8:18
|
LL | let _ = (1 + +2) * +3;
| ^
| |
| unexpected `+`
| help: try removing the `+`

error: leading `+` is not supported
--> $DIR/issue-88276-unary-plus.rs:8:24
|
LL | let _ = (1 + +2) * +3;
| ^
| |
| unexpected `+`
| help: try removing the `+`

error: leading `+` is not supported
--> $DIR/issue-88276-unary-plus.rs:10:14
|
LL | let _ = (+&"hello");
| ^
| |
| unexpected `+`
| help: try removing the `+`

error: leading `+` is not supported
--> $DIR/issue-88276-unary-plus.rs:11:13
|
LL | let _ = +[+3, 4+6];
| ^
| |
| unexpected `+`
| help: try removing the `+`

error: leading `+` is not supported
--> $DIR/issue-88276-unary-plus.rs:11:15
|
LL | let _ = +[+3, 4+6];
| ^
| |
| unexpected `+`
| help: try removing the `+`

error: aborting due to 9 previous errors