Skip to content

Commit b418f23

Browse files
committed
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 <miniex@daminstudio.net>
1 parent ab41b71 commit b418f23

22 files changed

Lines changed: 39 additions & 1 deletion

File tree

clippy.toml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,4 +15,6 @@ disallowed-methods = [
1515
{ 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 },
1616
{ path = "std::result::Result::and", reason = "This method is a footgun, especially when working with `Result<Validity>`." },
1717
{ path = "std::thread::available_parallelism", reason = "This function might do an unbounded amount of work, use `vortex_utils::parallelism::get_available_parallelism instead" },
18+
{ 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 },
19+
{ 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 },
1820
]

encodings/zstd/src/zstd_buffers.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,7 @@ fn compute_output_layout(
348348
(offsets, total_size)
349349
}
350350

351+
#[expect(clippy::disallowed_methods, reason = "interning a dynamic id")]
351352
fn array_id_from_string(s: &str) -> ArrayId {
352353
ArrayId::new(s)
353354
}

vortex-array/src/aggregate_fn/proto.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ impl AggregateFnRef {
3535
///
3636
/// Note: the serialization format is not stable and may change between versions.
3737
pub fn from_proto(proto: &pb::AggregateFn, session: &VortexSession) -> VortexResult<Self> {
38+
#[expect(clippy::disallowed_methods, reason = "interning a dynamic id")]
3839
let agg_fn_id: AggregateFnId = AggregateFnId::new(proto.id.as_str());
3940
let agg_fn = if let Some(plugin) = session.aggregate_fns().find_plugin(&agg_fn_id) {
4041
plugin.deserialize(proto.metadata(), session)?
@@ -88,6 +89,7 @@ mod tests {
8889
type Options = EmptyOptions;
8990
type Partial = ();
9091

92+
#[expect(clippy::disallowed_methods, reason = "test-only id")]
9193
fn id(&self) -> AggregateFnId {
9294
AggregateFnId::new("vortex.test.proto")
9395
}

vortex-array/src/arrays/extension/compute/rules.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ mod tests {
120120
type Metadata = EmptyMetadata;
121121
type NativeValue<'a> = &'a str;
122122

123+
#[expect(clippy::disallowed_methods, reason = "test-only id")]
123124
fn id(&self) -> ExtId {
124125
ExtId::new("test_ext")
125126
}
@@ -244,6 +245,7 @@ mod tests {
244245
type Metadata = EmptyMetadata;
245246
type NativeValue<'a> = &'a str;
246247

248+
#[expect(clippy::disallowed_methods, reason = "test-only id")]
247249
fn id(&self) -> ExtId {
248250
ExtId::new("test_ext_2")
249251
}

vortex-array/src/arrow/session.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -314,6 +314,7 @@ impl ArrowSession {
314314
/// family, [`DataType::FixedSizeList`], [`DataType::Struct`]) so extension metadata
315315
/// on nested element/struct fields is preserved. Leaf types use the canonical
316316
/// Arrow → Vortex mapping via [`DType::try_from_arrow`].
317+
#[expect(clippy::disallowed_methods, reason = "interning a dynamic id")]
317318
pub fn from_arrow_field(&self, field: &Field) -> VortexResult<DType> {
318319
if let Some(name) = field.metadata().get(EXTENSION_TYPE_NAME_KEY) {
319320
for plugin in self.importers(&Id::new(name)).iter() {
@@ -406,6 +407,7 @@ impl ArrowSession {
406407
///
407408
/// With `target = None` the fallback path picks the array's preferred Arrow physical type
408409
/// and executes directly into that, ignoring extension types.
410+
#[expect(clippy::disallowed_methods, reason = "interning a dynamic id")]
409411
pub fn execute_arrow(
410412
&self,
411413
array: ArrayRef,
@@ -474,6 +476,7 @@ impl ArrowSession {
474476
/// through to the canonical Arrow → Vortex array conversion.
475477
pub fn from_arrow_array(&self, array: ArrowArrayRef, field: &Field) -> VortexResult<ArrayRef> {
476478
if let Some(extension_name) = field.metadata().get(EXTENSION_TYPE_NAME_KEY) {
479+
#[expect(clippy::disallowed_methods, reason = "interning a dynamic id")]
477480
let importers = self.importers(&Id::new(extension_name));
478481
if !importers.is_empty() {
479482
let dtype = self.from_arrow_field(field)?;

vortex-array/src/dtype/serde/flatbuffers.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,6 +207,7 @@ impl TryFrom<ViewedDType> for DType {
207207
let fb_ext = fb
208208
.type__as_extension()
209209
.ok_or_else(|| vortex_err!("failed to parse extension from flatbuffer"))?;
210+
#[expect(clippy::disallowed_methods, reason = "interning a dynamic id")]
210211
let id =
211212
ExtId::new(fb_ext.id().ok_or_else(|| {
212213
vortex_err!("failed to parse extension id from flatbuffer")

vortex-array/src/dtype/serde/proto.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ impl DType {
8989
DtypeType::Union(u) => Ok(Self::Union(u.nullable.into())),
9090
DtypeType::Variant(v) => Ok(Self::Variant(v.nullable.into())),
9191
DtypeType::Extension(e) => {
92+
#[expect(clippy::disallowed_methods, reason = "interning a dynamic id")]
9293
let id = ExtId::new(e.id.as_str());
9394
let storage_dtype = DType::from_proto(
9495
e.storage_dtype

vortex-array/src/dtype/serde/serde.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,6 +577,7 @@ impl<'de> DeserializeSeed<'de> for DTypeSerde<'_, ExtDTypeRef> {
577577
}
578578

579579
let id = id.ok_or_else(|| de::Error::missing_field("id"))?;
580+
#[expect(clippy::disallowed_methods, reason = "interning a dynamic id")]
580581
let id = ExtId::new(&id);
581582
let storage_dtype =
582583
storage_dtype.ok_or_else(|| de::Error::missing_field("storage_dtype"))?;

vortex-array/src/expr/proto.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ impl ExprSerializeProtoExt for Expression {
3939

4040
impl Expression {
4141
pub fn from_proto(expr: &pb::Expr, session: &VortexSession) -> VortexResult<Expression> {
42+
#[expect(clippy::disallowed_methods, reason = "interning a dynamic id")]
4243
let expr_id = ScalarFnId::new(expr.id.as_str());
4344
let children = expr
4445
.children

vortex-array/src/extension/tests/divisible_int.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ impl ExtVTable for DivisibleInt {
3434
type Metadata = Divisor;
3535
type NativeValue<'a> = u64;
3636

37+
#[expect(clippy::disallowed_methods, reason = "test-only id")]
3738
fn id(&self) -> ExtId {
3839
ExtId::new("test.divisible_int")
3940
}

0 commit comments

Comments
 (0)