New lint: `manual_midpoint` #13851

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.

Add is_floating_point_integer_literal utility

e9c2d82

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

New lint: manual_midpoint

d879aba

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New lint: `manual_midpoint` #13851

New lint: `manual_midpoint` #13851

samueltardieu commented Dec 17, 2024

rustbot commented Dec 17, 2024

Urgau left a comment

Urgau Dec 19, 2024

samueltardieu Dec 19, 2024

Urgau Dec 19, 2024 •

edited

Loading

samueltardieu Dec 19, 2024

Urgau Dec 19, 2024 •

edited

Loading

samueltardieu Dec 19, 2024

y21 Dec 30, 2024

Urgau Dec 19, 2024

samueltardieu Dec 19, 2024

Urgau Dec 19, 2024

Urgau Dec 19, 2024

samueltardieu Dec 19, 2024

GuillaumeGomez Dec 19, 2024

y21 Dec 30, 2024 •

edited

Loading

samueltardieu commented Dec 27, 2024

y21 Dec 30, 2024 •

edited

Loading

y21 Dec 30, 2024

samueltardieu commented Jan 2, 2025

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

New lint: manual_midpoint #13851

Are you sure you want to change the base?

New lint: manual_midpoint #13851

Conversation

samueltardieu commented Dec 17, 2024

rustbot commented Dec 17, 2024

Urgau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Urgau Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Urgau Dec 19, 2024 • edited Loading

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

Choose a reason for hiding this comment

y21 Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

samueltardieu commented Dec 27, 2024

y21 Dec 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samueltardieu commented Jan 2, 2025

New lint: `manual_midpoint` #13851

New lint: `manual_midpoint` #13851

Urgau Dec 19, 2024 •

edited

Loading

Urgau Dec 19, 2024 •

edited

Loading

y21 Dec 30, 2024 •

edited

Loading

y21 Dec 30, 2024 •

edited

Loading