Skip to content

Check if dropping an expression may have indirect side-effects #14594

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

Merged
merged 1 commit into from
May 19, 2025

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Apr 11, 2025

It is not enough to check if an expression type implements Drop to determine whether it can have a significant side-effect.

Also, add tests for unchecked cases which were explicitly handled in the code, such as checking for side effect in a struct's fields, or in its base expression.

Fix #14592

changelog: [no_effect_underscore_binding]: do not propose to remove the assignment of an object if dropping it might indirectly have a visible side effect

It is not enough to check if an expression type implements `Drop` to
determine whether it can have a significant side-effect.

Also, add tests for unchecked cases which were explicitly handled in the
code, such as checking for side effect in a `struct`'s fields, or in its
base expression.
@rustbot
Copy link
Collaborator

rustbot commented Apr 11, 2025

r? @blyxyas

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

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

blyxyas commented Apr 21, 2025

I'm not being able to test this, but are we also interested in replacing unnecessary_operation's has_drop uses into this new function?

@samueltardieu
Copy link
Contributor Author

I'm not being able to test this, but are we also interested in replacing unnecessary_operation's has_drop uses into this new function?

No, it would lose the ability to suggest replacing main()'s content in this code

struct WithDrop;
impl Drop for WithDrop {
    fn drop(&mut self) {
        todo!()
    }
}

struct WithoutDrop { inner: WithDrop }

fn main() {
    (WithoutDrop { inner: WithDrop });
}

by

    WithDrop;

as it is currently doing.

Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! ❤️ =^w^=

@samueltardieu
Copy link
Contributor Author

@blyxyas Did you forget to press merge by chance?

@blyxyas
Copy link
Member

blyxyas commented May 19, 2025

Yeah, I approved it before getting on the bus and suffered from a brain wipe (on the good side, I now have a draft for the blog post)

@blyxyas blyxyas added this pull request to the merge queue May 19, 2025
Merged via the queue into rust-lang:master with commit df33aaf May 19, 2025
11 of 13 checks passed
@samueltardieu samueltardieu deleted the issue-14592 branch May 19, 2025 11:51
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.

false positive for no_effect_underscore_binding when drop is implemented for a field of the assigned struct
3 participants