Support associated types and generic trait parameters#25
Merged
Conversation
Associated types get partial devirtualization: methods that don't reference any associated type in their signature get full vtable-cmp dispatch, while methods that do reference one fall back to vtable dispatch. Generic trait parameters disable devirtualization entirely but the trait compiles and works correctly with dyn Trait. https://claude.ai/code/session_017idUHxWBY7bSNFQQMb4Rm4
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughExtends devirtualization to support trait associated types and trait generics, updates proc-macro expansion to collect associated-type info and rewrite method signatures via a syn visitor, enables the Changes
Sequence Diagram(s)sequenceDiagram
participant Macro as Devirt Macro
participant Validator as Trait Validator
participant Collector as AssocType Collector
participant Rewriter as Visitor (syn::visit_mut)
participant Generator as Code Generator
Macro->>Validator: validate_trait(trait_def)
Validator-->>Macro: allow generics & assoc types
Macro->>Collector: collect_assoc_types(trait_def)
Collector-->>Macro: AssocTypeInfo
Macro->>Rewriter: rewrite methods (Self::AssocType -> generics)
Rewriter-->>Macro: rewritten method signatures
Macro->>Generator: build dyn type + fat-pointer assertion
Generator-->>Macro: vtable helpers, shims, impls (dispatch or fallback)
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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/macros/src/lib.rs`:
- Around line 209-230: The current build_assert_ty() hardcodes witness types (()
, 'static , { 0 }) which over-constrains generic bounds and breaks non-integer
const params; instead generate synthetic generic parameters that mirror the
original trait_generic_params and associated-type idents (preserving their
bounds and const types) and use those synthetic idents as the arguments when
constructing the assertion type (*const dyn `#name`<...>), so the compile-time
size_of assertion uses generics with the same bounds rather than concrete
placeholder values; update build_assert_ty to emit those synthetic
type/lifetime/const parameters and substitute them into the *const dyn
`#name`<#(`#args`),*> instantiation (and remove the hardcoded (), 'static, { 0 }
tokens).
- Around line 145-173: The rewrite currently drops generic args and trailing
projections when replacing Self::Assoc paths; update
RewriteSelfAssocTypes::visit_path_mut to preserve any remaining path segments
and their GenericArguments instead of replacing the entire path with the
synthetic ident from AssocTypeInfo (produced by collect_assoc_types).
Concretely: when matching a path of form Self::X{...} or Self::X::Y, locate the
segment index for the assoc type (currently using i.segments[1].ident), replace
only that segment's ident with the corresponding synthetic ident (from
AssocTypeInfo.idents/generics mapping) and keep the rest of i.segments
(including segment.arguments and any subsequent segments) intact; if you prefer
to reject unsupported forms, add an explicit check in visit_path_mut that
detects GenericArguments or extra segments beyond the assoc segment and emit a
compile_error or panic with a clear message mentioning RewriteSelfAssocTypes and
AssocTypeInfo so callers know to avoid GATs or projections.
🪄 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: a7ad8d34-400d-4a05-85d0-37f0e813ddae
📒 Files selected for processing (7)
Cargo.tomlcrates/core/tests/equivalence.rscrates/core/tests/ui_attr.rscrates/core/tests/ui_attr/attr_assoc_default.rscrates/core/tests/ui_attr/attr_assoc_type.rscrates/core/tests/ui_attr/attr_generic_trait.rscrates/macros/src/lib.rs
- Replace hardcoded witness types ((), 'static, { 0 }) in the fat-ptr
assertion with synthetic generic parameters that mirror the trait's
own generics and associated type bounds, avoiding breakage with
non-integer const params or custom-bounded associated types.
- Fix RewriteSelfAssocTypes to preserve generic arguments and trailing
path segments (e.g. Self::Color<T>::Inner → __Color<T>::Inner)
instead of replacing the entire path.
https://claude.ai/code/session_017idUHxWBY7bSNFQQMb4Rm4
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/macros/src/lib.rs`:
- Around line 319-327: predicate_references_self only inspects pt.bounded_ty and
misses occurrences of "Self" in bounds (e.g., `T: Into<Self>`); update
predicate_references_self to traverse the entire syn::WherePredicate using a
visitor: implement a SelfFinder struct implementing syn::visit::Visit that sets
found=true when visiting a syn::Path whose first segment ident == "Self", then
call syn::visit::visit_where_predicate(&mut finder, pred) and return
finder.found so any appearance of Self anywhere in the predicate is detected.
- Around line 287-296: The generated const block defines fn
__devirt_assert<#(`#fn_params`),*>() `#where_clause` but never calls it, so the
assert isn't monomorphized for generic traits; update the code generation to add
a clear doc comment above the const/ fn __devirt_assert explaining that this
compile-time fat-pointer size check cannot be evaluated for generic trait
parameters (the function is intentionally not invoked), and document the
limitation and rationale so future maintainers (reading the const block/
__devirt_assert and the surrounding generated code) understand why the assertion
only effectively validates non-generic traits.
🪄 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: 7dc4277a-0bba-4bc2-9f67-c7a942862bff
📒 Files selected for processing (1)
crates/macros/src/lib.rs
- predicate_references_self now uses a visitor to find Self anywhere in a where predicate (e.g. T: Into<Self>), not just as the bounded type. - Add comment explaining why the generic-trait fat-pointer assertion is a type-well-formedness check rather than a compile-time size assertion. https://claude.ai/code/session_017idUHxWBY7bSNFQQMb4Rm4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dyn Traitanddyn Trait + Sendsyn'svisitfeature for signature analysis, new helper functions for associated type detection andSelf::AssocTyperewriting in inherent impl signaturesTest plan
attr_assoc_type.rs— associated type trait with mixed devirt/fallback methods, multiple concrete Color bindings, Send dispatchattr_assoc_default.rs— associated type trait with default method body calling a devirtualizable methodattr_generic_trait.rs— generic trait with multiple impl specializations and Send dispatchcargo clippy --workspace --exclude devirt-fuzz --all-targets -- -D warningspasses clean