Fix an ICE on an invalid binding @ ... in a tuple struct pattern

[jakubadamw]

Fixes #74539.

[rust-highfive]

r? @oli-obk

(rust_highfive has picked a reviewer for you, use r? to override)

[jakubadamw]

@JohnTitor, oh, I see in #74539 (comment) that you have a better idea than this? 🙂

@jakubadamw Great! You should tweak src/librustc_ast_lowering/pat.rs not losing diags.

What do you mean by “not losing diags”? The thing is that AST lowering runs after name resolution. 🤔 How else could it “poison” that particular invalid binding? Sorry, I don't know the compiler very well, it's been years. 🙂

[nagisa]

@bors r+

[bors]

📌 Commit db77a9e516ad37a730c19e3c008c7279b5767aa7 has been approved by nagisa

[JohnTitor]

Hmm, x should be found here?

[jakubadamw]

@JohnTitor, this is a consequence of the nature of the fix. But I think you're right, this should be done differently.

[jakubadamw]

@JohnTitor, okay, I have now pushed a second commit with an alternate, simpler approach. But it leads to diagnostics that are, in my opinion, degraded compared to the status quo.

error: `x @` is not allowed in a tuple struct
  --> $DIR/issue-74539.rs:9:13
   |
LL |             x @ ..
   |             ^^^^^^ this is only allowed in slice patterns
   |
   = help: remove this and bind each tuple field independently
help: if you don't need to use the contents of x, discard the tuple's remaining fields
   |
LL |             ..
   |             ^^

error: `..` patterns are not allowed here
  --> $DIR/issue-74539.rs:9:17
   |
LL |             x @ ..
   |                 ^^
   |
   = note: only allowed in tuple, tuple struct, and slice patterns

error[E0023]: this pattern has 1 field, but the corresponding tuple variant has 2 fields
  --> $DIR/issue-74539.rs:8:9
   |
LL |       A(u8, u8),
   |       --------- tuple variant defined here
...
LL | /         E::A(
LL | |             x @ ..
LL | |         ) => { x }
   | |_________^ expected 2 fields, found 1

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0023`.

We end up with two extra diagnostics that do not contribute anything new.

Which solution seems better in your opinion?

[JohnTitor]

@jakubadamw Thanks for investigating more! I like the later diagnostics but let's hear opinion from @nagisa or someone from T-compiler.

[nagisa]

I personally like the former slightly more because its not as redundant (and x not being found is fine given that its a variable introduced by an error), but ultimately either error seems fine to me as either approach fixes the significantly more important problem – the ICE.

In the end the decision on diagnostics is something that @rust-lang/wg-diagnostics is responsible for and the diagnostics themselves can be iterated further in a separate PR that does not target a backport to beta or stable. If we want this to be backportable it needs to be as simple and obviously correct as it can possibly get.

[jakubadamw]

@nagisa, thank you. Looks like I stepped on your toes by pushing after your r+, sorry! 🙂 Yeah, I agree the first solution gives nicer results (although implementation wise it's not as simple as the second one). Should we await input from @rust-lang/wg-diagnostics or go ahead with the first approach? If the latter, then I can take back the redundant commits.

[jakubadamw]

@nagisa, okay, I brought back the initial version. 🙂

[nagisa]

@bors r+

[bors]

📌 Commit 77a2fbb86e9a0cddc18722390d939e9bcd874c64 has been approved by nagisa

[jakubadamw]

@nagisa, pardon, this will need one more r+, as I had to tidy up the style per the failed build. 🙂

[davidtwco]

@bors r=nagisa

[bors]

📌 Commit f5e5eb6 has been approved by nagisa

[estebank]

This PR can be merged as is, but I think we could also add another branch that detects the case with PatKind::Ident(..) where .. is present and record it with a PartialRes::new(Res::Err), which should eliminate the second unnecessary error that complains about x not being defined. (Haven't looked at this deeply enough to be sure.)

[pnkfelix]

The beta nomination was discussed at weekly T-compiler meeting.

The stable nomination was also discussed at that same meeting.

At the end of both discussions, I noted (and @Mark-Simulacrum agreed) that we'd be more comfortable backporting this if it guarded against the scenario where this guard fires but the AST-lowering code, for whatever reason, doesn't signal an error.

In particular, we think that should be implementable via a delay_span_bug invocation.

Having said that, adding a delay_span_bug invocation is not a strict prerequisite for the backport; it is merely something we see as a "nice to have" since it would give us confidence that this change cannot cause a miscompilation down the road.

Having said that: beta backport approved, and stable backport approved.

[cuviper]

I've removed the beta backport labels. This was already backported to stable for 1.45.1, then reverted in 1.45.2 due to #74954. We'll let this release naturally with the follow-up fix in #74963.