Skip to content

Update to Rust 1.67 #17378

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 27, 2023
Merged

Update to Rust 1.67 #17378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f492f39
sql: box `ResolvedObjectName` in `PlanError`
teskje Jan 27, 2023
0cca9c8
persist-client: ignore warning about `#[track_caller]`
teskje Jan 27, 2023
9178bfb
storage: don't assign JoinHandles to `_`
teskje Jan 27, 2023
22f044a
Bump Rust version to 1.67
teskje Jan 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ resolver = "2"

[workspace.package]
edition = "2021"
rust-version = "1.66.0"
rust-version = "1.67.0"

[profile.dev]
# TODO(gusywnn|benesch): remove this when incremental ice's are improved
Expand Down
4 changes: 2 additions & 2 deletions src/persist-client/examples/maelstrom/node.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry to be a bother, but what's the new warning here?

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's https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_future.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

oh you had this in the PR description 🤦, sorry!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

let service = service.get().await;
let () = service.eval(handle, msg.src, body).await;
});
Expand Down Expand Up @@ -164,7 +164,7 @@ where
// the AsyncInitOnceWaitable nonsense.
let args = self.args.clone();
let service_init = Arc::clone(&self.service);
let _ = mz_ore::task::spawn(|| "maelstrom::init".to_string(), async move {
mz_ore::task::spawn(|| "maelstrom::init".to_string(), async move {
let service = match S::init(&args, &handle).await {
Ok(x) => x,
Err(err) => {
Expand Down
2 changes: 1 addition & 1 deletion src/persist-client/src/internal/compact.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ where
let compact_span =
debug_span!(parent: None, "compact::apply", shard_id=%machine.shard_id());
compact_span.follows_from(&Span::current());
let _ = mz_ore::task::spawn(|| "PersistCompactionWorker", async move {
mz_ore::task::spawn(|| "PersistCompactionWorker", async move {
let res = Self::compact_and_apply(
cfg,
blob,
Expand Down
4 changes: 4 additions & 0 deletions src/persist-client/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@

#![doc = include_str!("../README.md")]
#![warn(missing_docs, missing_debug_implementations)]
// #[track_caller] is currently a no-op on async functions, but that hopefully won't be the case
// forever. So we already annotate those functions now and ignore the compiler warning until
// https://github.com/rust-lang/rust/issues/87417 pans out.
#![allow(ungated_async_fn_track_caller)]

use std::fmt::Debug;
use std::marker::PhantomData;
Expand Down
2 changes: 1 addition & 1 deletion src/persist-client/src/read.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -933,7 +933,7 @@ where
//
// Intentionally create the span outside the task to set the parent.
let expire_span = debug_span!("drop::expire");
let _ = handle.spawn_named(
handle.spawn_named(
|| format!("ReadHandle::expire ({})", self.reader_id),
async move {
machine.expire_leased_reader(&reader_id).await;
Expand Down
2 changes: 1 addition & 1 deletion src/persist-client/src/write.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,7 +739,7 @@ where
//
// Intentionally create the span outside the task to set the parent.
let expire_span = debug_span!("drop::expire");
let _ = handle.spawn_named(
handle.spawn_named(
|| format!("WriteHandle::expire ({})", self.writer_id),
async move {
machine.expire_writer(&writer_id).await;
Expand Down
9 changes: 5 additions & 4 deletions src/sql/src/names.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1127,11 +1127,12 @@ impl<'a> Fold<Raw, Aug> for NameResolver<'a> {
ResolvedObjectName::Object { id, .. } => {
let item = self.catalog.get_item(id);
if item.item_type() != CatalogItemType::Secret {
self.status = Err(PlanError::InvalidSecret(object_name.clone()));
self.status =
Err(PlanError::InvalidSecret(Box::new(object_name.clone())));
}
}
ResolvedObjectName::Cte { .. } => {
self.status = Err(PlanError::InvalidSecret(object_name.clone()));
self.status = Err(PlanError::InvalidSecret(Box::new(object_name.clone())));
}
ResolvedObjectName::Error => {}
}
Expand All @@ -1142,7 +1143,7 @@ impl<'a> Fold<Raw, Aug> for NameResolver<'a> {
match &object_name {
ResolvedObjectName::Object { .. } => {}
ResolvedObjectName::Cte { .. } => {
self.status = Err(PlanError::InvalidObject(object_name.clone()));
self.status = Err(PlanError::InvalidObject(Box::new(object_name.clone())));
}
ResolvedObjectName::Error => {}
}
Expand Down Expand Up @@ -1228,7 +1229,7 @@ impl Fold<Aug, Aug> for TransientResolver<'_> {
full_name: full_name.clone(),
print_id,
};
self.status = Err(PlanError::InvalidObject(obj));
self.status = Err(PlanError::InvalidObject(Box::new(obj)));
transient_id
}
};
Expand Down
4 changes: 2 additions & 2 deletions src/sql/src/plan/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,9 @@ pub enum PlanError {
UpsertSinkWithoutKey,
InvalidNumericMaxScale(InvalidNumericMaxScaleError),
InvalidCharLength(InvalidCharLengthError),
InvalidObject(ResolvedObjectName),
InvalidObject(Box<ResolvedObjectName>),
InvalidVarCharMaxLength(InvalidVarCharMaxLengthError),
InvalidSecret(ResolvedObjectName),
InvalidSecret(Box<ResolvedObjectName>),
InvalidTemporarySchema,
Parser(ParserError),
DropViewOnMaterializedView(String),
Expand Down
2 changes: 1 addition & 1 deletion src/storage/src/source/commit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ pub(crate) fn drive_offset_committer<S: OffsetCommitter + Send + Sync + 'static>
) -> OffsetCommitHandle {
let (tx, mut rx): (_, watch::Receiver<HashMap<PartitionId, MzOffset>>) =
watch::channel(Default::default());
let _ = task::spawn(
task::spawn(
|| format!("offset commiter({source_id}) {worker_id}/{worker_count}"),
async move {
let mut last_offsets: HashMap<PartitionId, MzOffset> = HashMap::new();
Expand Down
1 change: 1 addition & 0 deletions src/storage/src/source/generator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ impl SourceConnectionBuilder for LoadGeneratorSourceConnection {
LoadGeneratorSourceReader {
rows: Box::new(rows),
// Subtract tick so we immediately produce a row.
#[allow(clippy::unchecked_duration_subtraction)]
last: Instant::now() - tick,
tick,
offset,
Expand Down