diff --git a/CHANGELOG.md b/CHANGELOG.md index dc93753a2130..ec84177cf79f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6766,6 +6766,7 @@ Released 2018-09-13 [`ptr_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_as_ptr [`ptr_cast_constness`]: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_cast_constness [`ptr_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_eq +[`ptr_offset_by_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_offset_by_literal [`ptr_offset_with_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_offset_with_cast [`pub_enum_variant_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#pub_enum_variant_names [`pub_underscore_fields`]: https://rust-lang.github.io/rust-clippy/master/index.html#pub_underscore_fields diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index de7d01ce0d1b..84e19f6a7411 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -444,6 +444,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::methods::OR_THEN_UNWRAP_INFO, crate::methods::PATH_BUF_PUSH_OVERWRITE_INFO, crate::methods::PATH_ENDS_WITH_EXT_INFO, + crate::methods::PTR_OFFSET_BY_LITERAL_INFO, crate::methods::PTR_OFFSET_WITH_CAST_INFO, crate::methods::RANGE_ZIP_WITH_LEN_INFO, crate::methods::READONLY_WRITE_LOCK_INFO, diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index c22b0a548e3d..e6b9589233c9 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -94,6 +94,7 @@ mod or_fun_call; mod or_then_unwrap; mod path_buf_push_overwrite; mod path_ends_with_ext; +mod ptr_offset_by_literal; mod ptr_offset_with_cast; mod range_zip_with_len; mod read_line_without_trim; @@ -1728,6 +1729,40 @@ declare_clippy_lint! { "Check for offset calculations on raw pointers to zero-sized types" } +declare_clippy_lint! { + /// ### What it does + /// Checks for usage of the `offset` pointer method with an integer + /// literal. + /// + /// ### Why is this bad? + /// The `add` and `sub` methods more accurately express the intent. + /// + /// ### Example + /// ```no_run + /// let vec = vec![b'a', b'b', b'c']; + /// let ptr = vec.as_ptr(); + /// + /// unsafe { + /// ptr.offset(-8); + /// } + /// ``` + /// + /// Could be written: + /// + /// ```no_run + /// let vec = vec![b'a', b'b', b'c']; + /// let ptr = vec.as_ptr(); + /// + /// unsafe { + /// ptr.sub(8); + /// } + /// ``` + #[clippy::version = "1.92.0"] + pub PTR_OFFSET_BY_LITERAL, + pedantic, + "unneeded pointer offset" +} + declare_clippy_lint! { /// ### What it does /// Checks for usage of the `offset` pointer method with a `usize` casted to an @@ -4803,6 +4838,7 @@ impl_lint_pass!(Methods => [ UNINIT_ASSUMED_INIT, MANUAL_SATURATING_ARITHMETIC, ZST_OFFSET, + PTR_OFFSET_BY_LITERAL, PTR_OFFSET_WITH_CAST, FILETYPE_IS_FILE, OPTION_AS_REF_DEREF, @@ -5426,6 +5462,7 @@ impl Methods { zst_offset::check(cx, expr, recv); ptr_offset_with_cast::check(cx, name, expr, recv, arg, self.msrv); + ptr_offset_by_literal::check(cx, expr, self.msrv); }, (sym::ok_or_else, [arg]) => { unnecessary_lazy_eval::check(cx, expr, recv, arg, "ok_or"); diff --git a/clippy_lints/src/methods/ptr_offset_by_literal.rs b/clippy_lints/src/methods/ptr_offset_by_literal.rs new file mode 100644 index 000000000000..b5d2add65cf1 --- /dev/null +++ b/clippy_lints/src/methods/ptr_offset_by_literal.rs @@ -0,0 +1,138 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::source::SpanRangeExt; +use clippy_utils::sym; +use rustc_ast::LitKind; +use rustc_errors::Applicability; +use rustc_hir::{Expr, ExprKind, Lit, UnOp}; +use rustc_lint::LateContext; +use std::cmp::Ordering; +use std::fmt; + +use super::PTR_OFFSET_BY_LITERAL; + +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Msrv) { + // `pointer::add` and `pointer::wrapping_add` are only stable since 1.26.0. These functions + // became const-stable in 1.61.0, the same version that `pointer::offset` became const-stable. + if !msrv.meets(cx, msrvs::POINTER_ADD_SUB_METHODS) { + return; + } + + let ExprKind::MethodCall(method_name, recv, [arg_expr], _) = expr.kind else { + return; + }; + + let method = match method_name.ident.name { + sym::offset => Method::Offset, + sym::wrapping_offset => Method::WrappingOffset, + _ => return, + }; + + if !cx.typeck_results().expr_ty_adjusted(recv).is_raw_ptr() { + return; + } + + // Check if the argument to the method call is a (negated) literal. + let Some((literal, literal_text)) = expr_as_literal(cx, arg_expr) else { + return; + }; + + match method.suggestion(literal) { + None => { + let msg = format!("use of `{method}` with zero"); + span_lint_and_then(cx, PTR_OFFSET_BY_LITERAL, expr.span, msg, |diag| { + diag.span_suggestion( + expr.span.with_lo(recv.span.hi()), + format!("remove the call to `{method}`"), + String::new(), + Applicability::MachineApplicable, + ); + }); + }, + Some(method_suggestion) => { + let msg = format!("use of `{method}` with a literal"); + span_lint_and_then(cx, PTR_OFFSET_BY_LITERAL, expr.span, msg, |diag| { + diag.multipart_suggestion( + format!("use `{method_suggestion}` instead"), + vec![ + (method_name.ident.span, method_suggestion.to_string()), + (arg_expr.span, literal_text), + ], + Applicability::MachineApplicable, + ); + }); + }, + } +} + +fn get_literal_bits<'tcx>(expr: &'tcx Expr<'tcx>) -> Option { + match expr.kind { + ExprKind::Lit(Lit { + node: LitKind::Int(packed_u128, _), + .. + }) => Some(packed_u128.get()), + _ => None, + } +} + +// If the given expression is a (negated) literal, return its value. +fn expr_as_literal<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option<(i128, String)> { + if let Some(literal_bits) = get_literal_bits(expr) { + // The value must fit in a isize, so we can't have overflow here. + return Some((literal_bits.cast_signed(), format_isize_literal(cx, expr)?)); + } + + if let ExprKind::Unary(UnOp::Neg, inner) = expr.kind + && let Some(literal_bits) = get_literal_bits(inner) + { + return Some((-(literal_bits.cast_signed()), format_isize_literal(cx, inner)?)); + } + + None +} + +fn format_isize_literal<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Option { + let text = expr.span.get_source_text(cx)?; + let text = peel_parens_str(&text); + Some(text.trim_end_matches("isize").trim_end_matches('_').to_string()) +} + +fn peel_parens_str(snippet: &str) -> &str { + let mut s = snippet.trim(); + while let Some(next) = s.strip_prefix("(").and_then(|suf| suf.strip_suffix(")")) { + s = next.trim(); + } + s +} + +#[derive(Copy, Clone)] +enum Method { + Offset, + WrappingOffset, +} + +impl Method { + fn suggestion(self, literal: i128) -> Option<&'static str> { + match Ord::cmp(&literal, &0) { + Ordering::Greater => match self { + Method::Offset => Some("add"), + Method::WrappingOffset => Some("wrapping_add"), + }, + // `ptr.offset(0)` is equivalent to `ptr`, so no adjustment is needed + Ordering::Equal => None, + Ordering::Less => match self { + Method::Offset => Some("sub"), + Method::WrappingOffset => Some("wrapping_sub"), + }, + } + } +} + +impl fmt::Display for Method { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Offset => write!(f, "offset"), + Self::WrappingOffset => write!(f, "wrapping_offset"), + } + } +} diff --git a/tests/ui/borrow_as_ptr.fixed b/tests/ui/borrow_as_ptr.fixed index bfe826508f36..a4689a7840ce 100644 --- a/tests/ui/borrow_as_ptr.fixed +++ b/tests/ui/borrow_as_ptr.fixed @@ -1,6 +1,6 @@ //@aux-build:proc_macros.rs #![warn(clippy::borrow_as_ptr)] -#![allow(clippy::useless_vec)] +#![allow(clippy::useless_vec, clippy::ptr_offset_by_literal)] extern crate proc_macros; diff --git a/tests/ui/borrow_as_ptr.rs b/tests/ui/borrow_as_ptr.rs index ce248f157c6e..d7468f37a3a4 100644 --- a/tests/ui/borrow_as_ptr.rs +++ b/tests/ui/borrow_as_ptr.rs @@ -1,6 +1,6 @@ //@aux-build:proc_macros.rs #![warn(clippy::borrow_as_ptr)] -#![allow(clippy::useless_vec)] +#![allow(clippy::useless_vec, clippy::ptr_offset_by_literal)] extern crate proc_macros; diff --git a/tests/ui/crashes/ice-4579.rs b/tests/ui/crashes/ice-4579.rs index 14c8113e315b..a2592e74c66d 100644 --- a/tests/ui/crashes/ice-4579.rs +++ b/tests/ui/crashes/ice-4579.rs @@ -1,6 +1,6 @@ //@ check-pass -#![allow(clippy::single_match)] +#![allow(clippy::single_match, clippy::ptr_offset_by_literal)] use std::ptr; diff --git a/tests/ui/ptr_offset_by_literal.fixed b/tests/ui/ptr_offset_by_literal.fixed new file mode 100644 index 000000000000..bd9e41def938 --- /dev/null +++ b/tests/ui/ptr_offset_by_literal.fixed @@ -0,0 +1,50 @@ +#![warn(clippy::ptr_offset_by_literal)] +#![allow(clippy::inconsistent_digit_grouping)] + +fn main() { + let arr = [b'a', b'b', b'c']; + let ptr = arr.as_ptr(); + + let var = 32; + const CONST: isize = 42; + + unsafe { + let _ = ptr; + //~^ ptr_offset_by_literal + let _ = ptr; + //~^ ptr_offset_by_literal + + let _ = ptr.add(5); + //~^ ptr_offset_by_literal + let _ = ptr.sub(5); + //~^ ptr_offset_by_literal + + let _ = ptr.offset(var); + let _ = ptr.offset(CONST); + + let _ = ptr.wrapping_add(5); + //~^ ptr_offset_by_literal + let _ = ptr.wrapping_sub(5); + //~^ ptr_offset_by_literal + + let _ = ptr.sub(5); + //~^ ptr_offset_by_literal + let _ = ptr.wrapping_sub(5); + //~^ ptr_offset_by_literal + + // isize::MAX and isize::MIN on 32-bit systems. + let _ = ptr.add(2_147_483_647); + //~^ ptr_offset_by_literal + let _ = ptr.sub(2_147_483_648); + //~^ ptr_offset_by_literal + + let _ = ptr.add(5_0); + //~^ ptr_offset_by_literal + let _ = ptr.sub(5_0); + //~^ ptr_offset_by_literal + + macro_rules! offs { { $e:expr, $offs:expr } => { $e.offset($offs) }; } + offs!(ptr, 6); + offs!(ptr, var); + } +} diff --git a/tests/ui/ptr_offset_by_literal.rs b/tests/ui/ptr_offset_by_literal.rs new file mode 100644 index 000000000000..b8e3f9b26c68 --- /dev/null +++ b/tests/ui/ptr_offset_by_literal.rs @@ -0,0 +1,50 @@ +#![warn(clippy::ptr_offset_by_literal)] +#![allow(clippy::inconsistent_digit_grouping)] + +fn main() { + let arr = [b'a', b'b', b'c']; + let ptr = arr.as_ptr(); + + let var = 32; + const CONST: isize = 42; + + unsafe { + let _ = ptr.offset(0); + //~^ ptr_offset_by_literal + let _ = ptr.offset(-0); + //~^ ptr_offset_by_literal + + let _ = ptr.offset(5); + //~^ ptr_offset_by_literal + let _ = ptr.offset(-5); + //~^ ptr_offset_by_literal + + let _ = ptr.offset(var); + let _ = ptr.offset(CONST); + + let _ = ptr.wrapping_offset(5isize); + //~^ ptr_offset_by_literal + let _ = ptr.wrapping_offset(-5isize); + //~^ ptr_offset_by_literal + + let _ = ptr.offset(-(5)); + //~^ ptr_offset_by_literal + let _ = ptr.wrapping_offset(-(5)); + //~^ ptr_offset_by_literal + + // isize::MAX and isize::MIN on 32-bit systems. + let _ = ptr.offset(2_147_483_647isize); + //~^ ptr_offset_by_literal + let _ = ptr.offset(-2_147_483_648isize); + //~^ ptr_offset_by_literal + + let _ = ptr.offset(5_0__isize); + //~^ ptr_offset_by_literal + let _ = ptr.offset(-5_0__isize); + //~^ ptr_offset_by_literal + + macro_rules! offs { { $e:expr, $offs:expr } => { $e.offset($offs) }; } + offs!(ptr, 6); + offs!(ptr, var); + } +} diff --git a/tests/ui/ptr_offset_by_literal.stderr b/tests/ui/ptr_offset_by_literal.stderr new file mode 100644 index 000000000000..f85fef87d55f --- /dev/null +++ b/tests/ui/ptr_offset_by_literal.stderr @@ -0,0 +1,141 @@ +error: use of `offset` with zero + --> tests/ui/ptr_offset_by_literal.rs:12:17 + | +LL | let _ = ptr.offset(0); + | ^^^---------- + | | + | help: remove the call to `offset` + | + = note: `-D clippy::ptr-offset-by-literal` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::ptr_offset_by_literal)]` + +error: use of `offset` with zero + --> tests/ui/ptr_offset_by_literal.rs:14:17 + | +LL | let _ = ptr.offset(-0); + | ^^^----------- + | | + | help: remove the call to `offset` + +error: use of `offset` with a literal + --> tests/ui/ptr_offset_by_literal.rs:17:17 + | +LL | let _ = ptr.offset(5); + | ^^^^^^^^^^^^^ + | +help: use `add` instead + | +LL - let _ = ptr.offset(5); +LL + let _ = ptr.add(5); + | + +error: use of `offset` with a literal + --> tests/ui/ptr_offset_by_literal.rs:19:17 + | +LL | let _ = ptr.offset(-5); + | ^^^^^^^^^^^^^^ + | +help: use `sub` instead + | +LL - let _ = ptr.offset(-5); +LL + let _ = ptr.sub(5); + | + +error: use of `wrapping_offset` with a literal + --> tests/ui/ptr_offset_by_literal.rs:25:17 + | +LL | let _ = ptr.wrapping_offset(5isize); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `wrapping_add` instead + | +LL - let _ = ptr.wrapping_offset(5isize); +LL + let _ = ptr.wrapping_add(5); + | + +error: use of `wrapping_offset` with a literal + --> tests/ui/ptr_offset_by_literal.rs:27:17 + | +LL | let _ = ptr.wrapping_offset(-5isize); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `wrapping_sub` instead + | +LL - let _ = ptr.wrapping_offset(-5isize); +LL + let _ = ptr.wrapping_sub(5); + | + +error: use of `offset` with a literal + --> tests/ui/ptr_offset_by_literal.rs:30:17 + | +LL | let _ = ptr.offset(-(5)); + | ^^^^^^^^^^^^^^^^ + | +help: use `sub` instead + | +LL - let _ = ptr.offset(-(5)); +LL + let _ = ptr.sub(5); + | + +error: use of `wrapping_offset` with a literal + --> tests/ui/ptr_offset_by_literal.rs:32:17 + | +LL | let _ = ptr.wrapping_offset(-(5)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `wrapping_sub` instead + | +LL - let _ = ptr.wrapping_offset(-(5)); +LL + let _ = ptr.wrapping_sub(5); + | + +error: use of `offset` with a literal + --> tests/ui/ptr_offset_by_literal.rs:36:17 + | +LL | let _ = ptr.offset(2_147_483_647isize); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `add` instead + | +LL - let _ = ptr.offset(2_147_483_647isize); +LL + let _ = ptr.add(2_147_483_647); + | + +error: use of `offset` with a literal + --> tests/ui/ptr_offset_by_literal.rs:38:17 + | +LL | let _ = ptr.offset(-2_147_483_648isize); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `sub` instead + | +LL - let _ = ptr.offset(-2_147_483_648isize); +LL + let _ = ptr.sub(2_147_483_648); + | + +error: use of `offset` with a literal + --> tests/ui/ptr_offset_by_literal.rs:41:17 + | +LL | let _ = ptr.offset(5_0__isize); + | ^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `add` instead + | +LL - let _ = ptr.offset(5_0__isize); +LL + let _ = ptr.add(5_0); + | + +error: use of `offset` with a literal + --> tests/ui/ptr_offset_by_literal.rs:43:17 + | +LL | let _ = ptr.offset(-5_0__isize); + | ^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `sub` instead + | +LL - let _ = ptr.offset(-5_0__isize); +LL + let _ = ptr.sub(5_0); + | + +error: aborting due to 12 previous errors + diff --git a/tests/ui/zero_offset.rs b/tests/ui/zero_offset.rs index bedb09536c53..5a9c3ac9248f 100644 --- a/tests/ui/zero_offset.rs +++ b/tests/ui/zero_offset.rs @@ -1,4 +1,4 @@ -#[allow(clippy::borrow_as_ptr)] +#[allow(clippy::borrow_as_ptr, clippy::ptr_offset_by_literal)] fn main() { unsafe { let m = &mut () as *mut ();