rust
rust copied to clipboard
rust-lang
Uplift the `let_underscore` lints from clippy into rustc.
This PR resolves #97241.
This PR adds three lints from clippy--let_underscore_drop, let_underscore_lock, and let_underscore_must_use, which are meant to capture likely-incorrect uses of let _ = ... bindings (in particular, doing this on a type with a non-trivial Drop causes the Drop to occur immediately, instead of at the end of the scope. For a type like MutexGuard, this effectively releases the lock immediately, which is almost certainly the wrong behavior)
In porting the lints from clippy I had to copy over a bunch of utility functions from clippy_util that these lints also relied upon. Is that the right approach?
Note that I've set the must_use and drop lints to Allow by default and set lock to Deny by default (this matches the same settings that clippy has). In talking with @estebank he informed me to do a Crater run (I am not sure what type of Crater run to request here--I think it's just "check only"?)
On the linked issue, there's some discussion about using must_use and Drop together as a heuristic for when to warn--I did not implement this yet.
r? @estebank
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @estebank (or someone else) soon.
Please see the contribution instructions for more information.
The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
running 59 tests
.........ii....................F...........................
failures:
---- src/let_underscore.rs - let_underscore::LET_UNDERSCORE_LOCK (line 57) stdout ----
error[E0433]: failed to resolve: use of undeclared crate or module `thread`
--> src/let_underscore.rs:61:1
|
6 | thread::spawn(move || {
| ^^^^^^ use of undeclared crate or module `thread`
error: aborting due to previous error
For more information about this error, try `rustc --explain E0433`.
Couldn't compile the test.
The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
running 59 tests
.........ii.......F........................................
failures:
---- src/let_underscore.rs - let_underscore::LET_UNDERSCORE_LOCK (line 57) stdout ----
error: non-binding let on a synchronization lock
--> src/let_underscore.rs:65:5
|
10 | let _ = data.lock().unwrap();
|
|
= note: `#[deny(let_underscore_lock)]` on by default
help: consider binding to an unused variable
|
10 | let _unused = data.lock().unwrap();
help: consider explicitly droping with `std::mem::drop`
|
|
10 | let _ = drop(...);
error: aborting due to previous error
Couldn't compile the test.
The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Compiling tempfile v3.2.0
Compiling serde_json v1.0.59
Compiling lint-docs v0.1.0 (/checkout/src/tools/lint-docs)
Finished release [optimized] target(s) in 5.79s
warning: lint group `let-underscore` is missing from the GROUP_DESCRIPTIONS list
If this is a new lint group, please update the GROUP_DESCRIPTIONS in src/tools/lint-docs/src/groups.rs
Rustbook (x86_64-unknown-linux-gnu) - cargo
Rustbook (x86_64-unknown-linux-gnu) - embedded-book
Rustbook (x86_64-unknown-linux-gnu) - edition-guide
Building stage0 tool linkchecker (x86_64-unknown-linux-gnu)
---
doc tests for: /checkout/src/doc/rustc/src/tests/index.md
doc tests for: /checkout/src/doc/rustc/src/what-is-rustc.md
finished in 1.184 seconds
Generating lint docs (x86_64-unknown-linux-gnu)
error: failed to test example in lint docs for `let_underscore_drop` in /checkout/compiler/rustc_lint/src/let_underscore.rs:10: lint docs should contain the line `{{produces}}`
This error was generated by the lint-docs tool.
This error was generated by the lint-docs tool.
This tool extracts documentation for lints from the source code and places
them in the rustc book. See the declare_lint! documentation
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint_defs/macro.declare_lint.html
Build completed unsuccessfully in 0:32:58
Build completed unsuccessfully in 0:32:58
To re-run these tests, run: ./x.py test --keep-stage=0 src/tools/lint-docs
The --keep-stage flag should be used if you have already built the compiler
The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
........................................................................................ 13288/13303
...............
failures:
---- [ui] src/test/ui/lint/let_underscore/let_underscore_lock.rs stdout ----
error: ui test compiled successfully!
status: exit status: 0
status: exit status: 0
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/lint/let_underscore/let_underscore_lock.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "--emit" "metadata" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/lint/let_underscore/let_underscore_lock" "-A" "unused" "-Crpath" "-O" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/lint/let_underscore/let_underscore_lock/auxiliary"
stdout: none
stderr: none
---- [ui] src/test/ui/lint/let_underscore/let_underscore_must_use.rs stdout ----
- warning: non-binding let on a expression marked `must_use`
- --> $DIR/let_underscore_must_use.rs:11:5
- |
- LL | let _ = MustUseType;
- |
- |
- = note: requested on the command line with `-W let-underscore-must-use`
- help: consider binding to an unused variable
- |
- LL | let _unused = MustUseType;
- help: consider explicitly droping with `std::mem::drop`
- |
- |
- LL | let _ = drop(...);
-
-
- warning: non-binding let on a expression marked `must_use`
- --> $DIR/let_underscore_must_use.rs:12:5
- LL | let _ = must_use_function();
- | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
- |
- help: consider binding to an unused variable
- help: consider binding to an unused variable
- |
- LL | let _unused = must_use_function();
- help: consider explicitly droping with `std::mem::drop`
- |
- |
- LL | let _ = drop(...);
-
- warning: 2 warnings emitted
-
-
---
To only update this specific test, also pass `--test-args lint/let_underscore/let_underscore_must_use.rs`
error: 1 errors occurred comparing output.
status: exit status: 0
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/lint/let_underscore/let_underscore_must_use.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "--json" "future-incompat" "-Ccodegen-units=1" "-Zui-testing" "-Zdeduplicate-diagnostics=no" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/lint/let_underscore/let_underscore_must_use/a" "-Crpath" "-O" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-W" "let_underscore_must_use" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/lint/let_underscore/let_underscore_must_use/auxiliary"
stdout: none
stderr: none
failures:
[ui] src/test/ui/lint/let_underscore/let_underscore_lock.rs
:+1: for adding let_underscore_lock and making it deny by default; that seems likely to catch bugs.
Adding let_underscore_drop and making it allow by default seems fine as well; I don't think we ever want to make that warn or error, since it's legitimate code, but I can imagine someone wanting to find and catch such code.
However, I'm not sure I understand the value of let_underscore_must_use. let _ = ... seems like exactly the pattern people normally use to intentionally ignore a must_use; under what circumstances would someone want to get a warning or error about this?
However, I'm not sure I understand the value of
let_underscore_must_use.let _ = ...seems like exactly the pattern people normally use to intentionally ignore amust_use; under what circumstances would someone want to get a warning or error about this?
I added it to match the corresponding clippy lint, but I think you're right--if people really want it, clippy would probably be a more appropriate tool for that, I'll remove let_underscore_must_use.
The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
doc tests for: /checkout/src/doc/rustc/src/tests/index.md
doc tests for: /checkout/src/doc/rustc/src/what-is-rustc.md
finished in 1.181 seconds
Generating lint docs (x86_64-unknown-linux-gnu)
error: failed to test example in lint docs for `let_underscore_drop` in /checkout/compiler/rustc_lint/src/let_underscore.rs:7: lint docs should contain the line `{{produces}}`
This error was generated by the lint-docs tool.
This error was generated by the lint-docs tool.
This tool extracts documentation for lints from the source code and places
them in the rustc book. See the declare_lint! documentation
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_lint_defs/macro.declare_lint.html
Build completed unsuccessfully in 0:32:35
Build completed unsuccessfully in 0:32:35
To re-run these tests, run: ./x.py test --keep-stage=0 src/tools/lint-docs
The --keep-stage flag should be used if you have already built the compiler
:hourglass: Trying commit cdf66060666fa53cbca9647ac7434bdfd2cc37a5 with merge 844341966fa4deedcf5964b5cbcd30dd60c394b4...
:sunny: Try build successful - checks-actions
Build commit: 844341966fa4deedcf5964b5cbcd30dd60c394b4 (844341966fa4deedcf5964b5cbcd30dd60c394b4)
The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Compiling rustc-demangle v0.1.21
error: non-binding let on a type that implements `Drop`
--> library/alloc/src/vec/into_iter.rs:324:21
|
324 | let _ = RawVec::from_raw_parts_in(self.0.buf.as_ptr(), self.0.cap, alloc);
|
|
= note: `#[deny(let_underscore_drop)]` on by default
help: consider binding to an unused variable to avoid immediately dropping the value
|
324 | let _unused = RawVec::from_raw_parts_in(self.0.buf.as_ptr(), self.0.cap, alloc);
help: consider immediately dropping the value
|
|
324 | drop(RawVec::from_raw_parts_in(self.0.buf.as_ptr(), self.0.cap, alloc));
error: could not compile `alloc` due to previous error
warning: build failed, waiting for other jobs to finish...
Build completed unsuccessfully in 0:04:32
The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Compiling rustc-demangle v0.1.21
error: non-binding let on a type that implements `Drop`
--> library/alloc/src/vec/into_iter.rs:324:21
|
324 | let _ = RawVec::from_raw_parts_in(self.0.buf.as_ptr(), self.0.cap, alloc);
|
|
= note: `-D let-underscore-drop` implied by `-D warnings`
help: consider binding to an unused variable to avoid immediately dropping the value
|
324 | let _unused = RawVec::from_raw_parts_in(self.0.buf.as_ptr(), self.0.cap, alloc);
help: consider immediately dropping the value
|
|
324 | drop(RawVec::from_raw_parts_in(self.0.buf.as_ptr(), self.0.cap, alloc));
error: could not compile `alloc` due to previous error
warning: build failed, waiting for other jobs to finish...
Build completed unsuccessfully in 0:03:59
@estebank ah it seems i cannot do a crater run. If there aren't any other changes I should make, can you run the crater run?
:hourglass: Trying commit a9095ff2139fb305b15006d1f2a1ff16da0b6c29 with merge 5c7108a2805ef56743d713d2789c5e802631c9e8...
:sunny: Try build successful - checks-actions
Build commit: 5c7108a2805ef56743d713d2789c5e802631c9e8 (5c7108a2805ef56743d713d2789c5e802631c9e8)
:ok_hand: Experiment pr-97739 created and queued.
:robot: Automatically detected try build 5c7108a2805ef56743d713d2789c5e802631c9e8
:mag: You can check out the queue and this experiment's details.
:information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
:construction: Experiment pr-97739 is now running
:information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
:tada: Experiment pr-97739 is completed!
:bar_chart: 47 regressed and 4 fixed (238589 total)
:newspaper: Open the full report.
:warning: If you notice any spurious failure please add them to the blacklist! :information_source: Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more
Looks like the crater run finished! The report says 47 crates failed--I checked a few of the failures and they do seem to be accurate (people let _ = a mutex).
@a2aaron looking at the output, it seems indeed to be catching real issues 🎉
Looking at one of them with fresh eyes, I'm realizing that we're not telling people why this is a problem:
error: non-binding let on a synchronization lock
--> src/main.rs:106:9
|
106 | let _ = lock.lock().unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: `#[deny(let_underscore_lock)]` on by default
help: consider binding to an unused variable to avoid immediately dropping the value
|
106 | let _unused = lock.lock().unwrap();
| ~~~~~~~
help: consider immediately dropping the value
|
106 | drop(lock.lock().unwrap());
| ~~~~~ +
I think we should add more info to the lint:
error: non-binding let on a synchronization lock
--> src/main.rs:106:9
|
106 | let _ = lock.lock().unwrap();
| ^ ^^^^^^^^^^^^^^^^^^^^ this lock is not assigned to a binding and is immediately dropped
| |
| this binding will immediately drop the value assigned to it
|
= note: `#[deny(let_underscore_lock)]` on by default
help: consider binding to an unused variable to dropp the value at the end of its scope
|
106 | let _lock_guard = lock.lock().unwrap();
| ~~~~~~~~~~~
help: consider explicitly immediately dropping the value
|
106 | drop(lock.lock().unwrap());
| ~~~~~ +
In order to get that multiple span primary, you can use let span: MultiSpan = vec![span1, span2]; and span.push_span_label(span1, "text".to_string()); for the labels.
You can also make the _ span a secondary span, if that's easier, that's not a problem.
Hey! It looks like you've submitted a new PR for the library teams!
If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.
Examples of T-libs-api changes:
- Stabilizing library features
- Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
- Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
- Changing public documentation in ways that create new stability guarantees
- Changing observable runtime behavior of library APIs
The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Checking rustc_mir_build v0.0.0 (/checkout/compiler/rustc_mir_build)
error[E0603]: struct `LintDiagnosticBuilder` is private
--> compiler/rustc_lint/src/let_underscore.rs:4:26
|
4 | use rustc_middle::{lint::LintDiagnosticBuilder, ty};
| ^^^^^^^^^^^^^^^^^^^^^ private struct
note: the struct `LintDiagnosticBuilder` is defined here
--> /checkout/compiler/rustc_middle/src/lint.rs:5:46
|
|
5 | use rustc_errors::{Diagnostic, DiagnosticId, LintDiagnosticBuilder, MultiSpan};
For more information about this error, try `rustc --explain E0603`.
error: could not compile `rustc_lint` due to previous error
warning: build failed, waiting for other jobs to finish...
:pushpin: Commit d355ec94ff0ef409bfa953f715987a98de13fd64 has been approved by estebank
It is now in the queue for this repository.
:hourglass: Testing commit d355ec94ff0ef409bfa953f715987a98de13fd64 with merge 0fb29cc7184382b8ad840d84e40022dbb15351f2...
The job x86_64-gnu-aux failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
error: RPC failed; curl 56 GnuTLS recv error (-54): Error in the pull function.
fatal: The remote end hung up unexpectedly
fatal: early EOF
fatal: index-pack failed
thread 'main' panicked at 'assertion failed: status.success()', src/tools/cargotest/main.rs:148:13
finished in 217.073 seconds
Build completed unsuccessfully in 0:18:09
Build completed unsuccessfully in 0:18:09
make: *** [check-aux] Error 1
Makefile:44: recipe for target 'check-aux' failed