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

Lint for significant drops who may have surprising lifetimes #1

Closed
wants to merge 2 commits into from

Conversation

PrestonFrom
Copy link
Owner

@PrestonFrom PrestonFrom commented Feb 18, 2022

Add new trait (SignificantDrop) and lint to look for temporaries of types who implement it and who may have surprising lifetimes.

changelog: new lint [`significant_drop_in_scrutinee`] 

@PrestonFrom PrestonFrom changed the title Initial commit for adding new lint for significant drops who may have… Lint for significant drops who may have surprising lifetimes Feb 20, 2022
Copy link

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

While I have a few more things that we shoukd iterate on, (message text, more precise spans, documentation) I think we should merge it and then keep working on it in follow ups. This should put less of a burden on you and give everyone something to experiment with, even if it's not yet where we want it to be at the end

/// Trait to help linters look for drops that happen at unexpected times
/// Any type whose drop has a significant side effect can impl this trait
/// so linters can let users know if it has a potentially surprising lifetime.
#[stable(feature = "significant_drop", since = "1.60.0")]
Copy link

Choose a reason for hiding this comment

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

Please make it unstable for now

Copy link
Owner Author

Choose a reason for hiding this comment

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

Done!

} else {
found_significant_drop
}
}
Copy link

Choose a reason for hiding this comment

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

You probably need to format these files, but I'm unsure how to do that in the clippy copy inside the rustc repo

Copy link
Owner Author

Choose a reason for hiding this comment

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

I checked the clippy repo documentation, but I couldn't find anything on formatting, so I ran cargo fmt. I'm not sure if that will be satisfactory or not though.


};
if !found_significant_drop {
match expression.peel_drop_temps().kind {
Copy link

Choose a reason for hiding this comment

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

Whoa, magic function, good find!

Copy link
Owner Author

Choose a reason for hiding this comment

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

:)

#[clippy::version = "1.60.0"]
pub SIGNIFICANT_DROP_IN_SCRUTINEE,
nursery,
"default lint description"
Copy link

Choose a reason for hiding this comment

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

You forgot to fill in this placeholder

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh! Thank you!

@PrestonFrom PrestonFrom marked this pull request as ready for review February 21, 2022 03:14
@PrestonFrom
Copy link
Owner Author

Thank you for the initial review -- reviewing/merging an unstable version and iterating makes sense to me. I'll open a PR into the main repo.

@PrestonFrom PrestonFrom self-assigned this Feb 21, 2022
PrestonFrom added a commit that referenced this pull request Apr 15, 2022
author Preston From <[email protected]> 1645164142 -0600
committer Preston From <[email protected]> 1650005351 -0600

parent e7575f9
author Preston From <[email protected]> 1645164142 -0600
committer Preston From <[email protected]> 1650005248 -0600

Lint for significant drops who may have surprising lifetimes #1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants