chore: deny let_underscore_drop #5095

danking · 2025-10-27T21:38:27Z

I'm curious for opinions on this.

I stubbed my toe recently on:

let _ = span.enter();

Which does not enter the span for the remainder of the function (it immediately drops the span). There is already a by-default-deny lint for let _ = lock() but no such lint for other guard-like things:

danking · 2025-10-27T22:01:25Z

vortex-array/src/compute/conformance/cast.rs

    if nullability == Nullability::Nullable {
        // Try casting to non-nullable (may fail if nulls present)
-        let _ = cast(array, &DType::Bool(Nullability::NonNullable));
+        cast(array, &DType::Bool(Nullability::NonNullable)).vortex_unwrap();


This file had several tests that did not actually run their tests because they didn't try to unwrap the result.

danking · 2025-10-27T22:01:51Z

vortex-datafusion/src/persistent/sink.rs

        let session = SessionContext::new_with_state(session_state_builder.build());

-        let _ = session
+        let _unused = session


This is a bit suspicious to me? Shouldn't we fail if the external table fails to create?

isn't that handled by the .await? ?

Ah, yes, you're right. This is flagged because DataFrame has a non-trivial Drop (probably drops some Arcs).

danking · 2025-10-27T22:02:09Z

vortex-expr/src/exprs/cast.rs

    fn replace_children() {
        let expr = cast(root(), DType::Bool(Nullability::Nullable));
-        let _ = expr.with_children(vec![root()]);
+        expr.with_children(vec![root()]).vortex_unwrap();


This did not test what it claimed to test.

danking · 2025-10-27T22:02:15Z

vortex-expr/src/exprs/is_null.rs

    fn replace_children() {
        let expr = is_null(root());
-        let _ = expr.with_children(vec![root()]);
+        expr.with_children(vec![root()]).vortex_unwrap();


This did not test what it claimed to test.

danking · 2025-10-27T22:02:45Z

vortex-io/src/limit.rs


        // After consuming, should be able to push again
-        let _ = stream.next().await;
+        assert!(stream.next().await.is_some());


This seems like a reasonable assertion to me. The stream ought to have that one element we put into it!

can just do .unwrap() in that case?

danking · 2025-10-27T22:03:16Z

vortex-mask/src/bitops.rs

        let mask1 = Mask::new_true(5);
        let mask2 = Mask::new_true(3);
-        let _ = &mask1 & &mask2;
+        let _unused = &mask1 & &mask2;


~~bitand has a lint which prevents me from completely ignoring the result a la &mask1 & &mask2.~~

Masks have non-trivial drops (I think, an Arc).

codspeed-hq · 2025-10-27T22:07:37Z

CodSpeed Performance Report

Merging #5095 will not alter performance

_{Comparing dk/let-underscore-drop (1d817cf) with develop (28f5b3d)}

Summary

✅ 1325 untouched
⏩ 148 skipped¹

148 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

danking · 2025-10-28T18:57:15Z

Blocked on fixing our conformance tests: #5101

cloudflare-workers-and-pages · 2025-10-31T19:00:34Z

Deploying vortex-bench with Cloudflare Pages

Latest commit:	`304d033`
Status:	✅ Deploy successful!
Preview URL:	https://b2d55f2e.vortex-93b.pages.dev
Branch Preview URL:	https://dk-let-underscore-drop.vortex-93b.pages.dev

View logs

codecov · 2025-10-31T19:09:39Z

Codecov Report

❌ Patch coverage is 88.88889% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.96%. Comparing base (0962924) to head (1d817cf).
⚠️ Report is 3 commits behind head on develop.

Files with missing lines	Patch %	Lines
vortex-duckdb/build.rs	42.85%	4 Missing ⚠️
vortex-layout/src/layouts/dict/writer.rs	25.00%	3 Missing ⚠️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

a10y · 2025-10-31T19:41:44Z

the only place I really have used let _ = .. in production code is in FFI implementations, usually when you do let _ = Box::from_raw(..) to allow the drop handler to run. But I guess that the same can be achieved by just explicitly calling drop(Box::from_raw(..)). And pretty much anywhere else we use let _ = .. and actually mean it?

a10y · 2025-10-31T19:42:22Z

vortex-duckdb/build.rs

    let zip_source_path = download_duckdb_source_archive().unwrap();
    let extracted_source_path = extract_duckdb_source(zip_source_path).unwrap();
-    let _ = fs::remove_dir_all(&duckdb_repo);
+    drop(fs::remove_dir_all(&duckdb_repo));


unwrap the result

I went with the following because I think the intention is to not fail if the directory already exists (but I agree we should fail if there's, e.g. a permissions issue or some other weird problem).

match fs::remove_dir_all(&duckdb_dir) { Err(err) if err.kind() == ErrorKind::NotFound => (), otherwise => otherwise?, }

a10y · 2025-10-31T19:42:32Z

vortex-duckdb/build.rs

    let duckdb_library_dir = target_dir.join("duckdb-lib");

-    let _ = fs::remove_dir_all(&duckdb_library_dir);
+    drop(fs::remove_dir_all(&duckdb_library_dir));


unwrap the result here too

a10y · 2025-10-31T19:43:02Z

vortex-io/src/file/read/mod.rs

        // When the FileHandle is dropped, we can send a shutdown event to the I/O stream.
        // If the I/O stream has already been dropped, this will fail silently.
-        let _ = self.events.unbounded_send(ReadEvent::Dropped(self.id));
+        drop(self.events.unbounded_send(ReadEvent::Dropped(self.id)));


this should also unwrap the result i think?

oh i see, we don't want to panic in the drop impl here.

danking · 2025-11-03T18:18:48Z

the only place I really have used let _ = .. in production code is in FFI implementations, usually when you do let _ = Box::from_raw(..) to allow the drop handler to run. But I guess that the same can be achieved by just explicitly calling drop(Box::from_raw(..)). And pretty much anywhere else we use let _ = .. and actually mean it?

Yeah this is my argument. let _ = ... is surprisingly different from let _x = .... The surprisingly different behavior is exactly drop(...), so we should just say that directly.

I'm curious for opinions on this. I stubbed my toe recently on: ``` let _ = span.enter(); ``` Which does not enter the span for the remainder of the function (it immediately drops the span). There is already a by-default-deny lint for `let _ = lock()` but no such lint for other guard-like things: - [`tracing::Span::enter()`](https://docs.rs/tracing/latest/tracing/struct.Span.html#method.enter) - [`xshell::Shell::push_dir`](https://docs.rs/xshell/latest/xshell/struct.Shell.html#method.push_dir) Signed-off-by: Daniel King <[email protected]>

Signed-off-by: Daniel King <[email protected]>

danking added the chore Release label indicating a trivial change label Oct 27, 2025

danking commented Oct 27, 2025

View reviewed changes

danking marked this pull request as ready for review October 27, 2025 22:03

danking requested a review from a10y October 31, 2025 19:37

a10y reviewed Oct 31, 2025

View reviewed changes

danking requested a review from a10y November 4, 2025 20:21

a10y approved these changes Nov 4, 2025

View reviewed changes

danking added 5 commits November 5, 2025 10:03

fix tests too

767db7f

Signed-off-by: Daniel King <[email protected]>

more tests

f9775e2

Signed-off-by: Daniel King <[email protected]>

use expect instead of assert(..is_some())

c3a4858

Signed-off-by: Daniel King <[email protected]>

explicitly ignore only notfound

1d817cf

Signed-off-by: Daniel King <[email protected]>

a10y force-pushed the dk/let-underscore-drop branch from 2e5035d to 1d817cf Compare November 5, 2025 15:13

a10y enabled auto-merge (squash) November 5, 2025 15:15

a10y merged commit e3c7c3c into develop Nov 5, 2025
39 checks passed

a10y deleted the dk/let-underscore-drop branch November 5, 2025 15:24

chore: deny let_underscore_drop #5095

chore: deny let_underscore_drop #5095

Uh oh!

Conversation

danking commented Oct 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danking Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codspeed-hq bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #5095 will not alter performance

Summary

Footnotes

Uh oh!

danking commented Oct 28, 2025

Uh oh!

cloudflare-workers-and-pages bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying vortex-bench with Cloudflare Pages

Uh oh!

codecov bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

a10y commented Oct 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danking commented Nov 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danking Oct 27, 2025 •

edited

Loading

codspeed-hq bot commented Oct 27, 2025 •

edited

Loading

cloudflare-workers-and-pages bot commented Oct 31, 2025 •

edited

Loading

codecov bot commented Oct 31, 2025 •

edited

Loading