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.

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

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bend-n
Copy link
Contributor

@bend-n bend-n commented Jan 26, 2025

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
Copy link
Collaborator

rustbot commented Jan 26, 2025

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

@rustbot 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
@rust-log-analyzer

This comment has been minimized.

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

bend-n commented Jan 26, 2025

how do i fix stdarch?

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@bend-n bend-n Jan 26, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what about bool as integer though?

Copy link
Member

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@Noratrieb
Copy link
Member

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
Copy link
Contributor Author

bend-n commented Jan 26, 2025

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

@bend-n
Copy link
Contributor Author

bend-n commented Jan 26, 2025

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

@Noratrieb
Copy link
Member

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?

@Noratrieb Noratrieb added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 26, 2025
@traviscross traviscross added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Jan 29, 2025
@joshtriplett
Copy link
Member

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.)

@rust-log-analyzer

This comment has been minimized.

@scottmcm
Copy link
Member

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
Copy link
Contributor

traviscross commented Feb 12, 2025

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.

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

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 14, 2025

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2025

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2025

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2025

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2025

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

@bend-n bend-n force-pushed the ⃤⃤ branch 2 times, most recently from ad356e8 to 97810ff Compare March 8, 2025 02:11
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Mar 8, 2025

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

@bend-n bend-n force-pushed the ⃤⃤ branch 6 times, most recently from f7995c6 to 576ac47 Compare March 8, 2025 08:32
@bors
Copy link
Contributor

bors commented Mar 8, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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-lang Relevant to the language 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants