refactor: improve macro derive - #318
Conversation
📝 WalkthroughWalkthroughCentralizes string-based enum parsing and ProtocolNode attribute handling: adds StringEnum support and ParseStringEnum, extends derive macros for string_enum/u64/u32 and fallbacks, and replaces many manual as_str/TryFrom and ProtocolNode impls with derive-based implementations across multiple modules. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bencher Report![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wacore/derive/src/lib.rs (2)
725-790:⚠️ Potential issue | 🟠 MajorFix fallback enum validation and Default impl generation in
StringEnummacro.Two edge cases generate invalid code:
Fallback-only enum (no regular variants): Line 741 accesses
variant_infos[0]without checking if the collection is empty. The guard on line 733 only triggers if bothvariant_infos.is_empty() && fallback_variant.is_none()are true, allowing a fallback-only enum to pass and panic on indexing.
#[string_default]on fallback variant: If the fallback variant is marked as default, line 802 generates#name::#default_variant`` (e.g.,Enum::Other), which requires a `String` argument but none is provided. This produces a compile error.Neither issue currently affects enums in the codebase (all existing fallback enums have ≥2 regular variants and none mark the fallback as default), but the macro should either reject these patterns with a clear
syn::Erroror generate valid code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/derive/src/lib.rs` around lines 725 - 790, The macro must reject unsupported fallback-only enums and cases where the fallback variant is marked #[string_default]; update the validation around variant_infos, fallback_variant, and default_variant: instead of allowing a fallback-only enum, return a syn::Error when variant_infos.is_empty() (even if fallback_variant.is_some()), and add a check after computing default_variant to error if default_variant == fallback_variant (i.e., the fallback variant was annotated #[string_default]); reference the symbols variant_infos, fallback_variant, default_variant and the Default impl generation so the macro emits a clear syn::Error for these invalid patterns rather than producing code that indexes variant_infos[0] or generates an invalid default constructor.
276-299:⚠️ Potential issue | 🟠 Major
defaultsupport incomplete for non-String attribute types.
all_have_defaultsnow admits fields withdefault,optional, orStringEnumtype, butdefault_fieldsonly generates valid code forStringandStringEnum. A non-optional field like#[attr(name = "size", u32, default = "0")] pub size: u32will hit theunreachable!()panic. Additionally, optional numeric/enum fields with defaults generateSome(String)instead of parsing to the correct type (e.g.,Some<u64>orSome<MyEnum>).Either add parsing support for numeric defaults (converting string to u32/u64) or reject
defaultfor non-String attribute types during extraction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wacore/derive/src/lib.rs` around lines 276 - 299, The Default impl generation currently handles only AttrType::String and AttrType::StringEnum and therefore panics for numeric/non-string defaults; update the default_impl generation loop (the code mapping over attr_fields used when all_have_defaults is true) to handle all AttrType variants: for numeric types (u32/u64/i32/etc.) call .parse() on the default string and emit the parsed literal (and wrap in Some(...) when info.optional is true), for boolean parse via .parse::<bool>(), and for enum/string-enum types reuse ::wacore::protocol::parse_string_enum to produce the concrete enum value; ensure errors during parsing are surfaced as compile-time panics with clear messages, and keep the existing branches for optional fields (emit Some(parsed) or None) and non-optional fields (emit parsed or Default::default() for StringEnum when no default).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/types/message.rs`:
- Around line 83-89: The to_string_val implementation for EditAttribute
currently maps Unknown(_) to "", which collapses Unknown and Empty and breaks
round-tripping; change EditAttribute::to_string_val so that the Unknown(String)
variant returns the stored inner string slice (the preserved wire value) instead
of "", leaving suppression/omission decisions to the caller (e.g., the code in
send.rs) so Unknown and Empty remain distinct.
---
Outside diff comments:
In `@wacore/derive/src/lib.rs`:
- Around line 725-790: The macro must reject unsupported fallback-only enums and
cases where the fallback variant is marked #[string_default]; update the
validation around variant_infos, fallback_variant, and default_variant: instead
of allowing a fallback-only enum, return a syn::Error when
variant_infos.is_empty() (even if fallback_variant.is_some()), and add a check
after computing default_variant to error if default_variant == fallback_variant
(i.e., the fallback variant was annotated #[string_default]); reference the
symbols variant_infos, fallback_variant, default_variant and the Default impl
generation so the macro emits a clear syn::Error for these invalid patterns
rather than producing code that indexes variant_infos[0] or generates an invalid
default constructor.
- Around line 276-299: The Default impl generation currently handles only
AttrType::String and AttrType::StringEnum and therefore panics for
numeric/non-string defaults; update the default_impl generation loop (the code
mapping over attr_fields used when all_have_defaults is true) to handle all
AttrType variants: for numeric types (u32/u64/i32/etc.) call .parse() on the
default string and emit the parsed literal (and wrap in Some(...) when
info.optional is true), for boolean parse via .parse::<bool>(), and for
enum/string-enum types reuse ::wacore::protocol::parse_string_enum to produce
the concrete enum value; ensure errors during parsing are surfaced as
compile-time panics with clear messages, and keep the existing branches for
optional fields (emit Some(parsed) or None) and non-optional fields (emit parsed
or Default::default() for StringEnum when no default).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ec8d917-4430-48c1-86cf-fb851f6f1196
📒 Files selected for processing (12)
src/features/status.rswacore/derive/src/lib.rswacore/src/ib.rswacore/src/iq/blocklist.rswacore/src/iq/contacts.rswacore/src/iq/dirty.rswacore/src/iq/groups.rswacore/src/iq/privacy.rswacore/src/iq/usync.rswacore/src/protocol.rswacore/src/types/lid_pn.rswacore/src/types/message.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wacore/src/types/message.rs`:
- Around line 57-73: The Unknown(EditAttribute::Unknown(_)) variant is currently
implicitly treated as an edit by the edit-only predicate (which only excluded
Empty and AdminRevoke); explicitly decide and implement the outbound behavior by
updating the edit-only check (e.g., the function computing edit-only send flags
/ is_edit_only) to match EditAttribute::Unknown(_) -> false (do not treat
unknown wire values as edits) and add a regression test that constructs
EditAttribute::Unknown("99".into()) and asserts it does not produce
edit-specific send flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6cd77c8a-4743-4e93-b5f0-53586b9864fd
📒 Files selected for processing (1)
wacore/src/types/message.rs
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, crate::StringEnum)] | ||
| pub enum EditAttribute { | ||
| #[default] | ||
| #[string_default] | ||
| #[str = ""] | ||
| Empty, | ||
| #[str = "1"] | ||
| MessageEdit, | ||
| #[str = "2"] | ||
| PinInChat, | ||
| #[str = "3"] | ||
| AdminEdit, | ||
| #[str = "7"] | ||
| SenderRevoke, | ||
| #[str = "8"] | ||
| AdminRevoke, | ||
| #[string_fallback] | ||
| Unknown(String), |
There was a problem hiding this comment.
Make Unknown explicit in edit-only send logic.
Line 72 makes unknown wire values representable, but the predicate documented on Lines 171-173 now treats every Unknown(_) as an edit because it only excludes Empty and AdminRevoke. That means a future value like "99" will start emitting edit-specific send flags, even though Line 169 already notes the server rejects some mismatches. Please decide the outbound behavior for Unknown explicitly and lock it in with a regression test here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wacore/src/types/message.rs` around lines 57 - 73, The
Unknown(EditAttribute::Unknown(_)) variant is currently implicitly treated as an
edit by the edit-only predicate (which only excluded Empty and AdminRevoke);
explicitly decide and implement the outbound behavior by updating the edit-only
check (e.g., the function computing edit-only send flags / is_edit_only) to
match EditAttribute::Unknown(_) -> false (do not treat unknown wire values as
edits) and add a regression test that constructs
EditAttribute::Unknown("99".into()) and asserts it does not produce
edit-specific send flags.
1 participant
Summary by CodeRabbit