`unused_io_amount` captures `Ok(_)`s #12005

m-rph · 2023-12-24T09:22:42Z

Partial rewrite of unused_io_amount to lint over Ok(_) and Ok(..).

Moved the check to check_block to simplify context checking for expressions and allow us to check only some expressions.

For match (expr, arms) we emit a lint for io ops used on expr when an arm is Ok(_)|Ok(..). Also considers the cases when there are guards in the arms and if let Ok(_) = ... cases.

For Ok(_) and Ok(..) it emits a note indicating where the value is ignored.

changelog: False Negatives [unused_io_amount]: Extended unused_io_amount to catch Ok(_)s in If let and match exprs.

Closes #11713

r? @giraffate

m-rph · 2023-12-24T09:23:26Z

PS, sorry for the double PR, I couldn't open the previous after closing the fork :/

GuillaumeGomez · 2024-01-12T21:37:30Z

clippy_lints/src/unused_io_amount.rs

+/// We need to check that read/write call sizes are handled.
+/// We need to be careful with this to avoid false positives.
+/// If we do the check directly on Expr, we will miss cases like:
+/// `


Suggested change

/// `

///

Typo? :)

Ah wait, I'm not sure it's a typo. If you want a code block, why not use ```?

This seems to be just a self-note for Quinn specifically (at least, it seems like it isn't finished). I would suggest removing it, or making it clearer =^w^=

I think the comments need to be cleaned up a bit 😬

GuillaumeGomez

Apart from the typo in the comment, looks good to me. Big 👍 for the comments!

GuillaumeGomez · 2024-01-12T21:39:50Z

Let's assign someone else as reviewer too.

r? @blyxyas

blyxyas · 2024-01-12T21:57:09Z

A month without reviewing anything, I'll do my best =^w^=
My skills may have rusted a little bit...

...I'm such a comedian

m-rph · 2024-01-12T22:43:07Z

clippy_lints/src/unused_io_amount.rs

+///
+/// Here's what we will do:
+///     1. Get the callsite Expr
+///     2. Check if it's a call to read/write
+///     3. Decide whether it should be linted based on the surrounding context, e.g. arms in a match
+///        statement, etc
+///     4. Lint if necessary
+/// This is different to the current impl. Current impl which is limited in the match statements it
+/// catches and doesn't catch cases where match is just returned.
+///
+/// We will use logic similar to


I just re-read this. I will clean it up.

blyxyas

Meow meow (=^‥^=) Pretty good iteration, just some minor changes (performance changes that may or may not actually improve performance, but improve readability nonetheless UwU)

blyxyas · 2024-01-12T22:02:34Z

clippy_lints/src/unused_io_amount.rs

+/// We need to check that read/write call sizes are handled.
+/// We need to be careful with this to avoid false positives.
+/// If we do the check directly on Expr, we will miss cases like:
+/// `


