Skip to content

Remove the missing_copy_implementations lint. #126293

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

Conversation

nnethercote
Copy link
Contributor

This lint made a certain amount of sense for people writing code during the period leading up to to Rust 1.0. It doesn't make much sense today. There are many valid cases of types that could impl Copy but do not. The lint is extremely low value and moderately complicated. This commit removes it.

r? @lcnr

This lint made a certain amount of sense for people writing code during
the period leading up to to Rust 1.0. It doesn't make much sense today.
There are many valid cases of types that could impl `Copy` but do not.
The lint is extremely low value and moderately complicated. This commit
removes it.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jun 12, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 12, 2024

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

@nnethercote
Copy link
Contributor Author

Like #126018, this will need an FCP.

@scottmcm scottmcm removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 12, 2024
@lcnr
Copy link
Contributor

lcnr commented Jun 12, 2024

r=me on the code changes themselves, however, I am less certain about removing this lint.

I definitely wouldn't be in favor of adding it to rustc right now, but unlike box_pointers, I can at least imagine a somewhat coherent argument for why this lint should exist. There are some people who use it https://github.com/search?q=missing_copy_implementations+language%3ARust&type=code&l=Rust&p=3.

otoh, It does have false positives and if people are unhappy about us removing it, it can get added to clippy (where it belongs)

@rfcbot fcp merge

@rfcbot
Copy link
Collaborator

rfcbot commented Jun 12, 2024

Team member @lcnr has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 12, 2024
@nagisa
Copy link
Member

nagisa commented Jun 12, 2024

Question is if we can add this to clippy from the get-go (are they interested in maintaining such a lint at all? is it technically feasible? is there something that makes this lint uniquely easier to implement in rustc?)

@petrochenkov
Copy link
Contributor

There's a very similar lint called missing_debug_implementations, do you plan to remove it too?

There are many valid cases of types that could impl Copy but do not.

Many of them are private though.
Both missing Copy and missing Debug are supposed to be used by people writing libraries with a stable public interface, and the lints only apply to that public interface.
That seems useful enough.

I'd be fine with putting these lints into clippy if the lints were new.
But since the lints are already in the compiler for long time, then it's better to keep things as is and not break existing code.

@nnethercote
Copy link
Contributor Author

There's a very similar lint called missing_debug_implementations, do you plan to remove it too?

No. I think the usefulness for public APIs is more obvious there. The motivation for this PR was the fact that the lint description talks about this lint existing to assist with a pre-1.0 behavioural change, which seems to not be of much relevance any more.

@nnethercote
Copy link
Contributor Author

The API guidelines on interoperability say "crates that define new types should eagerly implement all applicable, common traits" and specifically names Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Debug, Display, and Default.

The API guidelines on debuggability say "All public types implement Debug" and "If there are exceptions, they are rare."

A couple of possible reactions to that.

  • Why do Debug and Copy get a lint when none of the other traits do?
  • Having said that, Debug gets mentioned twice, and with arguably stronger language the second time, so if any single trait deserves a lint maybe it's the one?

@bors
Copy link
Collaborator

bors commented Jun 26, 2024

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

@jhpratt
Copy link
Member

jhpratt commented Jun 26, 2024

I have personally found this lint to be useful in the past. I don't agree with the motivation at all.

@lcnr lcnr added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 22, 2024
@apiraino
Copy link
Contributor

apiraino commented Jul 25, 2024

@rfcbot concern lint-actually-useful

From comment: I have personally found this lint to be useful in the past. I don't agree with the motivation at all

@oli-obk
Copy link
Contributor

oli-obk commented Jul 25, 2024

@rfcbot concern lint-actually-useful

From https://github.com/rust-lang/rust/pull/126293#issuecomment-2192690954: I have personally found this lint to be useful in the past. I don't agree with the motivation at all

bot wasn't listening

@nnethercote
Copy link
Contributor Author

There is enough opposition to this that I will close it.

@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.