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

manual_midpoint: support signed integer types as well #14298

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
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 @@ -5762,6 +5762,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_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_err
Expand Down
1 change: 1 addition & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -768,6 +768,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio
* [`manual_hash_one`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_hash_one)
* [`manual_is_ascii_check`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_is_ascii_check)
* [`manual_let_else`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_let_else)
* [`manual_midpoint`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_midpoint)
* [`manual_non_exhaustive`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive)
* [`manual_option_as_slice`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_option_as_slice)
* [`manual_pattern_char_comparison`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_pattern_char_comparison)
Expand Down
1 change: 1 addition & 0 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -623,6 +623,7 @@ define_Conf! {
manual_hash_one,
manual_is_ascii_check,
manual_let_else,
manual_midpoint,
manual_non_exhaustive,
manual_option_as_slice,
manual_pattern_char_comparison,
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 @@ -604,6 +604,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
64 changes: 64 additions & 0 deletions clippy_lints/src/operators/manual_midpoint.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::sugg::Sugg;
use clippy_utils::{is_float_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::{self, 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 !left.span.from_expansion()
&& !right.span.from_expansion()
&& op == BinOpKind::Div
&& (is_integer_literal(right, 2) || is_float_literal(right, 2.0))
&& let Some((ll_expr, lr_expr)) = add_operands(left)
&& add_operands(ll_expr).is_none() && add_operands(lr_expr).is_none()
&& 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
// Do not lint on `(_+1)/2` and `(1+_)/2`, it is likely a `div_ceil()` operation
&& !is_integer_literal(ll_expr, 1) && !is_integer_literal(lr_expr, 1)
&& is_midpoint_implemented(left_ty, msrv)
{
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})");
span_lint_and_sugg(
cx,
MANUAL_MIDPOINT,
expr.span,
"manual implementation of `midpoint` which can overflow",
format!("use `{left_ty}::midpoint` instead"),
sugg,
app,
);
}
}

/// Return the left and right operands if `expr` represents an addition
fn add_operands<'e, 'tcx>(expr: &'e Expr<'tcx>) -> Option<(&'e Expr<'tcx>, &'e Expr<'tcx>)> {
match expr.kind {
ExprKind::Binary(op, left, right) if op.node == BinOpKind::Add => Some((left, right)),
_ => None,
}
}

fn is_midpoint_implemented(ty: Ty<'_>, msrv: &Msrv) -> bool {
match ty.kind() {
ty::Uint(_) | ty::Float(_) => msrv.meets(msrvs::UINT_FLOAT_MIDPOINT),
ty::Int(_) => msrv.meets(msrvs::INT_MIDPOINT),
_ => false,
}
}
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 @@ -834,17 +836,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.87.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 @@ -876,6 +904,7 @@ impl_lint_pass!(Operators => [
NEEDLESS_BITWISE_BOOL,
PTR_EQ,
SELF_ASSIGNMENT,
MANUAL_MIDPOINT,
]);

impl<'tcx> LateLintPass<'tcx> for Operators {
Expand All @@ -893,6 +922,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 @@ -943,6 +973,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
11 changes: 11 additions & 0 deletions clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1750,6 +1750,17 @@ pub fn is_integer_literal(expr: &Expr<'_>, value: u128) -> bool {
false
}

/// Checks whether the given expression is a constant literal of the given value.
pub fn is_float_literal(expr: &Expr<'_>, value: f64) -> bool {
if let ExprKind::Lit(spanned) = expr.kind
&& let LitKind::Float(v, _) = spanned.node
{
v.as_str().parse() == Ok(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
2 changes: 2 additions & 0 deletions clippy_utils/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ macro_rules! msrv_aliases {

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

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

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

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

#[clippy::msrv = "1.84"]
fn older_msrv() {
let a: u32 = 10;
let _ = (a + 5) / 2;
}

#[clippy::msrv = "1.85"]
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 in presence of an addition with more than 2 operands
let _: u32 = (10 + 20 + 30) / 2;

// Do not lint if whole or part is coming from a macro
let _ = mac!(10, 20);
let _: u32 = add!(10u32, 20u32) / 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;

// Do not lint on (x+1)/2 or (1+x)/2 as this looks more like a `div_ceil()` operation
let _ = (i + 1) / 2;
let _ = (1 + i) / 2;

// But if we see (x+1.0)/2.0 or (x+1.0)/2.0, it is probably a midpoint operation
let _ = f32::midpoint(f, 1.0); //~ ERROR: manual implementation of `midpoint`
let _ = f32::midpoint(1.0, f); //~ ERROR: manual implementation of `midpoint`
}

#[clippy::msrv = "1.86"]
fn older_signed_midpoint(i: i32) {
// Do not lint
let _ = (i + 10) / 2;
}

#[clippy::msrv = "1.87"]
fn signed_midpoint(i: i32) {
let _ = i32::midpoint(i, 10); //~ ERROR: manual implementation of `midpoint`
}
75 changes: 75 additions & 0 deletions tests/ui/manual_midpoint.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#![warn(clippy::manual_midpoint)]
#![feature(num_midpoint_signed)]

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

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

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

#[clippy::msrv = "1.84"]
fn older_msrv() {
let a: u32 = 10;
let _ = (a + 5) / 2;
}

#[clippy::msrv = "1.85"]
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`

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 in presence of an addition with more than 2 operands
let _: u32 = (10 + 20 + 30) / 2;

// Do not lint if whole or part is coming from a macro
let _ = mac!(10, 20);
let _: u32 = add!(10u32, 20u32) / 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;

// Do not lint on (x+1)/2 or (1+x)/2 as this looks more like a `div_ceil()` operation
let _ = (i + 1) / 2;
let _ = (1 + i) / 2;

// But if we see (x+1.0)/2.0 or (x+1.0)/2.0, it is probably a midpoint operation
let _ = (f + 1.0) / 2.0; //~ ERROR: manual implementation of `midpoint`
let _ = (1.0 + f) / 2.0; //~ ERROR: manual implementation of `midpoint`
}

#[clippy::msrv = "1.86"]
fn older_signed_midpoint(i: i32) {
// Do not lint
let _ = (i + 10) / 2;
}

#[clippy::msrv = "1.87"]
fn signed_midpoint(i: i32) {
let _ = (i + 10) / 2; //~ ERROR: manual implementation of `midpoint`
}
Loading