Preliminary Changes for Updating Cargo

[apfitzge]

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.

• ee65ea3 - don't need to call default() here to create PhantomData
• 040b0f7 - new lint incorrect_clone_impl_on_copy_type doesn't like if Clone != Copy for Copyable types. Ignore this lint on our CloneZeroed macro.
• 1245cef - bitflags doesn't work as expected with Zero-flags. This is even a listed edge-case in the docs https://docs.rs/bitflags/latest/bitflags/#zero-flags. I replaced the single use of the NONE with empty().
• b00cc2e - new lint useless_vec which detects usage of vec![...] where we could just use [...].

Fixes #

[codecov]

Codecov Report

Merging #32547 (9b08354) into master (3fbfac4) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

@@            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     

[brooksprumo]

Thanks for tackling these! I had started on various rust 1.71.0 clippy fixes, but didn't get to all of them.

We may end up having to wait until rust 1.72.0 to resolve the cargo fmt issue w.r.t. let-else, but maybe we can at least bump nightly a bit more.

[brooksprumo]

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.

[apfitzge]

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.

[apfitzge]

9b08354

[Lichtso]

We want a custom copy operation. Probably should derive both copy and clone, I guess.

[apfitzge]

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

[apfitzge]

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?

[Lichtso]

We can try unimplementing Copy for these types, but that might result in a lot of .clone() where implicit deref happened before.

[apfitzge]

👍 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.

[apfitzge]

#32588

[brooksprumo]

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.