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: Suggest ptr.add([usize]) over ptr.offset([usize] as isize). #3104

Merged
merged 12 commits into from Aug 29, 2018
4 changes: 4 additions & 0 deletions clippy_lints/src/lib.rs
Expand Up @@ -142,6 +142,7 @@ pub mod panic_unimplemented;
pub mod partialeq_ne_impl;
pub mod precedence;
pub mod ptr;
pub mod ptr_offset_with_cast;
pub mod question_mark;
pub mod ranges;
pub mod redundant_field_names;
Expand Down Expand Up @@ -408,6 +409,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
reg.register_late_lint_pass(box default_trait_access::DefaultTraitAccess);
reg.register_late_lint_pass(box indexing_slicing::IndexingSlicing);
reg.register_late_lint_pass(box non_copy_const::NonCopyConst);
reg.register_late_lint_pass(box ptr_offset_with_cast::Pass);

reg.register_lint_group("clippy_restriction", vec![
arithmetic::FLOAT_ARITHMETIC,
Expand Down Expand Up @@ -631,6 +633,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
ptr::CMP_NULL,
ptr::MUT_FROM_REF,
ptr::PTR_ARG,
ptr_offset_with_cast::PTR_OFFSET_WITH_CAST,
question_mark::QUESTION_MARK,
ranges::ITERATOR_STEP_BY_ZERO,
ranges::RANGE_MINUS_ONE,
Expand Down Expand Up @@ -755,6 +758,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
panic_unimplemented::PANIC_PARAMS,
ptr::CMP_NULL,
ptr::PTR_ARG,
ptr_offset_with_cast::PTR_OFFSET_WITH_CAST,
question_mark::QUESTION_MARK,
redundant_field_names::REDUNDANT_FIELD_NAMES,
regex::REGEX_MACRO,
Expand Down
130 changes: 130 additions & 0 deletions clippy_lints/src/ptr_offset_with_cast.rs
@@ -0,0 +1,130 @@
use rustc::{declare_lint, hir, lint, lint_array, ty};
use syntax::ast;
use crate::utils;

/// **What it does:** Checks for usage of the `offset` pointer method with a `usize` casted to an
/// `isize`.
///
/// **Why is this bad?** If we’re always increasing the pointer address, we can avoid the numeric
/// cast by using the `add` method instead.
///
/// **Known problems:** None
///
/// **Example:**
/// ```rust
/// let vec = vec![b'a', b'b', b'c'];
/// let ptr = vec.as_ptr();
/// let offset = 1_usize;
///
/// unsafe { ptr.offset(offset as isize); }
/// ```
///
/// Could be written:
///
/// ```rust
/// let vec = vec![b'a', b'b', b'c'];
/// let ptr = vec.as_ptr();
/// let offset = 1_usize;
///
/// unsafe { ptr.add(offset); }
/// ```
declare_clippy_lint! {
pub PTR_OFFSET_WITH_CAST,
style,
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this the right category?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would fit the complexity category. Feels less like a stylistic choice, more like in the scope of "use x.is_empty() instead of x.len() == 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

"uneeded pointer offset cast"
}

#[derive(Copy, Clone, Debug)]
pub struct Pass;

impl lint::LintPass for Pass {
fn get_lints(&self) -> lint::LintArray {
lint_array!(PTR_OFFSET_WITH_CAST)
}
}

impl<'a, 'tcx> lint::LateLintPass<'a, 'tcx> for Pass {
fn check_expr(&mut self, cx: &lint::LateContext<'a, 'tcx>, expr: &'tcx hir::Expr) {
// Check if the expressions is a ptr.offset method call
let [receiver_expr, arg_expr] = match expr_as_ptr_offset_call(cx, expr) {
Some(call_arg) => call_arg,
None => return,
};

// Check if the parameter to ptr.offset is a cast from usize to isize
let cast_lhs_expr = match expr_as_cast_from_usize(cx, arg_expr) {
Some(cast_lhs_expr) => cast_lhs_expr,
None => return,
};

utils::span_lint_and_sugg(
cx,
PTR_OFFSET_WITH_CAST,
expr.span,
"use of `offset` with a `usize` casted to an `isize`",
"try",
build_suggestion(cx, receiver_expr, cast_lhs_expr),
);
}
}

// If the given expression is a cast from a usize, return the lhs of the cast
fn expr_as_cast_from_usize<'a, 'tcx>(
cx: &lint::LateContext<'a, 'tcx>,
expr: &'tcx hir::Expr,
) -> Option<&'tcx hir::Expr> {
if let hir::ExprKind::Cast(ref cast_lhs_expr, _) = expr.node {
if is_expr_ty_usize(cx, &cast_lhs_expr) {
return Some(cast_lhs_expr);
}
}
None
}

// If the given expression is a ptr::offset method call, return the receiver and the arg of the
// method call.
fn expr_as_ptr_offset_call<'a, 'tcx>(
cx: &lint::LateContext<'a, 'tcx>,
expr: &'tcx hir::Expr,
) -> Option<[&'tcx hir::Expr; 2]> {
if let hir::ExprKind::MethodCall(ref path_segment, _, ref args) = expr.node {
if path_segment.ident.name == "offset" && is_expr_ty_raw_ptr(cx, &args[0]) {
return Some([&args[0], &args[1]]);
}
}
None
}

// Is the type of the expression a usize?
fn is_expr_ty_usize<'a, 'tcx>(
cx: &lint::LateContext<'a, 'tcx>,
expr: &hir::Expr,
) -> bool {
cx.tables.expr_ty(expr).sty == ty::TyKind::Uint(ast::UintTy::Usize)
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use cx.tcx.types.usize to compare directly with the result of expr_ty

Copy link
Member Author

Choose a reason for hiding this comment

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

}

// Is the type of the expression a raw pointer?
fn is_expr_ty_raw_ptr<'a, 'tcx>(
cx: &lint::LateContext<'a, 'tcx>,
expr: &hir::Expr,
) -> bool {
if let ty::RawPtr(..) = cx.tables.expr_ty(expr).sty {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the is_unsafe_ptr method instead of destructuring

Copy link
Member Author

Choose a reason for hiding this comment

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

true
} else {
false
}
}

fn build_suggestion<'a, 'tcx>(
cx: &lint::LateContext<'a, 'tcx>,
receiver_expr: &hir::Expr,
cast_lhs_expr: &hir::Expr,
) -> String {
match (
utils::snippet_opt(cx, receiver_expr.span),
Copy link
Contributor

Choose a reason for hiding this comment

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

Now your code can be simplified to

let receiver = utils::snippet_opt(cx, receiver_expr.span)?;
let cast_lhs = utils::snippet_opt(cx, cast_lhs_expr.span)?;
Some(format!("{}.add({})", receiver, cast_lhs))

Copy link
Member Author

Choose a reason for hiding this comment

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

oh right, ? with Option is now a thing 💅

utils::snippet_opt(cx, cast_lhs_expr.span)
) {
(Some(receiver), Some(cast_lhs)) => format!("{}.add({})", receiver, cast_lhs),
_ => String::new(),
Copy link
Contributor

Choose a reason for hiding this comment

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

if the snippet is not obtainable, instead of reporting an empty snippet, we should not produce any suggestion at all

Copy link
Member Author

Choose a reason for hiding this comment

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

}
}
14 changes: 14 additions & 0 deletions tests/ui/ptr_offset_with_cast.rs
@@ -0,0 +1,14 @@
fn main() {
let vec = vec![b'a', b'b', b'c'];
let ptr = vec.as_ptr();

let offset_u8 = 1_u8;
let offset_usize = 1_usize;
let offset_isize = 1_isize;

unsafe {
ptr.offset(offset_usize as isize);
ptr.offset(offset_isize as isize);
ptr.offset(offset_u8 as isize);
}
}
10 changes: 10 additions & 0 deletions tests/ui/ptr_offset_with_cast.stderr
@@ -0,0 +1,10 @@
error: use of `offset` with a `usize` casted to an `isize`
--> $DIR/ptr_offset_with_cast.rs:10:9
|
10 | ptr.offset(offset_usize as isize);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `ptr.add(offset_usize)`
|
= note: `-D ptr-offset-with-cast` implied by `-D warnings`

error: aborting due to previous error