-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Suggests turbofish in patterns #114300
Suggests turbofish in patterns #114300
Conversation
This comment has been minimized.
This comment has been minimized.
b60f7bb
to
7146f92
Compare
This comment has been minimized.
This comment has been minimized.
7146f92
to
326e61a
Compare
326e61a
to
049c728
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can manage to keep the suggestion for wrong arbitrary self types that I highlighted, the current approach should work.
@estebank The new commit keeps the suggestions for wrong arbitrary self types that you highlighted. Please review again, thanks! ;) |
This comment has been minimized.
This comment has been minimized.
Some changes occurred in src/tools/rustfmt cc @rust-lang/rustfmt |
help: if this is a `self` type, give it a parameter name | ||
| | ||
LL | fn foo_with_qualified_path(self: <Bar as T>::Baz); | ||
| +++++ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm... Arbitrary self types only support a handful of pointer types (Box
, Arc
, Rc
). General arbitrary self types are unlikely to be supported any time soon, so adding this suggestion would be confusing to users. (I know, I know, but we have to try to minimize confusion to users, the only thing worse than an error without a suggestion is an error with a wrong suggestion.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned below (#114300 (comment))
@@ -1,5 +1,5 @@ | |||
fn main() { | |||
let caller<F> = |f: F| //~ ERROR expected one of `:`, `;`, `=`, `@`, or `|`, found `<` | |||
let caller<F> = |f: F| //~ ERROR generic args in patterns require the turbofish syntax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at this error, the suggestion we should be giving is to add a type, but don't know how hard it would be to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like function parameters, maybe we can also suggest name: caller<F>
or _: caller<F>
? I think it may be more general, because caller<F>
is more like a type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think the later is the better suggestion (particularly if the rest can be parsed as a type until ;
or =
is reached).
tests/ui/span/issue-34264.stderr
Outdated
--> $DIR/issue-34264.rs:1:14 | ||
| | ||
LL | fn foo(Option<i32>, String) {} | ||
| ^ expected one of `:`, `@`, or `|` | ||
| ^ expected one of 9 possible tokens | ||
| | ||
= note: anonymous parameters are removed in the 2018 edition (see RFC 1685) | ||
help: if this is a `self` type, give it a parameter name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see that we already provide a bad arbitrary self type suggestion :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and I think we can't emit suggestions only for pointer types in the parsing stage, since we can use type alias type P<T> = Box<T>
. So I think this is acceptable and the current behaviour extends the old arbitrary self type suggestions.
@@ -1579,7 +1579,7 @@ impl<'a> Parser<'a> { | |||
self.expect(&token::ModSep)?; | |||
|
|||
let mut path = ast::Path { segments: ThinVec::new(), span: DUMMY_SP, tokens: None }; | |||
self.parse_path_segments(&mut path.segments, T::PATH_STYLE, None)?; | |||
self.parse_path_segments(&mut path.segments, T::PATH_STYLE, None, None)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not super happy with the proliferation of None
s this causes, but I also don't have much of a better proposal (short of having a *_with_pat_location
family of methods).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new changes reduce the None
s, but there are still some (seemingly unavoidable at the moment)
tests/ui/span/issue-34264.stderr
Outdated
@@ -1,8 +1,8 @@ | |||
error: expected one of `:`, `@`, or `|`, found `<` | |||
error: expected one of `!`, `(`, `...`, `..=`, `..`, `::`, `:`, `{`, or `|`, found `<` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can, I'd like to revert whatever causes this message change, as I feel the previous one did a good job leading people towards the problem of a missing :
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is reverted now! ;)
25264ee
to
dce7e87
Compare
@bors r+ |
…bank Suggests turbofish in patterns Fixes rust-lang#114112 r? `@estebank`
…bank Suggests turbofish in patterns Fixes rust-lang#114112 r? ``@estebank``
…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#113657 (Expand, rename and improve `incorrect_fn_null_checks` lint) - rust-lang#114237 (parser: more friendly hints for handling `async move` in the 2015 edition) - rust-lang#114300 (Suggests turbofish in patterns) - rust-lang#114372 (const validation: point at where we found a pointer but expected an integer) - rust-lang#114395 ([rustc_span][perf] Hoist lookup sorted by words out of the loop.) - rust-lang#114403 (fix the span in the suggestion of remove question mark) - rust-lang#114408 (Temporary remove myself from review rotation) - rust-lang#114415 (Skip checking of `rustc_codegen_gcc` with vendoring enabled) r? `@ghost` `@rustbot` modify labels: rollup
…bank Suggests turbofish in patterns Fixes rust-lang#114112 r? ```@estebank```
…bank Suggests turbofish in patterns Fixes rust-lang#114112 r? ```@estebank```
Fixes #114112
r? @estebank