From f58aa0b8305e4c84dae0d6ad404f020da3c7260c Mon Sep 17 00:00:00 2001 From: Han Damin Date: Mon, 29 Jun 2026 11:21:41 +0900 Subject: [PATCH] chore: forbid the locking Id constructors with a clippy lint `Id::new`/`Id::new_static` intern into a globally locked table on every call. A disallowed-methods lint now forbids them: static ids use a `CachedId` static, and the few dynamic-string sites keep `Id::new` behind `#[expect]`. `CachedId` interns via `Id::new_static`. Refs: #8380 Signed-off-by: Han Damin --- clippy.toml | 2 ++ encodings/zstd/src/zstd_buffers.rs | 1 + vortex-array/src/aggregate_fn/proto.rs | 2 ++ vortex-array/src/arrays/extension/compute/rules.rs | 2 ++ vortex-array/src/arrow/session.rs | 3 +++ vortex-array/src/dtype/serde/flatbuffers.rs | 1 + vortex-array/src/dtype/serde/proto.rs | 1 + vortex-array/src/dtype/serde/serde.rs | 1 + vortex-array/src/expr/proto.rs | 1 + vortex-array/src/extension/tests/divisible_int.rs | 1 + vortex-array/src/scalar/arrow.rs | 1 + vortex-array/src/scalar/tests/casting.rs | 3 +++ vortex-array/src/scalar/typed_view/extension/tests.rs | 3 +++ vortex-duckdb/src/convert/dtype.rs | 1 + vortex-file/src/footer/mod.rs | 2 ++ vortex-ipc/src/messages/decoder.rs | 1 + vortex-layout/src/flatbuffers.rs | 1 + vortex-layout/src/layouts/dict/reader.rs | 3 +++ vortex-layout/src/layouts/zoned/schema.rs | 1 + vortex-python/src/arrays/py/mod.rs | 1 + vortex-python/src/serde/context.rs | 1 + vortex-session/src/registry.rs | 7 ++++++- 22 files changed, 39 insertions(+), 1 deletion(-) diff --git a/clippy.toml b/clippy.toml index e761d3c17f7..9f6d5bd21b8 100644 --- a/clippy.toml +++ b/clippy.toml @@ -15,4 +15,6 @@ disallowed-methods = [ { path = "itertools::Itertools::counts", reason = "It uses the default hasher which is slow for primitives. Just inline the loop for better performance.", allow-invalid = true }, { path = "std::result::Result::and", reason = "This method is a footgun, especially when working with `Result`." }, { path = "std::thread::available_parallelism", reason = "This function might do an unbounded amount of work, use `vortex_utils::parallelism::get_available_parallelism instead" }, + { path = "vortex_session::registry::Id::new", reason = "Interning a static id on every call grabs the interner lock (#8380). Use a `CachedId` static for static ids; for a dynamic string, annotate the call with `#[expect(clippy::disallowed_methods)]`.", allow-invalid = true }, + { path = "vortex_session::registry::Id::new_static", reason = "Interning a static id on every call grabs the interner lock; use a `CachedId` static instead (#8380).", allow-invalid = true }, ] diff --git a/encodings/zstd/src/zstd_buffers.rs b/encodings/zstd/src/zstd_buffers.rs index f6cb9af586c..3252605459f 100644 --- a/encodings/zstd/src/zstd_buffers.rs +++ b/encodings/zstd/src/zstd_buffers.rs @@ -348,6 +348,7 @@ fn compute_output_layout( (offsets, total_size) } +#[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] fn array_id_from_string(s: &str) -> ArrayId { ArrayId::new(s) } diff --git a/vortex-array/src/aggregate_fn/proto.rs b/vortex-array/src/aggregate_fn/proto.rs index 3ca4df06d84..322b6cf3bd6 100644 --- a/vortex-array/src/aggregate_fn/proto.rs +++ b/vortex-array/src/aggregate_fn/proto.rs @@ -35,6 +35,7 @@ impl AggregateFnRef { /// /// Note: the serialization format is not stable and may change between versions. pub fn from_proto(proto: &pb::AggregateFn, session: &VortexSession) -> VortexResult { + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] let agg_fn_id: AggregateFnId = AggregateFnId::new(proto.id.as_str()); let agg_fn = if let Some(plugin) = session.aggregate_fns().find_plugin(&agg_fn_id) { plugin.deserialize(proto.metadata(), session)? @@ -88,6 +89,7 @@ mod tests { type Options = EmptyOptions; type Partial = (); + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> AggregateFnId { AggregateFnId::new("vortex.test.proto") } diff --git a/vortex-array/src/arrays/extension/compute/rules.rs b/vortex-array/src/arrays/extension/compute/rules.rs index 611be1510a4..396729b6347 100644 --- a/vortex-array/src/arrays/extension/compute/rules.rs +++ b/vortex-array/src/arrays/extension/compute/rules.rs @@ -120,6 +120,7 @@ mod tests { type Metadata = EmptyMetadata; type NativeValue<'a> = &'a str; + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> ExtId { ExtId::new("test_ext") } @@ -244,6 +245,7 @@ mod tests { type Metadata = EmptyMetadata; type NativeValue<'a> = &'a str; + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> ExtId { ExtId::new("test_ext_2") } diff --git a/vortex-array/src/arrow/session.rs b/vortex-array/src/arrow/session.rs index 0c4cc7b2a6d..432428cc1aa 100644 --- a/vortex-array/src/arrow/session.rs +++ b/vortex-array/src/arrow/session.rs @@ -314,6 +314,7 @@ impl ArrowSession { /// family, [`DataType::FixedSizeList`], [`DataType::Struct`]) so extension metadata /// on nested element/struct fields is preserved. Leaf types use the canonical /// Arrow → Vortex mapping via [`DType::try_from_arrow`]. + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] pub fn from_arrow_field(&self, field: &Field) -> VortexResult { if let Some(name) = field.metadata().get(EXTENSION_TYPE_NAME_KEY) { for plugin in self.importers(&Id::new(name)).iter() { @@ -406,6 +407,7 @@ impl ArrowSession { /// /// With `target = None` the fallback path picks the array's preferred Arrow physical type /// and executes directly into that, ignoring extension types. + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] pub fn execute_arrow( &self, array: ArrayRef, @@ -474,6 +476,7 @@ impl ArrowSession { /// through to the canonical Arrow → Vortex array conversion. pub fn from_arrow_array(&self, array: ArrowArrayRef, field: &Field) -> VortexResult { if let Some(extension_name) = field.metadata().get(EXTENSION_TYPE_NAME_KEY) { + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] let importers = self.importers(&Id::new(extension_name)); if !importers.is_empty() { let dtype = self.from_arrow_field(field)?; diff --git a/vortex-array/src/dtype/serde/flatbuffers.rs b/vortex-array/src/dtype/serde/flatbuffers.rs index f697174e062..a61fc8f5527 100644 --- a/vortex-array/src/dtype/serde/flatbuffers.rs +++ b/vortex-array/src/dtype/serde/flatbuffers.rs @@ -207,6 +207,7 @@ impl TryFrom for DType { let fb_ext = fb .type__as_extension() .ok_or_else(|| vortex_err!("failed to parse extension from flatbuffer"))?; + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] let id = ExtId::new(fb_ext.id().ok_or_else(|| { vortex_err!("failed to parse extension id from flatbuffer") diff --git a/vortex-array/src/dtype/serde/proto.rs b/vortex-array/src/dtype/serde/proto.rs index 78d4c62f412..2f992dce591 100644 --- a/vortex-array/src/dtype/serde/proto.rs +++ b/vortex-array/src/dtype/serde/proto.rs @@ -89,6 +89,7 @@ impl DType { DtypeType::Union(u) => Ok(Self::Union(u.nullable.into())), DtypeType::Variant(v) => Ok(Self::Variant(v.nullable.into())), DtypeType::Extension(e) => { + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] let id = ExtId::new(e.id.as_str()); let storage_dtype = DType::from_proto( e.storage_dtype diff --git a/vortex-array/src/dtype/serde/serde.rs b/vortex-array/src/dtype/serde/serde.rs index 0be3e877f0e..789fd17306b 100644 --- a/vortex-array/src/dtype/serde/serde.rs +++ b/vortex-array/src/dtype/serde/serde.rs @@ -577,6 +577,7 @@ impl<'de> DeserializeSeed<'de> for DTypeSerde<'_, ExtDTypeRef> { } let id = id.ok_or_else(|| de::Error::missing_field("id"))?; + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] let id = ExtId::new(&id); let storage_dtype = storage_dtype.ok_or_else(|| de::Error::missing_field("storage_dtype"))?; diff --git a/vortex-array/src/expr/proto.rs b/vortex-array/src/expr/proto.rs index ebf1f877f15..9f2b81bde84 100644 --- a/vortex-array/src/expr/proto.rs +++ b/vortex-array/src/expr/proto.rs @@ -39,6 +39,7 @@ impl ExprSerializeProtoExt for Expression { impl Expression { pub fn from_proto(expr: &pb::Expr, session: &VortexSession) -> VortexResult { + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] let expr_id = ScalarFnId::new(expr.id.as_str()); let children = expr .children diff --git a/vortex-array/src/extension/tests/divisible_int.rs b/vortex-array/src/extension/tests/divisible_int.rs index 4ab08f7b9f5..2e42f9fa138 100644 --- a/vortex-array/src/extension/tests/divisible_int.rs +++ b/vortex-array/src/extension/tests/divisible_int.rs @@ -34,6 +34,7 @@ impl ExtVTable for DivisibleInt { type Metadata = Divisor; type NativeValue<'a> = u64; + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> ExtId { ExtId::new("test.divisible_int") } diff --git a/vortex-array/src/scalar/arrow.rs b/vortex-array/src/scalar/arrow.rs index 3af9f842cad..cca6749cb19 100644 --- a/vortex-array/src/scalar/arrow.rs +++ b/vortex-array/src/scalar/arrow.rs @@ -455,6 +455,7 @@ mod tests { type Metadata = String; type NativeValue<'a> = &'a str; + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> ExtId { ExtId::new("some_ext") } diff --git a/vortex-array/src/scalar/tests/casting.rs b/vortex-array/src/scalar/tests/casting.rs index 8f287d2e87b..20eb442078a 100644 --- a/vortex-array/src/scalar/tests/casting.rs +++ b/vortex-array/src/scalar/tests/casting.rs @@ -32,6 +32,7 @@ mod tests { type Metadata = usize; type NativeValue<'a> = &'a str; + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> ExtId { ExtId::new("apples") } @@ -247,6 +248,7 @@ mod tests { type Metadata = usize; type NativeValue<'a> = &'a str; + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> ExtId { ExtId::new("f16_ext") } @@ -304,6 +306,7 @@ mod tests { type Metadata = usize; type NativeValue<'a> = &'a str; + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> ExtId { ExtId::new("struct_ext") } diff --git a/vortex-array/src/scalar/typed_view/extension/tests.rs b/vortex-array/src/scalar/typed_view/extension/tests.rs index 8bf52aa5369..2b08c530af6 100644 --- a/vortex-array/src/scalar/typed_view/extension/tests.rs +++ b/vortex-array/src/scalar/typed_view/extension/tests.rs @@ -20,6 +20,7 @@ impl ExtVTable for TestI32Ext { type Metadata = EmptyMetadata; type NativeValue<'a> = &'a str; + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> ExtId { ExtId::new("test_ext") } @@ -103,6 +104,7 @@ fn test_ext_scalar_partial_ord_different_types() { type Metadata = EmptyMetadata; type NativeValue<'a> = &'a str; + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> ExtId { ExtId::new("test_ext_2") } @@ -285,6 +287,7 @@ fn test_ext_scalar_with_metadata() { type Metadata = usize; type NativeValue<'a> = &'a str; + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> ExtId { ExtId::new("test_ext_metadata") } diff --git a/vortex-duckdb/src/convert/dtype.rs b/vortex-duckdb/src/convert/dtype.rs index 4238b354182..03aed0631aa 100644 --- a/vortex-duckdb/src/convert/dtype.rs +++ b/vortex-duckdb/src/convert/dtype.rs @@ -625,6 +625,7 @@ mod tests { type Metadata = EmptyMetadata; type NativeValue<'a> = &'a str; + #[expect(clippy::disallowed_methods, reason = "test-only id")] fn id(&self) -> ExtId { ExtId::new("unknown.extension") } diff --git a/vortex-file/src/footer/mod.rs b/vortex-file/src/footer/mod.rs index 32c82d4966e..f4bf7e19760 100644 --- a/vortex-file/src/footer/mod.rs +++ b/vortex-file/src/footer/mod.rs @@ -83,6 +83,7 @@ impl Footer { // Create a LayoutContext from the registry. let layout_specs = fb_footer.layout_specs(); + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] let layout_ids: Arc<[_]> = layout_specs .iter() .flat_map(|e| e.iter()) @@ -92,6 +93,7 @@ impl Footer { // Create an ArrayContext from the registry. let array_specs = fb_footer.array_specs(); + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] let array_ids: Arc<[_]> = array_specs .iter() .flat_map(|e| e.iter()) diff --git a/vortex-ipc/src/messages/decoder.rs b/vortex-ipc/src/messages/decoder.rs index 190461df96f..eccbcc6370a 100644 --- a/vortex-ipc/src/messages/decoder.rs +++ b/vortex-ipc/src/messages/decoder.rs @@ -121,6 +121,7 @@ impl MessageDecoder { .header_as_array_message() .vortex_expect("header is array"); + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] let encoding_ids: Arc<_> = header .encodings() .iter() diff --git a/vortex-layout/src/flatbuffers.rs b/vortex-layout/src/flatbuffers.rs index 92d34cd5de3..ced2b4c6a39 100644 --- a/vortex-layout/src/flatbuffers.rs +++ b/vortex-layout/src/flatbuffers.rs @@ -232,6 +232,7 @@ mod tests { use crate::LayoutEncodingId; use crate::session::LayoutSession; + #[expect(clippy::disallowed_methods, reason = "test-only id")] #[test] fn unknown_layout_encoding_allow_unknown() { let mut fbb = FlatBufferBuilder::new(); diff --git a/vortex-layout/src/layouts/dict/reader.rs b/vortex-layout/src/layouts/dict/reader.rs index a4b3e20cbff..489d8823d39 100644 --- a/vortex-layout/src/layouts/dict/reader.rs +++ b/vortex-layout/src/layouts/dict/reader.rs @@ -419,6 +419,7 @@ mod tests { (layout, segments) } + #[expect(clippy::disallowed_methods, reason = "test-only id")] #[test] fn reading_nested_packs_works() { block_on(|handle| async move { @@ -570,6 +571,7 @@ mod tests { }) } + #[expect(clippy::disallowed_methods, reason = "test-only id")] #[test] fn reading_is_null_works() { block_on(|handle| async move { @@ -644,6 +646,7 @@ mod tests { }) } + #[expect(clippy::disallowed_methods, reason = "test-only id")] #[test] fn reading_byte_length_pushdown_works() { let array = VarBinArray::from_iter( diff --git a/vortex-layout/src/layouts/zoned/schema.rs b/vortex-layout/src/layouts/zoned/schema.rs index bcd1069bcf1..d05f5672640 100644 --- a/vortex-layout/src/layouts/zoned/schema.rs +++ b/vortex-layout/src/layouts/zoned/schema.rs @@ -44,6 +44,7 @@ impl AggregateSpecProto { } pub(crate) fn to_aggregate_fn(&self, session: &VortexSession) -> VortexResult { + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] let aggregate_fn_id = AggregateFnId::new(self.id.as_str()); let Some(plugin) = session.aggregate_fns().find_plugin(&aggregate_fn_id) else { vortex_bail!("unknown aggregate function id: {}", self.id); diff --git a/vortex-python/src/arrays/py/mod.rs b/vortex-python/src/arrays/py/mod.rs index c0aab2cb98c..dbed811f522 100644 --- a/vortex-python/src/arrays/py/mod.rs +++ b/vortex-python/src/arrays/py/mod.rs @@ -17,6 +17,7 @@ pub(crate) use vtable::*; use crate::error::PyVortexResult; /// Extract the array id from a Python class `id` attribute. +#[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] pub fn id_from_obj(cls: &Bound) -> PyVortexResult { let id_str: String = cls .getattr("id") diff --git a/vortex-python/src/serde/context.rs b/vortex-python/src/serde/context.rs index 298556262e1..30e4a4ea90a 100644 --- a/vortex-python/src/serde/context.rs +++ b/vortex-python/src/serde/context.rs @@ -72,6 +72,7 @@ impl Deref for PyReadContext { #[pymethods] impl PyReadContext { #[new] + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] fn new(ids: Vec) -> Self { Self(ReadContext::new( ids.into_iter().map(|i| Id::new(&i)).collect::>(), diff --git a/vortex-session/src/registry.rs b/vortex-session/src/registry.rs index 50bbb8c981e..ed269dba7eb 100644 --- a/vortex-session/src/registry.rs +++ b/vortex-session/src/registry.rs @@ -55,6 +55,7 @@ impl Id { } impl From<&str> for Id { + #[expect(clippy::disallowed_methods, reason = "interning a dynamic id")] fn from(s: &str) -> Self { Self::new(s) } @@ -136,8 +137,12 @@ impl CachedId { impl Deref for CachedId { type Target = Id; + #[expect( + clippy::disallowed_methods, + reason = "CachedId interns its static id once here" + )] fn deref(&self) -> &Id { - self.cached.get_or_init(|| Id::new(self.s)) + self.cached.get_or_init(|| Id::new_static(self.s)) } }