add suggestion for inverted function parameters #54804
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
src/libsyntax/parse/parser.rs
Outdated
| if let Err(mut err) = self.expect(&token::Colon) { | ||
| // If we find a pattern followed by an identifier, it could be an (incorrect) | ||
| // C-style parameter declaration. | ||
| match self.parse_ident() { |
There was a problem hiding this comment.
Can you check what happens if you write fn foo(i32)? I've got a feeling that as you're trying to parse an ident if it fails the parser is one step forward, which wold mean you are now standing past the ), causing knock on parse errors. You could use check_ident + checking wether it is followed by a comma or a closing brace instead. That way you have a slightly higher degree of confidence that you're emitting the suggestion in an appropriate time.
There was a problem hiding this comment.
The current code seems to work, so I've pushed a test case with it. I agree it'd be nicer to use check_ident, but wouldn't that remove the suggestion from the fn one(i32 a b) case?
There was a problem hiding this comment.
It would if you check for the following comma or brace, but what would fn one(i32 a b) be? At that point the code is so removed from the accepted syntax that all we can do is give up and let the user figure it out.
src/libsyntax/parse/parser.rs
Outdated
| if let Err(mut err) = self.expect(&token::Colon) { | ||
| // If we find a pattern followed by an identifier, it could be an (incorrect) | ||
| // C-style parameter declaration. | ||
| match self.parse_ident() { |
There was a problem hiding this comment.
It would if you check for the following comma or brace, but what would fn one(i32 a b) be? At that point the code is so removed from the accepted syntax that all we can do is give up and let the user figure it out.
| | --^^^^ | ||
| | | | | ||
| | | expected one of `:` or `@` here | ||
| | help: declare the type after the parameter binding: `quux: S` |
There was a problem hiding this comment.
I'm thinking it may make sense to not provide a suggestion but rather a help message, otherwise we could confuse people that just forgot about the colon a non-sensical suggestion. For example, given fn baz(quuz S) {} we would suggest incorrect code.
There was a problem hiding this comment.
Ah, good catch. That is unfortunate. Something that's nice about the suggestion is that it illustrates the syntax, which is hard to describe with words alone. Would a suggestion like <identifier>: <type> with Applicability::HasPlaceholders suffice?
There was a problem hiding this comment.
Yes, I think that's an appropriate way to express it.
estebank
added
S-waiting-on-author
euclio
force-pushed
the
inverted-parameters
branch
from
879e4f7
to
f5db411
Compare
|
@estebank Ready for another review. |
|
@bors r+ rollup |
|
📌 Commit f5db411 has been approved by |
bors
added
S-waiting-on-bors
add suggestion for inverted function parameters Fixes rust-lang#54065.
Rollup of 11 pull requests Successful merges: - #54078 (Expand the documentation for the `std::sync` module) - #54717 (Cleanup rustc/ty part 1) - #54781 (Add examples to `TyKind::FnDef` and `TyKind::FnPtr` docs) - #54787 (Only warn about unused `mut` in user-written code) - #54804 (add suggestion for inverted function parameters) - #54812 (Regression test for #32382.) - #54833 (make `Parser::parse_foreign_item()` return a foreign item or error) - #54834 (rustdoc: overflow:auto doesn't work nicely on small screens) - #54838 (Fix typo in src/libsyntax/parse/parser.rs) - #54851 (Fix a regression in 1.30 by reverting #53564) - #54853 (Remove unneccessary error from test, revealing NLL error.) Failed merges: r? @ghost
Fixes #54065.