Conversation
typestates 0.7.x defaults strictTransitions=true; the verifier requires
every proc that operates on a typestate value to carry {.transition.} or
{.notATransition.}. These 25 procs are pure data extraction or non-
transitioning operations (CAS attempts, fullness checks, virtual value
field reads, push-full extraction). All are classified as
{.notATransition.} per their actual semantics: each returns a non-state-
typed value (int, bool, CASResult) without producing another typestate.
The verifier's substring matcher requires the pragma block to contain
solely {.notATransition.} (combined forms like {.inline, notATransition.}
are not recognized), so {.inline.} was dropped from the affected procs.
Trivial one-line bodies inline at the C compiler level regardless of
the hint. Added 'import typestates' to fullness_checks.nim so the
notATransition pragma macro is in scope.
Adopt v0.6 opaqueStates = true and v0.5 initial:/terminal: DSL blocks together on the 5 SET typestates per the per-typestate decision matrix: CASAttempt, SPSCPopOp, SPSCPushOp, VirtualValueN, VirtualValueN1. The two flags are coupled (opaqueStates without initial: emits opaque-states-no- initials), so they land in a single commit. The remaining 14 typestates (6 bounded + 8 unbounded queue ops) are DROP rows in the matrix, declined because every state in those typestates is a transition target due to retry/re-loop transitions, so no state is initial-eligible.
…ASSucceeded | CASFailed as CASResult)
… name, MPMC claim)
|
✅ Momus review posted — verdict APPROVE, 0 findings
|
|
| Branch | feat/typestates-0.7-uplift |
| Testbed | ubuntu-latest |
Click to view all benchmark results
| Benchmark | Throughput | Benchmark Result operations / second (ops/s) (Result Δ%) | Lower Boundary operations / second (ops/s) (Limit %) |
|---|---|---|---|
| channels/1p1c | 📈 view plot 🚷 view threshold | 2,636.00 ops/s(+1.88%)Baseline: 2,587.44 ops/s | 2,263.28 ops/s (85.86%) |
| channels/2p2c | 📈 view plot 🚷 view threshold | 1,409.00 ops/s(+0.10%)Baseline: 1,407.66 ops/s | 1,276.91 ops/s (90.63%) |
| channels/4p4c | 📈 view plot 🚷 view threshold | 1,666.30 ops/s(+0.54%)Baseline: 1,657.34 ops/s | 1,465.43 ops/s (87.95%) |
| mupmuc/1p1c | 📈 view plot 🚷 view threshold | 25,355.90 ops/s(-21.67%)Baseline: 32,371.15 ops/s | 22,732.69 ops/s (89.65%) |
| mupmuc/2p2c | 📈 view plot 🚷 view threshold | 21,172.70 ops/s(+22.87%)Baseline: 17,231.82 ops/s | 14,951.61 ops/s (70.62%) |
| mupmuc/4p4c | 📈 view plot 🚷 view threshold | 13,067.60 ops/s(-8.81%)Baseline: 14,330.27 ops/s | 9,166.28 ops/s (70.15%) |
| mupmuc/8p8c | 📈 view plot 🚷 view threshold | 20,636.20 ops/s(+60.32%)Baseline: 12,871.92 ops/s | -6,726.62 ops/s (-32.60%) |
| sipsic/1p1c | 📈 view plot 🚷 view threshold | 6,582.50 ops/s(-26.89%)Baseline: 9,002.96 ops/s | 6,489.38 ops/s (98.59%) |
| unbounded_mupsic/1p1c | 📈 view plot 🚷 view threshold | 8,854.80 ops/s(-7.73%)Baseline: 9,596.12 ops/s | 3,940.79 ops/s (44.50%) |
| unbounded_mupsic/2p1c | 📈 view plot 🚷 view threshold | 12,792.40 ops/s(-1.56%)Baseline: 12,995.28 ops/s | 12,141.97 ops/s (94.92%) |
| unbounded_mupsic/4p1c | 📈 view plot 🚷 view threshold | 12,985.30 ops/s(-0.50%)Baseline: 13,050.90 ops/s | 12,426.02 ops/s (95.69%) |
There was a problem hiding this comment.
Code Review
This pull request restructures the CASAttempt typestate into a proper union, requiring callers to use the match macro for handling results. It also updates the typestates and debra dependencies, introduces opaqueStates and explicit initial/terminal blocks across several typestate definitions, and replaces manual case dispatches with match for compile-time exhaustiveness. Feedback is provided regarding the systematic removal of {.inline.} pragmas from various accessors and transitions; these should be restored alongside the new typestate pragmas to ensure zero-cost abstractions in performance-critical lock-free code.
| proc expectedVal*(p: CASPending): int {.notATransition.} = | ||
| CASAttempt(p).expected | ||
|
|
||
| proc desiredVal*(p: CASPending): int {.inline.} = | ||
| proc desiredVal*(p: CASPending): int {.notATransition.} = | ||
| CASAttempt(p).desired | ||
|
|
||
| proc newVal*(s: CASSucceeded): int {.inline.} = | ||
| CASResult(s).newVal | ||
|
|
||
| proc actualVal*(f: CASFailed): int {.inline.} = | ||
| CASResult(f).actualVal | ||
|
|
||
| # Constructor | ||
| proc prepareCAS*(atom: ptr Atomic[int], expected, desired: int): CASPending {.inline.} = | ||
| CASPending(CASAttempt(atom: atom, expected: expected, desired: desired)) | ||
|
|
||
| # Execute CAS - returns result that must be checked | ||
| proc executeCAS*(p: CASPending): CASResult {.inline.} = | ||
| # Execute CAS - returns the typestate union; caller MUST match on it. | ||
| proc executeCAS*(p: CASPending): CASResult {.transition.} = |
There was a problem hiding this comment.
The {.inline.} pragma was removed from these accessors and the executeCAS transition when adding the typestate-specific pragmas. In performance-critical lock-free code, ensuring these small wrappers are inlined is important to maintain zero-cost abstractions. Please restore {.inline.} alongside {.notATransition.} and {.transition.}.
proc expectedVal*(p: CASPending): int {.inline, notATransition.} =
CASAttempt(p).expected
proc desiredVal*(p: CASPending): int {.inline, notATransition.} =
CASAttempt(p).desired
# Constructor
proc prepareCAS*(atom: ptr Atomic[int], expected, desired: int): CASPending {.inline.} =
CASPending(CASAttempt(atom: atom, expected: expected, desired: desired))
# Execute CAS - returns the typestate union; caller MUST match on it.
proc executeCAS*(p: CASPending): CASResult {.inline, transition.} =
| proc usedN*[N: static int](head, tail: WrappedValueN[N]): int {.notATransition.} = | ||
| result = tail.value - head.value | ||
| if result < 0: | ||
| result += 2 * N | ||
|
|
||
| proc availableN*[N: static int](head, tail: WrappedValueN[N]): int {.inline.} = | ||
| proc availableN*[N: static int](head, tail: WrappedValueN[N]): int {.notATransition.} = | ||
| N - usedN(head, tail) | ||
|
|
||
| proc fullN*[N: static int](head, tail: WrappedValueN[N]): bool {.inline.} = | ||
| proc fullN*[N: static int](head, tail: WrappedValueN[N]): bool {.notATransition.} = | ||
| usedN(head, tail) >= N | ||
|
|
||
| proc emptyN*[N: static int](head, tail: WrappedValueN[N]): bool {.inline.} = | ||
| proc emptyN*[N: static int](head, tail: WrappedValueN[N]): bool {.notATransition.} = | ||
| head.value == tail.value | ||
|
|
||
| # N+1-slot checks (SPSC) | ||
| proc usedN1*[N: static int](head, tail: WrappedValueN1[N]): int {.inline.} = | ||
| proc usedN1*[N: static int](head, tail: WrappedValueN1[N]): int {.notATransition.} = | ||
| result = tail.value - head.value | ||
| if result < 0: | ||
| result += 2 * (N + 1) | ||
|
|
||
| proc availableN1*[N: static int](head, tail: WrappedValueN1[N]): int {.inline.} = | ||
| proc availableN1*[N: static int]( | ||
| head, tail: WrappedValueN1[N] | ||
| ): int {.notATransition.} = | ||
| N - usedN1(head, tail) | ||
|
|
||
| proc fullN1*[N: static int](head, tail: WrappedValueN1[N]): bool {.inline.} = | ||
| proc fullN1*[N: static int](head, tail: WrappedValueN1[N]): bool {.notATransition.} = | ||
| usedN1(head, tail) >= N | ||
|
|
||
| proc emptyN1*[N: static int](head, tail: WrappedValueN1[N]): bool {.inline.} = | ||
| proc emptyN1*[N: static int](head, tail: WrappedValueN1[N]): bool {.notATransition.} = | ||
| head.value == tail.value |
There was a problem hiding this comment.
Systematic removal of {.inline.} from these fullness check helpers may lead to performance regressions, as they are used in hot paths within the queue's push/pop loops. These procs are very small and should be inlined to avoid function call overhead. Please restore {.inline.} alongside {.notATransition.}.
proc usedN*[N: static int](head, tail: WrappedValueN[N]): int {.inline, notATransition.} =
result = tail.value - head.value
if result < 0:
result += 2 * N
proc availableN*[N: static int](head, tail: WrappedValueN[N]): int {.inline, notATransition.} =
N - usedN(head, tail)
proc fullN*[N: static int](head, tail: WrappedValueN[N]): bool {.inline, notATransition.} =
usedN(head, tail) >= N
proc emptyN*[N: static int](head, tail: WrappedValueN[N]): bool {.inline, notATransition.} =
head.value == tail.value
# N+1-slot checks (SPSC)
proc usedN1*[N: static int](head, tail: WrappedValueN1[N]): int {.inline, notATransition.} =
result = tail.value - head.value
if result < 0:
result += 2 * (N + 1)
proc availableN1*[N: static int](
head, tail: WrappedValueN1[N]
): int {.inline, notATransition.} =
N - usedN1(head, tail)
proc fullN1*[N: static int](head, tail: WrappedValueN1[N]): bool {.inline, notATransition.} =
usedN1(head, tail) >= N
proc emptyN1*[N: static int](head, tail: WrappedValueN1[N]): bool {.inline, notATransition.} =
| true | ||
|
|
||
| proc extractFalse*[N: static int](op: MPMCPushFull[N]): bool {.inline.} = | ||
| proc extractFalse*[N: static int](op: MPMCPushFull[N]): bool {.notATransition.} = |
There was a problem hiding this comment.
| proc rawValue*[N: static int](r: RawLoadedN[N]): int {.notATransition.} = | ||
| VirtualValueN[N](r).v | ||
|
|
||
| proc value*[N: static int](w: WrappedValueN[N]): int {.inline.} = | ||
| proc value*[N: static int](w: WrappedValueN[N]): int {.notATransition.} = | ||
| VirtualValueN[N](w).v | ||
|
|
||
| proc unwrappedValue*[N: static int](u: UnwrappedSumN[N]): int {.inline.} = | ||
| proc unwrappedValue*[N: static int](u: UnwrappedSumN[N]): int {.notATransition.} = | ||
| VirtualValueN[N](u).v | ||
|
|
||
| proc slotValue*[N: static int](p: PhysicalSlotN[N]): int {.inline.} = | ||
| proc slotValue*[N: static int](p: PhysicalSlotN[N]): int {.notATransition.} = | ||
| VirtualValueN[N](p).v |
There was a problem hiding this comment.
Please restore {.inline.} to these accessors to ensure zero-cost abstractions for the typestate values, especially since the root object fields are now opaque.
proc rawValue*[N: static int](r: RawLoadedN[N]): int {.inline, notATransition.} =
VirtualValueN[N](r).v
proc value*[N: static int](w: WrappedValueN[N]): int {.inline, notATransition.} =
VirtualValueN[N](w).v
proc unwrappedValue*[N: static int](u: UnwrappedSumN[N]): int {.inline, notATransition.} =
VirtualValueN[N](u).v
proc slotValue*[N: static int](p: PhysicalSlotN[N]): int {.inline, notATransition.} =
VirtualValueN[N](p).v
There was a problem hiding this comment.
The PR uplifts lockfreequeues to the typestates 0.7.x feature set: 8 case-of-kind dispatches replaced with the match macro, 22 accessors marked .notATransition., opaqueStates/initial/terminal added to 5 SET typestates, and the CASAttempt typestate restructured into a proper union (CASSucceeded | CASFailed). The changes are consistent and well-scoped. However, tests/t_cas.nim — which tests the breaking CAS union change — is not imported by any test runner, meaning the CAS typestate change has no test coverage in CI.
No findings.
Noteworthy
- All 8 case-of-kind dispatches correctly and exhaustively converted to the match macro across 4 facade modules.
- 22 .notATransition. annotations placed exactly on the read-only accessors flagged by
typestates verify -W, with no false positives. - CHANGELOG.md is detailed and accurate about the breaking change and migration path.
Verdict: APPROVE.
Commands
- Comment
/ai-reviewor mention @axiomantic-momus[bot] to request a re-review of the latest changes. - Reply to a finding with
won't fix,by design, ornot a bugto decline it. - Reply with
instead, ...to propose an alternative fix.
Powered by Momus running deepseek/deepseek-v4-pro via openrouter.ai.
|
/ai-review |
Adopts the typestates 0.7.x feature set on lockfreequeues, mirroring the parallel uplift just merged on nim-debra (PR #9 → v0.7.0).
What changed
opaqueStates = trueand explicitinitial:/terminal:blocks on 5 SET typestates (CASAttempt, SPSCPopOp, SPSCPushOp, VirtualValueN, VirtualValueN1){.notATransition.}case .kinddispatches across 4 facade modules (sipsic, mupmuc, mupsic, sipmuc) replaced with the typestatesmatchmacroCASAttemptconverted to a proper typestate union (CASPending -> CASSucceeded | CASFailed as CASResultviaexecuteCAS);assumeSuccess/assumeFailureremoved. These helpers were only consumed bytests/t_cas.nim; the bundled MPMC machinery callscompareExchangeWeakdirectly and was unaffected. No public lock-free queue API is affected.typestates verify -W --format=github src/step added tobuild.ymlto gate the typestate model against driftVerification
nimble test: 198 OK across orc/refc/arc/cpp + lock-free arc/orc + tsan + asantypestates verify -W: exit 0 (38 files, 22 transitions, "All checks passed!")nph --check: clean06031fb), green-mirage (0 mirages)