Preliminary Changes for Updating Cargo #32547
Conversation
apfitzge
force-pushed
the
2023-07-19/cargo_upgrade_prelim
branch
from
c3fc19b
to
b00cc2e
Compare
Codecov Report
@@ Coverage Diff @@
## master #32547 +/- ##
=========================================
- Coverage 82.0% 82.0% -0.1%
=========================================
Files 780 780
Lines 210789 210782 -7
=========================================
- Hits 172985 172910 -75
- Misses 37804 37872 +68 |
| @@ -426,6 +426,7 @@ pub fn derive_clone_zeroed(input: proc_macro::TokenStream) -> proc_macro::TokenS | |||
| let name = &item_struct.ident; | |||
| quote! { | |||
| impl Clone for #name { | |||
| #[allow(clippy::incorrect_clone_impl_on_copy_type)] | |||
There was a problem hiding this comment.
Could you add a comment indicating (1) why this is needed, and (2) when it can be removed/re-evaluated?
I think it'll be helpful to have that information here in the code, in case it becomes hard to find this PR later.
There was a problem hiding this comment.
So this lint: https://rust-lang.github.io/rust-clippy/master/index.html#/incorrect_clone_impl_on_copy_type
If both Clone and Copy are implemented, they must agree. This is done by dereferencing self in Clone’s implementation. Anything else is incorrect.
In this case, we are "manually" implementing Clone to make sure all padding bytes are zeroed out.
Based on that description I don't think we'll ever be able to remove the allow, since it's a correct flagging of this scenario.
IIRC rust makes no guarantees for Copy in terms of padding bytes bein zero (probably not since it's just a memcpy?) - would defer to @Lichtso since he's the one who added the macro.
In anycase I will add a comment on why this allow is there/necessary.
There was a problem hiding this comment.
We want a custom copy operation. Probably should derive both copy and clone, I guess.
There was a problem hiding this comment.
It's not possible to derive a custom Copy in rust as far as I am aware - it's a special trait that effectively allows types to be memcpy'd
There was a problem hiding this comment.
Re-thinking about this, I'm not sure we even want these types to be Copy. It's probably safer to only support Clone w/ our zeroed padding stuff.
Otherwise it doesn't really protect us from sending something over the network w/ non-zero padding because we could have just copied our value instead of cloning?
There was a problem hiding this comment.
We can try unimplementing Copy for these types, but that might result in a lot of .clone() where implicit deref happened before.
There was a problem hiding this comment.
👍 I think that sounds reasonable. I'm going to add this clippy-lint allow for now, and just self-assign an issue for experimenting with removing Copy.
Without removing Copy this allow is necessary to make clippy happy.
There was a problem hiding this comment.
Lgtm. The #[allow(clippy::incorrect_clone_impl_on_copy_type)] is the one interesting change, but isn't a change of behavior from what we already have. Should we want/need to change this later, we always can.
With that, I'm fine to merge the PR. If you'd like to wait for @Lichtso, that's also fine by me.
Problem
We've been sitting on an oldish version of Cargo that spits out a ton of warning on nightly. This is extremely frustrating as we need to look more carefully at our output warning to make sure they are not from changes we made, instead of just checking if there are warnings.
Summary of Changes
Wanted to begin tackling some of the clippy warnings and errors that have been introduced since our cargo freeze. This is a step towards upgrading, but does not include fixes necessary to upgrade.
PhantomDataincorrect_clone_impl_on_copy_typedoesn't like ifClone != CopyforCopyable types. Ignore this lint on ourCloneZeroedmacro.NONEwithempty().useless_vecwhich detects usage ofvec![...]where we could just use[...].Fixes #