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

Syntax: Recover on for ( $pat in $expr ) $block #62928

Merged
merged 3 commits into from
Jul 30, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Jul 24, 2019

Fixes #62724 by adding some recovery:

error: unexpected closing `)`
  --> $DIR/recover-for-loop-parens-around-head.rs:10:23
   |
LL |     for ( elem in vec ) {
   |         --------------^
   |         |
   |         opening `(`
   |         help: remove parenthesis in `for` loop: `elem in vec`

The last 2 commits are drive-by cleanups.

r? @estebank

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2019
@bors

This comment has been minimized.

@Centril Centril force-pushed the recover-parens-around-for-head branch from 731cd32 to ee903d4 Compare July 28, 2019 05:20
@Centril Centril force-pushed the recover-parens-around-for-head branch from ee903d4 to 56b39fb Compare July 28, 2019 18:44
@Centril
Copy link
Contributor Author

Centril commented Jul 28, 2019

#62550 merged.

@estebank this should be ready for review now.

@estebank
Copy link
Contributor

I'll take a look at it later today.

| --------------^
| |
| opening `(`
| help: remove parenthesis in `for` loop: `elem in vec`
Copy link
Contributor

@estebank estebank Jul 29, 2019

Choose a reason for hiding this comment

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

I'm not entirely happy with the wording:

  • The span pointing at the opening brace is slightly redundant.
  • The issue is already known to be the superfluous parenthesis, so it thee maim message could be incorrect parenthesis surrounding for head or similar (probably something more user friendly).
  • The primary span points at the closing parenthesis but it doesn't have a label.
  • Suggestions that have very subtle changes sometimes make people not pay attention to the text, in this case people might not realize the lack of parenthesis in the suggested code.
  • We have two errors pointing at the problem here (the prior "in not expected" error, but it's ok emitting both for now)

IMO something like the following would be better:

error: unnecessary parenthesis surroundinig `for` loop head
  --> $DIR/recover-for-loop-parens-around-head.rs:10:23
   |
LL |     for ( elem in vec ) {
   |         ^             ^ help: remove these surrounding parenthesis

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #63113 for following up.

@estebank
Copy link
Contributor

@bors r+

@Centril I have some nitpicks. If you can address them shortly, go ahead, otherwise I'm ok with the PR being merged and my nitpicks addressed later.

@bors
Copy link
Contributor

bors commented Jul 29, 2019

📌 Commit 56b39fb has been approved by estebank

@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 Jul 29, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 29, 2019
…ead, r=estebank

Syntax: Recover on `for ( $pat in $expr ) $block`

Fixes rust-lang#62724 by adding some recovery:

```
error: unexpected closing `)`
  --> $DIR/recover-for-loop-parens-around-head.rs:10:23
   |
LL |     for ( elem in vec ) {
   |         --------------^
   |         |
   |         opening `(`
   |         help: remove parenthesis in `for` loop: `elem in vec`
```

The last 2 commits are drive-by cleanups.

r? @estebank
Centril added a commit to Centril/rust that referenced this pull request Jul 29, 2019
…ead, r=estebank

Syntax: Recover on `for ( $pat in $expr ) $block`

Fixes rust-lang#62724 by adding some recovery:

```
error: unexpected closing `)`
  --> $DIR/recover-for-loop-parens-around-head.rs:10:23
   |
LL |     for ( elem in vec ) {
   |         --------------^
   |         |
   |         opening `(`
   |         help: remove parenthesis in `for` loop: `elem in vec`
```

The last 2 commits are drive-by cleanups.

r? @estebank
Centril added a commit to Centril/rust that referenced this pull request Jul 30, 2019
…ead, r=estebank

Syntax: Recover on `for ( $pat in $expr ) $block`

Fixes rust-lang#62724 by adding some recovery:

```
error: unexpected closing `)`
  --> $DIR/recover-for-loop-parens-around-head.rs:10:23
   |
LL |     for ( elem in vec ) {
   |         --------------^
   |         |
   |         opening `(`
   |         help: remove parenthesis in `for` loop: `elem in vec`
```

The last 2 commits are drive-by cleanups.

r? @estebank
Centril added a commit to Centril/rust that referenced this pull request Jul 30, 2019
…ead, r=estebank

Syntax: Recover on `for ( $pat in $expr ) $block`

Fixes rust-lang#62724 by adding some recovery:

```
error: unexpected closing `)`
  --> $DIR/recover-for-loop-parens-around-head.rs:10:23
   |
LL |     for ( elem in vec ) {
   |         --------------^
   |         |
   |         opening `(`
   |         help: remove parenthesis in `for` loop: `elem in vec`
```

The last 2 commits are drive-by cleanups.

r? @estebank
bors added a commit that referenced this pull request Jul 30, 2019
Rollup of 12 pull requests

Successful merges:

 - #61965 (Remove mentions of removed `offset_to` method from `align_offset` docs)
 - #62928 (Syntax: Recover on `for ( $pat in $expr ) $block`)
 - #63000 (Impl Debug for Chars)
 - #63083 (Make generic parameters always use modern hygiene)
 - #63087 (Add very simple edition check to tidy.)
 - #63093 (Properly check the defining scope of existential types)
 - #63096 (Add tests for some `existential_type` ICEs)
 - #63099 (vxworks: Remove Linux-specific comments.)
 - #63106 (ci: Skip installing SWIG/xz on OSX )
 - #63108 (Add links to None in Option doc)
 - #63109 (std: Fix a failing `fs` test on Windows)
 - #63111 (Add syntactic and semantic tests for rest patterns, i.e. `..`)

Failed merges:

r? @ghost
@bors bors merged commit 56b39fb into rust-lang:master Jul 30, 2019
@Centril Centril deleted the recover-parens-around-for-head branch July 30, 2019 07:28
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.

Improving error for unnecessary parentheses in for loop head
4 participants