Rust::com Derive macro for CommData#58
Rust::com Derive macro for CommData#58saksham-chawla wants to merge 2 commits intocodex/replay-20260319023913-pr-156-basefrom
Conversation
* Implemented derive macro for CommData trait impl
* CommData should explicitly derive Reloc. This makes the dependency clear in the code.
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
There was a problem hiding this comment.
Several important macro logic issues and minor docs/UX fixes need addressing before merge.
Status: Changes Suggested | Risk: Medium | Readiness: 5/10 (Needs Polish)
Issues Identified & Suggestions
- Preserve stable IDs; avoid breaking changes to generated identifiers:
com_api_gen.rs - Handle generics and add required bounds in CommData derive:
lib.rs - Fix stray doc fence and improve attribute error messages:
lib.rs
Review Details
📁 3 files reviewed | 💬 5 comments
👍 / 👎 individual comments to help improve reviews for you
| #[derive(Debug, Reloc, CommData)] | ||
| #[repr(C)] | ||
| // No explicit ID provided, so it will be auto-generated as "com_api_gen::Exhaust" | ||
| pub struct Exhaust {} |
There was a problem hiding this comment.
[Logic] Backward-incompatible ID change: Exhaust previously had const ID: &'static str = "Exhaust" and will now auto-generate as "com_api_gen::Exhaust" (or whatever module_path!() resolves to). Since these IDs are used by the framework to match types across process boundaries (as stated in the CommData docs), this silently breaks any existing consumer/producer that was matching on "Exhaust". Either preserve the old ID with #[comm_data(id = "Exhaust")] or document this as an intentional breaking change.
| #[derive(Debug, Reloc, CommData)] | |
| #[repr(C)] | |
| // No explicit ID provided, so it will be auto-generated as "com_api_gen::Exhaust" | |
| pub struct Exhaust {} | |
| #[derive(Debug, Reloc, CommData)] | |
| #[repr(C)] | |
| #[comm_data(id = "Exhaust")] | |
| pub struct Exhaust {} |
Context for Agents
Backward-incompatible ID change: `Exhaust` previously had `const ID: &'static str = "Exhaust"` and will now auto-generate as `"com_api_gen::Exhaust"` (or whatever `module_path!()` resolves to). Since these IDs are used by the framework to match types across process boundaries (as stated in the `CommData` docs), this silently breaks any existing consumer/producer that was matching on `"Exhaust"`. Either preserve the old ID with `#[comm_data(id = "Exhaust")]` or document this as an intentional breaking change.
```suggestion
#[derive(Debug, Reloc, CommData)]
#[repr(C)]
#[comm_data(id = "Exhaust")]
pub struct Exhaust {}
```
File: score/mw/com/example/com-api-example/com-api-gen/com_api_gen.rs
Line: 26Chain of Thought
- Validation Decision: accepted
- Confidence: 0.70
- Likelihood Score: 3/5
- Likelihood Reason: The ID does change from "Exhaust" to "com_api_gen::Exhaust", which would break any consumer/producer matching on the old ID. However, this is example code explicitly documenting the auto-generation feature, so the change appears intentional. The likelihood of real breakage depends on whether any production code depends on this example's ID.
- Suggested Action: modify
- Tool Calls Used: 5
Reasoning
The comment is technically correct about the mechanics:
- In BASE,
Exhausthadconst ID: &'static str = "Exhaust"(manual impl). - In HEAD, the
#[derive(CommData)]without#[comm_data(id = "...")]will auto-generate the ID asconcat!(module_path!(), "::", stringify!(Exhaust)), confirmed by macro code at lib.rs lines 80-83. - This changes the ID from "Exhaust" to something like "com_api_gen::Exhaust" — a backward-incompatible change.
- The
Tirestruct does use#[comm_data(id = "Tire")]to preserve its old ID, showing the reviewer's suggestion is valid.
However, there are mitigating factors:
- This is example code in
score/mw/com/example/..., designed to demonstrate both usage patterns (explicit ID and auto-generated ID). - The code already has an explicit inline comment: "No explicit ID provided, so it will be auto-generated as 'com_api_gen::Exhaust'" — this documents the intentional nature of the change.
- The reviewer's own suggestion says "or document this as an intentional breaking change" — and the inline comment arguably does this.
The comment is valid in identifying the breaking change, but the suggestion to add #[comm_data(id = "Exhaust")] would defeat the purpose of demonstrating the auto-generation feature. The alternative suggestion ("document this as an intentional breaking change") is already partially addressed by the inline comment. Still, the reviewer raises a valid concern worth acknowledging.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/rust/com-api/com-api-concept/com_api_concept.rs
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
3. File Snapshot (BASE)
- File:
score/mw/com/example/com-api-example/com-api-gen/com_api_gen.rs
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
4. File Snapshot (HEAD)
- File:
score/mw/com/example/com-api-example/com-api-gen/com_api_gen.rs
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| } | ||
| Ok(None) => { | ||
| // Auto-generated: use fully qualified type name with module_path!() | ||
| quote! { concat!(module_path!(), "::", stringify!(#ident_name)) } |
There was a problem hiding this comment.
[Logic] Auto-generated ID ignores generic parameters. If a generic type like Wrapper<T> derives CommData, <Wrapper<u32> as CommData>::ID and <Wrapper<u64> as CommData>::ID both become module::Wrapper, which can cause type collisions across IPC. If generics aren't supported, reject them explicitly:
if !input_args.generics.params.is_empty() {
return syn::Error::new_spanned(
&ident_name,
"CommData cannot be derived for generic types; use a concrete wrapper or an explicit ID",
)
.to_compile_error()
.into();
}Context for Agents
Auto-generated ID ignores generic parameters. If a generic type like `Wrapper<T>` derives `CommData`, `<Wrapper<u32> as CommData>::ID` and `<Wrapper<u64> as CommData>::ID` both become `module::Wrapper`, which can cause type collisions across IPC. If generics aren't supported, reject them explicitly:
```rust
if !input_args.generics.params.is_empty() {
return syn::Error::new_spanned(
&ident_name,
"CommData cannot be derived for generic types; use a concrete wrapper or an explicit ID",
)
.to_compile_error()
.into();
}
```
File: score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs
Line: 82Chain of Thought
- Validation Decision: accepted
- Confidence: 0.95
- Likelihood Score: 4/5
- Likelihood Reason: The macro explicitly handles generics in the impl block (lines 70-72, 91) — it generates valid
impl<T> CommData for Wrapper<T>code — but the auto-generated ID at line 82 uses onlystringify!(#ident_name)which strips type parameters. Since the Reloc derive (which must precede CommData) fully supports generics, a developer will naturally try to derive both on a generic type, leading to silent ID collisions across IPC. - Suggested Action: keep
- Tool Calls Used: 2
Reasoning
The comment identifies a genuine bug in newly introduced code (this PR adds the entire derive_comm_data function).
The issue is real:
- Line 68:
let ident_name = &input_args.ident;—DeriveInput::identis just the type name without generic parameters (e.g.,Wrapper, notWrapper<T>). - Line 82:
quote! { concat!(module_path!(), "::", stringify!(#ident_name)) }—stringify!(Wrapper)produces"Wrapper"regardless of the concrete type parameter. - Therefore,
<Wrapper<u32> as CommData>::IDand<Wrapper<u64> as CommData>::IDwould both be"some_module::Wrapper".
The macro explicitly supports generics but gets the ID wrong:
- Lines 70-72 handle generics:
let (impl_generics, type_generics, where_clause) = generics.split_for_impl(); - Line 91 generates:
impl #impl_generics CommData for #ident_name #type_generics #where_clause - The companion
Relocderive also supports generics (line 182, 213-214). - So the code compiles fine for generic types but silently produces identical IDs.
This is introduced by this PR — the entire derive_comm_data function is new.
The comment is actionable — it provides a concrete code snippet to reject generics at the macro level, which is a clean and safe fix. The ID is described as being used "to match types across process boundaries" (line 21 of the doc), so collisions could cause type confusion in IPC, which is a serious correctness issue.
No existing guards prevent this: There is no check on input_args.generics.params in the derive_comm_data function.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs - Lines:
65-98
65: #[proc_macro_derive(CommData, attributes(comm_data))]
66: pub fn derive_comm_data(input: TokenStream) -> TokenStream {
67: let input_args = parse_macro_input!(input as DeriveInput);
68: let ident_name = &input_args.ident;
69:
70: //Add where clause if there are generics
71: let generics = input_args.generics.clone();
72: let (impl_generics, type_generics, where_clause) = generics.split_for_impl();
73:
74: // Generate ID string: use user-provided ID if available, otherwise use fully qualified type name
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs - Lines:
167-260
167: #[proc_macro_derive(Reloc)]
168: pub fn derive_reloc(input: TokenStream) -> TokenStream {
169: let input_args = parse_macro_input!(input as DeriveInput);
170: let ident_name = &input_args.ident;
171:
172: // Ensure #[repr(C)] on the struct itself
173: if !has_repr_c(&input_args.attrs) {
174: return syn::Error::new_spanned(
175: &ident_name,
176: "The #[derive(Reloc)] macro requires #[repr(C)] on the type",
...
| let comm_data_impl = quote! { | ||
| impl #impl_generics CommData for #ident_name #type_generics #where_clause { | ||
| const ID: &'static str = #id_str; | ||
| } | ||
| }; |
There was a problem hiding this comment.
[Logic] The generated impl CommData never carries over the bounds required by the CommData supertraits. For a generic type
#[derive(Reloc, CommData)]
#[repr(C)]
pub struct Wrapper<T> {
inner: T,
}Reloc derive expands to unsafe impl<T> Reloc for Wrapper<T> where T: Reloc {}, but the code here emits impl<T> CommData for Wrapper<T> { … } with an empty where clause. The compiler therefore has to prove Wrapper<T>: Reloc + Send + Debug + 'static for all T, which is impossible and currently makes the derive fail for every generic struct/enum (the trait bound Wrapper<T>: Reloc is not satisfied).
Please push the required predicate onto the generated where clause before split_for_impl(), e.g.
let mut generics = input_args.generics.clone();
{
let where_clause = generics.make_where_clause();
where_clause.predicates.push(parse_quote!(
#ident_name #type_generics: Reloc + ::core::fmt::Debug + ::core::marker::Send + 'static
));
}
let (impl_generics, type_generics, where_clause) = generics.split_for_impl();(Using #ident_name #type_generics or Self for the predicate is fine.) This restricts the CommData impl to exactly the cases where the supertraits are satisfied, allowing generic types to derive successfully.
Context for Agents
The generated `impl CommData` never carries over the bounds required by the `CommData` supertraits. For a generic type
```rust
#[derive(Reloc, CommData)]
#[repr(C)]
pub struct Wrapper<T> {
inner: T,
}
```
`Reloc` derive expands to `unsafe impl<T> Reloc for Wrapper<T> where T: Reloc {}`, but the code here emits `impl<T> CommData for Wrapper<T> { … }` with an empty `where` clause. The compiler therefore has to prove `Wrapper<T>: Reloc + Send + Debug + 'static` for *all* `T`, which is impossible and currently makes the derive fail for every generic struct/enum (`the trait bound Wrapper<T>: Reloc is not satisfied`).
Please push the required predicate onto the generated `where` clause before `split_for_impl()`, e.g.
```rust
let mut generics = input_args.generics.clone();
{
let where_clause = generics.make_where_clause();
where_clause.predicates.push(parse_quote!(
#ident_name #type_generics: Reloc + ::core::fmt::Debug + ::core::marker::Send + 'static
));
}
let (impl_generics, type_generics, where_clause) = generics.split_for_impl();
```
(Using `#ident_name #type_generics` or `Self` for the predicate is fine.) This restricts the `CommData` impl to exactly the cases where the supertraits are satisfied, allowing generic types to derive successfully.
File: score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs
Line: 94Chain of Thought
- Validation Decision: accepted
- Confidence: 0.95
- Likelihood Score: 5/5
- Likelihood Reason: Any use of
#[derive(CommData)]on a generic struct/enum will fail to compile. The code even has a comment saying "Add where clause if there are generics" but doesn't actually add any bounds. This is inevitable for any generic type usage. - Suggested Action: keep
- Tool Calls Used: 2
Reasoning
The comment is accurate, well-supported by the code, and actionable.
-
Accuracy: The
CommDatatrait requires supertraitsReloc + Send + Debug + 'static(line 211 ofcom_api_concept.rs). Thederive_comm_datamacro (lines 70-72) clones the input generics and callssplit_for_impl()without pushing any supertrait bounds onto the where clause. For a generic type likeWrapper<T>, this producesimpl<T> CommData for Wrapper<T> {}with no bounds, making it impossible for the compiler to verify thatWrapper<T>satisfies theCommDatasupertraits for allT. -
Contrast with derive_reloc: The
derive_relocmacro correctly handles generics by callingcreate_bounds_with_reloc()(which addsT: Relocto type params) and then pushing field-type bounds. Thederive_comm_datamacro does neither. -
Introduced by this PR: The entire
derive_comm_datafunction is new code introduced in this PR. -
Actionable: The comment provides a specific, correct fix — push
#ident_name #type_generics: Reloc + Debug + Send + 'staticonto the where clause before callingsplit_for_impl(). The code pattern is consistent with howderive_relochandles similar issues. -
Ironic evidence: Line 70 has the comment "Add where clause if there are generics" but the subsequent code doesn't actually add any bounds — it just clones and splits.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
2. File Snapshot (HEAD)
- File:
score/mw/com/impl/rust/com-api/com-api-concept/com_api_concept.rs
1: /********************************************************************************
2: * Copyright (c) 2025 Contributors to the Eclipse Foundation
3: *
4: * See the NOTICE file(s) distributed with this work for additional
5: * information regarding copyright ownership.
6: *
7: * This program and the accompanying materials are made available under the
8: * terms of the Apache License Version 2.0 which is available at
9: * https://www.apache.org/licenses/LICENSE-2.0
10: *
...
| /// value: u32, | ||
| /// } | ||
| /// ``` | ||
| /// ``` |
There was a problem hiding this comment.
[Documentation] Stray doc fence: line 64 opens a new ``` code block but the doc comment ends immediately after, leaving it unterminated. This will cause a rustdoc warning or error. Remove the orphaned fence.
| /// ``` | |
| /// ``` | |
| #[proc_macro_derive(CommData, attributes(comm_data))] |
Context for Agents
Stray doc fence: line 64 opens a new ` ``` ` code block but the doc comment ends immediately after, leaving it unterminated. This will cause a rustdoc warning or error. Remove the orphaned fence.
```suggestion
/// ```
#[proc_macro_derive(CommData, attributes(comm_data))]
```
File: score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs
Line: 64Chain of Thought
- Validation Decision: accepted
- Confidence: 0.97
- Likelihood Score: 5/5
- Likelihood Reason: The unterminated code fence is clearly present at line 64. Rustdoc will inevitably produce a warning or error about the unclosed code block when documentation is generated.
- Suggested Action: keep
- Tool Calls Used: 1
Reasoning
Tracing the code fence pairs in the doc comment for derive_comm_data (lines 18-64):
- Line 33
opens, line 35closes → code block 1 (attribute example) - Line 38
ignore opens, line 49closes → code block 2 (usage example) - Line 51
ignore opens, line 63closes → code block 3 (custom ID example) - Line 64 ``` opens → code block 4... NEVER CLOSED
Line 65 is #[proc_macro_derive(CommData, attributes(comm_data))], which is not a doc comment, so the doc comment ends at line 64. The fence at line 64 opens a code block with no closing fence, making it unterminated.
This is new code introduced by this PR (line 64 is marked as "added"). The fix is straightforward: remove line 64. The comment correctly identifies the issue and provides an actionable suggestion.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs - Lines:
32-66
32: /// It also supports an optional attribute to specify a custom ID string:
33: /// ```
34: /// #[comm_data(id = "CustomID_String")]
35: /// ```
36: /// If no custom ID is provided, a fully qualified type name is used as the ID.
37: /// Example usage:
38: /// ```ignore
39: /// pub unsafe trait Reloc {}
40: /// pub trait CommData: Reloc + Send + 'static {
41: /// const ID: &'static str;
...
| if !nv.path.is_ident("id") { | ||
| return Err(syn::Error::new_spanned( | ||
| &nv.path, | ||
| "Expected #[comm_data(id = \"...\")]", |
There was a problem hiding this comment.
[Maintainability] The error message here is generic. Since you've already confirmed the attribute is comm_data and are now checking the key inside it, you can provide a more specific error message to the user.
| if !nv.path.is_ident("id") { | |
| return Err(syn::Error::new_spanned( | |
| &nv.path, | |
| "Expected #[comm_data(id = \"...\")]", | |
| "unexpected attribute key, expected `id`", |
This principle applies to other error cases in this function as well (e.g., for non-string literal values). More specific error messages greatly improve the usability of procedural macros.
Context for Agents
The error message here is generic. Since you've already confirmed the attribute is `comm_data` and are now checking the key inside it, you can provide a more specific error message to the user.
```suggestion
"unexpected attribute key, expected `id`",
```
This principle applies to other error cases in this function as well (e.g., for non-string literal values). More specific error messages greatly improve the usability of procedural macros.
File: score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs
Line: 124Chain of Thought
- Validation Decision: accepted
- Confidence: 0.82
- Likelihood Score: 3/5
- Likelihood Reason: Users who mistype the attribute key will see a generic "Expected #[comm_data(id = ...)]" message rather than one that specifically says their key name is wrong. While the generic message still shows the correct format, a more targeted message improves usability. This is a plausible usability issue for proc macro users.
- Suggested Action: keep
- Tool Calls Used: 1
Reasoning
The comment targets line 124 in the newly added extract_id_from_attribute function. At this point in the code, the function has already confirmed:
- The attribute is
comm_data(line 103) - The meta is a
Meta::List(line 107) - The parsed args are a
Meta::NameValue(line 114) - But the key is NOT
id(line 121)
The error at line 124 currently says "Expected #[comm_data(id = \"...\")]" — the same generic message used for multiple different validation failures (lines 110, 117, 124). The reviewer correctly identifies that since we know the specific failure mode (wrong key name), a more targeted message like "unexpected attribute key, expected \id`"` would help users diagnose their mistake faster.
The comment is:
- Accurate: It correctly identifies that the same error message is reused across different failure modes, reducing diagnostic precision.
- Actionable: It provides a concrete replacement string.
- Introduced by this PR: The entire function is new code added in this PR.
- Valid suggestion: More specific proc macro error messages are a well-established best practice, and the suggestion is reasonable.
The existing generic message is not terrible (it does show the expected syntax), so this is more of a quality improvement than a critical bug, but it's still a valid and actionable review comment.
Evidence Summary
1. File Snapshot (HEAD)
- File:
score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs - Lines:
100-153
100: /// Extract custom ID from #[comm_data(id = "...")] attribute
101: fn extract_id_from_attribute(attrs: &[syn::Attribute]) -> Result<Option<String>, syn::Error> {
102: for attr in attrs {
103: if !attr.path().is_ident("comm_data") {
104: continue;
105: }
106:
107: let Meta::List(list) = &attr.meta else {
108: return Err(syn::Error::new_spanned(
109: &attr,
...
2 participants
eclipse-score#155