Suggest {to,from}_ne_bytes for transmutations between arrays and integers, etc #136083

bend-n · 2025-01-26T04:12:57Z

implements #136067

Rust has helper methods for many kinds of safe transmutes, for example integer<->bytes. This is a lint against using transmute for these cases.

fn bytes_at_home(x: [u8; 4]) -> u32 {
   transmute(x)
}

// other examples
transmute::<[u8; 2], u16>();
transmute::<[u8; 8], f64>();
transmute::<u32, [u8; 4]>();
transmute::<char, u32>();
transmute::<u32, char>();

It would be handy to suggest u32::from_ne_bytes(x).
This is implemented for [u8; _] -> {float int}

This also implements the cases:
fXX <-> uXX = {from_bits, to_bits}
char -> u32 via from
u32 -> char via from_u32_unchecked (note: notes from_u32().unwrap()) (contested)
uXX -> iXX via cast_unsigned and cast_signed
bool -> n8 via from
u8 -> bool via == (debatable)

rustbot · 2025-01-26T04:13:05Z

r? @wesleywiser

rustbot has assigned @wesleywiser.
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

bend-n · 2025-01-26T05:37:43Z

how do i fix stdarch?

juntyr · 2025-01-26T08:46:13Z

compiler/rustc_mir_transform/src/check_redundant_transmutes.rs

