New lint: `manual_midpoint` by samueltardieu · Pull Request #13851 · rust-lang/rust-clippy

samueltardieu · 2024-12-17T23:40:11Z

changelog: [manual_midpoint]: new lint

rustbot · 2024-12-17T23:40:17Z

rustbot has assigned @dswij.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Urgau

❤️ Thanks for working on it, appreciate it.

Left a few remarks/nits.

Urgau · 2024-12-19T18:33:22Z

clippy_lints/src/operators/manual_midpoint.rs

+            cx,
+            MANUAL_MIDPOINT,
+            expr.span,
+            "manual implementation of `midpoint`",


Suggested change

"manual implementation of `midpoint`",

"manual implementation of `midpoint` which can overflow",

I'm reluctant to add this on the lint message, because this particular use may well never overflow depending on the context. This is appropriate for the lint short description though, I'll add it there. What do you think?

We should add it to the lint short description, but I still think we should mention it in the main message as it's the main issue (not the fact that it's a manual implementation). We could reduce the assertion by saying "which may overflow" (instead of "can").

Any addition may overflow, and we do not suggest using .checked_add(). I'll let other weigh in, but I'm not comfortable saying "which may overflow" on (a + b) / 2 if a and b are indices in a vector for example, they will never overflow as they will be nowhere near usize::MAX/2 for any realistic vector.

(a + b) / 2 if a and b are indices in a vector for example

u8::MAX = 255, u16::MAX = 65536 are fairly realistic numbers to me (which could be indices of custom containers).

they will never overflow as they will be nowhere near usize::MAX/2 for any realistic vector.

reminder of https://research.google/blog/extra-extra-read-all-about-it-nearly-all-binary-searches-and-mergesorts-are-broken/

I was specifically talking about the usize case which is used to index a vector. And let's not confuse the issue: I'm not trying to argue that we should not lint, we will, only that I am not comfortable saying that a particular computation may overflow.

I couldn't find lints that warn about the potential risk when linting an expression if the risk is remote or non-existing for this particular expression, even though they still lint to make it clearer and safer should this code be changed later.

I think I agree that it's a good idea to mention this even in the message. IMO it doesn't particularly hurt to mention even if the user knows it could never overflow, but it can definitely help in cases where that's an edge case the user hadn't considered, and I could see how the lint could otherwise be dismissed as 'just a nit like the other manual_* lints, not that important'.

The fact that it's an incorrect implementation due to overflow is the reason why it's deny-by-default/correctness in the first place, right? The current primary message makes this sound more like a complexity or style lint

clippy_lints/src/operators/manual_midpoint.rs

Urgau · 2024-12-19T18:38:01Z

tests/ui/manual_midpoint.stderr

+  --> tests/ui/manual_midpoint.rs:12:13
+   |
+LL |     let _ = (f + 5.0) / 2.0;
+   |             ^^^^^^^^^^^^^^^ help: use instead: `f32::midpoint(f, 5.0)`


I don't if it's in Clippy customs but would be possible to add a help linking to the documentation, something like this (which rustc does to further guide the user):

help: for more information visit <https://doc.rust-lang.org/nightly/std/primitive.u32.html#method.midpoint>

This is done only for niche or complex situations in the existing codebase, such as implementing a local trait for a foreign type (orphan rule).

There's a first time for everything. :-)

More seriously, I'm fine if the link it's not included.

clippy_lints/src/operators/manual_midpoint.rs

tests/ui/manual_midpoint.rs

Urgau · 2024-12-19T18:48:13Z

clippy_lints/src/operators/manual_midpoint.rs

+        let mut app = Applicability::MachineApplicable;
+        let left_sugg = Sugg::hir_with_applicability(cx, ll_expr, "..", &mut app);
+        let right_sugg = Sugg::hir_with_applicability(cx, lr_expr, "..", &mut app);
+        let sugg = format!("{left_ty}::midpoint({left_sugg}, {right_sugg})");


Interesting choice of form, I would have probably use the method syntax (ie. .midpoint(..)) instead, as to not have to much shifts from the original code, but that works as well.

