Make missing_copy_implementations more cautious
#102406
Conversation
rustbot
added
the
T-compiler
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
| // If the type contains a raw pointer, it may represent something like a handle, | ||
| // and recommending Copy might be a bad idea. | ||
| for field in def.all_fields() { | ||
| let did = field.did; | ||
| if cx.tcx.type_of(did).is_unsafe_ptr() { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
While I think the rationale makes sense, it seems very inconsistent to me for pointers to actually be Copy but then we don't treat them as Copy in this lint. I would prefer that we don't make this change without a larger consensus that this is the right approach to take.
There was a problem hiding this comment.
Pointers themselves are just locations in memory with no semantics attached. However, the moment a user puts a pointer into their own type, they can add additional semantics, e.g., that the type uniquely borrows the data behind the pointer. For that reason, the decision to make a type with a pointer Copy is probably best made on a case-by-case basis, IMHO.
There was a problem hiding this comment.
We shouldn't recommend implementing Copy if might have certain other semantics. I think it's a reasonable assumption that when raw pointers are involved that is the case.
Of course that can never be prevented, for example some kind of Token ZST not being Copy could be very intentional, but that is impossible to determine.
I'm happy to remove it if you disagree.
| // Default value of clippy::trivially_copy_pass_by_ref | ||
| const MAX_SIZE: u64 = 256; | ||
|
|
||
| if let Some(size) = cx.layout_of(ty).ok().map(|l| l.size.bytes()) { | ||
| if size > MAX_SIZE { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not quite sure how I feel about this. It seems very similar to the points raised in the discussion of #83518 in that we may want to allow users to control this value, we may want different values on different targets, etc.
However, since this controls silencing the lint instead of triggering the lint, I don't have an objection to landing this change.
|
Thanks for the discussion @LegionMammal978 and @mejrs! On further reflection, I agree with your POV so I'm ok with landing this. @bors r+ |
bors
added
S-waiting-on-bors
Make `missing_copy_implementations` more cautious - Fixes rust-lang#98348 - Also makes the lint not fire on large types and types containing raw pointers. Thoughts?
bors
added
S-waiting-on-author
|
@rustbot ready |
rustbot
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #103917) made this pull request unmergeable. Please resolve the merge conflicts. |
|
This just needs a rebase and then it's good to go! |
|
@rustbot ready |
|
@bors r=wesleywiser |
bors
added
S-waiting-on-bors
Make `missing_copy_implementations` more cautious - Fixes rust-lang#98348 - Also makes the lint not fire on large types and types containing raw pointers. Thoughts?
…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#102406 (Make `missing_copy_implementations` more cautious) - rust-lang#105265 (Add `rustc_on_unimplemented` to `Sum` and `Product` trait.) - rust-lang#105385 (Skip test on s390x as LLD does not support the platform) - rust-lang#105453 (Make `VecDeque::from_iter` O(1) from `vec(_deque)::IntoIter`) - rust-lang#105468 (Mangle "main" as "__main_void" on wasm32-wasi) - rust-lang#105480 (rustdoc: remove no-op mobile CSS `#sidebar-toggle { text-align }`) - rust-lang#105489 (Fix typo in apple_base.rs) - rust-lang#105504 (rustdoc: make stability badge CSS more consistent) - rust-lang#105506 (Tweak `rustc_must_implement_one_of` diagnostic output) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
missing_copy_implementationsprobably shouldn't fire if the type implementsIterator#98348