+            Bool | Uint(..) | Int(..) => {
+                matches!(fn_sig.output().kind(), Int(..) | Uint(..)).then(|| {
+                    errors::RedundantTransmute {
+                        sugg: format!("({arg}) as {}", fn_sig.output()),


I wouldn't suggest an as conversion here as as is so overloaded that I at least always forget what each conversion does. I remember there was an attempt in the past to create explicit methods for each as behaviour - if they've been merged, perhaps we could recommend them?

I'm not sure what methods you're referring to... I don't think i should suggest i32::from_ne_bytes(x.to_ne_bytes())? And try_from has different behavior.

There are the unstable cast_unsigned and cast_signed methods, and there's an ACP for additional methods rust-lang/libs-team#453.

Perhaps for now the code just gets a FIXME so that the recommendation is changed once #125882 is stabilized (feature integer_sign_cast)?

The cast_(un)signed methods will be stable on nightly in another 2 days: #125882 (comment)

So maybe we just hold off on merging this PR until that stabilization PR lands? I'm also not a fan of as, so would like to avoid suggesting something that clippy will soon then start suggesting to change again.

oh wow! i guess ill make it suggest those methods then

what about bool as integer though?

@bend-n

let b: bool = true; let x = u8::from(b); assert_eq!(x, 1);

.into() would be nicer but depends on knowing u8 or i8 from type inference. u8::from and i8::from work in any context, though they don't work in postfix position. For the purposes of this lint, though, the only integer types you can transmute a bool to would be u8 or i8, and transmute already isn't postfix, so I'd suggest that transmutes from bool to u8/i8 should suggest u8::from and i8::from, respectively.

i can do that with u32::from(char) i think, as well

Noratrieb · 2025-01-26T09:47:04Z

Is there a reason you made it a MIR lint instead of a HIR lint? HIR lints will make it easier to do the spans.

bend-n · 2025-01-26T10:04:33Z

@Noratrieb i dont know the difference; i looked for transmute and copied that (ptr_to_integer_transmute_in_consts) lint.

bend-n · 2025-01-26T10:07:43Z

Also, how should i fix stdarch? Should i make a PR over there to remove the unnecessary trans?

Noratrieb · 2025-01-26T10:15:32Z

I would first recommend checking out if the lang team wants this lint.
Can you write out a clear PR description with the exact cases you're covering and some examples for the lang team?

joshtriplett · 2025-02-12T16:13:41Z

We talked about this briefly in today's @rust-lang/lang meeting. This seems likely to always be desirable, and it helps people rewrite unsafe code as safe code. Let's give it a try in nightly and see how it goes! (In particular, that'll let us evaluate possible false positives.)

scottmcm · 2025-02-12T16:48:20Z

u32 -> char via from_u32_unchecked

I'm slightly torn on this one. It feels more like a clippy lint to me, because to me what we're approving here as a lang team is "suggesting safe alternatives to unsafe transmutes", not "well there's a different unsafe version that would be better" -- it's the removing of unsafe that gets it to the "yes this is in rustc" bar, in my mind. (Other team members may disagree, however.)

traviscross · 2025-02-12T17:23:11Z

I'm torn on it too. It is marginally safer, because then at least it goes through the asssert_unsafe_precondition:

pub(super) const unsafe fn from_u32_unchecked(i: u32) -> char {
    // SAFETY: the caller must guarantee that `i` is a valid char value.
    unsafe {
        assert_unsafe_precondition!(
            check_language_ub,
            "invalid value for `char`",
            (i: u32 = i) => char_try_from_u32(i).is_ok()
        );
        transmute(i)
    }
}

Playground link

The assertion only fires on debug builds, though.

rustbot · 2025-02-14T14:20:17Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

bors · 2025-03-08T16:11:43Z

☔ The latest upstream changes (presumably #138208) made this pull request unmergeable. Please resolve the merge conflicts.

bend-n · 2025-03-16T10:36:02Z

r? compiler

lcnr · 2025-03-19T15:05:39Z

compiler/rustc_lint_defs/src/builtin.rs

+    /// People dont realize that safer methods such as
+    /// [`u32::to_ne_bytes`](https://doc.rust-lang.org/std/primitive.u32.html#method.to_ne_bytes) exist,
+    /// so this lint exists to lint on cases where people write transmutes that dont need to be there.


Suggested change

/// People dont realize that safer methods such as

/// [`u32::to_ne_bytes`](https://doc.rust-lang.org/std/primitive.u32.html#method.to_ne_bytes) exist,

/// so this lint exists to lint on cases where people write transmutes that dont need to be there.

/// Using an explicit method is preferable over calls to `transmute` as

/// they more clearly communicate the intent, are easier to review, and

/// are less likely to accidentally result in unsoundness.

I feel like you have a subtly wrong perspective here 🤔

your explanation justifies the lint to the project, as in, why does this lint exist

as this is user-facing, we should instead explain why this lint matters for the user, as in, why the suggested alternative is preferable

Not sure if that distinction makes sense to you 🤔

Yeah that makes sense to me; i just sort of wrote it from the issue

lcnr · 2025-03-19T15:12:08Z

tests/ui/transmute/redundant-transmutation.stderr

+error: this transmute could be performed safely
+  --> $DIR/redundant-transmutation.rs:7:14
+   |
+LL |     unsafe { transmute(x) }
+   |              ^^^^^^^^^^^^ help: replace `transmute`: `u32::to_ne_bytes(x)`
+   |
+   = help: there's also `to_le_bytes` and `to_ne_bytes` if you expect a particular byte order


Suggested change

error: this transmute could be performed safely

--> $DIR/redundant-transmutation.rs:7:14

|

LL | unsafe { transmute(x) }

| ^^^^^^^^^^^^ help: replace `transmute`: `u32::to_ne_bytes(x)`

|

= help: there's also `to_le_bytes` and `to_ne_bytes` if you expect a particular byte order

error: unnecessary transmute

--> $DIR/redundant-transmutation.rs:7:14

|

LL | unsafe { transmute(x) }

| ^^^^^^^^^^^^ help: replace this with `u32::to_ne_bytes(x)`

|

= help: there's also `to_le_bytes` and `to_ne_bytes` if you expect a particular byte order

This currently looks a bit odd to me 🤔 like the error message doesn't quite match the vibe of our existing diagnostics

lcnr

calling the lint redundant_transmute feels wrong to me 🤔

the transmutes are required after all, it's just that there are better alternatives available. I feel like something like unnecessary_transmute would be better here

scottmcm · 2025-03-19T15:31:15Z

Spitballing: maybe there's a phrasing that's about how the problem is that you're doing it using unsafe when there's a safe way to do it? Obviously unnecessary_unsafe is the wrong name, since it's not about the unsafe block being wrong, but maybe that'll inspire one of you to have a better idea.

rustbot · 2025-03-19T16:10:45Z

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

lcnr · 2025-03-19T16:14:44Z

compiler/rustc_lint_defs/src/builtin.rs

@@ -83,6 +83,7 @@ declare_lint_pass! {
        PUB_USE_OF_PRIVATE_EXTERN_CRATE,
        REDUNDANT_IMPORTS,
        REDUNDANT_LIFETIMES,
+        UNNECESSARY_TRANSMUTATION,


please call it unnecessary_transmute, not transmutation.

transmute

Plural, right? So unnecessary_transmutes?

compiler/rustc_mir_transform/src/check_unnecessary_transmutes.rs

…gers, etc

rustbot assigned wesleywiser Jan 26, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 26, 2025

This comment has been minimized.

Sign in to view

bend-n force-pushed the ⃤⃤ branch 2 times, most recently from 0d3bf7f to 9666623 Compare January 26, 2025 05:36

This comment has been minimized.

Sign in to view

juntyr reviewed Jan 26, 2025

View reviewed changes

Noratrieb added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 26, 2025

traviscross added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jan 29, 2025

This comment has been minimized.

Sign in to view

traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Feb 12, 2025

bend-n force-pushed the ⃤⃤ branch from 74978b3 to 79f53d2 Compare February 12, 2025 17:32

This comment has been minimized.

Sign in to view

bend-n mentioned this pull request Feb 12, 2025

change redundant transmutations of sign to cast_unsigned rust-lang/stdarch#1711

Merged

bend-n marked this pull request as ready for review February 14, 2025 14:20

This comment has been minimized.

Sign in to view

bend-n force-pushed the ⃤⃤ branch 5 times, most recently from f7995c6 to 576ac47 Compare March 8, 2025 08:32

bend-n force-pushed the ⃤⃤ branch from 576ac47 to bd07ceb Compare March 8, 2025 16:13

rustbot assigned lcnr and unassigned wesleywiser Mar 16, 2025

lcnr reviewed Mar 19, 2025

View reviewed changes

bend-n force-pushed the ⃤⃤ branch from bd07ceb to 0e970e6 Compare March 19, 2025 16:10

lcnr reviewed Mar 19, 2025

View reviewed changes

compiler/rustc_mir_transform/src/check_unnecessary_transmutes.rs Outdated Show resolved Hide resolved

bend-n force-pushed the ⃤⃤ branch from 0e970e6 to 714a583 Compare March 19, 2025 16:17

lcnr reviewed Mar 19, 2025

View reviewed changes

compiler/rustc_mir_transform/src/check_unnecessary_transmutes.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

bend-n force-pushed the ⃤⃤ branch from 714a583 to 691f223 Compare March 19, 2025 16:30

This comment has been minimized.

Sign in to view

bend-n force-pushed the ⃤⃤ branch from 691f223 to c0aa594 Compare March 19, 2025 16:45

This comment has been minimized.

Sign in to view

Suggest {to,from}_ne_bytes for transmutations between arrays and inte…

0abd167

…gers, etc

bend-n force-pushed the ⃤⃤ branch from c0aa594 to 0abd167 Compare March 19, 2025 17:01

This comment has been minimized.

Sign in to view

apply suggestion

c499a24

bend-n force-pushed the ⃤⃤ branch from 268b798 to c499a24 Compare March 19, 2025 18:46

Suggest {to,from}_ne_bytes for transmutations between arrays and integers, etc #136083

Are you sure you want to change the base?

Suggest {to,from}_ne_bytes for transmutations between arrays and integers, etc #136083

Conversation

bend-n commented Jan 26, 2025 • edited Loading

rustbot commented Jan 26, 2025

This comment has been minimized.

bend-n commented Jan 26, 2025

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

bend-n Jan 26, 2025 • edited Loading

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

Noratrieb commented Jan 26, 2025

bend-n commented Jan 26, 2025

bend-n commented Jan 26, 2025

Noratrieb commented Jan 26, 2025

joshtriplett commented Feb 12, 2025

This comment has been minimized.

scottmcm commented Feb 12, 2025

traviscross commented Feb 12, 2025 • edited Loading

This comment has been minimized.

This comment has been minimized.

rustbot commented Feb 14, 2025

This comment has been minimized.

bors commented Mar 8, 2025

bend-n commented Mar 16, 2025

lcnr Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr left a comment

Choose a reason for hiding this comment

scottmcm commented Mar 19, 2025

rustbot commented Mar 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bend-n commented Jan 26, 2025 •

edited

Loading

bend-n Jan 26, 2025 •

edited

Loading

traviscross commented Feb 12, 2025 •

edited

Loading

lcnr Mar 19, 2025 •

edited

Loading