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

Transmute_undefined_repr to nursery again #8432

Merged
merged 1 commit into from
Mar 2, 2022

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Feb 14, 2022

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

r? @Manishearth

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Feb 14, 2022
@dtolnay
Copy link
Member Author

dtolnay commented Feb 14, 2022

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

@rust-highfive rust-highfive assigned llogiq and unassigned Manishearth Feb 14, 2022
@Manishearth
Copy link
Member

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

dtolnay commented Feb 14, 2022

@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
Copy link
Member

@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
Copy link
Member

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.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 15, 2022
…oup, r=dtolnay

Move transmute_undefined_repr back to nursery

There's still open discussion if this lint is ready to be enabled by
default. We want to give us more time to figure this out and prevent
this lint from getting to stable as an enabled-by-default lint.

cc rust-lang/rust-clippy#8432

r? `@Manishearth` `@dtolnay`

I think this is the way to go here. We can re-enable this lint with the next sync, if we should decide to do so. But I would hold of for this release.

We have until Friday (beta branching) to decide if we want to merge this.
@Jarcho
Copy link
Contributor

Jarcho commented Feb 16, 2022

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

Tamschi commented Feb 16, 2022

[…]

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

Jarcho commented Feb 16, 2022

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

bors commented Feb 16, 2022

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

flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Feb 24, 2022
…olnay

Move transmute_undefined_repr back to nursery

There's still open discussion if this lint is ready to be enabled by
default. We want to give us more time to figure this out and prevent
this lint from getting to stable as an enabled-by-default lint.

cc rust-lang#8432

r? `@Manishearth` `@dtolnay`

I think this is the way to go here. We can re-enable this lint with the next sync, if we should decide to do so. But I would hold of for this release.

We have until Friday (beta branching) to decide if we want to merge this.
@flip1995
Copy link
Member

flip1995 commented Mar 2, 2022

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

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

bors commented Mar 2, 2022

📌 Commit fd9dd04 has been approved by Manishearth

@bors
Copy link
Collaborator

bors commented Mar 2, 2022

⌛ Testing commit fd9dd04 with merge 14f3d05...

@Manishearth
Copy link
Member

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

@bors
Copy link
Collaborator

bors commented Mar 2, 2022

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

@bors bors merged commit 14f3d05 into rust-lang:master Mar 2, 2022
@Manishearth
Copy link
Member

#8497

@dtolnay dtolnay deleted the transmuteundefinedrepr2 branch March 2, 2022 21:00
bors added a commit that referenced this pull request Mar 3, 2022
…ip1995

comment about transmute_undefined_repr in nursery

See discussion in #8432

changelog: none
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 3, 2022
Move transmute_undefined_repr back to nursery again

This PR reapplies rust-lang#94014, which was reverted unintentionally I think by rust-lang#94329. The combination of rust-lang/rust-clippy#8432 + rust-lang/rust-clippy#8497 in clippy should prevent this from happening again.

r? `@Manishearth`
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants