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

New lint: wildcard pattern in let #4090

Open
Vlad-Shcherbina opened this issue May 13, 2019 · 8 comments
Open

New lint: wildcard pattern in let #4090

Vlad-Shcherbina opened this issue May 13, 2019 · 8 comments
Assignees
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-restriction Lint: Belongs in the restriction lint group

Comments

@Vlad-Shcherbina
Copy link
Contributor

Vlad-Shcherbina commented May 13, 2019

I think there should be a warning about the wildcard pattern in let.
I can't imagine any situation where this makes sense, and there are cases where it's a mistake caused by taking _ for identifier though it's not.

Wrong:

{
    let _ = my scope guard or whatever
    // dropped right away

    do stuff
}

Correct:

{
    let _g = my scope guard or whatever

    do stuff

    // dropped at the end of the scope
}


<!-- TRIAGEBOT_START -->

<!-- TRIAGEBOT_ASSIGN_START -->

<!-- TRIAGEBOT_ASSIGN_DATA_START$${"user":"lolbinarycat"}$$TRIAGEBOT_ASSIGN_DATA_END -->

<!-- TRIAGEBOT_ASSIGN_END -->
<!-- TRIAGEBOT_END -->
@flip1995
Copy link
Member

flip1995 commented May 13, 2019

One use case is to prevent warnings on #[must_use] functions.

let _ = returning_result_i_dont_want_to_deal_with()

But this could be implemented as a restriction lint.

@flip1995 flip1995 added good-first-issue These issues are a good way to get started with Clippy A-lint Area: New lints labels May 13, 2019
@Manishearth
Copy link
Member

We could specifically lint on known guard types

@Vlad-Shcherbina
Copy link
Contributor Author

Many known guards (like std::cell::Ref, std::sync::MutexGuard) are meant to be accessed in scope, so this warning would be unlikely to trigger.
And I ran into this problem with a custom guard type in the first place.

Indeed I forgot that let _ = is idiomatically used to suppress must_use warnings.
I wish there was unused() function in the standard library (same behavior as std::mem::drop()) instead.


Two ideas that don't work:

  1. Warn iff there is custom Drop impl. The problem is that it will cause false positives on functions returning strings.
  2. Warn iff the expression isn't annotated with must_use. The problem is that properly implemented guards are likely must_use.

@lolbinarycat
Copy link

i feel like this makes a decent restriction lint. let _ is fine for most codebases, but some may wish to actually use all must_use types, and this enables that (along with forbid(unused_must_use))

@lolbinarycat
Copy link

@rustbot claim

@flip1995
Copy link
Member

flip1995 commented Jun 4, 2024

and this enables that (along with forbid(unused_must_use))

unused_must_use doesn't trigger on let _ = must_use();: Playground. So this lint would be a prior step to get this lint to trigger.

@flip1995 flip1995 added the L-restriction Lint: Belongs in the restriction lint group label Jun 4, 2024
@lolbinarycat
Copy link

yes, that was my point. this lint (once implemented) would be mainly useful in conjunction with unused_must_use.

i have a partial implementation of this, and i successfully registered the lint.. but for some reason it never produces any output, not even debug prints?

@flip1995
Copy link
Member

flip1995 commented Jun 7, 2024

To add a new lint, using cargo dev new_lint is usually the easiest way. Now that you have down that manually, you still have to register the lint (bottom of the page)

lolbinarycat added a commit to lolbinarycat/rust-clippy that referenced this issue Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy L-restriction Lint: Belongs in the restriction lint group
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants