Create `implicit_transmute_types` lint. #11047

Flying-Toast · 2023-06-29T17:36:00Z

Type inference is useful in most cases, but it can cause issues around usage of transmute. Even though a specific usage of transmute may have been confirmed to be sound, someone might unintentionally change a piece of surrounding code that causes the transmuted types to change and possibly become unsound. Despite that, transmute() will happily continue to infer these changed types.

This commit adds a lint implicit_transmute_types that catches calls to transmute without an explicit turbofish. The idea is that the initial author of code that uses a transmute will verify the soundness of converting between the two types, and enfore those types on the transmute call. If the types end up changing later, typecheck will fail and force the transmute call to be updated and remind the user to reassess the soundness of transmuting between the new types.

rustbot · 2023-06-29T17:36:04Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Centri3

This looks pretty good, thank you! I've left some comments.

Centri3 · 2023-06-30T10:14:23Z

clippy_lints/src/implicit_transmute_types.rs

+    /// ```
+    ///
+    /// If you decide that you *do* want the types to be inferred,
+    /// you can silence the lint by conveying your intention explicitly:


I feel like having a configuration option to also disallow this would be nice, you can probably check the 1st and 2nd arguments of the last segment to see if either are _

clippy_lints/src/implicit_transmute_types.rs

Type inference is useful in most cases, but it can cause issues around usage of transmute. Even though a specific usage of transmute may have been confirmed to be sound, someone might unintentionally change a piece of surrounding code that causes the transmuted types to change and possibly become unsound. Despite that, transmute() will happily continue to infer these changed types. This commit adds a lint `implicit_transmute_types` that catches calls to transmute without an explicit turbofish. The idea is that the initial author of code that uses a transmute will verify the soundness of converting between the two types, and enfore those types on the transmute call. If the types end up changing later, typecheck will fail and force the transmute call to be updated and remind the user to reassess the soundness of transmuting between the new types.

xFrednet · 2023-07-01T22:38:38Z

Hey @Flying-Toast welcome to Clippy 👋

Thank you for starting the review @Centri3. You're welcome to continue it, and ping me once everything is done :)

Centri3 · 2023-07-01T23:09:03Z

I think I've already said everything I wanted to ^^ feel free to give your thoughts

xFrednet

Hey, I think this is a good start. I left some comments. You're welcome to reach out to @Centri3 or me if you have any questions! :)

xFrednet · 2023-07-04T16:41:07Z

clippy_lints/src/implicit_transmute_types.rs

+    /// [`transmute`]: https://doc.rust-lang.org/core/mem/fn.transmute.html
+    #[clippy::version = "1.72.0"]
+    pub IMPLICIT_TRANSMUTE_TYPES,
+    style,


I would classify this more as a restriction lint and also think it should be allow-by-default.

I've looked at examples where I used transmute and found this one, where I like the code more without the types:

#[must_use] pub fn a_to_b(&self, src: A) -> B { assert_eq!(size_of::<A>(), 4); assert_eq!(size_of::<B>(), 4); unsafe { transmute(src) } }

There are definitely good reasons to have the types, but most projects would probably not directly agree with this. So, could you please change to group to restriction?

I think the amount of tests, which needed to be adjusted also speaks for this change :)

xFrednet · 2023-07-04T16:55:56Z

clippy_lints/src/implicit_transmute_types.rs

+    /// In most cases Rust's type inference is helpful, however it can cause
+    /// problems with [`transmute`]. `transmute` is wildly unsafe
+    /// unless the types being transmuted are known to be compatible. As such,
+    /// a seemingly innocent change in something's type can end up making a
+    /// previously-valid transmute suddenly become unsound. Thus it is
+    /// good practice to always be explicit about the types you expect to be
+    /// transmuting between, so that the compiler will force you to
+    /// reexamine the transmute if either type changes.


I mostly agree with this statement, but feel like the explanation could be clearer. Also the example of changing a type might be confusing. I imagined this:

struct A(u32); struct B(f32); let a = A(1); let b: B = unsafe { transmute(src) };

changing B to be B(f64) would be accepted by the lint, but would be unsound. I think it should be more highlighted that there could be a problem if the usage changes. How about this description:

/// ### Why is this bad? /// Rust uses inference to determine the types if they're not specified. /// This can cause the transmuted types to change accidentally and the /// transmute becoming unsound. Specifying the types ensures that any /// changes will either be compatible or cause an error. /// /// ### Example /// ```rust /// # use core::mem::transmute; /// # fn accept_dest(_: u32) {} /// let source = 42; /// let dest = unsafe { transmute(source) }; /// /// // Changing `accept_dest` might change the target type of the transmute /// // and make it unsound. /// accept_dest(dest); /// ``` /// /// Use instead: /// ```rust /// # use core::mem::transmute; /// # fn accept_dest(_: u32) {} /// let source = 42; /// let dest = unsafe { transmute::<i32, u32>(source) }; /// /// // Changing `accept_dest` will cause an error if the argument is /// // no longer an u32. /// accept_dest(dest); /// ```

xFrednet · 2023-07-04T16:59:46Z

tests/ui/blocks_in_if_conditions.fixed

