Add FooBase trait so cold types can implement devirt traits without #[devirt]#40
Add FooBase trait so cold types can implement devirt traits without #[devirt]#40Kab1r wants to merge 7 commits into
Conversation
…[devirt]
When a library exposes a devirt trait, downstream crates previously had
to depend on devirt and use #[devirt] on their impl blocks even for cold
types. This was because the public trait is a marker with no methods —
the actual methods live on a hidden __FooImpl trait with __spec_* names.
This commit adds a public FooBase trait with the original method names,
plus a blanket impl bridging FooBase → __FooImpl. Cold types can now
implement FooBase with standard Rust, no devirt dependency needed:
impl my_lib::ShapeBase for MyColdType {
fn area(&self) -> f64 { ... }
}
Hot types continue using #[devirt] for the optimized direct __FooImpl
path. The change is fully backward-compatible — all existing code using
#[devirt] on impl blocks works identically.
https://claude.ai/code/session_018DyxQYLHG2JCYbUvoVA7Ey
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR refactors the devirt macro system to introduce an explicit base trait layer that separates method declarations from hot-type dispatch logic. The macro now generates a ChangesBase trait dispatch layer refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
💡 Codex Reviewdevirt/crates/macros/src/lib.rs Lines 505 to 510 in a4cfef5  Preserve wildcard args when forwarding base trait methods
devirt/crates/macros/src/lib.rs Lines 472 to 475 in a4cfef5  Keep default method bodies in generated *Base traits
The generated base trait rewrites every method to a bare signature ( ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a 'Base implementation trait' pattern to the devirt crate, allowing cold types to implement a base trait directly without a dependency on the devirt macro while still participating in dynamic dispatch. The changes involve significant updates to the __devirt_define! and devirt! macros to generate these base traits and blanket implementations. Review feedback highlighted critical issues in the procedural macro implementation, including the use of unstable Rust syntax (let chains), incorrect handling of wildcard patterns in function arguments, and missing support for generic traits in the generated bridge code.
| fn build_base_bridge_items( | ||
| trait_item: &syn::ItemTrait, | ||
| base_name: &syn::Ident, | ||
| ) -> Vec<proc_macro2::TokenStream> { | ||
| trait_item | ||
| .items | ||
| .iter() | ||
| .filter_map(|item| match item { | ||
| syn::TraitItem::Type(t) => { | ||
| let type_name = &t.ident; | ||
| Some(quote! { | ||
| type #type_name = <Self as #base_name>::#type_name; | ||
| }) | ||
| } | ||
| syn::TraitItem::Fn(m) => { | ||
| let method_name = &m.sig.ident; | ||
| let spec_name = format_ident!("__spec_{method_name}"); | ||
| let mut bridge_sig = m.sig.clone(); | ||
| bridge_sig.ident = spec_name; | ||
| let arg_names: Vec<_> = m | ||
| .sig | ||
| .inputs | ||
| .iter() | ||
| .filter_map(|arg| { | ||
| if let syn::FnArg::Typed(pat) = arg | ||
| && let syn::Pat::Ident(pi) = &*pat.pat | ||
| { | ||
| return Some(&pi.ident); | ||
| } | ||
| None | ||
| }) | ||
| .collect(); | ||
| Some(quote! { | ||
| #[inline(always)] | ||
| #bridge_sig { | ||
| #base_name::#method_name(self, #(#arg_names),*) | ||
| } | ||
| }) | ||
| } | ||
| _ => None, | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
The current implementation of build_base_bridge_items has several issues that will lead to compilation failures:
- Unstable Syntax: It uses "let chains" (
&& let), which is currently an unstable Rust feature and will fail on stable compilers. - Wildcard Handling: It skips arguments with wildcard patterns (
_). This results in a mismatch between the function signature and the call site in the generated bridge code. - Missing Generics: It does not include trait generics in the UFCS calls or associated type aliases. If the trait is generic (e.g.,
Trait<T>), the generated code will fail to compile because the base trait name will be used without its required type arguments.
Using the existing rewrite_sig_with_named_args helper and passing trait_ty_generics addresses all these points.
fn build_base_bridge_items(
trait_item: &syn::ItemTrait,
base_name: &syn::Ident,
trait_ty_generics: &syn::TypeGenerics<'_>,
) -> Vec<proc_macro2::TokenStream> {
trait_item
.items
.iter()
.filter_map(|item| match item {
syn::TraitItem::Type(t) => {
let type_name = &t.ident;
Some(quote! {
type #type_name = <Self as #base_name #trait_ty_generics>::#type_name;
})
}
syn::TraitItem::Fn(m) => {
let method_name = &m.sig.ident;
let spec_name = format_ident!("__spec_{method_name}");
let (mut bridge_sig, arg_names) = rewrite_sig_with_named_args(&m.sig);
bridge_sig.ident = spec_name;
Some(quote! {
#[inline(always)]
#bridge_sig {
<Self as #base_name #trait_ty_generics>::#method_name(self, #(#arg_names),*)
}
})
}
_ => None,
})
.collect()
}There was a problem hiding this comment.
Rust edition 2024 has been stable for years
There was a problem hiding this comment.
Items 2 and 3 were fixed in 16205cb — build_base_bridge_items now uses rewrite_sig_with_named_args for proper wildcard handling and takes trait_ty_generics for fully-qualified UFCS calls.
Generated by Claude Code
| ) -> proc_macro2::TokenStream { | ||
| let vis = &trait_item.vis; | ||
| let base_trait_items = build_base_trait_items(trait_item); | ||
| let base_bridge_items = build_base_bridge_items(trait_item, base_name); |
There was a problem hiding this comment.
The call to build_base_bridge_items must be updated to pass trait_ty_generics to ensure that the generated bridge code correctly handles generic traits.
| let base_bridge_items = build_base_bridge_items(trait_item, base_name); | |
| let base_bridge_items = build_base_bridge_items(trait_item, base_name, trait_ty_generics); |
The hot-path dispatch previously used `$raw[0] as *const $first` to recover the data pointer from a transmuted [usize; 2]. This integer- to-pointer cast strips provenance, causing Miri UB when the hot path is taken. Fix: extract the data pointer directly from the trait object reference via `ptr::from_ref`/`ptr::from_mut` + `.cast::<()>()`, preserving provenance. The vtable comparison still uses the transmuted usize (comparing integers is fine). The provenance-carrying data pointer is threaded through the dispatch chain alongside the vtable value. https://claude.ai/code/session_018DyxQYLHG2JCYbUvoVA7Ey
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/macros/src/lib.rs`:
- Around line 501-513: The current extraction of argument idents from
m.sig.inputs only collects Pat::Ident and thus drops Pat::Wild (`_`) parameters
causing arity mismatch in the bridge call; update the logic that builds
arg_names (used for the bridge invocation `#base_name::`#method_name`(self,
#(`#arg_names`),*)`) to handle Pat::Wild by generating synthetic identifiers (or
simply reuse the existing rewrite_sig_with_named_args helper which already
produces named args for wildcards) so every parameter has a corresponding ident;
ensure this change aligns with validate_trait_method which permits Pat::Wild and
preserves argument ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 73bdeeb8-3c53-4aaa-b403-731b98dbfd68
📒 Files selected for processing (16)
crates/core/examples/shapes.rscrates/core/src/lib.rscrates/core/tests/kani.rscrates/core/tests/ui/all_arms.rscrates/core/tests/ui/method_attrs.rscrates/core/tests/ui/missing_method.rscrates/core/tests/ui/missing_method.stderrcrates/core/tests/ui/multi_arg.rscrates/core/tests/ui/multi_hot.rscrates/core/tests/ui/pub_trait.rscrates/core/tests/ui/single_hot.rscrates/core/tests/ui/unsafe_trait.rscrates/core/tests/ui/wrong_signature.rscrates/core/tests/ui_attr/attr_args_on_impl.rscrates/core/tests/ui_attr/attr_args_on_impl.stderrcrates/macros/src/lib.rs
- Use rewrite_sig_with_named_args for wildcard parameter handling - Preserve default method bodies in generated Base traits - Pass trait_ty_generics through to UFCS calls in bridge methods https://claude.ai/code/session_018DyxQYLHG2JCYbUvoVA7Ey
Update README, CLAUDE.md, and lib.rs module docs to show both ways of implementing cold types: via #[devirt] (same as hot types) or via the generated FooBase trait (no devirt dependency needed). https://claude.ai/code/session_018DyxQYLHG2JCYbUvoVA7Ey
The main example now demonstrates both ways to implement a cold type: - 3a: #[devirt] on the impl block (adds #[inline], requires devirt dep) - 3b: impl ShapeBase directly (no devirt dependency needed) The separate section is refocused on the cross-crate use case. https://claude.ai/code/session_018DyxQYLHG2JCYbUvoVA7Ey
|
@coderabbitai review Generated by Claude Code |
|
@codex review Generated by Claude Code |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 82409
Overall ImpressionThe architecture is sound: introducing A few things are worth addressing before merging. 1.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13ec1604fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| $crate::__paste! { | ||
| #[inline(always)] | ||
| fn [<__spec_ $method>](&self $(, $arg: $argty)*) $(-> $ret)? { | ||
| [<$trait_name Base>]::$method(self $(, $arg)*) |
There was a problem hiding this comment.
__devirt_define!(@trait ...) now takes an explicit $base_name, but the generated blanket bridge still hardcodes [<$trait_name Base>] when forwarding each __spec_* method. If a caller provides any base name other than the exact {Trait}Base spelling, expansion produces references to a non-existent trait and fails to compile. This is a functional regression in the new macro signature and should use $base_name consistently in bridge generation.
Useful? React with 👍 / 👎.
| ); | ||
| let assoc_type_decls = &assoc_info.decls; | ||
|
|
||
| let base_name = format_ident!("{name}Base"); |
There was a problem hiding this comment.
{Trait}Base symbol name
The proc-macro expansion unconditionally creates a new public trait named {Trait}Base via format_ident!("{name}Base"). That introduces a source-breaking collision for existing modules that already define an item with that name, causing duplicate-definition compile errors after upgrade even when previous #[devirt] usage was valid. This breaks backward compatibility and should be made configurable or collision-safe.
Useful? React with 👍 / 👎.
The @blanket_bridge macro arms were reconstructing the base trait name via paste ([<$trait_name Base>]) instead of using the $base_name parameter already accepted by @trait. This made the internal API inconsistent — callers pass an explicit base name but it was ignored. https://claude.ai/code/session_018DyxQYLHG2JCYbUvoVA7Ey
|
@gemini-code-assist review Generated by Claude Code |
|
@coderabbitai review Generated by Claude Code |
|
@codex review Generated by Claude Code |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a public base trait (e.g., FooBase) for devirtualized traits, enabling downstream crates to implement cold types without a direct dependency on the devirt crate. It also refines pointer provenance handling in the dispatch logic by using from_ref and from_mut. Review feedback identifies a potential MSRV increase to 1.76 due to the use of without_provenance, a limitation in the declarative macro where default method bodies are not supported in the new blanket bridge, and the omission of method attributes in the internal implementation trait.
| // A dangling, non-null, aligned `*const __DevirtT`. | ||
| // We never dereference it — the coercion below only | ||
| // reads the vtable metadata the compiler attaches. | ||
| let fake: *const __DevirtT = ::core::ptr::without_provenance( |
There was a problem hiding this comment.
The use of ::core::ptr::without_provenance was stabilized in Rust 1.76. If this crate intends to support older Rust versions (MSRV), this will be a breaking change. However, given the use of other modern features like the expect attribute (stabilized in 1.81 for some uses), this is likely acceptable. If a lower MSRV is required, consider using addr as *const T which is the older way to create a pointer from an address.
| (@blanket_bridge $base_name:ident, | ||
| $(#[$_attr:meta])* | ||
| fn $method:ident(&self $(, $arg:ident : $argty:ty)*) $(-> $ret:ty)?; | ||
| $($rest:tt)* | ||
| ) => { | ||
| $crate::__paste! { | ||
| #[inline(always)] | ||
| fn [<__spec_ $method>](&self $(, $arg: $argty)*) $(-> $ret)? { | ||
| $base_name::$method(self $(, $arg)*) | ||
| } | ||
| } | ||
| $crate::__devirt_define!{@blanket_bridge $base_name, $($rest)*} | ||
| }; | ||
|
|
||
| // &mut self | ||
| (@blanket_bridge $base_name:ident, | ||
| $(#[$_attr:meta])* | ||
| fn $method:ident(&mut self $(, $arg:ident : $argty:ty)*) $(-> $ret:ty)?; | ||
| $($rest:tt)* | ||
| ) => { | ||
| $crate::__paste! { | ||
| #[inline(always)] | ||
| fn [<__spec_ $method>](&mut self $(, $arg: $argty)*) $(-> $ret)? { | ||
| $base_name::$method(self $(, $arg)*) | ||
| } | ||
| } | ||
| $crate::__devirt_define!{@blanket_bridge $base_name, $($rest)*} | ||
| }; |
There was a problem hiding this comment.
The @blanket_bridge macro arms in the declarative macro expansion only match method signatures ending in a semicolon (e.g., fn method(&self);). This means that traits defined using the declarative devirt! macro with default method bodies will fail to generate the necessary bridge implementations for cold types. While the declarative macro is documented as having limited syntax support, this is a notable limitation for the new Base trait functionality.
| #[inline(always)] | ||
| #bridge_sig { | ||
| <Self as #base_name #trait_ty_generics>::#method_name(self, #(#arg_names),*) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The bridge methods generated for the blanket implementation do not preserve attributes from the original trait methods. While #[inline(always)] is added, other functional attributes (like #[must_use]) or documentation are lost on the __FooImpl side. Since __FooImpl is an internal implementation detail used for dispatch, this is likely fine, but ensuring the public FooBase trait (handled in build_base_trait_items) retains them is critical.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad216cdb6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[inline(always)] | ||
| #bridge_sig { | ||
| <Self as #base_name #trait_ty_generics>::#method_name(self, #(#arg_names),*) | ||
| } |
There was a problem hiding this comment.
build_base_bridge_items drops the original method attributes when generating __spec_* methods, so a trait method with #[cfg(...)] is conditionally removed from __{Trait}Impl but still emitted unconditionally in the blanket impl __{Trait}Impl for T. In cfg-disabled builds this produces a hard compile error (method ... is not a member of trait). The bridge method needs to carry through method attributes (at least #[cfg]) to stay structurally consistent with the generated inner trait.
Useful? React with 👍 / 👎.
Without this, a trait method gated by #[cfg(feature = "x")] would be conditionally removed from __FooImpl but unconditionally emitted in the blanket bridge, causing a compile error when the cfg is disabled. Only #[cfg] attributes are carried through — other attributes like #[must_use] don't belong on internal bridge methods. https://claude.ai/code/session_018DyxQYLHG2JCYbUvoVA7Ey
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/macros/src/lib.rs`:
- Around line 501-503: The code collects conditional attributes into cfg_attrs
by filtering m.attrs for .path().is_ident("cfg"), but this omits cfg_attr and
can cause mismatched method presence; change the predicate to accept both "cfg"
and "cfg_attr" (e.g., check is_ident("cfg") || is_ident("cfg_attr")) wherever
cfg_attrs is built (referencing the binding cfg_attrs and the iteration over
m.attrs) so generated bridge methods preserve both cfg and cfg_attr attributes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 13827ee8-d455-409d-8211-a3fc1211c44d
📒 Files selected for processing (1)
crates/macros/src/lib.rs
| let cfg_attrs: Vec<_> = m.attrs.iter() | ||
| .filter(|a| a.path().is_ident("cfg")) | ||
| .collect(); |
There was a problem hiding this comment.
Include cfg_attr when propagating conditional method attributes.
Filtering only cfg drops cfg_attr, so conditionally-present/absent methods can diverge between trait items and generated bridge methods, causing trait-impl signature mismatches at compile time.
Suggested fix
- let cfg_attrs: Vec<_> = m.attrs.iter()
- .filter(|a| a.path().is_ident("cfg"))
- .collect();
+ let cfg_attrs: Vec<_> = m
+ .attrs
+ .iter()
+ .filter(|a| {
+ let p = a.path();
+ p.is_ident("cfg") || p.is_ident("cfg_attr")
+ })
+ .collect();As per coding guidelines: **/*.rs proc-macro expansion should support method attributes with full signature fidelity.
Also applies to: 509-513
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/macros/src/lib.rs` around lines 501 - 503, The code collects
conditional attributes into cfg_attrs by filtering m.attrs for
.path().is_ident("cfg"), but this omits cfg_attr and can cause mismatched method
presence; change the predicate to accept both "cfg" and "cfg_attr" (e.g., check
is_ident("cfg") || is_ident("cfg_attr")) wherever cfg_attrs is built
(referencing the binding cfg_attrs and the iteration over m.attrs) so generated
bridge methods preserve both cfg and cfg_attr attributes.
2 participants
When a library exposes a devirt trait, downstream crates previously had
to depend on devirt and use #[devirt] on their impl blocks even for cold
types. This was because the public trait is a marker with no methods —
the actual methods live on a hidden __FooImpl trait with _spec* names.
This commit adds a public FooBase trait with the original method names,
plus a blanket impl bridging FooBase → __FooImpl. Cold types can now
implement FooBase with standard Rust, no devirt dependency needed:
Hot types continue using #[devirt] for the optimized direct __FooImpl
path. The change is fully backward-compatible — all existing code using
#[devirt] on impl blocks works identically.
https://claude.ai/code/session_018DyxQYLHG2JCYbUvoVA7Ey