Skip to content

Emit warning on non-FFI safe spine types#75

Merged
IzaacMammadov merged 2 commits into
mainfrom
im/spine_messages_ffi_safety
Apr 20, 2026
Merged

Emit warning on non-FFI safe spine types#75
IzaacMammadov merged 2 commits into
mainfrom
im/spine_messages_ffi_safety

Conversation

@IzaacMammadov
Copy link
Copy Markdown
Contributor

The warning will look like this underlined on the type in the spine:

extern block uses type std::option::Option<flux::flux_timing::Nanos>, which is not FFI-safe
consider adding a #[repr(C)], #[repr(transparent)], or integer #[repr(...)] attribute to this enum
enum has no representation hint
#[warn(improper_ctypes)] on by default (rustc improper_ctypes)

In this case, it's pointing out that one of the fields in a type of the spine is an Option which is not FFI-safe.

@IzaacMammadov IzaacMammadov changed the title Im/spine messages ffi safety Emit warning on non-FFI safe spine types Apr 20, 2026
@IzaacMammadov IzaacMammadov marked this pull request as ready for review April 20, 2026 10:45
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review

@IzaacMammadov IzaacMammadov changed the base branch from im/clippy to main April 20, 2026 11:16
@IzaacMammadov IzaacMammadov requested a review from a team April 20, 2026 11:16
@IzaacMammadov IzaacMammadov force-pushed the im/spine_messages_ffi_safety branch from aec2218 to 309edb2 Compare April 20, 2026 11:17
@IzaacMammadov IzaacMammadov merged commit b0e1cd0 into main Apr 20, 2026
1 of 2 checks passed
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 4 additional findings in Devin Review.

Open in Devin Review

Comment on lines +172 to +175
let check_fn = format_ident!("_ffi_check_{}", field_ident);
let inner_ty_span = inner_ty.span();
ffi_check_items
.push(quote_spanned! { inner_ty_span => fn #check_fn(var: *const #inner_ty); });
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Generated FFI check function names lack struct-name scoping, causing duplicate symbol errors when multiple #[from_spine] structs exist in the same module

The generated extern "C" function names use only the field name (_ffi_check_{field_ident}), not the struct name. If two #[from_spine] structs in the same module share any field name (e.g., every spine struct has tile_info: ShmemData<TileInfo>), the macro expansions produce duplicate extern function declarations (fn _ffi_check_tile_info(...)) in the module namespace, triggering a Rust E0428 duplicate definition error.

Example collision

Two structs in the same module:

#[from_spine("app1")]
struct Spine1 { pub tile_info: ShmemData<TileInfo>, pub qa: SpineQueue<MsgA> }
#[from_spine("app2")]
struct Spine2 { pub tile_info: ShmemData<TileInfo>, pub qb: SpineQueue<MsgB> }

Both expand to:

unsafe extern "C" { fn _ffi_check_tile_info(var: *const TileInfo); ... }
unsafe extern "C" { fn _ffi_check_tile_info(var: *const TileInfo); ... }

This is a duplicate definition error.

Prompt for agents
The generated FFI check function names at two locations (lines 172 and 322 in crates/spine-derive/src/lib.rs) use format_ident!("_ffi_check_{}", field_ident) which only incorporates the field name. To avoid duplicate symbol errors when multiple from_spine structs coexist in the same module, the struct name should be included in the generated function name. Change both occurrences to format_ident!("_ffi_check_{}_{}", struct_ident, field_ident). The struct_ident variable is already in scope at both locations.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@IzaacMammadov IzaacMammadov deleted the im/spine_messages_ffi_safety branch April 20, 2026 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants