clippy fix: non_canonical_clone_impl

[hkBst]

Fixes:

warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/clone.rs:682:33
|
682 | fn clone(&self) -> Self {
| _________________________________^
683 | | self
684 | | }
| |_________^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
= note: requested on the command line with `-W clippy::non-canonical-clone-impl`
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/convert/mod.rs:935:35
|
935 | fn clone(&self) -> Infallible {
| ___________________________________^
936 | | match *self {}
937 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/marker.rs:856:29
|
856 | fn clone(&self) -> Self {
| _____________________________^
857 | | Self
858 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[scottmcm]

TBH, it's not really obvious to me that this is better for all these types.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Do you disagree, and think these are better? Or were you just doing what clippy said?

[hkBst]

TBH, it's not really obvious to me that this is better for all these types.

That is fair (and I don't necessarily disagree), but perhaps the wrong way to view this change.

My goal is to enable a broader set of clippy lints for the compiler, specifically all of clippy::suspicious. This would have prevented bugs like #146940.

But in order to get there, all the positives need to be dealt with in some way. The possibilities for that are:

• a simple code change (like this PR proposes)
• a change to the lint category (like multiple_bound_locations lint should not be in suspicious category rust-clippy#15736)
• a change to the lint behavior (empty_loop triggers on function prototypes in core rust-clippy#15200, crate_in_macro_def lints on a macro use rust-clippy#15620)
• an allow/expect annotation.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

My thinking was that the argument being Infallible is enough. If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

I'm intrigued about the MIR implications, but I must confess knowing almost nothing about MIR. If it is important, can you explain why?

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Again, I'm intrigued, but I'm not sure what you're getting at here, besides that you think this decreases the quality of the MIR.

Do you disagree, and think these are better?

I think that these changes are largely neutral by themselves, but they are a small step towards an end goal which does have positive value. I'm happy to discuss alternative ways of dealing with these.

Or were you just doing what clippy said?

I hope I have addressed this.

[scottmcm]

If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

No, because that's a panic!(). Having it be match *self {} is the safe way of getting unsafe { hint::unreachable_unchecked() }.

---

I think my inclination is that I don't really care for the reference and phantomdata examples, so merging that would be fine.

For the uninhabited type, though, I think clippy should change to not suggest this clone implementation, because the rule of "for something uninhabited, all the method implementations should be match self {} or match *self {}" should win. After all, that's what it is for Debug, for PartialEq, for ...

(Allowing would be fine for now since this PR probably shouldn't also change clippy.)

[hkBst]

Alright, I've removed the change to Infallible for now, and will look into a fix from the clippy side for that.

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[scottmcm]

@bors r+ rollup

[rust-bors]

📌 Commit 3a9981a has been approved by scottmcm

It is now in the queue for this repository.

[hkBst]

Fixes:

warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/clone.rs:682:33
|
682 | fn clone(&self) -> Self {
| _________________________________^
683 | | self
684 | | }
| |_________^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
= note: requested on the command line with `-W clippy::non-canonical-clone-impl`
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/convert/mod.rs:935:35
|
935 | fn clone(&self) -> Infallible {
| ___________________________________^
936 | | match *self {}
937 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/marker.rs:856:29
|
856 | fn clone(&self) -> Self {
| _____________________________^
857 | | Self
858 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[scottmcm]

TBH, it's not really obvious to me that this is better for all these types.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Do you disagree, and think these are better? Or were you just doing what clippy said?

[hkBst]

TBH, it's not really obvious to me that this is better for all these types.

That is fair (and I don't necessarily disagree), but perhaps the wrong way to view this change.

My goal is to enable a broader set of clippy lints for the compiler, specifically all of clippy::suspicious. This would have prevented bugs like #146940.

But in order to get there, all the positives need to be dealt with in some way. The possibilities for that are:

• a simple code change (like this PR proposes)
• a change to the lint category (like multiple_bound_locations lint should not be in suspicious category rust-clippy#15736)
• a change to the lint behavior (empty_loop triggers on function prototypes in core rust-clippy#15200, crate_in_macro_def lints on a macro use rust-clippy#15620)
• an allow/expect annotation.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

My thinking was that the argument being Infallible is enough. If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

I'm intrigued about the MIR implications, but I must confess knowing almost nothing about MIR. If it is important, can you explain why?

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Again, I'm intrigued, but I'm not sure what you're getting at here, besides that you think this decreases the quality of the MIR.

Do you disagree, and think these are better?

I think that these changes are largely neutral by themselves, but they are a small step towards an end goal which does have positive value. I'm happy to discuss alternative ways of dealing with these.

Or were you just doing what clippy said?

I hope I have addressed this.

[scottmcm]

If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

No, because that's a panic!(). Having it be match *self {} is the safe way of getting unsafe { hint::unreachable_unchecked() }.

---

I think my inclination is that I don't really care for the reference and phantomdata examples, so merging that would be fine.

For the uninhabited type, though, I think clippy should change to not suggest this clone implementation, because the rule of "for something uninhabited, all the method implementations should be match self {} or match *self {}" should win. After all, that's what it is for Debug, for PartialEq, for ...

(Allowing would be fine for now since this PR probably shouldn't also change clippy.)

[hkBst]

Alright, I've removed the change to Infallible for now, and will look into a fix from the clippy side for that.

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[scottmcm]

@bors r+ rollup

[rust-bors]

📌 Commit 3a9981a has been approved by scottmcm

It is now in the queue for this repository.

[hkBst]

Fixes:

warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/clone.rs:682:33
|
682 | fn clone(&self) -> Self {
| _________________________________^
683 | | self
684 | | }
| |_________^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
= note: requested on the command line with `-W clippy::non-canonical-clone-impl`
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/convert/mod.rs:935:35
|
935 | fn clone(&self) -> Infallible {
| ___________________________________^
936 | | match *self {}
937 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/marker.rs:856:29
|
856 | fn clone(&self) -> Self {
| _____________________________^
857 | | Self
858 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[scottmcm]

TBH, it's not really obvious to me that this is better for all these types.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Do you disagree, and think these are better? Or were you just doing what clippy said?

[hkBst]

TBH, it's not really obvious to me that this is better for all these types.

That is fair (and I don't necessarily disagree), but perhaps the wrong way to view this change.

My goal is to enable a broader set of clippy lints for the compiler, specifically all of clippy::suspicious. This would have prevented bugs like #146940.

But in order to get there, all the positives need to be dealt with in some way. The possibilities for that are:

• a simple code change (like this PR proposes)
• a change to the lint category (like multiple_bound_locations lint should not be in suspicious category rust-clippy#15736)
• a change to the lint behavior (empty_loop triggers on function prototypes in core rust-clippy#15200, crate_in_macro_def lints on a macro use rust-clippy#15620)
• an allow/expect annotation.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

My thinking was that the argument being Infallible is enough. If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

I'm intrigued about the MIR implications, but I must confess knowing almost nothing about MIR. If it is important, can you explain why?

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Again, I'm intrigued, but I'm not sure what you're getting at here, besides that you think this decreases the quality of the MIR.

Do you disagree, and think these are better?

I think that these changes are largely neutral by themselves, but they are a small step towards an end goal which does have positive value. I'm happy to discuss alternative ways of dealing with these.

Or were you just doing what clippy said?

I hope I have addressed this.

[scottmcm]

If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

No, because that's a panic!(). Having it be match *self {} is the safe way of getting unsafe { hint::unreachable_unchecked() }.

---

I think my inclination is that I don't really care for the reference and phantomdata examples, so merging that would be fine.

For the uninhabited type, though, I think clippy should change to not suggest this clone implementation, because the rule of "for something uninhabited, all the method implementations should be match self {} or match *self {}" should win. After all, that's what it is for Debug, for PartialEq, for ...

(Allowing would be fine for now since this PR probably shouldn't also change clippy.)

[hkBst]

Alright, I've removed the change to Infallible for now, and will look into a fix from the clippy side for that.

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[scottmcm]

@bors r+ rollup

[rust-bors]

📌 Commit 3a9981a has been approved by scottmcm

It is now in the queue for this repository.

[hkBst]

Fixes:

warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/clone.rs:682:33
|
682 | fn clone(&self) -> Self {
| _________________________________^
683 | | self
684 | | }
| |_________^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
= note: requested on the command line with `-W clippy::non-canonical-clone-impl`
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/convert/mod.rs:935:35
|
935 | fn clone(&self) -> Infallible {
| ___________________________________^
936 | | match *self {}
937 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/marker.rs:856:29
|
856 | fn clone(&self) -> Self {
| _____________________________^
857 | | Self
858 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[scottmcm]

TBH, it's not really obvious to me that this is better for all these types.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Do you disagree, and think these are better? Or were you just doing what clippy said?

[hkBst]

TBH, it's not really obvious to me that this is better for all these types.

That is fair (and I don't necessarily disagree), but perhaps the wrong way to view this change.

My goal is to enable a broader set of clippy lints for the compiler, specifically all of clippy::suspicious. This would have prevented bugs like #146940.

But in order to get there, all the positives need to be dealt with in some way. The possibilities for that are:

• a simple code change (like this PR proposes)
• a change to the lint category (like multiple_bound_locations lint should not be in suspicious category rust-clippy#15736)
• a change to the lint behavior (empty_loop triggers on function prototypes in core rust-clippy#15200, crate_in_macro_def lints on a macro use rust-clippy#15620)
• an allow/expect annotation.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

My thinking was that the argument being Infallible is enough. If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

I'm intrigued about the MIR implications, but I must confess knowing almost nothing about MIR. If it is important, can you explain why?

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Again, I'm intrigued, but I'm not sure what you're getting at here, besides that you think this decreases the quality of the MIR.

Do you disagree, and think these are better?

I think that these changes are largely neutral by themselves, but they are a small step towards an end goal which does have positive value. I'm happy to discuss alternative ways of dealing with these.

Or were you just doing what clippy said?

I hope I have addressed this.

[scottmcm]

If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

No, because that's a panic!(). Having it be match *self {} is the safe way of getting unsafe { hint::unreachable_unchecked() }.

---

I think my inclination is that I don't really care for the reference and phantomdata examples, so merging that would be fine.

For the uninhabited type, though, I think clippy should change to not suggest this clone implementation, because the rule of "for something uninhabited, all the method implementations should be match self {} or match *self {}" should win. After all, that's what it is for Debug, for PartialEq, for ...

(Allowing would be fine for now since this PR probably shouldn't also change clippy.)

[hkBst]

Alright, I've removed the change to Infallible for now, and will look into a fix from the clippy side for that.

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[scottmcm]

@bors r+ rollup

[rust-bors]

📌 Commit 3a9981a has been approved by scottmcm

It is now in the queue for this repository.

[hkBst]

Fixes:

warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/clone.rs:682:33
|
682 | fn clone(&self) -> Self {
| _________________________________^
683 | | self
684 | | }
| |_________^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
= note: requested on the command line with `-W clippy::non-canonical-clone-impl`
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/convert/mod.rs:935:35
|
935 | fn clone(&self) -> Infallible {
| ___________________________________^
936 | | match *self {}
937 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/marker.rs:856:29
|
856 | fn clone(&self) -> Self {
| _____________________________^
857 | | Self
858 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[scottmcm]

TBH, it's not really obvious to me that this is better for all these types.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Do you disagree, and think these are better? Or were you just doing what clippy said?

[hkBst]

TBH, it's not really obvious to me that this is better for all these types.

That is fair (and I don't necessarily disagree), but perhaps the wrong way to view this change.

My goal is to enable a broader set of clippy lints for the compiler, specifically all of clippy::suspicious. This would have prevented bugs like #146940.

But in order to get there, all the positives need to be dealt with in some way. The possibilities for that are:

• a simple code change (like this PR proposes)
• a change to the lint category (like multiple_bound_locations lint should not be in suspicious category rust-clippy#15736)
• a change to the lint behavior (empty_loop triggers on function prototypes in core rust-clippy#15200, crate_in_macro_def lints on a macro use rust-clippy#15620)
• an allow/expect annotation.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

My thinking was that the argument being Infallible is enough. If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

I'm intrigued about the MIR implications, but I must confess knowing almost nothing about MIR. If it is important, can you explain why?

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Again, I'm intrigued, but I'm not sure what you're getting at here, besides that you think this decreases the quality of the MIR.

Do you disagree, and think these are better?

I think that these changes are largely neutral by themselves, but they are a small step towards an end goal which does have positive value. I'm happy to discuss alternative ways of dealing with these.

Or were you just doing what clippy said?

I hope I have addressed this.

[scottmcm]

If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

No, because that's a panic!(). Having it be match *self {} is the safe way of getting unsafe { hint::unreachable_unchecked() }.

---

I think my inclination is that I don't really care for the reference and phantomdata examples, so merging that would be fine.

For the uninhabited type, though, I think clippy should change to not suggest this clone implementation, because the rule of "for something uninhabited, all the method implementations should be match self {} or match *self {}" should win. After all, that's what it is for Debug, for PartialEq, for ...

(Allowing would be fine for now since this PR probably shouldn't also change clippy.)

[hkBst]

Alright, I've removed the change to Infallible for now, and will look into a fix from the clippy side for that.

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[scottmcm]

@bors r+ rollup

[rust-bors]

📌 Commit 3a9981a has been approved by scottmcm

It is now in the queue for this repository.

[hkBst]

Fixes:

warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/clone.rs:682:33
|
682 | fn clone(&self) -> Self {
| _________________________________^
683 | | self
684 | | }
| |_________^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
= note: requested on the command line with `-W clippy::non-canonical-clone-impl`
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/convert/mod.rs:935:35
|
935 | fn clone(&self) -> Infallible {
| ___________________________________^
936 | | match *self {}
937 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/marker.rs:856:29
|
856 | fn clone(&self) -> Self {
| _____________________________^
857 | | Self
858 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[scottmcm]

TBH, it's not really obvious to me that this is better for all these types.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Do you disagree, and think these are better? Or were you just doing what clippy said?

[hkBst]

TBH, it's not really obvious to me that this is better for all these types.

That is fair (and I don't necessarily disagree), but perhaps the wrong way to view this change.

My goal is to enable a broader set of clippy lints for the compiler, specifically all of clippy::suspicious. This would have prevented bugs like #146940.

But in order to get there, all the positives need to be dealt with in some way. The possibilities for that are:

• a simple code change (like this PR proposes)
• a change to the lint category (like multiple_bound_locations lint should not be in suspicious category rust-clippy#15736)
• a change to the lint behavior (empty_loop triggers on function prototypes in core rust-clippy#15200, crate_in_macro_def lints on a macro use rust-clippy#15620)
• an allow/expect annotation.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

My thinking was that the argument being Infallible is enough. If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

I'm intrigued about the MIR implications, but I must confess knowing almost nothing about MIR. If it is important, can you explain why?

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Again, I'm intrigued, but I'm not sure what you're getting at here, besides that you think this decreases the quality of the MIR.

Do you disagree, and think these are better?

I think that these changes are largely neutral by themselves, but they are a small step towards an end goal which does have positive value. I'm happy to discuss alternative ways of dealing with these.

Or were you just doing what clippy said?

I hope I have addressed this.

[scottmcm]

If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

No, because that's a panic!(). Having it be match *self {} is the safe way of getting unsafe { hint::unreachable_unchecked() }.

---

I think my inclination is that I don't really care for the reference and phantomdata examples, so merging that would be fine.

For the uninhabited type, though, I think clippy should change to not suggest this clone implementation, because the rule of "for something uninhabited, all the method implementations should be match self {} or match *self {}" should win. After all, that's what it is for Debug, for PartialEq, for ...

(Allowing would be fine for now since this PR probably shouldn't also change clippy.)

[hkBst]

Alright, I've removed the change to Infallible for now, and will look into a fix from the clippy side for that.

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[scottmcm]

@bors r+ rollup

[rust-bors]

📌 Commit 3a9981a has been approved by scottmcm

It is now in the queue for this repository.

[hkBst]

Fixes:

warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/clone.rs:682:33
|
682 | fn clone(&self) -> Self {
| _________________________________^
683 | | self
684 | | }
| |_________^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
= note: requested on the command line with `-W clippy::non-canonical-clone-impl`
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/convert/mod.rs:935:35
|
935 | fn clone(&self) -> Infallible {
| ___________________________________^
936 | | match *self {}
937 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/marker.rs:856:29
|
856 | fn clone(&self) -> Self {
| _____________________________^
857 | | Self
858 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[scottmcm]

TBH, it's not really obvious to me that this is better for all these types.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Do you disagree, and think these are better? Or were you just doing what clippy said?

[hkBst]

TBH, it's not really obvious to me that this is better for all these types.

That is fair (and I don't necessarily disagree), but perhaps the wrong way to view this change.

My goal is to enable a broader set of clippy lints for the compiler, specifically all of clippy::suspicious. This would have prevented bugs like #146940.

But in order to get there, all the positives need to be dealt with in some way. The possibilities for that are:

• a simple code change (like this PR proposes)
• a change to the lint category (like multiple_bound_locations lint should not be in suspicious category rust-clippy#15736)
• a change to the lint behavior (empty_loop triggers on function prototypes in core rust-clippy#15200, crate_in_macro_def lints on a macro use rust-clippy#15620)
• an allow/expect annotation.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

My thinking was that the argument being Infallible is enough. If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

I'm intrigued about the MIR implications, but I must confess knowing almost nothing about MIR. If it is important, can you explain why?

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Again, I'm intrigued, but I'm not sure what you're getting at here, besides that you think this decreases the quality of the MIR.

Do you disagree, and think these are better?

I think that these changes are largely neutral by themselves, but they are a small step towards an end goal which does have positive value. I'm happy to discuss alternative ways of dealing with these.

Or were you just doing what clippy said?

I hope I have addressed this.

[scottmcm]

If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

No, because that's a panic!(). Having it be match *self {} is the safe way of getting unsafe { hint::unreachable_unchecked() }.

---

I think my inclination is that I don't really care for the reference and phantomdata examples, so merging that would be fine.

For the uninhabited type, though, I think clippy should change to not suggest this clone implementation, because the rule of "for something uninhabited, all the method implementations should be match self {} or match *self {}" should win. After all, that's what it is for Debug, for PartialEq, for ...

(Allowing would be fine for now since this PR probably shouldn't also change clippy.)

[hkBst]

Alright, I've removed the change to Infallible for now, and will look into a fix from the clippy side for that.

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[scottmcm]

@bors r+ rollup

[rust-bors]

📌 Commit 3a9981a has been approved by scottmcm

It is now in the queue for this repository.

[hkBst]

Fixes:

warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/clone.rs:682:33
|
682 | fn clone(&self) -> Self {
| _________________________________^
683 | | self
684 | | }
| |_________^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
= note: requested on the command line with `-W clippy::non-canonical-clone-impl`
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/convert/mod.rs:935:35
|
935 | fn clone(&self) -> Infallible {
| ___________________________________^
936 | | match *self {}
937 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl
warning: non-canonical implementation of `clone` on a `Copy` type
--> library/core/src/marker.rs:856:29
|
856 | fn clone(&self) -> Self {
| _____________________________^
857 | | Self
858 | | }
| |_____^ help: change this to: `{ *self }`
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#non_canonical_clone_impl

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[scottmcm]

TBH, it's not really obvious to me that this is better for all these types.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Do you disagree, and think these are better? Or were you just doing what clippy said?

[hkBst]

TBH, it's not really obvious to me that this is better for all these types.

That is fair (and I don't necessarily disagree), but perhaps the wrong way to view this change.

My goal is to enable a broader set of clippy lints for the compiler, specifically all of clippy::suspicious. This would have prevented bugs like #146940.

But in order to get there, all the positives need to be dealt with in some way. The possibilities for that are:

• a simple code change (like this PR proposes)
• a change to the lint category (like multiple_bound_locations lint should not be in suspicious category rust-clippy#15736)
• a change to the lint behavior (empty_loop triggers on function prototypes in core rust-clippy#15200, crate_in_macro_def lints on a macro use rust-clippy#15620)
• an allow/expect annotation.

Like here having the match emphasizes that this is unreachable better than just *self does, to me -- in MIR it's the difference between _0 = copy *_1; Return and Unreachable, for example.

My thinking was that the argument being Infallible is enough. If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

I'm intrigued about the MIR implications, but I must confess knowing almost nothing about MIR. If it is important, can you explain why?

And for phantomdata just being _0 = PhantomData; is more obviously address-uncaring than _0 = copy *_1; is.

Again, I'm intrigued, but I'm not sure what you're getting at here, besides that you think this decreases the quality of the MIR.

Do you disagree, and think these are better?

I think that these changes are largely neutral by themselves, but they are a small step towards an end goal which does have positive value. I'm happy to discuss alternative ways of dealing with these.

Or were you just doing what clippy said?

I hope I have addressed this.

[scottmcm]

If it is important to emphasize this, then perhaps unreachable!() would be the better implementation?

No, because that's a panic!(). Having it be match *self {} is the safe way of getting unsafe { hint::unreachable_unchecked() }.

---

I think my inclination is that I don't really care for the reference and phantomdata examples, so merging that would be fine.

For the uninhabited type, though, I think clippy should change to not suggest this clone implementation, because the rule of "for something uninhabited, all the method implementations should be match self {} or match *self {}" should win. After all, that's what it is for Debug, for PartialEq, for ...

(Allowing would be fine for now since this PR probably shouldn't also change clippy.)

[hkBst]

Alright, I've removed the change to Infallible for now, and will look into a fix from the clippy side for that.

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[scottmcm]

@bors r+ rollup

[rust-bors]

📌 Commit 3a9981a has been approved by scottmcm

It is now in the queue for this repository.