add lint for transmute from &T to &mut T of a ADT argument

[mendelsshop]

View all comments

Checks transmutes between the same ADT, making sure type arguments don't go from &T to &mut T.

Like it would catch something like:

let _: Option<&mut i32> = std::mem::transmute(Some(&5i32));

changelog: new lint [mutable_adt_argument_transmute]

fixes #372

Caution

Concerns (1 active)

• File names are inconsistent yet, should be fixed after FCP

Managed by @rustbot—see help for details.

[mendelsshop]

Edit:
I now skip the parts that don't match (like Option<_> and Result<_, _>), but this does require me to match on each type's .kind() to see if they are the same ADT, slice, ..., and if not I use skip_current_subtree().

The way I am doing it now, I take the from_ty and to_ty and just zip the ty.walk() of both of them, which leads to potential false positive like catching:

let _: Result<Option<&mut f32>, &usize> = std::mem::transmute(Option::<Result<&f32, usize>>::Some(Ok(&2f32)));

Because in the order of the walk() &mut f32 and &f32 are at the same position in their respective walk().

[rustbot]

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[github-actions]

No changes for 7ac7844

[mendelsshop]

r? clippy

[mendelsshop]

r? clippy

[samueltardieu]

We should avoid false positives and get a better output, I would suggest that:

• When different ADT or different primitive types are detected, return false immediately, as the types are structurally different
• Instead of signaling the lint immediately (which will trigger several times if several references are cast into their mutable version), accumulate the pairs (from_ty, to_ty) into a vector. At the end, if the vector is not empty, trigger the lint and add a note for every different pair (you can use .dedup_by() from Itertools) through span_lint_and_then().
• Add an example which checks that having several conversions is displayed properly (one lint, three notes), as in:

        let _: Option<(&mut i32, &mut i32, &mut u32)> = std::mem::transmute(Some((&5i32, &10i32, &15u32)));
        //~^ mutable_adt_argument_transmute

This should print something like:

error: transmuting references into their mutable version is unsound
  --> tests/ui/mutable_adt_argument_transmute.rs:17:57
   |
LL |         let _: Option<(&mut i32, &mut i32, &mut u32)> = std::mem::transmute(Some((&5i32, &10i32, &15u32)));
   |                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: transmuting from &i32 to &mut i32
   = note: transmuting from &u32 to &mut u32

View changes since this review

[samueltardieu]

Suggested change

	#[clippy::version = "1.92.0"]
	#[clippy::version = "1.95.0"]

[samueltardieu]

This should ensure that only adts are matched against, or it may also trigger for other types if the behavior of wrong_transmute is changed for example, won't it?

[mendelsshop]

Edit:
If you are talking about the arguments themselves, then they can be any type as long as they are both behind a reference, I currently do not check if the actual argument types that are behind the reference are the same.

I don't fully understand what you are asking, but it triggers for closure, fn, slice, reference, tuple, array and pointer types along with standard adts, basically any type that can have "arguments".

[samueltardieu]

Take for example the ty::FnPtr type: the type of the parameters may be safely transmuted from &T to &mut T, while the return type might not. In the two transmute examples below, only the second one should be linted:

        // This one is not dangerous, as the function will not try
        // to mutate its parameter anyway.
        fn f(x: &i32) {}
        let a: fn(&i32) = f;
        let b: fn(&mut u32) = std::mem::transmute(a);

        // This one should be linted because it would not be safe
        // to assume that the return value can be mutated.
        fn g() -> &'static i32 {
            &0
        }
        let c: fn() -> &'static i32 = g;
        let d: fn() -> &'static mut i32 = std::mem::transmute(c);

The same thing probably happens in closures as well, and maybe others. In this case, you should either special-case the lint (such as linting only FnPtr and closures return types and ignore parameters), or not lint at all in those cases.

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[samueltardieu]

Suggested change

	/// This is an a more complicated version of https://doc.rust-lang.org/rustc/lints/listing/deny-by-default.html#mutable-transmutes.
	/// This is a more complicated version of https://doc.rust-lang.org/rustc/lints/listing/deny-by-default.html#mutable-transmutes.

Also, maybe not "complicated".

[mendelsshop]

Also, maybe not "complicated".

Is "advanced" a better word?

[samueltardieu]

"specialized"?

[mendelsshop]

Done.

[Jarcho]

Ty::walk can't be used like this. It deduplicates types which means you can't keep the two iterators in sync and you'll miss problematic pairs.

Walking (&T, &mut T) will result in:

• (&T, &mut T)
• &T
• T
• &mut T

Walking (&T, &T) will result in:

• (&T, &T)
• &T
• T

Here you would miss a &mut T to &T conversion because the second reference is skipped.

[mendelsshop]

Ty::walk can't be used like this. It deduplicates types which means you can't keep the two iterators in sync and you'll miss problematic pairs.

Does it make sense to duplicate Ty::walk and have a version that doesn't deduplicate (seems relatively easy to do)? Or should I just use recursion?
It seems plausible that there might be other uses for a Ty::walk that doesn't deduplicate (but I don't have examples).

[samueltardieu]

Does it make sense to duplicate Ty::walk and have a version that doesn't deduplicate (seems relatively easy to do)? Or should I just use recursion? It seems plausible that there might be other uses for a Ty::walk that doesn't deduplicate (but I don't have examples).

You need to deduplicate to avoid infinite recursion. But you could walk two types in parallel and deduplicate by pairs.

[Jarcho]

Infinite recursion only happens if you look at ADT fields.

[samueltardieu]

Infinite recursion only happens if you look at ADT fields.

Ah, yes, that is what I had in mind, types can't be recursive even through pointers.

[Jarcho]

Does it make sense to duplicate Ty::walk and have a version that doesn't deduplicate (seems relatively easy to do)? Or should I just use recursion? It seems plausible that there might be other uses for a Ty::walk that doesn't deduplicate (but I don't have examples).

You want to walk both types in lockstep with each other. This doesn't map well to an iterator and everything that does this has it's own rules for how to walk the structure. Ty::walk works because the result ignores structure and just returns every unique type.

[mendelsshop]

Ok, I moved away from ty::walk.
@rustbot ready

[samueltardieu]

What is walk here? Is the comment out of date?

View changes since the review

[mendelsshop]

It's an out of date comment.

[samueltardieu]

Link to the FCP thread

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.