I feel that linting (a + b) / 2 as a.midpoint(b) introduces an asymmetry between a and b while u32::midpoint(a, b) doesn't. But I'm not opposed to this change.

What do others think? Please upvote if you prefer a.midpoint(b) and downvote if you prefer u32::midpoint(a, b).

I can't pick between the 2 because the first one is more convenirnt but the second one is more readable...

Well, by writing this comment I realized I prefer more readable code so let's go for 2.

I initially preferred a.midpoint(b) but having thought more about it, the latter has some other advantages: u32::midpoint(a, b) doesn't require parentheses in some cases where the LHS is an expression with low precedence like (a + b + c) / 2, and it also sidesteps type inference issues in cases like

let c: u64 = (10 + 5) / 2;

where 10.midpoint(5) would not compile. So the current suggestion LGTM

samueltardieu · 2024-12-27T10:58:18Z

r? @y21

y21 · 2024-12-30T01:15:31Z

clippy_lints/src/operators/manual_midpoint.rs

+        let mut app = Applicability::MachineApplicable;
+        let left_sugg = Sugg::hir_with_applicability(cx, ll_expr, "..", &mut app);
+        let right_sugg = Sugg::hir_with_applicability(cx, lr_expr, "..", &mut app);
+        let sugg = format!("{left_ty}::midpoint({left_sugg}, {right_sugg})");


I initially preferred a.midpoint(b) but having thought more about it, the latter has some other advantages: u32::midpoint(a, b) doesn't require parentheses in some cases where the LHS is an expression with low precedence like (a + b + c) / 2, and it also sidesteps type inference issues in cases like

let c: u64 = (10 + 5) / 2;

where 10.midpoint(5) would not compile. So the current suggestion LGTM

clippy_lints/src/operators/manual_midpoint.rs

y21 · 2024-12-30T01:48:04Z

clippy_lints/src/operators/manual_midpoint.rs

+            cx,
+            MANUAL_MIDPOINT,
+            expr.span,
+            "manual implementation of `midpoint`",


I think I agree that it's a good idea to mention this even in the message. IMO it doesn't particularly hurt to mention even if the user knows it could never overflow, but it can definitely help in cases where that's an edge case the user hadn't considered, and I could see how the lint could otherwise be dismissed as 'just a nit like the other manual_* lints, not that important'.

The fact that it's an incorrect implementation due to overflow is the reason why it's deny-by-default/correctness in the first place, right? The current primary message makes this sound more like a complexity or style lint

samueltardieu · 2025-01-02T20:17:31Z

I have eliminated the lint on additions with more than 2 operands, as there is no reason to suggest u32::midpoint(a + b, c) or u32::midpoint(a, b + c) for an expression like (a + b + c) / 2 which is likely not a midpoint computation.

samueltardieu · 2025-01-06T10:25:24Z

I opened a thread on Zulip to discuss how to handle (x + 1) / 2 which might not semantically be a midpoint() operation, as the user might want to compute a div_ceil() here.

samueltardieu · 2025-01-15T08:02:46Z

Rebasing because of a new conflict in msrvs.rs.

y21

Looks good, going to start the FCP in a bit

clippy_lints/src/operators/manual_midpoint.rs

clippy_utils/src/lib.rs

y21 · 2025-01-18T23:51:05Z

FCP thread: https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/FCP.3A.20manual_midpoint

samueltardieu · 2025-01-19T08:55:41Z

Rebased

samueltardieu · 2025-02-03T22:49:47Z

Rebased

samueltardieu · 2025-02-21T21:00:35Z

Thanks, I'll add this. @rustbot author

samueltardieu · 2025-02-21T21:45:57Z

Thanks, I'll add this.

I'll have to wait until the next rustup until the stabilization of manual_midpoint for integer types is visible. It will be in another PR (the code is ready), and I'll let this one untouched.

@rustbot review

samueltardieu · 2025-02-26T12:59:15Z

Updated to set the lint version to 1.87.

samueltardieu · 2025-02-27T22:43:12Z

Rebased, category changed to pedantic, signed integer midpoint integrated as well.

rustbot assigned dswij Dec 17, 2024

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

samueltardieu force-pushed the push-pmstswltutkv branch 4 times, most recently from 906f3bd to 1561476 Compare December 18, 2024 00:06

