Skip to content

Add missing_transmute_annotations lint #12239

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

Merged
merged 4 commits into from
Mar 24, 2024
Merged
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 @@ -5426,6 +5426,7 @@ Released 2018-09-13
[`missing_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_safety_doc
[`missing_spin_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_spin_loop
[`missing_trait_methods`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_trait_methods
[`missing_transmute_annotations`]: https://rust-lang.github.io/rust-clippy/master/index.html#missing_transmute_annotations
[`mistyped_literal_suffixes`]: https://rust-lang.github.io/rust-clippy/master/index.html#mistyped_literal_suffixes
[`mixed_attributes_style`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_attributes_style
[`mixed_case_hex_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#mixed_case_hex_literals
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 @@ -677,6 +677,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::trait_bounds::TYPE_REPETITION_IN_BOUNDS_INFO,
crate::transmute::CROSSPOINTER_TRANSMUTE_INFO,
crate::transmute::EAGER_TRANSMUTE_INFO,
crate::transmute::MISSING_TRANSMUTE_ANNOTATIONS_INFO,
crate::transmute::TRANSMUTES_EXPRESSIBLE_AS_PTR_CASTS_INFO,
crate::transmute::TRANSMUTE_BYTES_TO_STR_INFO,
crate::transmute::TRANSMUTE_FLOAT_TO_INT_INFO,
Expand Down
87 changes: 87 additions & 0 deletions clippy_lints/src/transmute/missing_transmute_annotations.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use rustc_errors::Applicability;
use rustc_hir::{GenericArg, HirId, Local, Node, Path, TyKind};
use rustc_lint::LateContext;
use rustc_middle::lint::in_external_macro;
use rustc_middle::ty::Ty;

use crate::transmute::MISSING_TRANSMUTE_ANNOTATIONS;

fn get_parent_local_binding_ty<'tcx>(cx: &LateContext<'tcx>, expr_hir_id: HirId) -> Option<Local<'tcx>> {
let mut parent_iter = cx.tcx.hir().parent_iter(expr_hir_id);
if let Some((_, node)) = parent_iter.next() {
match node {
Node::Local(local) => Some(*local),
Node::Block(_) => {
if let Some((parent_hir_id, Node::Expr(expr))) = parent_iter.next()
&& matches!(expr.kind, rustc_hir::ExprKind::Block(_, _))
{
get_parent_local_binding_ty(cx, parent_hir_id)
} else {
None
}
},
_ => None,
}
} else {
None
}
}

fn is_function_block(cx: &LateContext<'_>, expr_hir_id: HirId) -> bool {
let def_id = cx.tcx.hir().enclosing_body_owner(expr_hir_id);
if let Some(body_id) = cx.tcx.hir().maybe_body_owned_by(def_id) {
let body = cx.tcx.hir().body(body_id);
return body.value.peel_blocks().hir_id == expr_hir_id;
}
false
}

pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
path: &Path<'tcx>,
from_ty: Ty<'tcx>,
to_ty: Ty<'tcx>,
expr_hir_id: HirId,
) -> bool {
let last = path.segments.last().unwrap();
if in_external_macro(cx.tcx.sess, last.ident.span) {
// If it comes from a non-local macro, we ignore it.
return false;
}
let args = last.args;
let missing_generic = match args {
Some(args) if !args.args.is_empty() => args.args.iter().any(|arg| match arg {
GenericArg::Infer(_) => true,
GenericArg::Type(ty) => matches!(ty.kind, TyKind::Infer),
_ => false,
}),
_ => true,
};
if !missing_generic {
return false;
}
// If it's being set as a local variable value...
Copy link
Member

Choose a reason for hiding this comment

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

We should include closures here imo, if they have type annotations. This could include the parameter if it's specified as well, but not something like:

// i64 is known, but [i32; 2] is never specified
|x: i32, y: i32| -> i64 unsafe { transmute::<[i32; 2], _>([x, y]) };

Copy link
Member Author

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 special case closures either. It adds a lot of complexity to detect such cases for a very marginal gain imo.

Copy link
Member

Choose a reason for hiding this comment

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

Not really. The return type part is easy, and the parameter is too - Get the Res from the QPath, the HirId of the local, then the parent's parent should include the types in FnDecl.

I wouldn't block it on this so if it's not done I can add it after, eventually.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not in this sense. I meant, for the same reason we want to enforce having type annotations when transmute is used as return expr, we want to enforce for it for closures as well.

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 agree with that as closures are usually private and not public. It's more akin to let than functions. I don't think you can fold a closure either in IDEs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see why closures being private or public changes anything. If you want to bring this back to debate, let's continue on zulip. ;)

Copy link
Member

Choose a reason for hiding this comment

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

I'll bring up my current points on zulip sometime.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 There is an open thread already, don't hesitate to comment there directly. ;)

if let Some(local) = get_parent_local_binding_ty(cx, expr_hir_id) {
// ... which does have type annotations.
if let Some(ty) = local.ty
// If this is a `let x: _ =`, we should lint.
&& !matches!(ty.kind, TyKind::Infer)
{
return false;
}
// We check if this transmute is not the only element in the function
} else if is_function_block(cx, expr_hir_id) {
return false;
}
span_lint_and_sugg(
cx,
MISSING_TRANSMUTE_ANNOTATIONS,
last.ident.span.with_hi(path.span.hi()),
"transmute used without annotations",
"consider adding missing annotations",
format!("{}::<{from_ty}, {to_ty}>", last.ident.as_str()),
Copy link
Member

Choose a reason for hiding this comment

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

If one/more of the types can be left inferred, can we keep it like that here? e.g., let _: T.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd rather not.

Copy link
Member

Choose a reason for hiding this comment

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

Why not? Is it because it'd require rewriting it? I don't see why this shouldn't be done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe I misunderstood what you meant but this lint is about making it explicit what the transmute types are (both input and output). So if we provide annotation for one of the two, better provide for both directly.

Copy link
Member

Choose a reason for hiding this comment

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

What I meant was that, if it's within a let statement for example, we could keep Dst as _. If it's better to provide both directly, and this is still an issue with let, then we shouldn't special case let at all, no?

I see what you mean that providing them is better, but special-casing it then not suggesting it is a bit contradictory.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea behind my position is that if you ever remove the let binding or change the type annotation, if we originally suggested both types, it'll prevent compilation and require user to check that the code is still valid. I think it's pretty important considering how easy it is to have unexpected behaviours with transmute.

Applicability::MaybeIncorrect,
);
true
}
34 changes: 34 additions & 0 deletions clippy_lints/src/transmute/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
mod crosspointer_transmute;
mod eager_transmute;
mod missing_transmute_annotations;
mod transmute_float_to_int;
mod transmute_int_to_bool;
mod transmute_int_to_char;
Expand Down Expand Up @@ -520,6 +521,37 @@ declare_clippy_lint! {
"eager evaluation of `transmute`"
}

declare_clippy_lint! {
/// ### What it does
/// Checks if transmute calls have all generics specified.
///
/// ### Why is this bad?
/// If not set, some unexpected output type could be retrieved instead of the expected one,
/// potentially leading to invalid code.
///
/// This is particularly dangerous in case a seemingly innocent/unrelated change can cause type
/// inference to start inferring a different type. E.g. the transmute is the tail expression of
/// an `if` branch, and a different branches type changes, causing the transmute to silently
/// have a different type, instead of a proper error.
///
/// ### Example
/// ```no_run
/// # unsafe {
/// let x: i32 = std::mem::transmute([1u16, 2u16]);
/// # }
/// ```
/// Use instead:
/// ```no_run
/// # unsafe {
/// let x = std::mem::transmute::<[u16; 2], i32>([1u16, 2u16]);
/// # }
/// ```
#[clippy::version = "1.77.0"]
pub MISSING_TRANSMUTE_ANNOTATIONS,
suspicious,
"warns if a transmute call doesn't have all generics specified"
}

pub struct Transmute {
msrv: Msrv,
}
Expand All @@ -542,6 +574,7 @@ impl_lint_pass!(Transmute => [
TRANSMUTING_NULL,
TRANSMUTE_NULL_TO_FN,
EAGER_TRANSMUTE,
MISSING_TRANSMUTE_ANNOTATIONS,
]);
impl Transmute {
#[must_use]
Expand Down Expand Up @@ -579,6 +612,7 @@ impl<'tcx> LateLintPass<'tcx> for Transmute {
| transmuting_null::check(cx, e, arg, to_ty)
| transmute_null_to_fn::check(cx, e, arg, to_ty)
| transmute_ptr_to_ref::check(cx, e, from_ty, to_ty, arg, path, &self.msrv)
| missing_transmute_annotations::check(cx, path, from_ty, to_ty, e.hir_id)
| transmute_int_to_char::check(cx, e, from_ty, to_ty, arg, const_context)
| transmute_ref_to_ref::check(cx, e, from_ty, to_ty, arg, const_context)
| transmute_ptr_to_ptr::check(cx, e, from_ty, to_ty, arg)
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/author/issue_3849.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
#![allow(dead_code)]
#![allow(clippy::zero_ptr)]
#![allow(clippy::transmute_ptr_to_ref)]
#![allow(clippy::transmuting_null)]
#![allow(clippy::transmuting_null, clippy::missing_transmute_annotations)]

pub const ZPTR: *const usize = 0 as *const _;

Expand Down
7 changes: 7 additions & 0 deletions tests/ui/auxiliary/macro_rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,10 @@ macro_rules! macro_with_panic {
panic!()
};
}

#[macro_export]
macro_rules! bad_transmute {
($e:expr) => {
std::mem::transmute($e)
};
}
7 changes: 6 additions & 1 deletion tests/ui/blocks_in_conditions.fixed
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
//@aux-build:proc_macro_attr.rs

#![warn(clippy::blocks_in_conditions)]
#![allow(unused, clippy::let_and_return, clippy::needless_if)]
#![allow(
unused,
clippy::let_and_return,
clippy::needless_if,
clippy::missing_transmute_annotations
)]
#![warn(clippy::nonminimal_bool)]

macro_rules! blocky {
Expand Down
7 changes: 6 additions & 1 deletion tests/ui/blocks_in_conditions.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
//@aux-build:proc_macro_attr.rs

#![warn(clippy::blocks_in_conditions)]
#![allow(unused, clippy::let_and_return, clippy::needless_if)]
#![allow(
unused,
clippy::let_and_return,
clippy::needless_if,
clippy::missing_transmute_annotations
)]
#![warn(clippy::nonminimal_bool)]

macro_rules! blocky {
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/blocks_in_conditions.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: in an `if` condition, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
--> tests/ui/blocks_in_conditions.rs:25:5
--> tests/ui/blocks_in_conditions.rs:30:5
|
LL | / if {
LL | |
Expand All @@ -20,13 +20,13 @@ LL ~ }; if res {
|

error: omit braces around single expression condition
--> tests/ui/blocks_in_conditions.rs:37:8
--> tests/ui/blocks_in_conditions.rs:42:8
|
LL | if { true } { 6 } else { 10 }
| ^^^^^^^^ help: try: `true`

error: this boolean expression can be simplified
--> tests/ui/blocks_in_conditions.rs:43:8
--> tests/ui/blocks_in_conditions.rs:48:8
|
LL | if true && x == 3 { 6 } else { 10 }
| ^^^^^^^^^^^^^^ help: try: `x == 3`
Expand All @@ -35,7 +35,7 @@ LL | if true && x == 3 { 6 } else { 10 }
= help: to override `-D warnings` add `#[allow(clippy::nonminimal_bool)]`

error: in a `match` scrutinee, avoid complex blocks or closures with blocks; instead, move the block or closure higher and bind it with a `let`
--> tests/ui/blocks_in_conditions.rs:70:5
--> tests/ui/blocks_in_conditions.rs:75:5
|
LL | / match {
LL | |
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/crashes/ice-1782.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#![allow(dead_code, unused_variables)]
#![allow(clippy::unnecessary_cast)]
#![allow(clippy::unnecessary_cast, clippy::missing_transmute_annotations)]

/// Should not trigger an ICE in `SpanlessEq` / `consts::constant`
///
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/eager_transmute.fixed
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![feature(rustc_attrs)]
#![warn(clippy::eager_transmute)]
#![allow(clippy::transmute_int_to_non_zero)]
#![allow(clippy::transmute_int_to_non_zero, clippy::missing_transmute_annotations)]

use std::num::NonZeroU8;

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/eager_transmute.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#![feature(rustc_attrs)]
#![warn(clippy::eager_transmute)]
#![allow(clippy::transmute_int_to_non_zero)]
#![allow(clippy::transmute_int_to_non_zero, clippy::missing_transmute_annotations)]

use std::num::NonZeroU8;

Expand Down
2 changes: 1 addition & 1 deletion tests/ui/missing_const_for_fn/could_be_const.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#![warn(clippy::missing_const_for_fn)]
#![allow(incomplete_features, clippy::let_and_return)]
#![allow(incomplete_features, clippy::let_and_return, clippy::missing_transmute_annotations)]
#![feature(const_mut_refs)]
#![feature(const_trait_impl)]

Expand Down
78 changes: 78 additions & 0 deletions tests/ui/missing_transmute_annotations.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
//@aux-build:macro_rules.rs

#![warn(clippy::missing_transmute_annotations)]
#![allow(clippy::let_with_type_underscore)]

#[macro_use]
extern crate macro_rules;

macro_rules! local_bad_transmute {
($e:expr) => {
std::mem::transmute::<[u16; 2], i32>($e)
//~^ ERROR: transmute used without annotations
};
}

fn bar(x: i32) -> i32 {
x
}

unsafe fn foo1() -> i32 {
// Should not warn!
std::mem::transmute([1u16, 2u16])
}

// Should not warn!
const _: i32 = unsafe { std::mem::transmute([1u16, 2u16]) };

#[repr(i32)]
enum Foo {
A = 0,
}

unsafe fn foo2() -> i32 {
let mut i: i32 = 0;
i = std::mem::transmute::<[u16; 2], i32>([1u16, 2u16]);
//~^ ERROR: transmute used without annotations
i = std::mem::transmute::<[u16; 2], i32>([1u16, 2u16]);
//~^ ERROR: transmute used without annotations
i = std::mem::transmute::<[u16; 2], i32>([1u16, 2u16]);
//~^ ERROR: transmute used without annotations
i = std::mem::transmute::<[u16; 2], i32>([1u16, 2u16]);
//~^ ERROR: transmute used without annotations

let x: i32 = bar(std::mem::transmute::<[u16; 2], i32>([1u16, 2u16]));
//~^ ERROR: transmute used without annotations
bar(std::mem::transmute::<[u16; 2], i32>([1u16, 2u16]));
//~^ ERROR: transmute used without annotations

i = local_bad_transmute!([1u16, 2u16]);

// Should not warn.
i = bad_transmute!([1u16, 2u16]);

i = std::mem::transmute::<[i16; 2], i32>([0i16, 0i16]);
//~^ ERROR: transmute used without annotations

i = std::mem::transmute::<Foo, i32>(Foo::A);
//~^ ERROR: transmute used without annotations

i
}

fn main() {
let x: _ = unsafe { std::mem::transmute::<[u16; 2], i32>([1u16, 2u16]) };
//~^ ERROR: transmute used without annotations
unsafe {
let x: _ = std::mem::transmute::<[u16; 2], i32>([1u16, 2u16]);
//~^ ERROR: transmute used without annotations

// Should not warn.
std::mem::transmute::<[u16; 2], i32>([1u16, 2u16]);
let x = std::mem::transmute::<[u16; 2], i32>([1u16, 2u16]);
let x: i32 = std::mem::transmute::<[u16; 2], _>([1u16, 2u16]);
let x: i32 = std::mem::transmute::<_, i32>([1u16, 2u16]);
let x: i32 = std::mem::transmute([1u16, 2u16]);
}
let x: i32 = unsafe { std::mem::transmute([1u16, 2u16]) };
}
Loading