Skip to content
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.

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

round two preliminaries for bumping nightly to 2023-08-25 #33064

Merged
merged 2 commits into from
Aug 30, 2023

Conversation

t-nelson
Copy link
Contributor

Problem

i left these off of #33047 since they might stir contention. the remainder of compatibility changes for bumping toolchain(s) need to go in with the bump as they reference lints that the current toolchain(s) don't know about

Summary of Changes

  • work around nightly ice triggered by something something const ref in generic context
  • allow clippy::needless_pass_by_ref_mut to avoid false-positive ignoring trait bounds

seems to be something related to `const ref`s in generic contexts...
@t-nelson t-nelson marked this pull request as ready for review August 30, 2023 06:39
//
// allow lint false positive trait bound requirement (`CryptoRng` only
// implemented on `&'a mut T`
#[rustversion::attr(since(1.73), allow(clippy::needless_pass_by_ref_mut))]
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seems like a false positive to me. The rng param seems wrong, or at least very weird, here.

It doesn't need to be mut rng: &'a mut R, it should be rng: &'a mut R and just passed as rng to Ping::new_rand(rng,...)`

Currently we're passing a &mut &mut R to Ping::new_rand which is probably immediately deref'd, but because we have &mut rng it's forcing us to have mut rng?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I pulled the branch and even with those changes it still flags it falsely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't need to be mut rng: &'a mut R, it should be rng: &'a mut R

haha, yeah i tried that too 🙃

Copy link
Contributor

@apfitzge apfitzge left a comment

Choose a reason for hiding this comment

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

The ICE work-around is imo probably a good change anyway.

Other false flag, potentially we should add an issue so we can remove that in our next nightly upgrade?

Changes themselves seem fine to me to get us to a newer nightly.

@t-nelson
Copy link
Contributor Author

Other false flag, potentially we should add an issue so we can remove that in our next nightly upgrade?

yeah i'm about to work on minimal repro to report upstream

@Lichtso Lichtso merged commit b13589b into solana-labs:master Aug 30, 2023
18 checks passed
@t-nelson t-nelson deleted the pnb2 branch August 30, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants