crates/derive-impl/src/pyclass.rs (2)

337-353: Named-field branch silently skips the assertion if ident is None.

For syn::Fields::Named, first_field.ident is guaranteed to be Some by the parser, so ident.as_ref().map(|id| quote! { #id }) returning None is unreachable in practice. Since the caller treats Ok(None) as "no assertion to emit", a logic bug here would silently disable the layout safety net instead of erroring. Consider expect‑ing the invariant or returning Some unconditionally so the guarantee is explicit:

♻️ Make the invariant explicit

-            let ident = first_field.ident.as_ref().map(|id| quote! { `#id` });
-            Ok(ident)
+            let ident = first_field
+                .ident
+                .as_ref()
+                .expect("syn::Fields::Named always has field idents");
+            Ok(Some(quote! { `#ident` }))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/derive-impl/src/pyclass.rs` around lines 337 - 353, In the named-field
branch of the match (handling syn::Fields::Named), the current mapping of
first_field.ident to ident can produce Ok(None) which silently disables the
assertion; make the parser invariant explicit by unwrapping/expect-ing
first_field.ident (e.g., use first_field.ident.as_ref().expect(...)) and return
an owned Some(quote! { `#id` }) so the function returns Ok(Some(...)) instead of
Ok(None); this ensures the #[pyclass] base-type assertion (checked via
type_matches_path and first_field) cannot be bypassed silently.

---

384-394: ensure_repr_c opts out on any #[repr(...)], including ones that don't guarantee offset‑0.

has_repr matches any repr attribute, so a user who writes only #[repr(align(N))] (or #[repr(packed)] without C) will not get #[repr(C)] auto‑inserted even though those reprs alone don't pin the base field to offset 0 under current Rust layout rules. In practice the offset_of! assertion emitted at lines 606–619 catches this at compile time, so this is not a correctness gap — just worth being explicit about in the comment so future maintainers don't assume ensure_repr_c alone is sufficient.

Optionally, you could narrow the opt‑out to reprs that already guarantee declaration order (C, transparent) and still layer the assert on top:

♻️ Narrower opt‑out

-    let has_repr = s.attrs.iter().any(|attr| attr.path().is_ident("repr"));
-    if !has_repr {
+    // Only treat reprs that guarantee declaration order as an opt-out;
+    // `align`/`packed` alone do not pin the base field to offset 0.
+    let has_layout_repr = s.attrs.iter().any(|attr| {
+        if !attr.path().is_ident("repr") {
+            return false;
+        }
+        let mut found = false;
+        let _ = attr.parse_nested_meta(|meta| {
+            if meta.path.is_ident("C") || meta.path.is_ident("transparent") {
+                found = true;
+            }
+            Ok(())
+        });
+        found
+    });
+    if !has_layout_repr {
         let repr_c: syn::Attribute = parse_quote!(#[repr(C)]);
         s.attrs.push(repr_c);
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/derive-impl/src/pyclass.rs` around lines 384 - 394, ensure_repr_c
currently treats any #[repr(...)] as reason to skip adding #[repr(C)], which
incorrectly skips structs that use #[repr(align(...))] or #[repr(packed)] that
don't guarantee declaration-order layout; update ensure_repr_c so it only opts
out when an existing repr explicitly guarantees declaration order (i.e., path
is_ident "repr" and contains "C" or "transparent"), otherwise push #[repr(C)] as
before; also add a short clarifying comment in the ensure_repr_c function noting
that the offset_of! assertion elsewhere still protects against remaining cases.

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/derive-impl/src/pyclass.rs`:
- Around line 337-353: In the named-field branch of the match (handling
syn::Fields::Named), the current mapping of first_field.ident to ident can
produce Ok(None) which silently disables the assertion; make the parser
invariant explicit by unwrapping/expect-ing first_field.ident (e.g., use
first_field.ident.as_ref().expect(...)) and return an owned Some(quote! { `#id` })
so the function returns Ok(Some(...)) instead of Ok(None); this ensures the
#[pyclass] base-type assertion (checked via type_matches_path and first_field)
cannot be bypassed silently.
- Around line 384-394: ensure_repr_c currently treats any #[repr(...)] as reason
to skip adding #[repr(C)], which incorrectly skips structs that use
#[repr(align(...))] or #[repr(packed)] that don't guarantee declaration-order
layout; update ensure_repr_c so it only opts out when an existing repr
explicitly guarantees declaration order (i.e., path is_ident "repr" and contains
"C" or "transparent"), otherwise push #[repr(C)] as before; also add a short
clarifying comment in the ensure_repr_c function noting that the offset_of!
assertion elsewhere still protects against remaining cases.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 91b82e56-aea9-4877-885c-ff2bee16fcd8

Reviewing files that changed from the base of the PR and between 5081f76 and 48311be.

• crates/derive-impl/src/pyclass.rs
• crates/stdlib/src/_asyncio.rs
• crates/vm/src/builtins/builtin_func.rs

• crates/vm/src/builtins/builtin_func.rs
• crates/stdlib/src/_asyncio.rs

crates/derive-impl/src/pyclass.rs (1)

378-394: Auto-inserted #[repr(C)] + compile-time offset_of! assertion is a solid two-layer defense.

The combination of (a) defaulting bare #[pyclass] derived structs to #[repr(C)] and (b) emitting a const _: () offset_of! assertion for any struct that already carries an explicit repr is a good belt-and-suspenders approach: the common case gets a correct layout for free, and the pathological case fails at compile time instead of segfaulting at runtime.

One minor nit on the diagnostic text in the const assert (Lines 617-621): #[repr(transparent)] is only valid for structs with a single non-ZST field, so suggesting it alongside #[repr(C)] as an equally-good fix could mislead users of multi-field derived classes (which is exactly the case the PR is motivated by). Consider softening the suggestion, e.g. "Add #[repr(C)] (or remove the explicit repr so the macro inserts it)".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/derive-impl/src/pyclass.rs` around lines 378 - 394, Update the
diagnostic text emitted alongside the compile-time `offset_of!` assertion so it
does not suggest `#[repr(transparent)]` as an equally-valid fix for multi-field
structs; instead soften the suggestion to something like "Add `#[repr(C)]` (or
remove the explicit repr so the macro inserts it)". Locate the code that
generates the `const _: ()` `offset_of!` assertion (the code that builds the
diagnostic string next to the `offset_of!` check for explicit reprs — referenced
in this diff as the assertion emitted when an explicit repr is present) and
replace the message text accordingly; keep the rest of the assertion logic
intact.

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crates/derive-impl/src/pyclass.rs`:
- Around line 378-394: Update the diagnostic text emitted alongside the
compile-time `offset_of!` assertion so it does not suggest
`#[repr(transparent)]` as an equally-valid fix for multi-field structs; instead
soften the suggestion to something like "Add `#[repr(C)]` (or remove the
explicit repr so the macro inserts it)". Locate the code that generates the
`const _: ()` `offset_of!` assertion (the code that builds the diagnostic string
next to the `offset_of!` check for explicit reprs — referenced in this diff as
the assertion emitted when an explicit repr is present) and replace the message
text accordingly; keep the rest of the assertion logic intact.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 0b5c333e-4a4f-47d8-aea0-aa944364fdf4

Reviewing files that changed from the base of the PR and between 48311be and 2980ad1.

• crates/derive-impl/src/pyclass.rs

I have a question about the current situation. As I understand it, #[repr(C)] is only required for types that have derived implementations. Is that correct?
If even base types without any derived implementations also require #[repr(C)], then I would agree with this patch. Otherwise, I would prefer to keep #[repr(C)] explicit, and rather than having the macro automatically add #[repr(C)], I would prefer it to emit a warning when a base type is missing #[repr(C)].

Thank you for the explanation.