Update to Rust 1.67 #17378
Update to Rust 1.67 #17378
Conversation
`ResolvedObjectName` types are larger than 128 bytes, which made clippy complain about the size of the `PlanError` enum. Solved by boxing occurences of this type within `PlanError`.
src/persist-client/src/lib.rs
Outdated
| @@ -580,7 +580,6 @@ impl PersistClient { | |||
|
|
|||
| /// Test helper for a [Self::open] call that is expected to succeed. | |||
| #[cfg(test)] | |||
| #[track_caller] | |||
There was a problem hiding this comment.
will it always be the case that it doesn't have an effect. If it's meant to have an effect in the future it might be better to keep them and add an allow. cc @danhhz
There was a problem hiding this comment.
Hopefully not! rust-lang/rust#87417 is the tracking issue for making it do something.
I was wondering the same thing, and the more I think about it, the more I believe you are right and we should add an allow here.
`#[track_caller]` has no effect on async functions currently, and the compiler warns about that. However, fixing this is in progress ([0]). Rather than removing the annotations from async functions, we instead ignore the compiler warning, so we don't forget to add the annotations again once [0] is implemented. [0]: rust-lang/rust#87417
Assigning the `JoinHandle` returned by `spawn` to `_` triggers a the clippy lint `let_underscore_future`. This is a known FP (see rust-lang/rust-clippy#9932). The easiest way to avoid it is to not do that assignment.
This requires us to silence a new clippy lint, `clippy::unchecked_duration_subtraction`. While the lint triggering is not an FP, it is not very helpful either in this case. Substracting a small tick duration from the current time will never underflow, and if it did we probably would like to panic anyway. Note that the lint seems to be "allow by default" on the current clippy main, so we should be able to remove the explicit #[allow] again in the future.
| @@ -125,7 +125,7 @@ where | |||
| }; | |||
|
|
|||
| let service = Arc::clone(&self.service); | |||
| let _ = mz_ore::task::spawn(|| "maelstrom::handle".to_string(), async move { | |||
| mz_ore::task::spawn(|| "maelstrom::handle".to_string(), async move { | |||
There was a problem hiding this comment.
sorry to be a bother, but what's the new warning here?
There was a problem hiding this comment.
There was a problem hiding this comment.
Futures must be polled for work to be done.
ah, what an odd interaction between the lint and the one case this isn't true
There was a problem hiding this comment.
oh you had this in the PR description 🤦, sorry!
There was a problem hiding this comment.
All good!
There is some discussion here, in case you haven't seen it: rust-lang/rust-clippy#9932. I would be fine with simply disabling this lint, if you'd rather keep the let _ =. Although it generally seems to be a good thing to have. I think the other lint that prevents us from forgetting to await futures is the one that checks for #[must_use], and that is disabled by assigning the return value.
There was a problem hiding this comment.
yeah, I was just reading that. it was indeed here for exactly the reason someone in that thread points out, but it's marginal enough that I don't particularly care
The let _ is indeed not strictly necessary (JoinHandle isn't must_use), however I do find it serves a legitimate documentation goal, it signals to the reader that the return value of spawn is intentionally dropped, whereas with just spawn there's a doubt left of whether the original developer simply forgot, or not.
It can be handled differently: comments, calling mem::drop, ... let _ = is the idiomatic way of signalling "I don't care about the return value", though.
| @@ -125,7 +125,7 @@ where | |||
| }; | |||
|
|
|||
| let service = Arc::clone(&self.service); | |||
| let _ = mz_ore::task::spawn(|| "maelstrom::handle".to_string(), async move { | |||
| mz_ore::task::spawn(|| "maelstrom::handle".to_string(), async move { | |||
There was a problem hiding this comment.
Futures must be polled for work to be done.
ah, what an odd interaction between the lint and the one case this isn't true
|
TFTRs! |
This PR updates the Rust version to 1.67.
There are a couple new lints that trigger on our code now, which are fixed/silenced in separate commits.
result_large_err: Fixed by boxing the offending error variant types.#[track_caller]on async functions is a no-op" (compiler warning):Fixed by removing the offendingSilenced by adding a manual#[track_caller]annotations.#[allow].let_underscore_future: These are FPs ontask::spawn. Avoided by removing thelet _bindings.unchecked_duration_subtraction: Silenced by adding a manual#[allow].Motivation
Tips for reviewer
Look at the commits separately!
Most of these lint failures can be solved differently (e.g. by globally ignoring the lint). I don't have strong feelings to any of the fixes in this PR, so please LMK if you'd prefer something different!
Checklist
This PR has adequate test coverage / QA involvement has been duly considered.
This PR evolves an existing
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way) and therefore is tagged with aT-protolabel.If this PR will require changes to cloud orchestration, there is a
companion cloud PR to account for those changes that is tagged with
the release-blocker label (example).
This PR includes the following user-facing behavior changes: N/A