Skip to content

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

Merged
bors merged 1 commit into
rust-lang:masterfrom
fmease:neg-copy-rules-out-missing-copy-impl
Aug 5, 2023
Merged

Make lint missing-copy-implementations honor negative Copy impls#114248
bors merged 1 commit into
rust-lang:masterfrom
fmease:neg-copy-rules-out-missing-copy-impl

Conversation

@fmease

@fmease fmease commented Jul 30, 2023

Copy link
Copy Markdown
Member

Fixes #101980.

@rustbot label A-lint F-negative_impls

@rustbot

rustbot commented Jul 30, 2023

Copy link
Copy Markdown
Collaborator

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
Comment thread tests/ui/lint/missing-copy-implementations-negative-copy.rs Outdated

@fmease fmease Jul 30, 2023

Copy link
Copy Markdown
Member Author

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
Comment thread compiler/rustc_lint/src/builtin.rs Outdated

@b-naber b-naber Aug 1, 2023

Copy link
Copy Markdown
Contributor

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?

@fmease fmease Aug 2, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

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?

@b-naber b-naber Aug 2, 2023

Copy link
Copy Markdown
Contributor

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.

@fmease fmease Aug 3, 2023

Copy link
Copy Markdown
Member Author

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

lqd commented Aug 3, 2023

Copy link
Copy Markdown
Member

cc @spastorino for negative impls change

@spastorino

spastorino commented Aug 4, 2023

Copy link
Copy Markdown
Member

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

b-naber commented Aug 5, 2023

Copy link
Copy Markdown
Contributor

@bors r+ rollup

@bors

bors commented Aug 5, 2023

Copy link
Copy Markdown
Collaborator

📌 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