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

Suggests turbofish in patterns #114300

Merged
merged 6 commits into from
Aug 3, 2023

Conversation

mu001999
Copy link
Contributor

Fixes #114112

r? @estebank

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 31, 2023
@rust-log-analyzer

This comment has been minimized.

compiler/rustc_parse/src/parser/path.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/pat.rs Outdated Show resolved Hide resolved
tests/ui/did_you_mean/issue-114112.stderr Outdated Show resolved Hide resolved
@mu001999 mu001999 force-pushed the fix/turbofish-pat branch 2 times, most recently from b60f7bb to 7146f92 Compare August 1, 2023 08:08
@mu001999
Copy link
Contributor Author

mu001999 commented Aug 1, 2023

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned estebank Aug 1, 2023
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@estebank estebank left a 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.

@mu001999 mu001999 requested a review from estebank August 2, 2023 16:01
@mu001999
Copy link
Contributor Author

mu001999 commented Aug 2, 2023

@estebank The new commit keeps the suggestions for wrong arbitrary self types that you highlighted. Please review again, thanks! ;)

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2023

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

compiler/rustc_parse/src/parser/pat.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/pat.rs Outdated Show resolved Hide resolved
Comment on lines 48 to 51
help: if this is a `self` type, give it a parameter name
|
LL | fn foo_with_qualified_path(self: <Bar as T>::Baz);
| +++++
Copy link
Contributor

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.)

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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).

--> $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
Copy link
Contributor

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 :-/

Copy link
Contributor Author

@mu001999 mu001999 Aug 3, 2023

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)?;
Copy link
Contributor

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 Nones this causes, but I also don't have much of a better proposal (short of having a *_with_pat_location family of methods).

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 new changes reduce the Nones, but there are still some (seemingly unavoidable at the moment)

@@ -1,8 +1,8 @@
error: expected one of `:`, `@`, or `|`, found `<`
error: expected one of `!`, `(`, `...`, `..=`, `..`, `::`, `:`, `{`, or `|`, found `<`
Copy link
Contributor

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 :

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 reverted now! ;)

@estebank
Copy link
Contributor

estebank commented Aug 3, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Aug 3, 2023

📌 Commit dce7e87 has been approved by estebank

It is now in the queue for this repository.

@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 3, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 3, 2023
…bank

Suggests turbofish in patterns

Fixes rust-lang#114112

r? `@estebank`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 3, 2023
…bank

Suggests turbofish in patterns

Fixes rust-lang#114112

r? ``@estebank``
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 3, 2023
…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
@bors bors merged commit 51d1dac into rust-lang:master Aug 3, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 3, 2023
@mu001999 mu001999 deleted the fix/turbofish-pat branch August 4, 2023 02:27
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Oct 23, 2023
…bank

Suggests turbofish in patterns

Fixes rust-lang#114112

r? ```@estebank```
calebcartwright pushed a commit to calebcartwright/rust that referenced this pull request Jun 22, 2024
…bank

Suggests turbofish in patterns

Fixes rust-lang#114112

r? ```@estebank```
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type specifiers with generic args in certain positions should generate errors that reference the turbofish
6 participants