Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New lint: manual_midpoint #13851

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5718,6 +5718,7 @@ Released 2018-09-13
[`manual_main_separator_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_main_separator_str
[`manual_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_map
[`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
[`manual_midpoint`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_midpoint
[`manual_next_back`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_next_back
[`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
[`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
crate::operators::IMPOSSIBLE_COMPARISONS_INFO,
crate::operators::INEFFECTIVE_BIT_MASK_INFO,
crate::operators::INTEGER_DIVISION_INFO,
crate::operators::MANUAL_MIDPOINT_INFO,
crate::operators::MISREFACTORED_ASSIGN_OP_INFO,
crate::operators::MODULO_ARITHMETIC_INFO,
crate::operators::MODULO_ONE_INFO,
Expand Down
48 changes: 48 additions & 0 deletions clippy_lints/src/operators/manual_midpoint.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::sugg::Sugg;
use clippy_utils::{is_floating_point_integer_literal, is_integer_literal};
use rustc_ast::BinOpKind;
use rustc_errors::Applicability;
use rustc_hir::{Expr, ExprKind};
use rustc_lint::LateContext;
use rustc_middle::ty;

use super::MANUAL_MIDPOINT;

pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
expr: &'tcx Expr<'_>,
op: BinOpKind,
left: &'tcx Expr<'_>,
right: &'tcx Expr<'_>,
msrv: &Msrv,
) {
if msrv.meets(msrvs::UINT_FLOAT_MIDPOINT)
&& !left.span.from_expansion()
&& !right.span.from_expansion()
&& op == BinOpKind::Div
&& (is_integer_literal(right, 2) || is_floating_point_integer_literal(right, 2))
&& let ExprKind::Binary(left_op, ll_expr, lr_expr) = left.kind
&& left_op.node == BinOpKind::Add
&& let left_ty = cx.typeck_results().expr_ty_adjusted(ll_expr)
&& let right_ty = cx.typeck_results().expr_ty_adjusted(lr_expr)
&& left_ty == right_ty
// FIXME: Also lint on signed integers when rust-lang/rust#134340 is merged
&& matches!(left_ty.kind(), ty::Uint(_) | ty::Float(_))
samueltardieu marked this conversation as resolved.
Show resolved Hide resolved
{
let mut app = Applicability::MachineApplicable;
let left_sugg = Sugg::hir_with_context(cx, ll_expr, expr.span.ctxt(), "..", &mut app);
let right_sugg = Sugg::hir_with_context(cx, lr_expr, expr.span.ctxt(), "..", &mut app);
let sugg = format!("{left_ty}::midpoint({left_sugg}, {right_sugg})");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

span_lint_and_sugg(
cx,
MANUAL_MIDPOINT,
expr.span,
"manual implementation of `midpoint`",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@Urgau Urgau Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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").

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@Urgau Urgau Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

format!("use `{left_ty}::midpoint` instead"),
sugg,
app,
);
}
}
32 changes: 32 additions & 0 deletions clippy_lints/src/operators/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ mod float_cmp;
mod float_equality_without_abs;
mod identity_op;
mod integer_division;
mod manual_midpoint;
mod misrefactored_assign_op;
mod modulo_arithmetic;
mod modulo_one;
Expand All @@ -24,6 +25,7 @@ mod verbose_bit_mask;
pub(crate) mod arithmetic_side_effects;

use clippy_config::Conf;
use clippy_utils::msrvs::Msrv;
use rustc_hir::{Body, Expr, ExprKind, UnOp};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::impl_lint_pass;
Expand Down Expand Up @@ -837,17 +839,43 @@ declare_clippy_lint! {
"explicit self-assignment"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for manual implementation of `midpoint`.
///
/// ### Why is this bad?
/// Using `(x + y) / 2` might cause an overflow on the intermediate
/// addition result.
///
/// ### Example
/// ```no_run
/// # let a: u32 = 0;
/// let c = (a + 10) / 2;
/// ```
/// Use instead:
/// ```no_run
/// # let a: u32 = 0;
/// let c = u32::midpoint(a, 10);
/// ```
#[clippy::version = "1.85.0"]
pub MANUAL_MIDPOINT,
correctness,
"manual implementation of `midpoint` which can overflow"
}

pub struct Operators {
arithmetic_context: numeric_arithmetic::Context,
verbose_bit_mask_threshold: u64,
modulo_arithmetic_allow_comparison_to_zero: bool,
msrv: Msrv,
}
impl Operators {
pub fn new(conf: &'static Conf) -> Self {
Self {
arithmetic_context: numeric_arithmetic::Context::default(),
verbose_bit_mask_threshold: conf.verbose_bit_mask_threshold,
modulo_arithmetic_allow_comparison_to_zero: conf.allow_comparison_to_zero,
msrv: conf.msrv.clone(),
}
}
}
Expand Down Expand Up @@ -879,6 +907,7 @@ impl_lint_pass!(Operators => [
NEEDLESS_BITWISE_BOOL,
PTR_EQ,
SELF_ASSIGNMENT,
MANUAL_MIDPOINT,
]);

impl<'tcx> LateLintPass<'tcx> for Operators {
Expand All @@ -896,6 +925,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators {
identity_op::check(cx, e, op.node, lhs, rhs);
needless_bitwise_bool::check(cx, e, op.node, lhs, rhs);
ptr_eq::check(cx, e, op.node, lhs, rhs);
manual_midpoint::check(cx, e, op.node, lhs, rhs, &self.msrv);
}
self.arithmetic_context.check_binary(cx, e, op.node, lhs, rhs);
bit_mask::check(cx, e, op.node, lhs, rhs);
Expand Down Expand Up @@ -946,6 +976,8 @@ impl<'tcx> LateLintPass<'tcx> for Operators {
fn check_body_post(&mut self, cx: &LateContext<'tcx>, b: &Body<'_>) {
self.arithmetic_context.body_post(cx, b);
}

extract_msrv_attr!(LateContext);
}

fn macro_with_not_op(e: &Expr<'_>) -> bool {
Expand Down
12 changes: 12 additions & 0 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1638,6 +1638,18 @@ pub fn is_integer_literal(expr: &Expr<'_>, value: u128) -> bool {
false
}

/// Checks whether the given expression is a constant integer of the given value
/// as a floating point literal.
pub fn is_floating_point_integer_literal(expr: &Expr<'_>, value: u32) -> bool {
if let ExprKind::Lit(spanned) = expr.kind
&& let LitKind::Float(v, _) = spanned.node
{
v.as_str().parse() == Ok(f64::from(value))
} else {
false
}
}

/// Returns `true` if the given `Expr` has been coerced before.
///
/// Examples of coercions can be found in the Nomicon at
Expand Down
1 change: 1 addition & 0 deletions clippy_utils/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ macro_rules! msrv_aliases {

// names may refer to stabilized feature flags or library items
msrv_aliases! {
1,85,0 { UINT_FLOAT_MIDPOINT }
1,83,0 { CONST_EXTERN_FN, CONST_FLOAT_BITS_CONV, CONST_FLOAT_CLASSIFY }
1,82,0 { IS_NONE_OR, REPEAT_N, RAW_REF_OP }
1,81,0 { LINT_REASONS_STABILIZATION, ERROR_IN_CORE }
Expand Down
45 changes: 45 additions & 0 deletions tests/ui/manual_midpoint.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#![warn(clippy::manual_midpoint)]

macro_rules! mac {
($a: expr, $b: expr) => {
($a + $b) / 2
};
}

macro_rules! add {
($a: expr, $b: expr) => {
($a + $b)
};
}

macro_rules! two {
() => {
2
};
}

fn main() {
let a: u32 = 10;
let _ = u32::midpoint(a, 5); //~ ERROR: manual implementation of `midpoint`

let f: f32 = 10.0;
let _ = f32::midpoint(f, 5.0); //~ ERROR: manual implementation of `midpoint`

let _: u32 = 5 + u32::midpoint(8, 8) + 2; //~ ERROR: manual implementation of `midpoint`
let _: u32 = const { u32::midpoint(8, 8) }; //~ ERROR: manual implementation of `midpoint`
let _: f64 = const { f64::midpoint(8.0f64, 8.) }; //~ ERROR: manual implementation of `midpoint`
let _: u32 = u32::midpoint(u32::default(), u32::default()); //~ ERROR: manual implementation of `midpoint`
let _: u32 = u32::midpoint(two!(), two!()); //~ ERROR: manual implementation of `midpoint`

// Do not lint if whole or part is coming from a macro
let _ = mac!(10, 20);
let _: u32 = add!(1u32, 2u32) / 2;
let _: u32 = (10 + 20) / two!();

// Do not lint if a literal is not present
let _ = (f + 5.0) / (1.0 + 1.0);

// Do not lint on signed integer types
let i: i32 = 10;
let _ = (i + 5) / 2;
}
45 changes: 45 additions & 0 deletions tests/ui/manual_midpoint.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
#![warn(clippy::manual_midpoint)]

macro_rules! mac {
($a: expr, $b: expr) => {
($a + $b) / 2
};
}

macro_rules! add {
($a: expr, $b: expr) => {
($a + $b)
};
}

macro_rules! two {
() => {
2
};
}

fn main() {
let a: u32 = 10;
let _ = (a + 5) / 2; //~ ERROR: manual implementation of `midpoint`

let f: f32 = 10.0;
let _ = (f + 5.0) / 2.0; //~ ERROR: manual implementation of `midpoint`

samueltardieu marked this conversation as resolved.
Show resolved Hide resolved
let _: u32 = 5 + (8 + 8) / 2 + 2; //~ ERROR: manual implementation of `midpoint`
let _: u32 = const { (8 + 8) / 2 }; //~ ERROR: manual implementation of `midpoint`
let _: f64 = const { (8.0f64 + 8.) / 2. }; //~ ERROR: manual implementation of `midpoint`
let _: u32 = (u32::default() + u32::default()) / 2; //~ ERROR: manual implementation of `midpoint`
let _: u32 = (two!() + two!()) / 2; //~ ERROR: manual implementation of `midpoint`

// Do not lint if whole or part is coming from a macro
let _ = mac!(10, 20);
let _: u32 = add!(1u32, 2u32) / 2;
let _: u32 = (10 + 20) / two!();

// Do not lint if a literal is not present
let _ = (f + 5.0) / (1.0 + 1.0);

// Do not lint on signed integer types
let i: i32 = 10;
let _ = (i + 5) / 2;
}
47 changes: 47 additions & 0 deletions tests/ui/manual_midpoint.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
error: manual implementation of `midpoint`
--> tests/ui/manual_midpoint.rs:23:13
|
LL | let _ = (a + 5) / 2;
| ^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(a, 5)`
|
= note: `-D clippy::manual-midpoint` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::manual_midpoint)]`

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

error: manual implementation of `midpoint`
--> tests/ui/manual_midpoint.rs:28:22
|
LL | let _: u32 = 5 + (8 + 8) / 2 + 2;
| ^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(8, 8)`

error: manual implementation of `midpoint`
--> tests/ui/manual_midpoint.rs:29:26
|
LL | let _: u32 = const { (8 + 8) / 2 };
| ^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(8, 8)`

error: manual implementation of `midpoint`
--> tests/ui/manual_midpoint.rs:30:26
|
LL | let _: f64 = const { (8.0f64 + 8.) / 2. };
| ^^^^^^^^^^^^^^^^^^ help: use `f64::midpoint` instead: `f64::midpoint(8.0f64, 8.)`

error: manual implementation of `midpoint`
--> tests/ui/manual_midpoint.rs:31:18
|
LL | let _: u32 = (u32::default() + u32::default()) / 2;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(u32::default(), u32::default())`

error: manual implementation of `midpoint`
--> tests/ui/manual_midpoint.rs:32:18
|
LL | let _: u32 = (two!() + two!()) / 2;
| ^^^^^^^^^^^^^^^^^^^^^ help: use `u32::midpoint` instead: `u32::midpoint(two!(), two!())`

error: aborting due to 7 previous errors

1 change: 1 addition & 0 deletions tests/ui/option_if_let_else.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
clippy::equatable_if_let,
clippy::let_unit_value,
clippy::redundant_locals,
clippy::manual_midpoint,
clippy::manual_unwrap_or_default,
clippy::manual_unwrap_or
)]
Expand Down
1 change: 1 addition & 0 deletions tests/ui/option_if_let_else.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
clippy::equatable_if_let,
clippy::let_unit_value,
clippy::redundant_locals,
clippy::manual_midpoint,
clippy::manual_unwrap_or_default,
clippy::manual_unwrap_or
)]
Expand Down
Loading
Loading