Skip to content

feat(xmtp_macro): #[derive(Retryable)] proc macro for RetryableError#3753

Open
insipx wants to merge 2 commits into
mainfrom
insipx/retryable-derive
Open

feat(xmtp_macro): #[derive(Retryable)] proc macro for RetryableError#3753
insipx wants to merge 2 commits into
mainfrom
insipx/retryable-derive

Conversation

@insipx

@insipx insipx commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a #[derive(Retryable)] proc macro that generates impl xmtp_common::RetryableError, replacing the hand-written per-enum boilerplate (satisfies the long-standing TODO in retry.rs). Mirrors the existing #[derive(ErrorCode)] macro: same crate (xmtp_macro), same re-export path (xmtp_common::Retryable), same spanned-error discipline.

Variant rules (first match wins)

Attribute Generated arm
#[retry(when = EXPR)] run EXPR with referenced fields bound (this / names / this0..)
#[retry(false)] / #[retry(true)] / bare #[retry] constant
#[retry(inherit)] forward to single inner field's is_retryable()
anything else container `#[retry(default = true

#[from] carries no retry semantics — forwarding is always explicit via #[retry(inherit)]. A workspace census found 98 #[from] variants whose hand-written behavior was a hardcoded true/false (foreign inner types like prost/http/derive_builder) vs only 33 that forward; auto-forwarding (tried first) forced a noisy #[retry(false)] on the 3×-more-common case and made unannotated variants non-local to read. With explicit inherit, an unannotated variant is always the baseline.

Other properties:

  • when binds only the fields the expression references (token scan) — no unused_variables under -D warnings.
  • Generic types supported (split_for_impl); empty enums emit match *self {}; structs supported (when over self).
  • Container misuse (e.g. #[retry(true)] on an enum) is a spanned compile error, not a silent no-op.

Testing

  • 18 behavior tests in xmtp_common::retry::derive_tests covering every rule incl. generics, subset-when, empty enum, bare-#[from]-takes-baseline, default = true over #[from].
  • 8 trybuild UI fixtures pinning each spanned compile_error path (crates/xmtp_macro/tests/ui/retryable/).
  • cargo clippy --locked --all-features --all-targets -Dwarnings clean on touched crates; fmt clean; hakari no-op.

Migration of existing hand-written impls is stacked on top: #3754.

🤖 Generated with Claude Code

Note

Add #[derive(Retryable)] proc macro for RetryableError in xmtp_macro

  • Adds a new Retryable derive macro in xmtp_macro/src/retryable.rs that generates xmtp_common::RetryableError implementations via is_retryable(&self).
  • Supports per-variant and container-level #[retry] attributes: #[retry(true|false)], #[retry(inherit)] (forwards to a single field), #[retry(when = expr)], and #[retry(default = ...)] on enum containers.
  • Re-exports Retryable from xmtp_common so consumers can import it as xmtp_common::Retryable.
  • Adds compile-fail UI tests via trybuild covering invalid attribute combinations (conflicting keys, wrong placement, unknown keys, arity errors, union types).
  • Updates nix/lib/filesets.nix to include tests/ui so trybuild .stderr snapshots are available in hermetic Nix builds.

Macroscope summarized 8a5996e.

@insipx insipx requested review from a team as code owners June 10, 2026 02:28
@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown

Claude finished @insipx's task —— View job


Code Review - June 10, 2026 17:13 UTC

Todo List:

  • Read test files and analyze coverage
  • Review macro implementation for issues not already addressed
  • Provide concise feedback

Review Summary

The #[derive(Retryable)] proc macro implementation is well-designed and production-ready. All previously identified issues (duplicate attributes, empty parens, container misuse, #[from] semantics) have been addressed with proper spanned compile errors.

Strengths:

  • Comprehensive test coverage: 18 behavior tests + 8 trybuild UI fixtures covering all decision paths, edge cases (empty enum, generics, subset-field binding), and error conditions
  • Robust error handling: All invalid attribute combinations produce clear, spanned diagnostics
  • Clean implementation: Proper separation of parsing (RetryAttr::parse) and code generation (variant_arm, when_arm, forward_arm)
  • No unsafe patterns: All unwrap() calls are justified by prior guards (e.g., f.named.len() == 1 at line 307)

Code quality notes:

  • The token scanning in collect_idents (line 324) intentionally over-approximates references (e.g., identifiers in nested macros) — acceptable tradeoff since it only costs extra bindings, never misses
  • The when expression binding strategy (lines 345-374) correctly avoids -D warnings by binding only referenced fields

No actionable issues found beyond what was already addressed in previous review rounds.


@insipx insipx force-pushed the insipx/retryable-derive branch from 8f10674 to 5642d07 Compare June 10, 2026 02:31
Comment thread crates/xmtp_macro/src/retryable.rs
Comment thread crates/xmtp_macro/src/retryable.rs Outdated
@macroscopeapp

macroscopeapp Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

This PR introduces a substantial new proc macro (~375 lines) with multiple parsing paths and code generation logic. While well-tested and compile-time only, new feature components of this scope warrant human review to verify the macro's design and generated code correctness.

You can customize Macroscope's approvability policy. Learn more.

Comment thread crates/xmtp_macro/src/lib.rs
@insipx insipx force-pushed the insipx/retryable-derive branch 2 times, most recently from 247dede to 193d75b Compare June 10, 2026 13:52
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Jun 10, 2026
@insipx

insipx commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

CI analysis of the test-workspace failure (both PRs):

  1. retryable_compile_fail (trybuild) — the hermetic nix build filters crate sources through commonCargoSources (.rs + Cargo.toml only), so the committed .stderr snapshots never reached CI; trybuild regenerated all 10 and failed by design. Fixed in 4701e13 by adding crates/xmtp_macro/tests/ui to the fileset one-offs (same pattern as xmtp_db/migrations).
  2. client::tests::should_reconnect — pre-existing flake, not from this stack: the identical test fails on a recent main run (evidence), the failing path (ApiClientError::ClientWithEndpoint → tonic Cancelled during toxiproxy churn) was never migrated (ApiClientError is in the keep-hand-written list), and the unwrap is in test code racing a 500ms sleep.

@insipx insipx force-pushed the insipx/retryable-derive branch from 193d75b to 4701e13 Compare June 10, 2026 14:42
@macroscopeapp macroscopeapp Bot dismissed their stale review June 10, 2026 14:42

Dismissing prior approval to re-evaluate 4701e13

Comment thread crates/xmtp_macro/src/retryable.rs
@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.49839% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.48%. Comparing base (b5cdc06) to head (4701e13).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/xmtp_macro/src/retryable.rs 94.27% 13 Missing ⚠️
crates/xmtp_macro/src/lib.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3753      +/-   ##
==========================================
+ Coverage   84.44%   84.48%   +0.03%     
==========================================
  Files         409      409              
  Lines       59606    60082     +476     
==========================================
+ Hits        50335    50761     +426     
- Misses       9271     9321      +50     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Derive that generates `impl xmtp_common::RetryableError`, mirroring the existing
ErrorCode derive. Satisfies the long-standing TODO in retry.rs.

Variant rules (first match wins): #[retry(when = EXPR)], #[retry(true|false)],
bare #[retry], #[retry(inherit)], #[from] auto-forward, else the container
#[retry(default = ...)] baseline (false by default). Supports structs.

Tested with 11 behavior tests (in xmtp_common) covering every rule, plus 5
trybuild UI tests pinning the spanned compile_error paths (conflicting keys,
default-on-variant, inherit arity, unknown key, union).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@insipx insipx force-pushed the insipx/retryable-derive branch from 4701e13 to 8a5996e Compare June 10, 2026 15:20
insipx added a commit that referenced this pull request Jun 11, 2026
…able)]

Replaces ~47 hand-written `impl RetryableError` blocks across 8 crates (+xnet)
with the derive. Net -456 lines. Behavior preserved per-variant:
- forwarding arms -> #[retry(inherit)]
- hardcoded trues -> #[retry(true)]; all-true / inverted enums -> #[retry(default = true)]
- hardcoded falses -> unannotated (baseline)
- multi-field forwards & collection aggregation -> #[retry(when = ...)]
- ClientError::Generic -> #[retry(when = this.contains("database is locked"))]
- GroupMessageProcessingError's nested match collapsed via a new derive on
  ProcessMessageWithAppDataError (gains a RetryableError<Mls> where-bound)

Equivalence was verified by an exhaustive variant-by-variant audit against the
deleted impls (~350 variants total, zero mismatches). Per-enum golden tests were
intentionally NOT kept: the macro's own test suite (18 behavior tests + 11
trybuild fixtures in #3753) is the behavioral contract, and annotations are the
source of truth thereafter — same trust model as the hand-written impls had.

Still hand-written by design: the 22 specialized RetryableError<Mls> impls for
foreign openmls/diesel types, GroupAppDataError (instantiation-specific),
ReceiveErrors/SyncSummary one-line aggregators, and test mocks.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
insipx added a commit that referenced this pull request Jun 11, 2026
…able)]

Replaces ~47 hand-written `impl RetryableError` blocks across 8 crates (+xnet)
with the derive. Net -456 lines. Behavior preserved per-variant:
- forwarding arms -> #[retry(inherit)]
- hardcoded trues -> #[retry(true)]; all-true / inverted enums -> #[retry(default = true)]
- hardcoded falses -> unannotated (baseline)
- multi-field forwards & collection aggregation -> #[retry(when = ...)]
- ClientError::Generic -> #[retry(when = this.contains("database is locked"))]
- GroupMessageProcessingError's nested match collapsed via a new derive on
  ProcessMessageWithAppDataError (gains a RetryableError<Mls> where-bound)

Equivalence was verified by an exhaustive variant-by-variant audit against the
deleted impls (~350 variants total, zero mismatches). Per-enum golden tests were
intentionally NOT kept: the macro's own test suite (18 behavior tests + 11
trybuild fixtures in #3753) is the behavioral contract, and annotations are the
source of truth thereafter — same trust model as the hand-written impls had.

Still hand-written by design: the 22 specialized RetryableError<Mls> impls for
foreign openmls/diesel types, GroupAppDataError (instantiation-specific),
ReceiveErrors/SyncSummary one-line aggregators, and test mocks.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

1 participant