Urgau reviewed Dec 19, 2024

View reviewed changes

samueltardieu force-pushed the push-pmstswltutkv branch 4 times, most recently from 6e01b0d to 8fc52b4 Compare December 19, 2024 20:07

rustbot assigned y21 and unassigned dswij Dec 27, 2024

y21 reviewed Dec 30, 2024

View reviewed changes

samueltardieu force-pushed the push-pmstswltutkv branch from 8fc52b4 to dee2bc8 Compare December 30, 2024 08:19

samueltardieu requested a review from y21 January 2, 2025 16:12

samueltardieu force-pushed the push-pmstswltutkv branch from dee2bc8 to d879aba Compare January 2, 2025 20:15

samueltardieu force-pushed the push-pmstswltutkv branch from d879aba to f28fbf1 Compare January 6, 2025 07:55

samueltardieu force-pushed the push-pmstswltutkv branch 4 times, most recently from b14c9fc to 7cd170d Compare January 12, 2025 08:37

samueltardieu force-pushed the push-pmstswltutkv branch 2 times, most recently from a0c770e to de46175 Compare January 15, 2025 08:02

y21 approved these changes Jan 18, 2025

View reviewed changes

clippy_lints/src/operators/manual_midpoint.rs Outdated Show resolved Hide resolved

clippy_utils/src/lib.rs Outdated Show resolved Hide resolved

y21 added S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jan 18, 2025

samueltardieu force-pushed the push-pmstswltutkv branch 2 times, most recently from 52d6d79 to 3c13085 Compare January 19, 2025 08:55

samueltardieu force-pushed the push-pmstswltutkv branch 2 times, most recently from 3c13085 to 71b6776 Compare February 3, 2025 22:49

samueltardieu force-pushed the push-pmstswltutkv branch from 71b6776 to 28fd9ba Compare February 15, 2025 15:33

rustbot added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Feb 21, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Feb 21, 2025

samueltardieu removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 21, 2025

samueltardieu mentioned this pull request Feb 26, 2025

manual_midpoint: support signed integer types as well #14298

Closed

samueltardieu force-pushed the push-pmstswltutkv branch from 28fd9ba to d8e4023 Compare February 26, 2025 11:29

samueltardieu added 2 commits February 27, 2025 22:10

Add is_float_literal utility

06175f4

New lint: manual_midpoint

baadee8

samueltardieu force-pushed the push-pmstswltutkv branch from d8e4023 to baadee8 Compare February 27, 2025 22:39

y21 added this pull request to the merge queue Feb 28, 2025

Merged via the queue into rust-lang:master with commit 2cdb90d Feb 28, 2025
11 checks passed

samueltardieu deleted the push-pmstswltutkv branch February 28, 2025 17:35

	"manual implementation of `midpoint`",
	"manual implementation of `midpoint` which can overflow",

Comments

Conversation

samueltardieu commented Dec 17, 2024

Uh oh!

rustbot commented Dec 17, 2024

Uh oh!

Urgau left a comment

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!

Urgau Dec 19, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Urgau Dec 19, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

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!

y21 Dec 30, 2024 • 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!

samueltardieu commented Dec 27, 2024

Uh oh!

y21 Dec 30, 2024 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu commented Jan 2, 2025

Uh oh!

samueltardieu commented Jan 6, 2025

Uh oh!

samueltardieu commented Jan 15, 2025

Uh oh!

y21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

y21 commented Jan 18, 2025

Uh oh!

samueltardieu commented Jan 19, 2025

Uh oh!

samueltardieu commented Feb 3, 2025

Uh oh!

samueltardieu commented Feb 21, 2025 via email

Uh oh!

samueltardieu commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

samueltardieu commented Feb 26, 2025

Uh oh!

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

Urgau Dec 19, 2024 •

edited

Loading

Urgau Dec 19, 2024 •

edited

Loading

y21 Dec 30, 2024 •

edited

Loading

y21 Dec 30, 2024 •

edited

Loading

samueltardieu commented Feb 21, 2025 •

edited

Loading

samueltardieu commented Feb 27, 2025 •

edited

Loading