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

Make missing_copy_implementations more cautious #102406

Merged
merged 3 commits into from
Dec 10, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ use rustc_span::source_map::Spanned;
use rustc_span::symbol::{kw, sym, Ident, Symbol};
use rustc_span::{BytePos, InnerSpan, Span};
use rustc_target::abi::{Abi, VariantIdx};
use rustc_trait_selection::traits::{self, misc::can_type_implement_copy};
use rustc_trait_selection::infer::{InferCtxtExt, TyCtxtInferExt};
use rustc_trait_selection::traits::{self, misc::can_type_implement_copy, EvaluationResult};

use crate::nonstandard_style::{method_context, MethodLateContext};

Expand Down Expand Up @@ -750,10 +751,39 @@ impl<'tcx> LateLintPass<'tcx> for MissingCopyImplementations {
if def.has_dtor(cx.tcx) {
return;
}

// 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;
}
}
Comment on lines +755 to +762
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let param_env = ty::ParamEnv::empty();
if ty.is_copy_modulo_regions(cx.tcx, param_env) {
return;
}

// We shouldn't recommend implementing `Copy` on stateful things,
// such as iterators.
if let Some(iter_trait) = cx.tcx.get_diagnostic_item(sym::Iterator) {
if cx.tcx.infer_ctxt().build().type_implements_trait(iter_trait, [ty], param_env)
== EvaluationResult::EvaluatedToOk
{
return;
}
}

// 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;
}
}
Comment on lines +778 to +785
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if can_type_implement_copy(
cx.tcx,
param_env,
Expand Down
35 changes: 35 additions & 0 deletions src/test/ui/lint/lint-missing-copy-implementations-allow.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// check-pass
#![deny(missing_copy_implementations)]

// Don't recommend implementing Copy on something stateful like an iterator.
pub struct MyIterator {
num: u8,
}

impl Iterator for MyIterator {
type Item = u8;

fn next(&mut self) -> Option<Self::Item> {
todo!()
}
}

pub struct Handle {
inner: *mut (),
}

pub struct Handle2 {
inner: *const (),
}

pub enum MaybeHandle {
Ptr(*mut ()),
}

pub union UnionHandle {
ptr: *mut (),
}

pub struct Array([u8; 2048]);

fn main() {}