Skip to content

Commit f03a0c7

Browse files
authored
Update vtables docs regarding forwarding from erased to typed (#7134)
DynFoo is a thin wrapper calling into functions on Foo<V>. Signed-off-by: Nicholas Gates <nick@nickgates.com>
1 parent 539288e commit f03a0c7

4 files changed

Lines changed: 138 additions & 62 deletions

File tree

docs/developer-guide/internals/vtables.md

Lines changed: 51 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ via normal method resolution.
4343
**Sealed trait** `DynFoo` is object-safe and has a blanket `impl<V: FooVTable> DynFoo for Foo<V>`.
4444
This blanket impl is a thin forwarder to `Foo<V>` inherent methods — no logic of its own.
4545
Its purpose is to enable dynamic dispatch from `FooRef` through to the typed `Foo<V>`.
46+
`DynFoo` must stay private (`pub(super)`) because it exposes internal plumbing
47+
(`as_any`, `metadata_any`) that should not be part of the public API.
4648

4749
**Erased form** `FooRef` wraps `Arc<dyn DynFoo>`. It delegates to `DynFoo` methods, which
4850
forward to `Foo<V>`. It can be stored in collections, serialized, and threaded through APIs
@@ -63,13 +65,13 @@ registered in the session by their ID.
6365

6466
For a concept `Foo`, the components are organized into these files:
6567

66-
| File | Contains |
67-
|--------------|----------------------------------------------------------------------|
68-
| `vtable.rs` | `FooVTable` trait definition |
69-
| `typed.rs` | `Foo<V>` data struct, inherent methods, `Deref` impl |
70-
| `erased.rs` | `FooRef` struct, `DynFoo` sealed trait, blanket impl |
71-
| `plugin.rs` | `FooPlugin` trait, registration |
72-
| `matcher.rs` | Downcasting helpers (`is`, `as_`, `as_opt`, pattern matching traits) |
68+
| File | Contains |
69+
|--------------|-----------------------------------------------------------------------------|
70+
| `vtable.rs` | `FooVTable` trait definition |
71+
| `typed.rs` | `Foo<V>` data struct, inherent methods, `DynFoo` sealed trait, blanket impl |
72+
| `erased.rs` | `FooRef` struct |
73+
| `plugin.rs` | `FooPlugin` trait, registration |
74+
| `matcher.rs` | Downcasting helpers (`is`, `as_`, `as_opt`, pattern matching traits) |
7375

7476
For Array encodings, each encoding has its own module (e.g. `arrays/primitive/`):
7577

@@ -130,6 +132,48 @@ plugin authors implement) from the public API (what callers use). The public API
130132
inputs, enforce invariants, and transform outputs without exposing those concerns to vtable
131133
implementors. With `dyn Trait`, the trait surface is the public API.
132134

135+
## Method Overlap and `same_name_method`
136+
137+
`Foo<V>` and `DynFoo` necessarily share method names: `Foo<V>` needs inherent methods
138+
so callers (including VTable authors who receive `&Foo<Self>`) can use them directly,
139+
and `DynFoo` needs the same methods for object-safe dispatch from `FooRef`.
140+
141+
Because `Foo<V>` implements `DynFoo`, having both an inherent `id()` and a trait `id()`
142+
on the same type triggers `clippy::same_name_method`. The correct handling is:
143+
144+
1. **Logic lives in `Foo<V>` inherent methods.** Pre/post-conditions, field access, and
145+
delegation to `FooVTable` all happen here.
146+
2. **`DynFoo` blanket impl is a thin forwarder.** Each method body is just `self.method()`.
147+
Rust's method resolution picks inherent methods over trait methods, so this calls the
148+
inherent impl — no infinite recursion.
149+
3. **`#[allow(clippy::same_name_method)]`** on the `Foo<V>` inherent impl block
150+
acknowledges the intentional shadowing.
151+
152+
```rust
153+
#[allow(clippy::same_name_method)]
154+
impl<V: FooVTable> Foo<V> {
155+
pub fn id(&self) -> FooId {
156+
self.vtable.id() // logic lives here
157+
}
158+
}
159+
160+
impl<V: FooVTable> DynFoo for Foo<V> {
161+
fn id(&self) -> FooId {
162+
self.id() // thin forwarder to inherent
163+
}
164+
}
165+
```
166+
167+
`DynFoo` must stay **private** (`pub(super)`) because it exposes internal plumbing
168+
(`as_any`, `metadata_any`) that external callers should never reach. Making it public
169+
or implementing it for `FooRef` would leak these internals. Instead, `FooRef` has its
170+
own inherent methods that delegate to `DynFoo` — providing a clean public API without
171+
exposing the dispatch machinery.
172+
173+
Methods that exist only for erased dispatch (e.g. `as_any`, `metadata_any`,
174+
`metadata_hash`) have no inherent counterpart on `Foo<V>` and live exclusively in
175+
`DynFoo`.
176+
133177
## Registration and Deserialization
134178

135179
Vtables are registered in the session by their ID. When a serialized value is encountered

vortex-array/public-api.lock

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10222,18 +10222,30 @@ pub fn vortex_array::dtype::extension::ExtDType<V>::try_new(metadata: <V as vort
1022210222

1022310223
impl<V: vortex_array::dtype::extension::ExtVTable> vortex_array::dtype::extension::ExtDType<V>
1022410224

10225+
pub fn vortex_array::dtype::extension::ExtDType<V>::can_coerce_from(&self, other: &vortex_array::dtype::DType) -> bool
10226+
10227+
pub fn vortex_array::dtype::extension::ExtDType<V>::can_coerce_to(&self, other: &vortex_array::dtype::DType) -> bool
10228+
1022510229
pub fn vortex_array::dtype::extension::ExtDType<V>::erased(self) -> vortex_array::dtype::extension::ExtDTypeRef
1022610230

1022710231
pub fn vortex_array::dtype::extension::ExtDType<V>::id(&self) -> vortex_array::dtype::extension::ExtId
1022810232

10233+
pub fn vortex_array::dtype::extension::ExtDType<V>::least_supertype(&self, other: &vortex_array::dtype::DType) -> core::option::Option<vortex_array::dtype::DType>
10234+
1022910235
pub fn vortex_array::dtype::extension::ExtDType<V>::metadata(&self) -> &<V as vortex_array::dtype::extension::ExtVTable>::Metadata
1023010236

10237+
pub fn vortex_array::dtype::extension::ExtDType<V>::serialize_metadata(&self) -> vortex_error::VortexResult<alloc::vec::Vec<u8>>
10238+
1023110239
pub fn vortex_array::dtype::extension::ExtDType<V>::storage_dtype(&self) -> &vortex_array::dtype::DType
1023210240

1023310241
pub fn vortex_array::dtype::extension::ExtDType<V>::try_with_vtable(vtable: V, metadata: <V as vortex_array::dtype::extension::ExtVTable>::Metadata, storage_dtype: vortex_array::dtype::DType) -> vortex_error::VortexResult<Self>
1023410242

10243+
pub fn vortex_array::dtype::extension::ExtDType<V>::validate_scalar_value(&self, storage_value: &vortex_array::scalar::ScalarValue) -> vortex_error::VortexResult<()>
10244+
1023510245
pub fn vortex_array::dtype::extension::ExtDType<V>::vtable(&self) -> &V
1023610246

10247+
pub fn vortex_array::dtype::extension::ExtDType<V>::with_nullability(&self, nullability: vortex_array::dtype::Nullability) -> vortex_array::dtype::extension::ExtDTypeRef
10248+
1023710249
impl<V: core::clone::Clone + vortex_array::dtype::extension::ExtVTable> core::clone::Clone for vortex_array::dtype::extension::ExtDType<V> where <V as vortex_array::dtype::extension::ExtVTable>::Metadata: core::clone::Clone
1023810250

1023910251
pub fn vortex_array::dtype::extension::ExtDType<V>::clone(&self) -> vortex_array::dtype::extension::ExtDType<V>

vortex-array/src/dtype/extension/erased.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,12 @@ pub struct ExtDTypeRef(pub(super) Arc<dyn DynExtDType>);
3939
impl ExtDTypeRef {
4040
/// Returns the [`ExtId`] identifying this extension type.
4141
pub fn id(&self) -> ExtId {
42-
self.0.ext_id()
42+
self.0.id()
4343
}
4444

4545
/// Returns the storage dtype of the extension type.
4646
pub fn storage_dtype(&self) -> &DType {
47-
self.0.ext_storage_dtype()
47+
self.0.storage_dtype()
4848
}
4949

5050
/// Returns the nullability of the storage dtype.
@@ -78,7 +78,7 @@ impl ExtDTypeRef {
7878
// TODO(connor): We should add a different type that returns something that can be serialized.
7979
/// Serialize the metadata into a byte vector.
8080
pub fn serialize_metadata(&self) -> VortexResult<Vec<u8>> {
81-
self.0.metadata_serialize()
81+
self.0.serialize_metadata()
8282
}
8383

8484
/// Returns a `Display`-able view of just the metadata.
@@ -97,22 +97,22 @@ impl ExtDTypeRef {
9797

9898
/// Validates that the given storage scalar value is valid for this dtype.
9999
pub(crate) fn validate_storage_value(&self, storage_value: &ScalarValue) -> VortexResult<()> {
100-
self.0.value_validate(storage_value)
100+
self.0.validate_scalar_value(storage_value)
101101
}
102102

103103
/// Can a value of `other` be implicitly coerced into this extension type?
104104
pub fn can_coerce_from(&self, other: &DType) -> bool {
105-
self.0.coercion_can_coerce_from(other)
105+
self.0.can_coerce_from(other)
106106
}
107107

108108
/// Can this extension type be implicitly coerced into `other`?
109109
pub fn can_coerce_to(&self, other: &DType) -> bool {
110-
self.0.coercion_can_coerce_to(other)
110+
self.0.can_coerce_to(other)
111111
}
112112

113113
/// Compute the least supertype of this extension type and another type.
114114
pub fn least_supertype(&self, other: &DType) -> Option<DType> {
115-
self.0.coercion_least_supertype(other)
115+
self.0.least_supertype(other)
116116
}
117117
}
118118

@@ -160,7 +160,7 @@ impl ExtDTypeRef {
160160
.map_err(|this| {
161161
vortex_err!(
162162
"Failed to downcast ExtDTypeRef {} to {}",
163-
this.0.ext_id(),
163+
this.0.id(),
164164
type_name::<V>(),
165165
)
166166
})

vortex-array/src/dtype/extension/typed.rs

Lines changed: 67 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@
44
//! Typed and inner representations of extension dtypes.
55
//!
66
//! - [`ExtDType<V>`]: The public typed wrapper, parameterized by a concrete [`ExtVTable`].
7-
//! - [`ExtDTypeInner<V>`]: The private inner struct that holds the vtable + data.
87
//! - [`DynExtDType`]: The private sealed trait for type-erased dispatch.
98
109
use std::any::Any;
@@ -49,6 +48,7 @@ impl<V: ExtVTable + Default> ExtDType<V> {
4948
}
5049
}
5150

51+
#[allow(clippy::same_name_method)]
5252
impl<V: ExtVTable> ExtDType<V> {
5353
/// Creates a new extension dtype with the given metadata and storage dtype.
5454
pub fn try_with_vtable(
@@ -87,61 +87,87 @@ impl<V: ExtVTable> ExtDType<V> {
8787
&self.storage_dtype
8888
}
8989

90+
/// Returns a new [`ExtDTypeRef`] with the given nullability.
91+
pub fn with_nullability(&self, nullability: Nullability) -> ExtDTypeRef {
92+
let storage_dtype = self.storage_dtype.with_nullability(nullability);
93+
ExtDType::<V>::try_with_vtable(self.vtable.clone(), self.metadata.clone(), storage_dtype)
94+
.vortex_expect(
95+
"Extension DType should not fail validation with the same storage type \
96+
but different nullability",
97+
)
98+
.erased()
99+
}
100+
101+
/// Serializes the metadata into a byte vector.
102+
pub fn serialize_metadata(&self) -> VortexResult<Vec<u8>> {
103+
V::serialize_metadata(&self.vtable, &self.metadata)
104+
}
105+
106+
/// Validates that the given storage scalar value is valid for this dtype.
107+
pub fn validate_scalar_value(&self, storage_value: &ScalarValue) -> VortexResult<()> {
108+
V::validate_scalar_value(self, storage_value)
109+
}
110+
111+
/// Can a value of `other` be implicitly coerced into this extension type?
112+
pub fn can_coerce_from(&self, other: &DType) -> bool {
113+
V::can_coerce_from(self, other)
114+
}
115+
116+
/// Can this extension type be implicitly coerced into `other`?
117+
pub fn can_coerce_to(&self, other: &DType) -> bool {
118+
V::can_coerce_to(self, other)
119+
}
120+
121+
/// Compute the least supertype of this extension type and another type.
122+
pub fn least_supertype(&self, other: &DType) -> Option<DType> {
123+
V::least_supertype(self, other)
124+
}
125+
90126
/// Erase the concrete type information, returning a type-erased extension dtype.
91127
pub fn erased(self) -> ExtDTypeRef {
92128
ExtDTypeRef(Arc::new(self))
93129
}
94130
}
95131

96-
/// An object-safe, sealed trait encapsulating the behavior for extension dtypes.
132+
/// An object-safe, sealed trait for type-erased extension dtype dispatch.
97133
///
98-
/// This provides type-erased access to the extension dtype's identity, storage dtype, and
99-
/// metadata. The only implementor is [`ExtDTypeInner`].
134+
/// Methods that have a corresponding inherent method on [`ExtDType<V>`] are thin forwarders
135+
/// (e.g. `id`, `storage_dtype`). Methods that exist only for erased dispatch have no
136+
/// inherent counterpart (e.g. `as_any`, `metadata_any`, `metadata_eq`).
100137
pub(super) trait DynExtDType: 'static + Send + Sync + super::sealed::Sealed {
101-
/// Returns `self` as a trait object for downcasting.
102138
fn as_any(&self) -> &dyn Any;
103-
/// Returns the [`ExtId`] identifying this extension type.
104-
fn ext_id(&self) -> ExtId;
105-
/// Returns a reference to the storage [`DType`].
106-
fn ext_storage_dtype(&self) -> &DType;
107-
/// Returns the metadata as a trait object for downcasting.
139+
fn id(&self) -> ExtId;
140+
fn storage_dtype(&self) -> &DType;
108141
fn metadata_any(&self) -> &dyn Any;
109-
/// Formats the metadata using [`Debug`].
110142
fn metadata_debug(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result;
111-
/// Formats the metadata using [`Display`].
112143
fn metadata_display(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result;
113-
/// Checks equality of the metadata against a type-erased value.
114144
fn metadata_eq(&self, other: &dyn Any) -> bool;
115-
/// Hashes the metadata into the given [`Hasher`].
116145
fn metadata_hash(&self, state: &mut dyn Hasher);
117-
/// Serializes the metadata into a byte vector.
118-
fn metadata_serialize(&self) -> VortexResult<Vec<u8>>;
119-
/// Returns a new [`ExtDTypeRef`] with the given nullability.
146+
fn serialize_metadata(&self) -> VortexResult<Vec<u8>>;
120147
fn with_nullability(&self, nullability: Nullability) -> ExtDTypeRef;
121-
/// Validates that the given storage scalar value is valid for this dtype.
122-
fn value_validate(&self, storage_value: &ScalarValue) -> VortexResult<()>;
123-
/// Formats an extension scalar value using the current dtype for metadata context.
148+
fn validate_scalar_value(&self, storage_value: &ScalarValue) -> VortexResult<()>;
124149
fn value_display(&self, f: &mut fmt::Formatter<'_>, storage_value: &ScalarValue)
125150
-> fmt::Result;
126-
/// Can a value of `other` be implicitly coerced into this extension type?
127-
fn coercion_can_coerce_from(&self, other: &DType) -> bool;
128-
/// Can this extension type be implicitly coerced into `other`?
129-
fn coercion_can_coerce_to(&self, other: &DType) -> bool;
130-
/// Compute the least supertype of this extension type and another type.
131-
fn coercion_least_supertype(&self, other: &DType) -> Option<DType>;
151+
fn can_coerce_from(&self, other: &DType) -> bool;
152+
fn can_coerce_to(&self, other: &DType) -> bool;
153+
fn least_supertype(&self, other: &DType) -> Option<DType>;
132154
}
133155

156+
/// Blanket impl: thin forwarder to `ExtDType<V>` inherent methods.
157+
///
158+
/// Rust's method resolution picks inherent methods over trait methods, so `self.id()` etc.
159+
/// call the inherent impl, not this trait impl (no infinite recursion).
134160
impl<V: ExtVTable> DynExtDType for ExtDType<V> {
135161
fn as_any(&self) -> &dyn Any {
136162
self
137163
}
138164

139-
fn ext_id(&self) -> ExtId {
140-
self.vtable.id()
165+
fn id(&self) -> ExtId {
166+
self.id()
141167
}
142168

143-
fn ext_storage_dtype(&self) -> &DType {
144-
&self.storage_dtype
169+
fn storage_dtype(&self) -> &DType {
170+
self.storage_dtype()
145171
}
146172

147173
fn metadata_any(&self) -> &dyn Any {
@@ -167,22 +193,16 @@ impl<V: ExtVTable> DynExtDType for ExtDType<V> {
167193
<V::Metadata as Hash>::hash(&self.metadata, &mut state);
168194
}
169195

170-
fn metadata_serialize(&self) -> VortexResult<Vec<u8>> {
171-
V::serialize_metadata(&self.vtable, &self.metadata)
196+
fn serialize_metadata(&self) -> VortexResult<Vec<u8>> {
197+
self.serialize_metadata()
172198
}
173199

174200
fn with_nullability(&self, nullability: Nullability) -> ExtDTypeRef {
175-
let storage_dtype = self.storage_dtype.with_nullability(nullability);
176-
ExtDType::<V>::try_with_vtable(self.vtable.clone(), self.metadata.clone(), storage_dtype)
177-
.vortex_expect(
178-
"Extension DType should not fail validation with the same storage type \
179-
but different nullability",
180-
)
181-
.erased()
201+
self.with_nullability(nullability)
182202
}
183203

184-
fn value_validate(&self, storage_value: &ScalarValue) -> VortexResult<()> {
185-
V::validate_scalar_value(self, storage_value)
204+
fn validate_scalar_value(&self, storage_value: &ScalarValue) -> VortexResult<()> {
205+
self.validate_scalar_value(storage_value)
186206
}
187207

188208
fn value_display(
@@ -201,15 +221,15 @@ impl<V: ExtVTable> DynExtDType for ExtDType<V> {
201221
}
202222
}
203223

204-
fn coercion_can_coerce_from(&self, other: &DType) -> bool {
205-
V::can_coerce_from(self, other)
224+
fn can_coerce_from(&self, other: &DType) -> bool {
225+
self.can_coerce_from(other)
206226
}
207227

208-
fn coercion_can_coerce_to(&self, other: &DType) -> bool {
209-
V::can_coerce_to(self, other)
228+
fn can_coerce_to(&self, other: &DType) -> bool {
229+
self.can_coerce_to(other)
210230
}
211231

212-
fn coercion_least_supertype(&self, other: &DType) -> Option<DType> {
213-
V::least_supertype(self, other)
232+
fn least_supertype(&self, other: &DType) -> Option<DType> {
233+
self.least_supertype(other)
214234
}
215235
}

0 commit comments

Comments
 (0)