Skip to content

Make lint missing-copy-implementations honor negative Copy impls #114248

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.

Sign up for GitHub

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

Jump to bottom
Merged
bors merged 1 commit into from
Aug 5, 2023
Merged

Make lint missing-copy-implementations honor negative Copy impls #114248

bors merged 1 commit into from
Aug 5, 2023

Conversation

@fmease
Copy link
Member

@fmease fmease commented Jul 30, 2023

Fixes #101980.

@rustbot label A-lint F-negative_impls

@rustbot
Copy link
Collaborator

rustbot commented Jul 30, 2023

r? @b-naber

(rustbot has picked a reviewer for you, use r? to override)

@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. A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. F-negative_impls #![feature(negative_impls)] labels Jul 30, 2023
fmease
fmease commented Jul 30, 2023
View reviewed changes
tests/ui/lint/missing-copy-implementations-negative-copy.rs Outdated
Copy link
Member Author

@fmease fmease Jul 30, 2023

Choose a reason for hiding this comment

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

I haven't tested conditional negative Copy impls since those are broken anyway I think? See #79098.

b-naber
b-naber reviewed Aug 1, 2023
View reviewed changes
compiler/rustc_lint/src/builtin.rs Outdated
Copy link
Contributor

@b-naber b-naber Aug 1, 2023

Choose a reason for hiding this comment

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

Can we use predicate_may_hold here instead?

Copy link
Member Author

@fmease fmease Aug 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Now using predicate_may_hold. That's much shorter and more legible, thanks for the suggestion!

However, I'm wondering if that could yield too many false positives since we're now considering ambiguous negative impls to be applicable (as well as ones that apply modulo opaque types).
Wouldn't predicate_must_hold_modulo_regions be a better fit?

Copy link
Contributor

@b-naber b-naber Aug 2, 2023

Choose a reason for hiding this comment

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

Yes, sorry about that. I think you're correct, predicate_must_hold_modulo_regions is the right method to use here I think.

Copy link
Member Author

@fmease fmease Aug 3, 2023

Choose a reason for hiding this comment

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

Done

@fmease fmease force-pushed the neg-copy-rules-out-missing-copy-impl branch 2 times, most recently from 0fa45de to a0d8d28 Compare August 2, 2023 23:13
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@fmease fmease force-pushed the neg-copy-rules-out-missing-copy-impl branch from a0d8d28 to 2b8a3b4 Compare August 2, 2023 23:38
@lqd
Copy link
Member

lqd commented Aug 3, 2023

cc @spastorino for negative impls change

@spastorino
Copy link
Member

spastorino commented Aug 4, 2023

cc @spastorino for negative impls change

I'm working on different things related to negative impls but it makes sense to me that this lint honor negative Copy impls.

@b-naber
Copy link
Contributor

b-naber commented Aug 5, 2023

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Aug 5, 2023

📌 Commit 2b8a3b4 has been approved by b-naber

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 5, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 5, 2023
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 5, 2023
@matthiaskrgr
Rollup merge of rust-lang#114248 - fmease:neg-copy-rules-out-missing-…
2d142b4 
…copy-impl, r=b-naber

Make lint missing-copy-implementations honor negative `Copy` impls

Fixes rust-lang#101980.

`@rustbot` label A-lint F-negative_impls
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 5, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2023
@bors
Auto merge of rust-lang#114514 - matthiaskrgr:rollup-1rv4f3h, r=matth…
fbc11e9 
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#114029 (Explain more clearly why `fn() -> T` can't be `#[derive(Clone)]`)
 - rust-lang#114248 (Make lint missing-copy-implementations honor negative `Copy` impls)
 - rust-lang#114498 (Print tidy command with bless tidy check failure)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e722f6f into rust-lang:master Aug 5, 2023
@rustbot rustbot added this to the 1.73.0 milestone Aug 5, 2023
@fmease fmease deleted the neg-copy-rules-out-missing-copy-impl branch August 5, 2023 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@b-naber b-naber b-naber left review comments

Assignees

@b-naber b-naber

Labels

A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. F-negative_impls #![feature(negative_impls)] S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.73.0

Development

Successfully merging this pull request may close these issues.

The allow-by-default lint missing_copy_implementations does not honor !Copy impls

7 participants

@fmease @rustbot @rust-log-analyzer @lqd @spastorino @b-naber @bors