Move borrow_as_ptr lint in the suspicious category
#10892
Conversation
|
r? @llogiq (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
☔ The latest upstream changes (presumably #10921) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Sorry for taking so long to review. I worry about potential fallout for this lint, so r=me after rebase if lintcheck confirms it's not too bad. |
|
ReportsStats:
I didn't go through any of those to see if they are legit or FPs. |
|
// src/random_state.rs:142
counter: AtomicUsize::new(&PI as *const _ as usize),
// src/lib.rs:180
let params = &slices[..num_params] as *const [MaybeUninit<&[u8]>] as *const [&[u8]];
// src/array_string.rs:126
.copy_to_nonoverlapping(&mut vec.xs as *mut [MaybeUninit<u8>; CAP], 1);
// src/arrayvec.rs:760
(&*array as *const [T; CAP] as *const [MaybeUninit<T>; CAP])
// src/arrayvec.rs:761
.copy_to_nonoverlapping(&mut vec.xs as *mut [MaybeUninit<T>; CAP], 1);
// src/backtrace/libunwind.rs:93
uw::_Unwind_Backtrace(trace_fn, &mut cb as *mut _ as *mut _);
// src/symbolize/gimli/libs_dl_iterate_phdr.rs:15
libc::dl_iterate_phdr(Some(callback), &mut ret as *mut Vec<_> as *mut _);
// src/de/impl_core.rs:184
(&array as *const _ as *const [T; N]).read()
// src/de/impls.rs:454:23
let ptr = &mut buf as *mut _ as *mut [T; N];
// src/de/impls.rs:485:23
let ptr = &mut buf as *mut _ as *mut [T; N];
// clang.rs:495
let data = &mut visitor as *mut Visitor;
// src/lib.rs:339
data: unsafe { NonNull::new_unchecked(&EMPTY_CHUNK as *const EmptyChunkFooter as *mut u8) },
// src/lib.rs:343
NonNull::new_unchecked(&EMPTY_CHUNK as *const EmptyChunkFooter as *mut u8)
// src/lib.rs:349
NonNull::new_unchecked(&EMPTY_CHUNK as *const EmptyChunkFooter as *mut ChunkFooter)
// src/lib.rs:2002
let bytes = *(&n.to_be() as *const u64 as *const [u8; 8]);
// src/lib.rs:2016
let bytes = *(&n.to_be() as *const u128 as *const [u8; 16]);
// src/lib.rs:2107
*n = *(&int.to_be() as *const u32 as *const f32);
// src/lib.rs:2119
*n = *(&int.to_be() as *const u64 as *const f64);
// src/lib.rs:2188
let bytes = *(&n.to_le() as *const u64 as *const [u8; 8]);
// src/lib.rs:2198
let bytes = *(&n.to_le() as *const u128 as *const [u8; 16]);
// src/lib.rs:2285
*n = *(&int.to_le() as *const u32 as *const f32);
// src/lib.rs:2297
*n = *(&int.to_le() as *const u64 as *const f64);I'm stopping here since they all seem to be the same. Based on that, what do you want me to do? |
|
That is more fallout than I expected. It appears the pattern is pretty common. So even if the lint offers a valid and worthy suggestion, I think we should at least be careful how we communicate the benefits. "Better readability" is not a good reason to put errors or warnings into many Rustacean's CI builds. So I'd like a second opinion on this before merging. cc @rust-lang/clippy |
|
Yeah this is still quite tricky. I don't see this being a positive change at the time, there are legit risks around materializing references where you shouldn't, but if you're in a situation where you should, it doesn't really seem more readable to me. |
|
The current group is |
|
I also think that we must weigh the cost and benefits here, and the scale currently doesn't tip in favor of making the lint suspicious. If we somehow manage to restrict the lint to only warn on actually problematic cases, it would probably even warrant a correctness lint. Unfortunately detecting such cases is much harder than what the lint currently does. |
|
Let's close it then! Thanks for your feedback everyone. |
Part of #10872.
This lint should warn by default as it actually allows to uncover potentially problematic reference to pointer conversions. To do so, I moved it into the
suspiciouscategory. I also used this opportunity to extend its tests.The second issue described in the issue doesn't seem to be covered by the current lint implementation though.