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

Point at last valid token on failed expect_one_of #40811

Merged
merged 4 commits into from
Apr 5, 2017

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Mar 25, 2017

error: expected one of `.`, `;`, `?`, `}`, or an operator, found `)`
  --> $DIR/token-error-correct-3.rs:29:9
   |
25 |         foo()
   |              - expected one of `.`, `;`, `?`, `}`, or an operator after this
...
29 |     } else {
   |     ^ unexpected token

Fix #32540.

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@estebank estebank force-pushed the issue-32540 branch 2 times, most recently from ea6cde5 to 03eca71 Compare March 25, 2017 06:37
```rust
error: expected one of `.`, `;`, `?`, `}`, or an operator, found `)`
  --> $DIR/token-error-correct-3.rs:29:9
   |
25 |         foo()
   |             - expected one of `.`, `;`, `?`, `}`, or an operator after this
...
29 |     } else {
   |     ^ unexpected token
```
@nagisa
Copy link
Member

nagisa commented Mar 25, 2017

error: expected one of ., ;, ?, }, or an operator, found )

Sounds wrong. It says it expects those symbols instead of/before the parenthesis, although I think it might just be the example mis-copied and not actual output from the compiler (as the tests look good)?


I would also investigate if something like this is possible (note how label is shifted by 1):

   |
25 |         foo()
   |              - expected one of `.`, `;`, `?`, `}`, or an operator here
...
29 |     } else {
   |     ^ unexpected token

@estebank
Copy link
Contributor Author

I think it might just be the example mis-copied

Indeed, that was the case, @nagisa.

I would also investigate if something like this is possible (note how label is shifted by 1):

I was afraid the output when the unexpected token is right after the last correctly parsed token wouldn't be too great, but now that I mocked it out, it doesn't seem too bad:

   |
25 |         foo()} else {
   |              ^
   |              |
   |              expected one of `.`, `;`, `?`, `}`, or an operator here
   |              unexpected token

* Point at where the token was expected instead of the last token
  successfuly parsed.
* Only show `unexpected token` if the next char and the unexpected token
  don't have the same span.
* Change some cfail and pfail tests to ui test.
* Don't show all possible tokens in span label if they are more than 6.
@estebank
Copy link
Contributor Author

I've changed it so that when the next parsed token is in the same place as the next expected token, we only show the most detailed label

25 |         foo()} else {
   |              ^ expected one of `.`, `;`, `?`, `}`, or an operator here

vs.

25 |             fs::create_dir_all(path.as_ref()).map(|()| true)
   |                                                             - expected one of `.`, `;`, `?`, `}`, or an operator here
...
29 |         } else {
   |         ^ unexpected token

@estebank
Copy link
Contributor Author

estebank commented Apr 2, 2017

Ping.

@alexcrichton
Copy link
Member

r? @jonathandturner

or maybe @nrc

@rust-highfive rust-highfive assigned sophiajt and unassigned pnkfelix Apr 3, 2017
@sophiajt
Copy link
Contributor

sophiajt commented Apr 3, 2017

I'd probably detect this case:

   |
25 |         foo()} else {
   |              ^
   |              |
   |              expected one of `.`, `;`, `?`, `}`, or an operator here
   |              unexpected token

And just give a single message instead, like:

   |
25 |         foo()} else {
   |              ^
   |              |
   |              unexpected token, expected one of `.`, `;`, `?`, `}` or an operator

@estebank
Copy link
Contributor Author

estebank commented Apr 3, 2017

@jonathandturner I added a check for it that does that. From one of the tests:

error: expected one of `,`, `.`, `?`, or an operator, found `;`
  --> $DIR/token-error-correct-3.rs:23:35
   |
23 |             callback(path.as_ref();  //~ NOTE: unclosed delimiter
   |                                   ^ expected one of `,`, `.`, `?`, or an operator here

@sophiajt
Copy link
Contributor

sophiajt commented Apr 3, 2017

LGTM

@bors r+

@bors
Copy link
Contributor

bors commented Apr 3, 2017

📌 Commit b477682 has been approved by jonathandturner

@estebank
Copy link
Contributor Author

estebank commented Apr 4, 2017

@jonathandturner fixed merge conflict.

@bors r=jonathandturner

@bors
Copy link
Contributor

bors commented Apr 4, 2017

📌 Commit dedb7bb has been approved by jonathandturner

@bors
Copy link
Contributor

bors commented Apr 5, 2017

⌛ Testing commit dedb7bb with merge ad5dfec...

bors added a commit that referenced this pull request Apr 5, 2017
Point at last valid token on failed `expect_one_of`

```rust
error: expected one of `.`, `;`, `?`, `}`, or an operator, found `)`
  --> $DIR/token-error-correct-3.rs:29:9
   |
25 |         foo()
   |              - expected one of `.`, `;`, `?`, `}`, or an operator after this
...
29 |     } else {
   |     ^ unexpected token
```

Fix #32540.
@bors
Copy link
Contributor

bors commented Apr 5, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: jonathandturner
Pushing ad5dfec to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants