Don't lint lifetime-only transmutes #2759
Conversation
tests/ui/transmute.rs
Outdated
| // This is just modifying the lifetime, and is one of the recommended uses | ||
| // of transmute | ||
| let n = 1u32; | ||
| let _ = unsafe { std::mem::transmute::<&'_ u32, &'static u32>(&n) }; |
There was a problem hiding this comment.
Do we want transmute::<Foo<'a>, Foo<'b>> to also not lint?
There was a problem hiding this comment.
I think that's a good question. Are there reasonable cases where one of the transmuted lifetimes is not 'static? My suspicion is that the answer is yes but I've never run into one.
@sgrif, since you filed the original issue, do you have an opinion here?
There was a problem hiding this comment.
Yes, there's definitely use cases where neither lifetime is 'static. The main use case that I hit is literally Foo<'a> to Foo<'b> where 'b is known to be smaller than 'a, but is also unnameable. If Foo is invariant, this is an important use case.
There was a problem hiding this comment.
In all relevant cases for Diesel, 'static is not involved. 😄
https://github.com/diesel-rs/diesel/blob/a6b00b3cee355a314e3d2c6a83a772fea2fe5738/diesel/src/query_builder/debug_query.rs#L87-L94
https://github.com/diesel-rs/diesel/blob/a6b00b3cee355a314e3d2c6a83a772fea2fe5738/diesel/src/query_builder/ast_pass.rs#L106-L110
Interestingly, this is also our only use of mem::transmute which I didn't expect.
There was a problem hiding this comment.
@oli-obk In that case, I think transmute::<Foo<'a>, Foo<'b>> should not lint. Do you want me to add a test explicitly checking this?
devonhollowood
force-pushed
the
transmute-same-ptr-ptr
branch
from
d086e8d to
7d57aca
Compare
|
retriggering travis |
|
@phansch Thanks! |
|
Do you think it's worth adding a test for transmuting from |
|
@sgrif Oh sorry I had misunderstood the request earlier. That's an excellent idea, and this lint does indeed give a false positive for that currently. I'll poke around with this tonight to see if I can fix that. |
|
Whew, this was harder than expected! I think it should be working now. Let me know if there are any other test cases you want me to add. |
clippy_lints/src/transmute.rs
Outdated
| @@ -363,8 +364,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Transmute { | |||
| } | |||
| ) | |||
| } else { | |||
| // In this case they differ only in lifetime | |||
| if ty_from != ty_to { | |||
| if !differ_only_in_lifetime_params(from_ty, to_ty) { | |||
There was a problem hiding this comment.
There are some functions on TyCtxt which allow you to erase lifetimes. Maybe calling one of those has a similar effect?
There was a problem hiding this comment.
I don't know how I missed that... I've replaced differ_only_in_lifetime_params() with a couple calls to TyCtxt::erase_regions(). Good catch!
devonhollowood
force-pushed
the
transmute-same-ptr-ptr
branch
from
c9b6693 to
394cfed
Compare
|
Updated and rebased |
|
can you rebase again? there was a nightly update in between |
Now instead of reinventing the wheel with differ_only_in_lifetimes(), we use TyCtxt's erase_regions()
devonhollowood
force-pushed
the
transmute-same-ptr-ptr
branch
from
394cfed to
9118cd6
Compare
|
@oli-obk done |
|
👍 |
|
Will hopefully be able to test soon (A bug in Rust nightly prevents Diesel from compiling for the past 2 weeks) ❤️ |
|
@sgrif Let me know how it goes! |
Fix #2719