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

Uplift clippy::let_underscore_lock to rustc #97241

Closed
estebank opened this issue May 21, 2022 · 6 comments · Fixed by #97739
Closed

Uplift clippy::let_underscore_lock to rustc #97241

estebank opened this issue May 21, 2022 · 6 comments · Fixed by #97739
Assignees
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@estebank
Copy link
Contributor

estebank commented May 21, 2022

The lint clippy::let_underscore_lock should be uplifted into rustc from clippy, as this is such a big potential issue that it shouldn't require people to opt-into clippy to be detected.

@estebank estebank added A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. labels May 21, 2022
@a2aaron
Copy link
Contributor

a2aaron commented May 21, 2022

@rustbot claim

Hi! I'm the person that Cassie from twitter was talking about. I'd like to help work on this issue if possible.

There's some additional discussion about the clippy::let_underscore_lock lint here: rust-lang/rust-clippy#8859. The main suggestion was to also use the #[clippy::has_significant_drop] attribute or to possibly use #[must_use] + nontrivial Drop implementation as the heuristic to trigger the lint on.

@clarfonthey
Copy link
Contributor

clarfonthey commented May 25, 2022

So, I was thinking about the use cases for this and was wondering if this is necessarily the right approach to take. In general, locks should contain the values that need mutual exclusion, so that the scope of the lock can be determined simply by how long the values inside are needed. That way, you can simply annotate locking methods with #[must_use] to get the right behaviour.

However, there are definitely a fair number of cases where significant drops are useful, described in this section of the nomicon. One thing to consider here is that Rust 1.0 only had lexical drops, meaning that values were always kept until the end of the scope, but nowadays, that can't be guaranteed.

Because we don't enforce lexical drops any more, there could be genuine value to some kind of dedicated syntax for these; I'm thinking something along the lines of Python's with statements, like:

with mutex.lock() {
   // guarded code
}

We could then have some sort of attribute like must_scope which is similar to must_use, but encourages the caller to put the value into a with block.

Alternatively, if we don't want to completely enclose the code, we could use something like golang's defer keyword, which is essentially just sugar for "run this after scope ends, please."

However, if we don't think that this line of reasoning is justifiable, I think that maybe simply adding this method would do:

impl<T> MutexGuard<'_, T> {
    pub fn guard<U, F: FnOnce() -> U>(self, f: F) -> U {
        let val = f();
        drop(self);
        val
    }
}

And, if someone wants to use a lock purely for its effects, they can just call mutex.lock().guard(|| { ... }), and #[must_use] still applies.

@estebank
Copy link
Contributor Author

Even if we provide a guard method, we would still need a linting mechanism, we would "just" suggest a different remediation.

@clarfonthey
Copy link
Contributor

What would be wrong with #[must_use] as said linting mechanism?

@CAD97
Copy link
Contributor

CAD97 commented May 28, 2022

The problem is that let _ = f(); explicitly silences #[must_use]. In effect, let_underscore_drop as a lint is a strengthening of #[must_use] to not be silenced by let _.

The best way to handle this is probably something like #[must_use(and_let_underscore_is_a_footgun)] to ask for the stronger lint.

@clarfonthey
Copy link
Contributor

Oh, I see now what the issue is. Yeah… that makes a lot more sense. I seem to have gotten hyperfocused on one aspect of my argument that that part, which is the main part, flew over my head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants