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

Suggest comma when writing println!("{}" a); #52397

Merged
merged 3 commits into from
Aug 8, 2018

Conversation

estebank
Copy link
Contributor

Fix #49370.

@estebank estebank changed the title Suggest comma when writing println!("{}" a); [WIP] Suggest comma when writing println!("{}" a); Jul 15, 2018
@rust-highfive
Copy link
Collaborator

r? @pnkfelix

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 15, 2018
@estebank estebank changed the title [WIP] Suggest comma when writing println!("{}" a); Suggest comma when writing println!("{}" a); Jul 15, 2018
/// Given a `TokenStream` with a `Stream` of only two arguments, return a new `TokenStream`
/// separating the two arguments with a comma for diagnostic suggestions.
pub(crate) fn add_comma(&self) -> Option<(TokenStream, Span)> {
// Used ot suggest if a user writes `println!("{}" a);`
Copy link
Contributor

Choose a reason for hiding this comment

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

ot -> to

pub(crate) fn add_comma(&self) -> Option<(TokenStream, Span)> {
// Used ot suggest if a user writes `println!("{}" a);`
if let TokenStreamKind::Stream(ref slice) = self.kind {
if slice.len() == 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

So this doesn't work for println!("{} {}" a, b); or println!("{} {}", a b);?

If it does work, please add tests, if it does not work, would it be a lot of effort to fix that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The further we go from the "{}" a case, the less likely we're to find a suitable fix by just checking against comma separated list. Having said that, making sure that every element is separated by commas should be easy to add.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 16, 2018

r? @oli-obk

Advanced mode: Can we ask the macro matcher how far each arm got and suggest the next possible matches in the spirit of "expected one of foo, bar, boo, found a"?

@rust-highfive rust-highfive assigned oli-obk and unassigned pnkfelix Jul 16, 2018
@estebank
Copy link
Contributor Author

Can we ask the macro matcher how far each arm got and suggest the next possible matches in the spirit of "expected one of foo, bar, boo, found a"?

@oli-obk that sounds interesting to do... I'll look into it this week.

@stokhos stokhos added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 21, 2018
@pietroalbini
Copy link
Member

Ping from triage @estebank! It's been a while since we heard from you, will you have time to work on this again?

@estebank
Copy link
Contributor Author

@pietroalbini will do soonish.

@oli-obk I tried to get this for arbitrary sequences (finding a missing comma that is not the first one) and it was a bit more involved than I anticipated. I need to regroup and write a generalized version, but I feel like we could land this very specific one in the meantime, after I fix the typo.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 31, 2018

Sgtm

@estebank
Copy link
Contributor Author

Fixed the typo.

@oli-obk
Copy link
Contributor

oli-obk commented Jul 31, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Jul 31, 2018

📌 Commit c4b7b1eec4ea0b354bce8c1fb417598b63710966 has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 31, 2018
@rust-highfive

This comment has been minimized.

@estebank
Copy link
Contributor Author

@bors r-

gonna rebase against master, it's probably stale

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 31, 2018
@estebank

This comment has been minimized.

@estebank estebank closed this Jul 31, 2018
@bors
Copy link
Contributor

bors commented Aug 7, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 7, 2018
@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 7, 2018

@bors retry

@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 Aug 7, 2018
@bors
Copy link
Contributor

bors commented Aug 7, 2018

⌛ Testing commit daa5bd3 with merge 80be6b8e7f9b40181807bb6ea7afb5e856f7a1d0...

@bors
Copy link
Contributor

bors commented Aug 7, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 7, 2018
@rust-highfive

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 7, 2018

@bors retry

@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 Aug 7, 2018
@bors
Copy link
Contributor

bors commented Aug 7, 2018

⌛ Testing commit daa5bd3 with merge 3f4f18f...

bors added a commit that referenced this pull request Aug 7, 2018
Suggest comma when writing `println!("{}" a);`

Fix #49370.
@bors
Copy link
Contributor

bors commented Aug 8, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: oli-obk
Pushing 3f4f18f to master...

@bors bors merged commit daa5bd3 into rust-lang:master Aug 8, 2018
@sunjay
Copy link
Member

sunjay commented Aug 8, 2018

This is awesome! Thank you for doing this! I'm the one who filed the original issue about it and I really appreciate that it was fixed. 😁

cramertj added a commit to cramertj/rust that referenced this pull request Aug 8, 2018
Suggest comma when missing in macro call

When missing a comma in a macro call, suggest it, regardless of
position. When a macro call doesn't match any of the patterns, check
if the call's token stream could be missing a comma between two idents,
and if so, create a new token stream containing the comma and try to
match against the macro patterns. If successful, emit the suggestion.

This works on arbitrary macros, with no need of special support from
the macro writers.

```
error: no rules expected the token `d`
  --> $DIR/missing-comma.rs:26:18
   |
LL |     foo!(a, b, c d, e);
   |                 -^
   |                 |
   |                 help: missing comma here
```
Follow up to rust-lang#52397.
cramertj added a commit to cramertj/rust that referenced this pull request Aug 8, 2018
Suggest comma when missing in macro call

When missing a comma in a macro call, suggest it, regardless of
position. When a macro call doesn't match any of the patterns, check
if the call's token stream could be missing a comma between two idents,
and if so, create a new token stream containing the comma and try to
match against the macro patterns. If successful, emit the suggestion.

This works on arbitrary macros, with no need of special support from
the macro writers.

```
error: no rules expected the token `d`
  --> $DIR/missing-comma.rs:26:18
   |
LL |     foo!(a, b, c d, e);
   |                 -^
   |                 |
   |                 help: missing comma here
```
Follow up to rust-lang#52397.
cramertj added a commit to cramertj/rust that referenced this pull request Aug 8, 2018
Suggest comma when missing in macro call

When missing a comma in a macro call, suggest it, regardless of
position. When a macro call doesn't match any of the patterns, check
if the call's token stream could be missing a comma between two idents,
and if so, create a new token stream containing the comma and try to
match against the macro patterns. If successful, emit the suggestion.

This works on arbitrary macros, with no need of special support from
the macro writers.

```
error: no rules expected the token `d`
  --> $DIR/missing-comma.rs:26:18
   |
LL |     foo!(a, b, c d, e);
   |                 -^
   |                 |
   |                 help: missing comma here
```
Follow up to rust-lang#52397.
kennytm added a commit to kennytm/rust that referenced this pull request Aug 9, 2018
Suggest comma when missing in macro call

When missing a comma in a macro call, suggest it, regardless of
position. When a macro call doesn't match any of the patterns, check
if the call's token stream could be missing a comma between two idents,
and if so, create a new token stream containing the comma and try to
match against the macro patterns. If successful, emit the suggestion.

This works on arbitrary macros, with no need of special support from
the macro writers.

```
error: no rules expected the token `d`
  --> $DIR/missing-comma.rs:26:18
   |
LL |     foo!(a, b, c d, e);
   |                 -^
   |                 |
   |                 help: missing comma here
```
Follow up to rust-lang#52397.
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.

None yet

8 participants