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.

Sign up for GitHub

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

Add fn_to_numeric_cast_usize lint #5914

Closed
wants to merge 2 commits into from
Closed

Add fn_to_numeric_cast_usize lint #5914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
572a314
Add `fn_to_numeric_cast_usize` lint
Coder-256 Aug 17, 2020
0f849cd
Add missing `clippy::` prefix
Coder-256 Aug 18, 2020
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1501,6 +1501,7 @@ Released 2018-09-13
[`fn_address_comparisons`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_address_comparisons
[`fn_params_excessive_bools`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_params_excessive_bools
[`fn_to_numeric_cast`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_to_numeric_cast
[`fn_to_numeric_cast_usize`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_to_numeric_cast_usize
[`fn_to_numeric_cast_with_truncation`]: https://rust-lang.github.io/rust-clippy/master/index.html#fn_to_numeric_cast_with_truncation
[`for_kv_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_kv_map
[`for_loops_over_fallibles`]: https://rust-lang.github.io/rust-clippy/master/index.html#for_loops_over_fallibles
Expand Down
3 changes: 3 additions & 0 deletions clippy_lints/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,6 +822,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&types::CAST_SIGN_LOSS,
&types::CHAR_LIT_AS_U8,
&types::FN_TO_NUMERIC_CAST,
&types::FN_TO_NUMERIC_CAST_USIZE,
&types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION,
&types::IMPLICIT_HASHER,
&types::INVALID_UPCAST_COMPARISONS,
Expand Down Expand Up @@ -1457,6 +1458,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&types::CAST_REF_TO_MUT),
LintId::of(&types::CHAR_LIT_AS_U8),
LintId::of(&types::FN_TO_NUMERIC_CAST),
LintId::of(&types::FN_TO_NUMERIC_CAST_USIZE),
LintId::of(&types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION),
LintId::of(&types::REDUNDANT_ALLOCATION),
LintId::of(&types::TYPE_COMPLEXITY),
Expand Down Expand Up @@ -1575,6 +1577,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&to_digit_is_some::TO_DIGIT_IS_SOME),
LintId::of(&try_err::TRY_ERR),
LintId::of(&types::FN_TO_NUMERIC_CAST),
LintId::of(&types::FN_TO_NUMERIC_CAST_USIZE),
LintId::of(&types::FN_TO_NUMERIC_CAST_WITH_TRUNCATION),
LintId::of(&unsafe_removed_from_name::UNSAFE_REMOVED_FROM_NAME),
LintId::of(&unused_unit::UNUSED_UNIT),
Expand Down
19 changes: 19 additions & 0 deletions clippy_lints/src/transmute.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,25 @@ impl<'tcx> LateLintPass<'tcx> for Transmute {
);
}
},
(ty::FnDef(..), ty::Int(_) | ty::Uint(_)) => span_lint_and_then(
Copy link
Member

Choose a reason for hiding this comment

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

This is unrelated?

I wouldn't do this, since the extra cast to fn() -> T is code bloat. If people prefer these explicit casts, they should enable the new lint (which should be pedantic).

cx,
TRANSMUTES_EXPRESSIBLE_AS_PTR_CASTS,
e.span,
&format!(
"transmute from `{}` to `{}` which could be expressed as a pointer cast instead",
from_ty,
to_ty
),
|diag| {
if let Some(arg) = sugg::Sugg::hir_opt(cx, &args[0]) {
let sugg = arg
.as_ty(&from_ty.fn_sig(cx.tcx).skip_binder().to_string())
.as_ty(&to_ty.to_string())
.to_string();
diag.span_suggestion(e.span, "try", sugg, Applicability::MachineApplicable);
}
}
),
(_, _) if can_be_expressed_as_pointer_cast(cx, e, from_ty, to_ty) => span_lint_and_then(
cx,
TRANSMUTES_EXPRESSIBLE_AS_PTR_CASTS,
Expand Down
81 changes: 73 additions & 8 deletions clippy_lints/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1100,6 +1100,31 @@ declare_clippy_lint! {
"casting a function pointer to a numeric type other than usize"
}

declare_clippy_lint! {
/// **What it does:** Checks for casts of function directly to a usize
///
/// **Why is this bad?**
/// Casting a function directly to a usize is likely a mistake. Most likely, the intended behavior
/// is calling the function. If casting to a usize is intended, it would be more clearly expressed
/// by casting to a function pointer, then to a usize.
///
/// **Example**
///
/// ```rust
/// // Bad
/// fn fun() -> usize { 1 }
/// let a: usize = fun as usize;
///
/// // Good
/// fn fun2() -> usize { 1 }
/// let a: usize = fun2 as fn() -> usize as usize;
/// let b: usize = fun2();
/// ```
pub FN_TO_NUMERIC_CAST_USIZE,
style,
Copy link
Member

Choose a reason for hiding this comment

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

This should be pedantic or even restriction IMO, since this cast is usually fine.

"casting a function to a usize"
}

declare_clippy_lint! {
/// **What it does:** Checks for casts of a function pointer to a numeric type not wide enough to
/// store address.
Expand Down Expand Up @@ -1374,6 +1399,7 @@ declare_lint_pass!(Casts => [
UNNECESSARY_CAST,
CAST_PTR_ALIGNMENT,
FN_TO_NUMERIC_CAST,
FN_TO_NUMERIC_CAST_USIZE,
FN_TO_NUMERIC_CAST_WITH_TRUNCATION,
]);

Expand Down Expand Up @@ -1558,15 +1584,55 @@ fn lint_fn_to_numeric_cast(
) {
// We only want to check casts to `ty::Uint` or `ty::Int`
match cast_to.kind {
ty::Uint(_) | ty::Int(..) => { /* continue on */ },
ty::Uint(_) | ty::Int(_) => { /* continue on */ },
_ => return,
}
match cast_from.kind {
ty::FnDef(..) | ty::FnPtr(_) => {
let mut applicability = Applicability::MaybeIncorrect;
let from_snippet = snippet_with_applicability(cx, cast_expr.span, "x", &mut applicability);

let to_nbits = int_ty_to_nbits(cast_to, cx.tcx);
let is_def = matches!(cast_from.kind, ty::FnDef(..));
let is_ptr = matches!(cast_from.kind, ty::FnPtr(_));

if is_def || is_ptr {
let mut applicability = Applicability::MaybeIncorrect;
let from_snippet = snippet_with_applicability(cx, cast_expr.span, "x", &mut applicability);

let to_nbits = int_ty_to_nbits(cast_to, cx.tcx);

if is_def {
Copy link
Member

Choose a reason for hiding this comment

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

This repeats too much code.

match cast_from.kind {
    ty::FnDef(..) if cast_to.kind == ty::Uint(UintTy::Usize) => { /* lint FN_TO_NUMERIC_CAST_USIZE */ },
    ty::FnDef(..) | ty::FnPtr(_) => { /* keep old code */ },
    .. => {},
}

if to_nbits < cx.tcx.data_layout.pointer_size.bits() {
span_lint_and_sugg(
cx,
FN_TO_NUMERIC_CAST_WITH_TRUNCATION,
expr.span,
&format!(
"casting *pointer* to function `{}` as `{}`, which truncates the value",
from_snippet, cast_to
),
"try",
format!("{}() as {}", from_snippet, cast_to),
applicability,
);
} else if cast_to.kind == ty::Uint(UintTy::Usize) {
span_lint_and_sugg(
cx,
FN_TO_NUMERIC_CAST_USIZE,
expr.span,
&format!("casting *pointer* to function `{}` as `{}`", from_snippet, cast_to),
"try",
format!("{}() as {}", from_snippet, cast_to),
applicability,
);
} else {
span_lint_and_sugg(
cx,
FN_TO_NUMERIC_CAST,
expr.span,
&format!("casting *pointer* to function `{}` as `{}`", from_snippet, cast_to),
"try",
format!("{}() as {}", from_snippet, cast_to),
Comment on lines +1629 to +1631
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should assume here, that a cast of the return value was meant. Also I think the casting function pointer ... message was clear enough that it talks about a function pointer. Also emphasizing with * or something else isn't used in error messages.

What we can do is add two suggestions, one that suggests to cast the pointer to usize and one to call the function and cast the resulting value to usize, iff it is a FnDef(..)

applicability,
);
}
} else if is_ptr {
if to_nbits < cx.tcx.data_layout.pointer_size.bits() {
span_lint_and_sugg(
cx,
Expand All @@ -1591,8 +1657,7 @@ fn lint_fn_to_numeric_cast(
applicability,
);
}
},
_ => {},
}
}
}

Expand Down
7 changes: 7 additions & 0 deletions src/lintlist/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,6 +682,13 @@ pub static ref ALL_LINTS: Vec<Lint> = vec![
deprecation: None,
module: "types",
},
Lint {
name: "fn_to_numeric_cast_usize",
group: "style",
desc: "casting a function to a usize",
deprecation: None,
module: "types",
},
Lint {
name: "fn_to_numeric_cast_with_truncation",
group: "style",
Expand Down
11 changes: 8 additions & 3 deletions tests/ui/fn_to_numeric_cast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
// ignore-32bit

#![warn(clippy::fn_to_numeric_cast, clippy::fn_to_numeric_cast_with_truncation)]
#![warn(
clippy::fn_to_numeric_cast,
clippy::fn_to_numeric_cast_usize,
clippy::fn_to_numeric_cast_with_truncation
)]

fn foo() -> String {
String::new()
Expand All @@ -19,10 +23,11 @@ fn test_function_to_numeric_cast() {
let _ = foo as u32;
let _ = foo as u64;
let _ = foo as u128;

// Casting to usize is OK and should not warn
let _ = foo as usize;

// Casting to fn pointer is OK and should not warn
let _ = foo as fn() -> String;

// Cast `f` (a `FnDef`) to `fn()` should not warn
fn f() {}
let _ = f as fn();
Expand Down
Loading