-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
base: master
Are you sure you want to change the base?
The head ref may contain hidden characters: "\u20E4\u20E4"
Conversation
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
This comment has been minimized.
This comment has been minimized.
0d3bf7f
to
9666623
Compare
how do i fix stdarch? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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()), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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
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. |
@Noratrieb i dont know the difference; i looked for |
Also, how should i fix |
I would first recommend checking out if the lang team wants this lint. |
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.) |
This comment has been minimized.
This comment has been minimized.
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.) |
I'm torn on it too. It is marginally safer, because then at least it goes through the 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)
}
} The assertion only fires on debug builds, though. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
This comment has been minimized.
This comment has been minimized.
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 |
Some changes occurred in compiler/rustc_codegen_gcc |
This comment has been minimized.
This comment has been minimized.
ad356e8
to
97810ff
Compare
This comment has been minimized.
This comment has been minimized.
f7995c6
to
576ac47
Compare
☔ The latest upstream changes (presumably #138208) made this pull request unmergeable. Please resolve the merge conflicts. |
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.
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
viafrom
u32
->char
viafrom_u32_unchecked
(note: notesfrom_u32().unwrap()
) (contested)uXX
->iXX
viacast_unsigned
andcast_signed
bool
->n8
viafrom
u8
->bool
via==
(debatable)