This seems to be just a self-note for Quinn specifically (at least, it seems like it isn't finished). I would suggest removing it, or making it clearer =^w^=

blyxyas · 2024-01-12T22:43:31Z

clippy_lints/src/unused_io_amount.rs

+        if let Some(exp) = block.expr {
+            match exp.kind {
+                hir::ExprKind::If(_, _, _) | hir::ExprKind::Match(_, _, _) => check_expr(cx, exp),
+                _ => {},
+            }


Meow (just some formatting to make code more readable :D)

Suggested change

if let Some(exp) = block.expr {

match exp.kind {

hir::ExprKind::If(_, _, _) | hir::ExprKind::Match(_, _, _) => check_expr(cx, exp),

_ => {},

}

if let Some(exp) = block.expr

&& matches!(exp.kind, hir::ExprKind::If(_, _, _) | hir::ExprKind::Match(_, _, _)) {

check_expr(cx, exp)

}

clippy_lints/src/unused_io_amount.rs

blyxyas · 2024-01-12T22:58:05Z

clippy_lints/src/unused_io_amount.rs

+    is_okay_pat(cx, pat)
+        && matches!(
+            pat.kind,
+            PatKind::TupleStruct(
+                _,
+                [hir::Pat {
+                    kind: PatKind::Wild,
+                    ..
+                }],
+                _
+            )
+        )


As is_okay_pat is only used here, you could make it a single function to avoid matching for pat.kind -> PatKind::TupleStruct twice.

Suggested change

is_okay_pat(cx, pat)

&& matches!(

pat.kind,

PatKind::TupleStruct(

_,

[hir::Pat {

kind: PatKind::Wild,

..

}],

_

)

)

if let PatKind::TupleStruct(ref path, [inner_pat], ddp) = pat.kind

&& is_res_lang_ctor(cx, cx.qpath_res(path, inner_pat.hir_id), hir::LangItem::ResultOk)

&& matches!(inner_pat, hir::Pat { kind: PatKind::Wild,

.. }) {

// [...]

}

You can also keep the is_okay_pat function, but make it return inner_pat (using #[inline])

I decided to merge them as adding the dotdot check would be easier.

clippy_lints/src/unused_io_amount.rs

blyxyas · 2024-01-12T23:19:10Z

clippy_lints/src/unused_io_amount.rs

        }
-    }
+        for span in wild_cards {
+            diag.span_note(*span, "io bytes are ignored");


Hmmm I'm not sure if this should be "io", "IO", or "I/O".

I prefer I/O to be honest 🤔

The problem is that anything capitalized breaks guidelines 😅

I'll talk about this in Zulip. The style guide does indeed say:

Error, Warning, Note, and Help messages start with a lowercase letter and do not end with punctuation.

But, "I/O" is an acronym... And using io would break the first style convention imo...

It seems like there's an unanimously support for "I/O". Could you change it =^w^=

Of course! what should I do about the style check :D ?

? What style check? Like, the x test tidy in upstream Rust CI?

running cargo test with "I/O" results in the test suite failing because of the style check failing because of capitalisation of "I/O". I could reword the note if that's okay with you.

Yep, reword it if that's happening. I've tried using cargo test in this PR, and apart from having to bless the .stderr files and rename the //~ NOTE: annotations, everything went just fine. ¯ \ _ (ツ) _ / ¯

What do you think of this:

match s.write(b"test") { //~^ ERROR: written amount is not handled Ok(_) => todo!(), //~^ NOTE: the result is consumed here, but the amount of I/O bytes remains unhandled Err(_) => todo!(), }

blyxyas · 2024-01-14T00:57:27Z

Quinn... Could you not rebase until the final approval =*w*= now I don't know what was changed in that rebase :( (no biggie, don't worry, just for the next time)

blyxyas

Just a nit, and this should be merge-ready =^w^=

blyxyas · 2024-01-17T13:34:57Z

clippy_lints/src/unused_io_amount.rs

+    while let hir::ExprKind::MethodCall(path, receiver, ..) = expr.kind {
+        if matches!(
+            path.ident.as_str(),
+            "or" | "or_else" | "ok" | "expect" | "unwrap" | "unwrap_or" | "unwrap_or_else" | "is_ok" | "is_err"


I've looked up the frequency for each of these methods in GitHub search. Here are the results:

Search query No. of files (GitHub)

.or( 69.1k

.or_else( 102k

.ok( 565k

.expect( 1.5M

.unwrap( 2.6M

.unwrap_or( 635k

.unwrap_or_else( 567k

.is_ok( 356k

.is_err( 312k

Could you rearrange the options to avoid useless lookups? (e.g. unwrap will be much more frequent than or_else, so let's avoid looking for it first)

How silly would it be if we created a lint for this? 🤣 like, it would be beneficial even if its only usecase is dogfood tests 🤣.

Hmmmm maybe not a lint, but a PR (and taking this into account for future merges) would probably help.
Doing something similar with the methods/mod.rs file probably would benefit performance. But we would need to do some benchmarks.

If you want to do this, I'll use all our benchmarking infrastructure to verify the results.

blyxyas

LGTM, thanks! ❤️
Could you squash these commits into one?

Partial rewrite of `unused_io_account` to lint over Ok(_). Moved the check to `check_block` to simplify context checking for expressions and allow us to check only some expressions. For match (expr, arms) we emit a lint for io ops used on `expr` when an arm is `Ok(_)`. Also considers the cases when there are guards in the arms. It also captures `if let Ok(_) = ...` cases. For `Ok(_)` it emits a note indicating where the value is ignored. changelog: False Negatives [`unused_io_amount`]: Extended `unused_io_amount` to catch `Ok(_)`s in `If let` and match exprs.

blyxyas · 2024-01-21T02:00:58Z

@bors r+

bors · 2024-01-21T02:01:01Z

📌 Commit f73879e has been approved by blyxyas

It is now in the queue for this repository.

bors · 2024-01-21T02:02:08Z

⌛ Testing commit f73879e with merge f1eade1...

`unused_io_amount` captures `Ok(_)`s Partial rewrite of `unused_io_amount` to lint over `Ok(_)` and `Ok(..)`. Moved the check to `check_block` to simplify context checking for expressions and allow us to check only some expressions. For match (expr, arms) we emit a lint for io ops used on `expr` when an arm is `Ok(_)|Ok(..)`. Also considers the cases when there are guards in the arms and `if let Ok(_) = ...` cases. For `Ok(_)` and `Ok(..)` it emits a note indicating where the value is ignored. changelog: False Negatives [`unused_io_amount`]: Extended `unused_io_amount` to catch `Ok(_)`s in `If let` and match exprs. Closes #11713 r? `@giraffate`

bors · 2024-01-21T02:07:06Z

💔 Test failed - checks-action_test

blyxyas · 2024-01-21T02:11:52Z

... This is the second time today... The MacOS runner is doing weird things

@bors retry

bors · 2024-01-21T02:12:59Z

⌛ Testing commit f73879e with merge 64d08a8...

blyxyas · 2024-01-21T02:19:24Z

...
@bors retry

bors · 2024-01-21T02:20:03Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 64d08a8 to master...

bors · 2024-01-21T02:20:31Z

⌛ Testing commit f73879e with merge 64d08a8...

rustbot assigned giraffate Dec 24, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 24, 2023

m-rph closed this Dec 27, 2023

m-rph deleted the 11713 branch December 27, 2023 14:56

m-rph restored the 11713 branch December 27, 2023 15:07

m-rph reopened this Dec 27, 2023

GuillaumeGomez reviewed Jan 12, 2024

View reviewed changes

rustbot assigned blyxyas and unassigned giraffate Jan 12, 2024

m-rph commented Jan 12, 2024

View reviewed changes

blyxyas requested changes Jan 12, 2024

View reviewed changes

m-rph force-pushed the 11713 branch from 5cfc5e1 to e62854e Compare January 13, 2024 16:04

blyxyas requested changes Jan 17, 2024

View reviewed changes

blyxyas approved these changes Jan 21, 2024

View reviewed changes

m-rph force-pushed the 11713 branch from 8a00723 to f73879e Compare January 21, 2024 01:49

bors merged commit 64d08a8 into rust-lang:master Jan 21, 2024
5 checks passed

oxalica mentioned this pull request Jan 28, 2024

unused_io_amount false positive if the result is matched by Ok(<literal>) or unreachable!() cases #12208

Closed

Search query	No. of files (GitHub)
.or(	69.1k
.or_else(	102k
.ok(	565k
.expect(	1.5M
.unwrap(	2.6M
.unwrap_or(	635k
.unwrap_or_else(	567k
.is_ok(	356k
.is_err(	312k

unused_io_amount captures Ok(_)s #12005

unused_io_amount captures Ok(_)s #12005

Conversation

m-rph commented Dec 24, 2023 • edited

m-rph commented Dec 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez left a comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 12, 2024

blyxyas commented Jan 12, 2024

Choose a reason for hiding this comment

blyxyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxyas Jan 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxyas Jan 14, 2024 • edited

Choose a reason for hiding this comment

m-rph Jan 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxyas commented Jan 14, 2024 • edited

blyxyas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

blyxyas commented Jan 21, 2024

bors commented Jan 21, 2024

blyxyas commented Jan 21, 2024

bors commented Jan 21, 2024

bors commented Jan 21, 2024

`unused_io_amount` captures `Ok(_)`s #12005

`unused_io_amount` captures `Ok(_)`s #12005

m-rph commented Dec 24, 2023 •

edited

blyxyas Jan 14, 2024 •

edited

blyxyas Jan 14, 2024 •

edited

m-rph Jan 14, 2024 •

edited

blyxyas commented Jan 14, 2024 •

edited