Supress suggest let else when no let in refutable bindings

[xizheyin]

Fixes #145548

In this case, we define x first, and use refutable bindings. The compiler recognizes it as a thir Let statement. (I'm not sure if this is intentional.) So, it use check_let as all other let thir statement going.

 let x = 2; Some(x) = Some(1); 

Update final decision: suppress this suggestion when no let in Let Stmt.

[rustbot]

Some changes occurred in match checking

cc @Nadrieril

[xizheyin]

I couldn't find a way to check whether the Let statement contains the let keyword in THIR. I looked at the LetStmt Node in HIR and found that it has a source: LocalSource field, which can indicate whether it comes from Assign desugar.

There are bindings in the Pat Some(x...) = Some(...), so I found a binding and then search the hir parents that matches the LetStmt Node, and check whether it comes from Assign desugar.

And then, cases from Assign Desugar should not be suggested.

[rustbot]

Some changes occurred in match lowering

cc @Nadrieril

[xizheyin]

Both thir::ExprKind::Let and thir::StmtKind::Let are checked by check_let, and only the hir_id of the pattern can be found in hir::LetExpr, so I have uniformly stored the hir_id of the pattern. Then, as in the previous version, I checked whether the parent is assign desugar.

At the same time, I added this Some(()) case to the ui test.

[dianne]

Technically, I think the THIR already contains enough information to recover HirIds, so it doesn't need to be made bigger. It stores the HirId for each expression and let statement for tracking lint levels, and it redundantly stores the ItemLocalId a second time in their scopes (which can also be used to recover the HirId). Of course, those aren't meant for this purpose, so it'd be a bit of a hack to get a HirId from them, but I felt I should mention it. If anything, I think thir::Stmt and thir::StmtKind could be made smaller by removing some of that redundancy. I'm not aware of the history here, but for a start, the THIR seems to only contain LintLevel::Explicit, so maybe it could just store the HirId (or just the ItemLocalId) instead of wrapping it in LintLevel?

[xizheyin]

Thank you. Yeah, I also noticed that the LintLevel of Let stmt may contain hir_id (somewhat hidden), but I found that many THIR nodes do not have this field, so I am concerned that this may be a hack method. For this false suggestion, both ExprKind::Let and StmtKind::Let are checked by check_let (although the current test cases do not involve ExprKind::Let).

Discussion: Is explicitly storing hir_id in THIR a beneficial practice? 🤔 See: #145569 (comment)

[dianne]

I found that many THIR nodes do not have this field

For expressions, it's stored in the ExprKind::Scope expression wrapping its THIR lowering. It's not obvious from how thir::Expr is defined, but when a HIR expression is lowered to THIR, it's wrapped in an ExprKind::Scope containing its HirId. Per the doc comment, this is how HirIds are currently tracked. The part that feels hacky to me is just how it's stored in the LintLevel, which is seemingly always Explicit. Lots of diagnostics rely on the structure of generated IR, so I'd personally wouldn't see an issue with relying on it here (apart from the LintLevel thing, which I'm unsure about).

although the current test cases do not involve ExprKind::Let

If the diagnostic is just for let statements, would it make sense for this PR to only get the HirId for let statements, and not worry about it for let expressions? You wouldn't even have deal with recovering HirIds from ExprKind::Scopes then. The thir::StmtKind::Let's HirId corresponds to the hir::Node::LetStmt, which you should be able to check for the AssignDesugar source without traversing up the HIR.

See: #145569 (comment)

Having additional information attached to top-level patterns could be nice, but I don't think it's necessary in this particular case. If we go that route, I'd personally prefer it to be attached to the top-level pattern in a consistent way, rather than spread across each pattern-matching construct.

[xizheyin]

Thanks for your detailed comments!

If the diagnostic is just for let statements, would it make sense for this PR to only get the HirId for let statements, and not worry about it for let expressions?

It would be very nice approach if we just want to fix the pattern in #145548.

The part that feels hacky to me is just how it's stored in the LintLevel, which is seemingly always Explicit.

This part is also what I am concerned. LintLevel seems to be not designed for this usage. But maybe leaving a comment when using it is enough.

prefer it to be attached to the top-level pattern in a consistent way

Agreed, then we have two ways.

1. just fix the specific issue by using lint_level.
2. leave it open until there are some more consistent way to get hir_id (or something else). (maybe a MCP?)

Let's wait for more comments from the reviewers. :)

[xizheyin]

Based on the above discussion, I used the hir_id contained within LetStmt.lint_level as the current solution that is the least modification to address this issue. And since this is a hack, I included some comments.

[lcnr]

is it the case that the lint level is always LintLevel::Explicit in the THIR?

If so could store store the hir_id in the StmtKind instead and then have construct a LintLevel::Explicit from that id where necessar

[lcnr]

I think adding another Option as an argument to binding_is_irrefutable makes calls to the function quite confusing.

I personally also dislike passing in a &'static str and feel like it should ideally be an enum which unifies all the Option arguments as afaict each call-site mostly always or never provides some of the args

View changes since this review