@@ -44,7 +44,7 @@ fn condition_is_unsafe_block() {
    let a: i32 = 1;

    // this should not warn because the condition is an unsafe block
-    if unsafe { 1u32 == std::mem::transmute(a) } {
+    if unsafe { 1u32 == std::mem::transmute::<i32, u32>(a) } {


NIT: You can undo the changes in this and the other file, when you move the lint to restriction, as that will make the lint allow-by-default. This is fine, but that way we can make the PR a bit cleaner :)

xFrednet · 2023-07-04T17:01:36Z

clippy_lints/src/implicit_transmute_types.rs

+
+impl<'tcx> LateLintPass<'tcx> for ImplicitTransmuteTypes {
+    fn check_expr(&mut self, cx: &LateContext<'_>, e: &hir::Expr<'_>) {
+        if let ExprKind::Call(func, _) = &e.kind


Could you add a check, to ensure that the code doesn't come from macros? You can checkout in_external_macro for this :)

xFrednet · 2023-07-04T17:03:59Z

clippy_lints/src/implicit_transmute_types.rs

+                        suggestion_span,
+                        "consider specifying the types intended to be transmuted",
+                        format!("::<{srctype}, {dsttype}>"),
+                        Applicability::MachineApplicable


Printing types like this can sometimes cause problems, when rustc can't determine the correct path for an item. This should be:

Suggested change

Applicability::MachineApplicable

Applicability::MaybeIncorrect

xFrednet · 2023-07-04T17:05:24Z

tests/ui/implicit_transmute_types.rs

+        let _: u32 = mem::transmute::<i32, u32>(123i32);
+        let _: u32 = mem::transmute::<_, _>(123i32);


Can you add some more tests, with structs and other more complex types? Maybe play with String, struct A(u32) etc. These files don't get executed, so the transmutes can be unsound, without spawning nostril dragons 😉

xFrednet · 2023-07-04T17:07:44Z

clippy_lints/src/implicit_transmute_types.rs

+            && let ExprKind::Path(qpath) = &func.kind
+            && let Some(def_id) = cx.qpath_res(qpath, func.hir_id).opt_def_id()
+            && is_diagnostic_item_or_ctor(cx, def_id, sym::transmute)
+            && last_path_segment(qpath).args.is_none()


Here we could optionally check, that the two types are not the infer ones ::<_, _>. But that's an optional improvement. You're welcome to pick it up, or we just create a follow-up issue for this :)

xFrednet · 2024-02-12T21:07:57Z

Ping @Flying-Toast, this is a ping from triage. Do you plan to continue this PR in the near future?

Flying-Toast · 2024-02-12T21:12:09Z

Ping @Flying-Toast, this is a ping from triage. Do you plan to continue this PR in the near future?

Probably not, so I'm fine closing it or if someone else wants to take it over.

xFrednet · 2024-02-12T21:47:40Z

Okay, thank you for the time and effort you already put into this PR. :D

rustbot assigned xFrednet Jun 29, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jun 29, 2023

Flying-Toast force-pushed the implicit_transmute_types branch 2 times, most recently from e82a5f2 to f4963c2 Compare June 29, 2023 18:46

Centri3 reviewed Jun 30, 2023

View reviewed changes

Flying-Toast force-pushed the implicit_transmute_types branch from f4963c2 to fcb3401 Compare June 30, 2023 22:26

Flying-Toast force-pushed the implicit_transmute_types branch from fcb3401 to c6e0145 Compare June 30, 2023 22:31

xFrednet reviewed Jul 4, 2023

View reviewed changes

Centri3 mentioned this pull request Feb 7, 2024

Add missing_transmute_annotations lint #12239

Merged

xFrednet closed this Feb 12, 2024

xFrednet added S-inactive-closed Status: Closed due to inactivity and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create `implicit_transmute_types` lint. #11047

Create `implicit_transmute_types` lint. #11047

Flying-Toast commented Jun 29, 2023

rustbot commented Jun 29, 2023

Centri3 left a comment

Centri3 Jun 30, 2023

xFrednet commented Jul 1, 2023

Centri3 commented Jul 1, 2023

xFrednet left a comment

xFrednet Jul 4, 2023

xFrednet Jul 4, 2023

xFrednet Jul 4, 2023

xFrednet Jul 4, 2023

xFrednet Jul 4, 2023

xFrednet Jul 4, 2023

xFrednet Jul 4, 2023

xFrednet commented Feb 12, 2024

Flying-Toast commented Feb 12, 2024

xFrednet commented Feb 12, 2024

	Applicability::MachineApplicable
	Applicability::MaybeIncorrect

		let _: u32 = mem::transmute::<i32, u32>(123i32);
		let _: u32 = mem::transmute::<_, _>(123i32);

Create implicit_transmute_types lint. #11047

Create implicit_transmute_types lint. #11047

Conversation

Flying-Toast commented Jun 29, 2023

rustbot commented Jun 29, 2023

Centri3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xFrednet commented Jul 1, 2023

Centri3 commented Jul 1, 2023

xFrednet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xFrednet commented Feb 12, 2024

Flying-Toast commented Feb 12, 2024

xFrednet commented Feb 12, 2024

Create `implicit_transmute_types` lint. #11047

Create `implicit_transmute_types` lint. #11047