Rust::com Derive macro for CommData by bharatGoswami8 · Pull Request #156 · eclipse-score/communication

bharatGoswami8 · 2026-02-06T10:10:08Z

Implemented derive macro for CommData trait impl

pawelrutkaq

LGTM, one issues found

pawelrutkaq · 2026-03-17T12:22:02Z

+///
+/// This macro **only** validates the optional `#[comm_data(id = "…")]` attribute. It does
+/// **not** check `#[repr(C)]`, field relocatability, or enum layout — those invariants are
+/// already enforced by `#[derive(Reloc)]`, which must be placed before this macro.


I think this is not needed since compiler will tell you that anyway

pawelrutkaq · 2026-03-17T12:27:47Z

+/// ```
+/// ```
+#[proc_macro_derive(CommData, attributes(comm_data))]
+pub fn derive_comm_data(input: TokenStream) -> TokenStream {


I think this opens something like:

#[derive(Debug, Reloc, CommData)] #[repr(C)] pub struct Tire { #[comm_data(id = "Tire")] pub pressure: f32, }

And I dont see whether we forbit this anywhere as compilation error ?

Agreed, this is gap in current macro implementation, when #[comm_data(id = "Tire")] is placed on a field, it lives in the field's attribute, which the macro never inspects. So the macro falls back to the auto-generated ID (module_path::Tire), silently discarding the user's annotation. No error, no warning.

we will add validation check to iterate over all fields and emit a compile error if any field carries a comm_data attribute.

pawelrutkaq · 2026-03-17T12:30:50Z

+/// they can use the `#[comm_data(id = "CustomID")]` attribute on the struct definition.
+/// The custom ID must be unique across all communication data types to avoid conflicts in the system.
+/// If no custom ID is provided, the struct name will be used as the default ID with module path as prefix to ensure uniqueness.
 pub trait CommData: Reloc + Send + Debug + 'static {


Now I think Reloc shall be Reloc: Send + 'static because Reloc is responsible for "allowing a type to be safely shared between processes" which means it can be Send and it 'static.

The CommData is only extension over with custom ID thus CommData: Reloc

The suggested change does not affect users at the API level, users will continue to use the macro in exactly the same way: #[derive(Reloc, CommData)].

The difference is purely conceptual and relates to how constraints are owned and expressed.

Current approach: CommData: Reloc + Send + Debug + 'static
All requirements are explicitly declared on CommData. The full contract is visible in one place, making it easy to understand at a glance.

suggested approach: Reloc: Send + 'static, followed by CommData: Reloc + Debug
Some requirements are moved into the trait hierarchy. As a result, CommData’s full set of bounds becomes implicit through Reloc, rather than being stated directly.

Maybe even
Reloc: Send + Unpin + 'static and CommData: Reloc (NOT Debug, though, or why would that be strictly necessary..?)

Debug is required because we want to support printing the value of T and enforce the bound through CommData alone. Therefore, keeping the Debug bound within CommData is fine in my opinion, otherwise, we would have to introduce an unnecessary dual bound - T: CommData + Debug.

I think Debug shall be bound to API itself and not here, even though you need to state it in many places later (agree with @darkwisebear ). Is Unpin here something really necessary? Thie pin history is future workound in Rust as i remeber and not sure if here we really need to mention it ?

Unpin means that a type can be moved within the same address space by means of a simple memcpy. CommData means that a type can be moved between different address spaces. So while it's not strictly needed by the API, imo it's a strict subset of CommData, so for consistency reasons, it should be included.

@darkwisebear , @pawelrutkaq ,
Added the code changes as per above discussion.

have a feeling that this is just overspecification, but since Unpint is auto impl trait, I think it does not matter.
Anyway, user needs to know that it has !Unpin type (by ie placing PhantomPin) otherwise his type will be Unpin .

* Implemented derive macro for CommData trait impl

* CommData should explicitly derive Reloc. This makes the dependency clear in the code.

* Moved trait bound from CommData to Reloc trait * Added field check on comm_data attribute

* Removed Debug trait bound from CommData trait

pawelrutkaq

Fine for me. However I would do next PR with ScoreDebug trait as this is needed in Score to log.

darkwisebear · 2026-03-23T12:08:16Z

Fine for me. However I would do next PR with ScoreDebug trait as this is needed in Score to log.

We need to talk about ScoreDebug in our Wednesday meeting. I cannot believe that we need something like this.

pawelrutkaq · 2026-03-23T14:26:51Z

Fine for me. However I would do next PR with ScoreDebug trait as this is needed in Score to log.

We need to talk about ScoreDebug in our Wednesday meeting. I cannot believe that we need something like this.

We can, however, keep in mind we discussed it around Q3.2025 and at that point we said that if the logging team requires full type information, it has to be developed and it's nothing you can do.

bharatGoswami8 requested review from LittleHuba, castler and hoe-jo as code owners February 6, 2026 10:10

bharatGoswami8 marked this pull request as draft February 6, 2026 10:10

bharatGoswami8 requested review from darkwisebear and pawelrutkaq February 6, 2026 10:10

pawelrutkaq requested changes Feb 9, 2026

View reviewed changes

bharatGoswami8 force-pushed the rust_com_api_derive_macro_commData branch from 719a3c7 to 7524795 Compare February 10, 2026 03:18

bharatGoswami8 requested a review from pawelrutkaq February 10, 2026 03:30

pawelrutkaq requested changes Feb 10, 2026

View reviewed changes

Comment thread score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs Outdated

Comment thread score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs Outdated

Comment thread score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs Outdated

bharatGoswami8 force-pushed the rust_com_api_derive_macro_commData branch from 7524795 to 14a7f51 Compare February 12, 2026 08:14

bharatGoswami8 requested a review from pawelrutkaq February 12, 2026 08:18

pawelrutkaq reviewed Feb 18, 2026

View reviewed changes

Comment thread score/mw/com/example/com-api-example/com-api-gen/com_api_gen.rs

bharatGoswami8 self-assigned this Mar 6, 2026

pawelrutkaq previously approved these changes Mar 6, 2026

View reviewed changes

darkwisebear reviewed Mar 7, 2026

View reviewed changes

Comment thread score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs Outdated

Comment thread score/mw/com/impl/rust/com-api/com-api-concept-macros/lib.rs Outdated

bharatGoswami8 dismissed pawelrutkaq’s stale review via 57e641a March 17, 2026 03:21

bharatGoswami8 force-pushed the rust_com_api_derive_macro_commData branch from 14a7f51 to 57e641a Compare March 17, 2026 03:21

bharatGoswami8 marked this pull request as ready for review March 17, 2026 03:22

bharatGoswami8 requested review from bemerybmw, crimson11 and limdor as code owners March 17, 2026 03:22

bharatGoswami8 force-pushed the rust_com_api_derive_macro_commData branch 2 times, most recently from 57e641a to ba2b668 Compare March 17, 2026 03:26

bharatGoswami8 requested review from darkwisebear and pawelrutkaq March 17, 2026 03:26

bharatGoswami8 added the rust-api label Mar 17, 2026

bharatGoswami8 force-pushed the rust_com_api_derive_macro_commData branch 2 times, most recently from c5fd1f6 to 23e00d4 Compare March 17, 2026 04:03

pawelrutkaq requested changes Mar 17, 2026

View reviewed changes

saksham-chawla mentioned this pull request Mar 18, 2026

Rust::com Derive macro for CommData propel-gtm/communication#12

Closed

bharatGoswami8 force-pushed the rust_com_api_derive_macro_commData branch from 23e00d4 to 8fcb3a7 Compare March 20, 2026 10:14

bharatGoswami8 requested a review from pawelrutkaq March 20, 2026 10:14

bharatGoswami8 added 4 commits March 20, 2026 15:49

Rust::com Derive macro for CommData

8e5d36f

* Implemented derive macro for CommData trait impl

Rust::com Added Reloc macro separate from CommData derive macro

869766c

* CommData should explicitly derive Reloc. This makes the dependency clear in the code.

Rust::com Added Reloc trait bound Send + Unpin + 'static

4063147

* Moved trait bound from CommData to Reloc trait * Added field check on comm_data attribute

Rust::com Debug removed from CommData trait bound

1dae308

* Removed Debug trait bound from CommData trait

bharatGoswami8 force-pushed the rust_com_api_derive_macro_commData branch from 8fcb3a7 to 1dae308 Compare March 20, 2026 10:19

pawelrutkaq approved these changes Mar 20, 2026

View reviewed changes

darkwisebear approved these changes Mar 23, 2026

View reviewed changes

castler approved these changes Mar 23, 2026

View reviewed changes

castler added this pull request to the merge queue Mar 23, 2026

github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Mar 23, 2026

castler added this pull request to the merge queue Mar 24, 2026

github-merge-queue Bot removed this pull request from the merge queue due to no response for status checks Mar 24, 2026

LittleHuba added this pull request to the merge queue Mar 24, 2026

Merged via the queue into eclipse-score:main with commit 057f6d5 Mar 24, 2026
10 checks passed

bharatGoswami8 mentioned this pull request Mar 24, 2026

Improvement: COM-API CommData derive macro #155

Closed

2 tasks

Conversation

bharatGoswami8 commented Feb 6, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pawelrutkaq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pawelrutkaq left a comment

Choose a reason for hiding this comment

Uh oh!

darkwisebear commented Mar 23, 2026

Uh oh!

Uh oh!

pawelrutkaq commented Mar 23, 2026

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants