Transmute_undefined_repr to nursery again

[dtolnay]

This PR reinstates #8418, which was reverted in #8425 (incorrectly I think).

I don't want to start a revert war over this but I feel very strongly that this lint is not in a state that would be a net benefit to users of clippy. In its current form, making this an enabled-by-default correctness lint with authoritative-sounding proclamations of undefined behavior does more harm than the benefit of the true positive cases.

I can file a bunch more examples of false positives but I don't want to give the author of this lint the impression that it is ready to graduate from nursery as soon as I've exhausted the amount of time I am willing to spend revising this lint.

Instead I would recommend that the author of the lint try running it on some reputable codebases containing transmutes. Everywhere that the lint triggers please consider critically whether it should be triggering. For cases that you think are true positives, please raise a few of them with the crate authors (in a PR or issue) to better understand their perspective if they think the transmute is correct.

---

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: Re-remove [transmute_undefined_repr] from default set of enabled lints

[rust-highfive]

r? @Manishearth

(rust-highfive has picked a reviewer for you, use r? to override)

[dtolnay]

Sticking with the reviewers of the previous PRs:
r? @llogiq @giraffate

[Manishearth]

I think this needs to be discussed by @rust-lang/clippy : it's not just "the author of the lint" that makes this call in general.

I can file a bunch more examples of false positives but I don't want to give the author of this lint the impression that it is ready to graduate from nursery as soon as I've exhausted the amount of time I am willing to spend revising this lint.

I don't really agree with this justification. It is fine to say that you think such a list is nonexhaustive (and that you would like us to do more research here), but it would still help us when reviewing to be able to see the general space we are looking at so that we can at least approve this revert.

"Fix this but I won't tell you why" does not make us likely to merge this PR any time soon (since it gates the merging of this PR on us getting the time to do this research). "Fix this, here's why, and I don't think we should move it back to the main category until further research is done" makes us likely to merge this PR and make sure that the full research is done before stuff is moved back.

It would be trivial for us to add a note about this to the lint description and file a followup issue to ensure this. As it stands, we need to do this research to accept this revert right now. I haven't been involved in this lint and I'm happy to have other team members disagree with me on this but I personally would be uncomfortable approving a revert of a revert with no concrete justification, even if it comes from a community member I generally trust the opinion of.

[dtolnay]

@Manishearth my concern is that from skimming the implementation of the lint, I see very little connection between what is written there and what makes a transmute valid/invalid in general. I get the impression that the lint was written by someone without a strong understanding of real-world use of transmute, and accepted into clippy by someone without a strong understanding of real-world use of transmute, and it's a bit of a process failure that two such people get to speak with clippy's earned authority (as much as people have come to trust clippy) about what transmutes are considered UB. This is why I suggested that it would be a good idea to look at some real codebases before graduating this lint.

Concrete case that I think shows how far disconnected this lint is from reality:

use std::mem::transmute;

fn value<T>() -> T {
    unimplemented!()
}

struct Struct(usize, usize);

fn main() {
    let _ = unsafe { transmute::<Box<Struct>, *const Struct>(value()) };
}

error: transmute to `*const Struct` which has an undefined layout
  --> src/main.rs:10:22
   |
10 |     let _ = unsafe { transmute::<Box<Struct>, *const Struct>(value()) };
   |                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: `#[deny(clippy::transmute_undefined_repr)]` on by default
   = note: the contained type `Struct` has an undefined layout
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#transmute_undefined_repr

*const Struct absolutely has a defined layout. It is a pointer. So is Box, which is guaranteed ABI-compatible with a pointer.

[Manishearth]

@Manishearth my concern is that from skimming the implementation of the lint, I see very little connection between what is written there and what makes a transmute valid/invalid in general

Concrete case that I think shows how far disconnected this lint is from reality:
...

That's valid, thank you, I think in the future please include such reasoning in the PR itself alongside the suggestion to ask for deeper review when undoing, rather than deliberately withholding the reasoning.

I do agree that this should be reverted, and would r=me, but I'd also suggest we (maybe @Jarcho?) file a followup about this and link to it in the description. The next clippy sync is not soon so I don't see a rush here unless you feel this needs to be backported or released in nightly asap. I do have have thoughts about what a good version of this lint would look like but it's far more complicated and doesn't need to be discussed here.

---

In general our process for lints is to have a relatively low bar for accepting lints, but also be relatively trigger happy for nursery'ing them. The nature of clippy and how people use it means that it's kinda hard to get decent real world feedback without putting lints in a non-nursery category from the start. Just running on a bunch of crates is also not sufficient since clippy is intended to be run with a liberal sprinkling of codebase-specific allow and it's not always easy to pick up on user intent. In this case I think the process failure was that we should be more careful about promoting nursery lints, perhaps requiring additional review.

and it's a bit of a process failure that two such people get to speak with clippy's earned authority (as much as people have come to trust clippy)

I will point out that these are people who are behind clippy's earned authority, as well. @Jarcho isn't a team member but has contributed plenty of times, and the two people who reviewed the lint at various times are of course team members. I do feel that there was a process failure here, but I want to push back strongly against the suggestion that this is what the failure was. Mistakes get made, sometimes someone will review something and not realize a deeper implication. The process failure is more likely in that we need further review for denurseryification. Team members (and trusted contributors) of a project need not have a full breadth of knowledge of the subject matter to retain their status; they should be able to make reasonable judgement as to when they need outside input, and if a couple mistakes get made in the process, that is fine.

[flip1995]

The next clippy sync is not soon so I don't see a rush here unless you feel this needs to be backported or released in nightly asap

This will get into beta and ultimately into stable as a deny-by-default lint, if we don't change the lint group until Friday (when beta is branched).

So because this is currently a deny-by-default lint and we claim code that those lints trigger on is "outright wrong or useless", I'd suggest we put this lint into nursery for this release, so we have time to discuss it further.

We don't have to merge this PR for that, I can just adjust the group in the Rust repo directly and avoid syncing right before a release.

---

I do feel that there was a process failure here, but I want to push back strongly against the suggestion that this is what the failure was. Mistakes get made, sometimes someone will review something and not realize a deeper implication.

I want to emphasize this statement. Clippy has over 500 lints, so it is impossible for everyone on the team to know every lint. By now it is already really hard to keep up with the team members that might have a better/expert knowledge of a specific lint. So something like this can slip through. I expect that going in the direction of #8435 might help us test lints before shipping them to stable Clippy.

[Jarcho]

The lint is meant to catch transmutes of structs with multiple fields to another type where the order of the fields is not specified (e.g. struct Foo(u32, u32) to struct Bar(u32, u32)). Transmutes between struct Foo { .. } and struct Bar(Foo) were never supposed to be caught.

The approach taken for the lint is to reduce the the two types down and then check if both types are different and either type has multiple non-zero sized fields without defining the layout of those fields (with a few extra things allowed). The two reduction steps are;

1. If a type is a struct or tuple containing only a single sized field, treat it as though it were the type of the field
2. If both types are references, remove the reference.

[Tamschi]

[…]

1. If a type is a struct or tuple containing only a single sized field, treat it as though it were the type of the field
2. If both types are references, remove the reference.

I don't think it's implemented quite like that right now (just an example that came up in my CI just now), or maybe it's just unaware of pointers:

error: transmute to `&V` which has an undefined layout
   --> src/sync/injection.rs:117:12
    |
117 |         unsafe { mem::transmute::<Pin<*const V>, &V>(self.value) }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: `#[deny(clippy::transmute_undefined_repr)]` on by default
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#transmute_undefined_repr

(In a Deref implementation of a mapped handle… I should use a non-null dereferenceable there though, now that I look at the code again.)

[Jarcho]

What is currently in nightly doesn't quite do that. There are times where a reference on only one type is removed rather than both (#8440 has the fix for it). It would also stop reducing the type when a repr(C) or repr(packed) struct was found even though the type contained only a single field (fixed in a previous PR, but not in nightly yet).

[bors]

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

[flip1995]

We re-enabled this lint in the last sync, together with the recent fixes. @dtolnay do you still think that this lint should be moved to nursery?

[Manishearth]

@flip1995 I doubt his positition has changed: we should disable this until a more proper investigation has happened, and I agree on that. Is there a reason why we shouldn't?

Actually, I'm going to land this now and file the followup that this was blocked on.

@bors r+

[bors]

📌 Commit fd9dd04 has been approved by Manishearth

[bors]

⌛ Testing commit fd9dd04 with merge 14f3d05...

[Manishearth]

Filed #8496. We should add a note to the source about this.

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Manishearth
Pushing 14f3d05 to master...

[Manishearth]

#8497