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

theo-lw · 2021-09-01T03:16:23Z

This pull request improves the diagnostics emitted on parsing a unary plus operator. See #88276.

Before:

error: expected expression, found `+`
 --> src/main.rs:2:13
  |
2 |     let x = +1;
  |             ^ expected expression

After:

error: leading `+` is not supported
 --> main.rs:2:13
  |
2 |     let x = +1;
  |             ^
  |             |
  |             unexpected `+`
  |             help: try removing the `+`

rust-highfive · 2021-09-01T03:16:26Z

r? @wesleywiser

(rust-highfive has picked a reviewer for you, use r? to override)

estebank

I love these kind of parser fixes!

Would you mind addressing the comments?

estebank · 2021-09-03T17:09:40Z

compiler/rustc_parse/src/parser/expr.rs

            token::BinOp(token::And) | token::AndAnd => {
                make_it!(this, attrs, |this, _| this.parse_borrow_expr(lo))
            }
+            token::BinOp(token::Plus) => {


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.

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

estebank · 2021-09-03T17:16:04Z

compiler/rustc_parse/src/parser/expr.rs

+                    err.span_suggestion(
+                        lo,
+                        "try removing the `+`",
+                        "".to_string(),
+                        Applicability::MachineApplicable,
+                    );


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.

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.

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).

estebank · 2021-09-03T17:18:33Z

compiler/rustc_parse/src/parser/expr.rs

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


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();

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.

compiler/rustc_parse/src/parser/expr.rs

estebank · 2021-09-03T17:25:47Z

src/test/ui/parser/issue-88276-unary-plus.rs

+    let _ = -+-+(1+2)*3; //~ ERROR leading `+` is not supported
+                         //~| ERROR leading `+` is not supported


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 😅

That's a good idea for a lint!

Filed #88669 so we don't forget.

estebank · 2021-09-03T17:26:17Z

src/test/ui/parser/issue-88276-unary-plus.rs

+                         //~| 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


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

estebank · 2021-09-03T17:27:04Z

r? @estebank

…ric literal

theo-lw · 2021-09-05T03:15:09Z

Addressed your comments @estebank, thanks for reviewing this!

estebank · 2021-09-05T17:18:51Z

@bors r+ rollup

bors · 2021-09-05T17:18:52Z

📌 Commit 20eba43 has been approved by estebank

Rollup of 9 pull requests Successful merges: - rust-lang#86263 (Rustdoc: Report Layout of enum variants) - rust-lang#88541 (Add regression test for rust-lang#74400) - rust-lang#88553 (Improve diagnostics for unary plus operators (rust-lang#88276)) - rust-lang#88594 (More symbolic doc aliases) - rust-lang#88648 (Correct “copies” to “moves” in `<Option<T> as From<T>>::from` doc, and other copyediting) - rust-lang#88691 (Add a regression test for rust-lang#88649) - rust-lang#88694 (Drop 1.56 stabilizations from 1.55 release notes) - rust-lang#88712 (Fix docs for `uX::checked_next_multiple_of`) - rust-lang#88726 (Fix typo in `const_generics` replaced with `adt_const_params` note) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

theo-lw added 2 commits August 31, 2021 23:07

Improve error checking on unary plus

6cfe98f

Apply formatting

e7fb98e

rust-highfive assigned wesleywiser Sep 1, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 1, 2021

theo-lw marked this pull request as draft September 1, 2021 03:20

theo-lw added 3 commits September 1, 2021 11:54

Add checks for a block before a unary plus. Fix failing tests

5a863d5

Update formatting

ce9e765

Undo debug statements

bc9877c

theo-lw marked this pull request as ready for review September 1, 2021 16:05

estebank reviewed Sep 3, 2021

View reviewed changes

rust-highfive assigned estebank and unassigned wesleywiser Sep 3, 2021

theo-lw added 2 commits September 4, 2021 22:35

Use verbose suggestions and only match if the + is seen before a nume…

65eb7e5

…ric literal

Fix formatting

20eba43

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 Sep 5, 2021

estebank mentioned this pull request Sep 5, 2021

Lint against --foo when foo is a numeric binding #88669

Closed

jackh726 mentioned this pull request Sep 8, 2021

Rollup of 9 pull requests #88750

Merged

bors merged commit 77ac329 into rust-lang:master Sep 8, 2021

rustbot added this to the 1.57.0 milestone Sep 8, 2021

camelid mentioned this pull request Sep 8, 2021

Unary plus gives suboptimal parse error #88276

Closed

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

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

		let _ = -+-+(1+2)*3; //~ ERROR leading `+` is not supported
		//~\| ERROR leading `+` is not supported

Uh oh!

Conversation

theo-lw commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Sep 1, 2021

Uh oh!

estebank left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theo-lw Sep 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theo-lw Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estebank commented Sep 3, 2021

Uh oh!

theo-lw commented Sep 5, 2021

Uh oh!

estebank commented Sep 5, 2021

Uh oh!

bors commented Sep 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

theo-lw commented Sep 1, 2021 •

edited

Loading

theo-lw Sep 5, 2021 •

edited

Loading

theo-lw Sep 3, 2021 •

edited

Loading