Skip to content

Suggest comma when missing in macro call #53183

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 1 commit into from
Aug 9, 2018
Merged

Suggest comma when missing in macro call #53183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/libsyntax/ext/tt/macro_rules.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ fn generic_extension<'cx>(cx: &'cx mut ExtCtxt,
for lhs in lhses { // try each arm's matchers
let lhs_tt = match *lhs {
quoted::TokenTree::Delimited(_, ref delim) => &delim.tts[..],
_ => cx.span_bug(sp, "malformed macro lhs")
_ => continue,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is in the recovery path, we shouldn't be hitting this ever, as it would be caught above, but if we were we don't really care.

};
match TokenTree::parse(cx, lhs_tt, arg.clone()) {
Success(_) => {
Expand All @@ -191,7 +191,7 @@ fn generic_extension<'cx>(cx: &'cx mut ExtCtxt,
err.span_suggestion_short(
comma_span,
"missing comma here",
",".to_string(),
", ".to_string(),
);
}
}
Expand Down
48 changes: 35 additions & 13 deletions src/libsyntax/tokenstream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,21 +186,43 @@ impl TokenStream {
/// 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 to suggest if a user writes `println!("{}" a);`
// Used to suggest if a user writes `foo!(a b);`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

println case is being handled elsewhere, as it is no longer affected by this code since it no longer uses concat.

if let TokenStreamKind::Stream(ref slice) = self.kind {
if slice.len() == 2 {
let comma_span = match slice[0] {
TokenStream { kind: TokenStreamKind::Tree(TokenTree::Token(sp, _)) } |
TokenStream { kind: TokenStreamKind::Tree(TokenTree::Delimited(sp, _)) } => {
sp.shrink_to_hi()
let mut suggestion = None;
let mut iter = slice.iter().enumerate().peekable();
while let Some((pos, ts)) = iter.next() {
if let Some((_, next)) = iter.peek() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Neat trick. This solves most of the issues I guess. If it keeps coming up we can consider a generalized version that doesn't hardcode specific tokens

Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, my original idea was to try to do this in

rust/src/libsyntax/ext/tt/macro_rules.rs

Line 109 in 3f4f18f

let mut best_fail_tok = None;
and collect all failures with their possible expectations. But I'm not sure what kind of parser changes that would have required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's create a follow up ticket. The current approach was slightly less error prone and easier to implement, but that would be more helpful for more macros.

match (ts, next) {
(TokenStream {
kind: TokenStreamKind::Tree(TokenTree::Token(_, token::Token::Comma))
}, _) |
(_, TokenStream {
kind: TokenStreamKind::Tree(TokenTree::Token(_, token::Token::Comma))
}) => {}
(TokenStream {
kind: TokenStreamKind::Tree(TokenTree::Token(sp, _))
}, _) |
(TokenStream {
kind: TokenStreamKind::Tree(TokenTree::Delimited(sp, _))
}, _) => {
let sp = sp.shrink_to_hi();
let comma = TokenStream {
kind: TokenStreamKind::Tree(TokenTree::Token(sp, token::Comma)),
};
suggestion = Some((pos, comma, sp));
}
_ => {}
}
_ => DUMMY_SP,
};
let comma = TokenStream {
kind: TokenStreamKind::Tree(TokenTree::Token(comma_span, token::Comma)),
};
let slice = RcSlice::new(vec![slice[0].clone(), comma, slice[1].clone()]);
return Some((TokenStream { kind: TokenStreamKind::Stream(slice) }, comma_span));
}
}
if let Some((pos, comma, sp)) = suggestion {
let mut new_slice = vec![];
let parts = slice.split_at(pos + 1);
new_slice.extend_from_slice(parts.0);
new_slice.push(comma);
new_slice.extend_from_slice(parts.1);
let slice = RcSlice::new(new_slice);
return Some((TokenStream { kind: TokenStreamKind::Stream(slice) }, sp));
}
}
None
Expand Down
12 changes: 11 additions & 1 deletion src/test/ui/macros/missing-comma.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,22 @@
// except according to those terms.

macro_rules! foo {
($a:ident, $b:ident) => ()
($a:ident) => ();
($a:ident, $b:ident) => ();
($a:ident, $b:ident, $c:ident) => ();
($a:ident, $b:ident, $c:ident, $d:ident) => ();
($a:ident, $b:ident, $c:ident, $d:ident, $e:ident) => ();
}

fn main() {
println!("{}" a);
//~^ ERROR expected token: `,`
foo!(a b);
//~^ ERROR no rules expected the token `b`
foo!(a, b, c, d e);
//~^ ERROR no rules expected the token `e`
foo!(a, b, c d, e);
//~^ ERROR no rules expected the token `d`
foo!(a, b, c d e);
//~^ ERROR no rules expected the token `d`
}
28 changes: 25 additions & 3 deletions src/test/ui/macros/missing-comma.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,38 @@
error: expected token: `,`
--> $DIR/missing-comma.rs:16:19
--> $DIR/missing-comma.rs:20:19
|
LL | println!("{}" a);
| ^

error: no rules expected the token `b`
--> $DIR/missing-comma.rs:18:12
--> $DIR/missing-comma.rs:22:12
|
LL | foo!(a b);
| -^
| |
| help: missing comma here

error: aborting due to 2 previous errors
error: no rules expected the token `e`
--> $DIR/missing-comma.rs:24:21
|
LL | foo!(a, b, c, d e);
| -^
| |
| help: missing comma here

error: no rules expected the token `d`
--> $DIR/missing-comma.rs:26:18
|
LL | foo!(a, b, c d, e);
| -^
| |
| help: missing comma here

error: no rules expected the token `d`
--> $DIR/missing-comma.rs:28:18
|
LL | foo!(a, b, c d e);
| ^
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We purposely don't attempt further resolutions as the further we go from correct code the harder it'll be to fix automatically, but people have common cases for typos (missing =>? extra comma?), we could expand this code to handle it on a best effort manner.


error: aborting due to 5 previous errors