Skip to content

refactor(derive): remove variant run_async state - #1224

Merged
jdx merged 1 commit into
mainfrom
fix/remove-variant-run-async
Aug 22, 2026
Merged

refactor(derive): remove variant run_async state#1224
jdx merged 1 commit into
mainfrom
fix/remove-variant-run-async

Conversation

@jdx

@jdx jdx commented Aug 22, 2026

Copy link
Copy Markdown
Owner

Summary

  • reject redundant variant-level run_async while parsing the attribute
  • remove the private model field and downstream validation branches used only to carry that diagnostic
  • preserve enum-level run_async dispatch behavior

Testing

  • cargo test --all --all-features

Note

Low Risk
Derive-only refactor of a redundant diagnostic path; no runtime or dispatch behavior change.

Overview
Stops carrying a private run_async flag on enum variants. Redundant #[usage(run_async)] on a variant is now rejected while parsing the attribute, with the same diagnostic as before.

Downstream validation no longer checks that stored flag. Enum-level run_async / run_async_with dispatch is unchanged.

Reviewed by Cursor Bugbot for commit ef4093d. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • Bug Fixes
    • Async execution settings on individual variants are now rejected immediately when unsupported.
    • Improved validation for external-subcommand and undispatched enum configurations.
    • Async enum dispatch now consistently awaits variants unless explicitly overridden by run.

@coderabbitai

coderabbitai Bot commented Aug 22, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 71425399-8abe-40b1-907a-9bcc2b356f51

📥 Commits

Reviewing files that changed from the base of the PR and between 2a057ca and ef4093d.

📒 Files selected for processing (1)
  • derive/src/model.rs

Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The change removes stored per-variant run_async state. Variant-level run_async = true now produces an immediate diagnostic. Dispatch validation and variant construction use only run_sync and no_ctx.

Changes

Variant async dispatch validation

Layer / File(s) Summary
Dispatch contract and validation
derive/src/model.rs
Variant no longer stores run_async. Variant parsing rejects enabled run_async with a diagnostic. External-subcommand and undispatched-enum validation, plus variant construction, no longer reference the removed state.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to ef409

This localized derive refactor rejects redundant variant-level configuration earlier while preserving enum-level dispatch behavior; no actionable merge-blocking risk remains beyond normal checks and review.

Poem

A rabbit found async tucked away,
And cleared that field from the model today.
“Awaited by enums,” the new check will say,
While sync and context keep their place in the array.
Hop, hop—the dispatch paths are tidy and clear!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: removing variant-level run_async state.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@jdx
jdx enabled auto-merge (squash) August 22, 2026 22:12
@jdx
jdx merged commit 4bddddb into main Aug 22, 2026
9 checks passed
@jdx
jdx deleted the fix/remove-variant-run-async branch August 22, 2026 22:16
@github-actions

Copy link
Copy Markdown
Contributor

Instruction counts

benchmark trend instructions Δ wall (min) Δ
markdown ▁▁▁▁▁▁▁▁▁▁▁▁█████████ 286,216,149 → 286,170,478 -0.02% 25.42 → 25.48ms +0.22%
startup ▁▁▁▁▁▁▂▂▂▂▂▂▃▃▃▃▃████ 874,766 → 874,705 -0.01% 0.85 → 0.87ms +3.14%

No instruction-count regression above 1%.

Only instruction counts gate. Wall clock is shown for context — on identical hardware it moves 4-20% run to run.

Measured by tak — instruction-counted CLI benchmarks, stored in this repository's git notes.

Shadow comparison

Parsing mise use -g node@20 against a shadow of mise's committed spec.
Reported, not gated: the shadow grows as the derive learns to express more, so
what to watch is the ratio rather than either column.

framework instructions, cold parse vs usage
usage 8425
argh 6307 0.7x
clap 6315224 749x
bpaf 21909147 2600x
                                              min       p01       p10    median
usage-rs: argv -> struct                      423       427       431       438  ns
argh: argv -> struct                          270       278       282       288  ns
clap: build tree + parse -> struct         523239    523410    525131    530306  ns
bpaf: build parser + parse -> struct      1429988   1429988   1468243   1598084  ns

usage: argv -> struct                             449 ns      0.45 µs
clap: build tree + parse -> struct             529278 ns    529.28 µs
clap: parse -> struct, tree reused              23951 ns     23.95 µs
clap: build tree only                          324376 ns    324.38 µs

ef4093dbbaca vs 2a057ca19942 · measured on the runner, not pushed to the history.

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