Fix bad diagnostics for anon params with ref and/or qualified paths

[JohnTitor]

Fixes #82729
It's easier to review with hiding whitespace changes.

[rust-highfive]

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

[estebank]

@JohnTitor can you also add a test case for a fully qualified path, like in the original report? I get the feeling that the new code will work well only for the case of fn foo(&bar) and not for fn foo(&<Bar as T>::Baz). To handle the later, we might want to add a Parser snapshot here and try to parse a type. If successful, we know we can suggest _: <Ty> and recover the parser state.

[JohnTitor]

Good point! Yeah, the current parameter_without_type doesn't handle it with/without refs (1082ced). Should we address that case in this PR?

For now, I only added a note about anon params since I'm not sure how we deal with this: To handle the later, we might want to add a Parser snapshot here and try to parse a type. If successful, we know we can suggest _: <Ty> and recover the parser state.

[estebank]

You can see an example of what I mean here:

https://github.com/rust-lang/rust/blob/6f43f4c3fbc99c3a6699ed8d44608c8f086b10af/compiler/rustc_parse/src/parser/diagnostics.rs#L749-L761

You have a bounded lookahead to see that you might be in a weird known situation that will for sure emit an error. You clone the parser and try an alternative parse. If it is successful, you emit a targeted error, otherwise you "revert" the parser by overriding the parser state with your snapshot. Doing this you gain a lot of confidence in any suggestions you give because you know that the alternative parse worked.

[oli-obk]

r? @estebank

[estebank]

Using the snapshot strategy this can then be replaced with something like:

                PatKind::Ref(ref pat, mutab) if matches!(pat.clone().into_inner().kind, PatKind::Ident(_, ident, _)) => {
                    match pat.clone().into_inner().kind {
                        PatKind::Ident(_, ident, _) => {
                            let mutab = mutab.prefix_str();
                            (
                                ident,
                                format!("self: &{}{}", mutab, ident),
                                format!("{}: &{}TypeName", ident, mutab),
                                format!("_: &{}{}", mutab, ident),
                            )
                        }
                        _ => unreachable!(),
                    }
                }
                _ => {
                    let snapshot = self.clone();
                    match self.parse_ty_common(
                        AllowPlus::No,
                        AllowCVariadic::No,
                        RecoverQPath::No,
                        RecoverReturnSign::No,
                    ) {
                        Ok(ty) => {
                            err.span_suggestion_verbose(
                                pat.span,
                                "explicitly ignore the parameter name",
                                format!("_: {}", ty),
                                Applicability::MachineApplicable,
                            );
                            err.note("anonymous parameters are removed in the 2018 edition (see RFC 1685)");
                            return None;
                        }
                        Err(mut inner_err) => {
                            inner_err.cancel();
                            *self = snapshot;
                        }
                    }
                    return None,
                }
            };

[estebank]

Doing that we can even go further and actually bubble up the ty and continue to the type checking as if the user had written _: , but that is not entirely necessary.

[JohnTitor]

Thanks for the pointer! Just copy-pasted your suggestion but the diagnostics don't seem to be changed (a6c75e8). Am I missing something?

[estebank]

I think that parse_ty_common might not be the right thing to call then. Try using parse_ty_for_param (which should be almost the same thing, only Yes passed to all parse_ty_common arguments. Can you change the inner_err.cancel(); to inner_err.emit();? I'm wondering whether it is taking that branch always and why.

[JohnTitor]

Suggestions are applied via 01adaef. So looks like the parser looks at ) and cannot find a type.

[JohnTitor]

I've come up with another approach: call to_ty() and emit a suggestion if it successes, implemented via 2d99e68.
@estebank Could you check it?

[estebank]

Oh, that seems to work. Do we have a test for fn foo(<A as B>::X, <C as D>::Y)? I wonder how well this new method works with multiple arguments. Reviewing now, but I think it'll be ready to merge (and you can add a test if you have the time and r=me).

[JohnTitor]

Sure, added via 55bdf7f. The current rustc emits errors for each arg (https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=1a7575e24dd5fd79ad045613426147c2) and this PR's suggestion is also applied to both.

[estebank]

@bors r+

Feel free to update this PR with a multiple arg test if you get to it before bors does its thing.

[bors]

📌 Commit 2d99e68 has been approved by estebank

[JohnTitor]

@bors r=estebank

[bors]

📌 Commit 55bdf7f has been approved by estebank