dont handle bool transmute

[bend-n]

removes transmute(u8) -> bool suggestion due to ambiguity, leave it for clippy

elaboration on ambiguity in question:
transmute::<u8, bool>(x) will codegen to an assume(u8 < 2);
_ == 1 or _ != 0 or _ % 2 == 0 would remove that assumption
match _ { x @ (0 | 1) => x == 1, _ => std::hint::unreachable_unchecked() } is very verbose

@rustbot label L-unnecessary_transmutes

[rustbot]

r? @Nadrieril

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

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

Unknown labels: L-unnecessary-transmute

[rustbot]

Unknown labels: L-unnecessary-transmutes

[oli-obk]

Is there context/discussions that led to this change?

[bend-n]

sort of

[Nadrieril]

I don't know how we decide on this sort of thing.

I personally expect that this suggestion is useful, I can see myself thinking in terms of bytes and forgetting that there's a safe way to do this conversion. At the same time if I'm really doing byte-conversion shenanigans, writing _ == 1 doesn't correctly express my intent.

I'd personally err on the beginner-friendlier side, I've seen colleagues reach for transmute way too happily.

[bend-n]

well this already exists in clippy, as transmute_num_to_bool

[Nadrieril]

ok, that convinces me. very unsure on what basis I should decide whether to merge tho; @oli-obk do you know the etiquette? also wdyt of this?

[asquared31415]

Personally i think that when you're trying to give the compiler more information to optimize with, you should expect to have to write a little more or silence a warning because you're effectively saying "no, i really mean to do this, compiler." I don't know whether this should be demoted to only being in clippy, as the suggested change produces something with identical semantics and no possibility of UB on misuse.

[oli-obk]

I'm really doing byte-conversion shenanigans, writing _ == 1 doesn't correctly express my intent.

The alternative is UB, so I don't see why we'd remove this lint. As asquared said, when you want this opt hint, use expect and state why you consider this sound

[bend-n]

@scottmcm do you want to argue this?

[scottmcm]

as the suggested change produces something with identical semantics and no possibility of UB on misuse

Identical observable semantics, yes, but potentially very different performance behaviour.

I'm strongly in favour of rustc saying "use this non-unsafe thing when there's literally no downside to it", like char→u32 there's no reason to use transmute because as is exactly the same codegen. It's always (outside a macro) wrong to use transmute there.

But for u8→bool, it's not necessarily wrong. And if this is something that someone's doing intentionally -- maybe they needed to pass their bool through a C char -- then no they don't want the != 0 version necessarily. So then this lint ends up saying "you should write { assert_unchecked!(x <= 1); x != 0 } instead of transmute::<u8, bool>(x)", and it's not clear at all to me that that's good advice. I might actually say that the transmute there is better.

That means to me it's a "which is the recommended way to write it, even though both are in a sense fine" lint, which is more clippy's domain than rustc's. If the only way to avoid the lint is to allow it, and reasonable code might want to do that, then I think it's a bad lint for rustc.

---

TL/DR:

• I think the place for the "you should use x != 0" is the existing hint for trying to use x as bool, not here.
• This transmute is actually quite useful, because getting a bunch of extra icmps in the codegen can be problematic.

[scottmcm]

I think this PR is good as currently written. The current == 1 suggestion is mediocre (!= 0 would be a bit better, as it's less confusing to optimizers because you probably didn't want 1 and 2/3 to be different) and leaving it to clippy -- especially when clippy already has this -- sounds good to me.

[Nadrieril]

Trying to summarize:

• This lint is intended for ppl who may not have thought that writing != 0 would work for the purposes of u8 -> bool data conversion;
• This lint is a papercut for ppl who know what they're doing and really want a transmute;
• This lint will exist, the question is whether it is better located in rustc or clippy;
• This lint should probably suggest != 0 either way, for optimization purposes.

I'm on team "this belongs in clippy", but also not ready to override other's opinions on this. I'll note that at the very least, this PR should add a comment saying "We chose not to lint u8 -> bool transmutes, see #140431".

[bend-n]

@Nadrieril so regarding not wanting to override other's opinions, what do we do now? do we want approval from @oli-obk?

[oli-obk]

I understand y'all's points, so it's ok with me. Me having a different opinion shouldn't block this.

[Nadrieril]

Thanks @oli-obk! Sorry for the wait @bend-n, this was a bit of an edge case policy-wise.

This is small and reversible so I'm making the call to merge. Thanks for the input everyone.

@bors r+ rollup

[bors]

📌 Commit de8e864 has been approved by Nadrieril

It is now in the queue for this repository.