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

fix: correct suggestion for significant_drop_in_scrutinee in expressions #14019

Merged

Conversation

vishruth-thimmaiah
Copy link
Contributor

This PR fixes an issue with the significant_drop_in_scrutinee, where the lint generates invalid Rust syntax when suggesting fixes for match expressions that are part of larger expressions, such as in assignment contexts. For example:

    let mutex = Mutex::new(State {});
    let _ = match mutex.lock().unwrap().foo() {
        true => 0,
        false => 1,
    };

would suggest:

let _ = let value = mutex.lock().unwrap().foo();
match value {

With this PR, it now suggests:

let value = mutex.lock().unwrap().foo();
let _ = match value {

closes: #13986

changelog: [significant_drop_in_scrutinee] Fix incorrect suggestion for significant_drop_in_scrutinee lint in expression context

@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2025

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @y21 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 17, 2025
Copy link
Member

@y21 y21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could still break in some cases, e.g. if the match expression is an argument to a function call that is broken up into multiple lines like:

foo(
  arg1,
  arg2,
  match .. { // can't put the `let` here
    ..
  }
);

We could do more precise things like iterating parent nodes with cx.tcx.hir().parent_iter(expr.hir_id) until we see a Node::Stmt or Node::Block which should give us a place to put the let statement that is always syntactically valid at least, but even that wouldn't always compile (and it might also be a bit much for a suggestion that is marked as MaybeIncorrect anyways). It does fix the linked reproducer at least, so LGTM

@y21 y21 added this pull request to the merge queue Jan 21, 2025
Merged via the queue into rust-lang:master with commit d1b5fa2 Jan 21, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect suggestions for significant_drop_in_scrutinee
3 participants