add a demonstration on how to specify effects to be performed#675
add a demonstration on how to specify effects to be performed#675
Conversation
WalkthroughThe PR introduces a new publicly exposed typestate module implementing a comprehensive protocol framework. It includes macro-driven state generation, effect primitives, state transition traits, runtime scaffolding for async operations, and a chainsync example demonstrating practical usage patterns. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
based on conversations at the [Dagstuhl seminar on behavioural types](https://www.dagstuhl.de/26071) Signed-off-by: Roland Kuhn <rk@rkuhn.info>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@crates/pure-stage/src/typestate.rs`:
- Line 286: Fix the typo in the doc comment inside typestate.rs: change "THe
underlying low-level effects machinery; this could be hidden as well to remove"
to "The underlying low-level effects machinery; this could be hidden as well to
remove" (look for the comment containing "THe underlying low-level effects
machinery" near the Typestate module).
- Around line 273-275: The import currently uses the redundant nested module
path crate::typestate::typestate::Transition inside mod application_code;
flatten the inner module by removing the unnecessary inner module layer in the
typestate module and update the import in application_code to use
crate::typestate::Transition (keep StageRef and super::chainsync::* as-is) so
the code imports Transition from the flattened typestate module.
- Around line 15-16: The file defines an inner `pub mod typestate { ... }`
inside typestate.rs causing double nesting (e.g.
crate::typestate::typestate::Transition); remove the inner `pub mod typestate
{}` wrapper and hoist all its contents to the file root so types like Transition
and functions live at crate::typestate::Transition, remove the `#[macro_use]`
annotation on the inner module and either leave macros as normal items (visible
to sibling modules chainsync and application_code) or add `#[macro_export]` to
specific macro_rules! definitions if they must be exported to downstream crates,
then update any paths that referenced the inner module to the flattened
crate::typestate::* paths.
- Around line 216-226: The async method send on the typestate impl captures self
(which holds crate::Effects<M>) and produces a non-Send future; fix by adding
the appropriate Send bounds to the impl/generic parameters (e.g., require M:
Send, and if needed S: Send) so the returned future is Send; also inspect the
definition of Effects<M> to determine and propagate any additional trait bounds
(for example M must satisfy whatever makes Effects<M> Send) and update the
impl/generics for the send method and its surrounding impl block accordingly.
🧹 Nitpick comments (2)
crates/pure-stage/src/typestate.rs (2)
201-214: Minor nit: redundantSinPhantomData.
Sis already stored concretely innext: S, so including it again inPhantomData<(Eff, S)>is carrying extra inventory you don't need. Won't cause any bugs — just a wee bit of noise.🧹 Optional cleanup
pub struct Effects<M, Eff, S> { effects: crate::Effects<M>, next: S, - _ph: PhantomData<(Eff, S)>, + _ph: PhantomData<Eff>, }(And update the
PhantomDataconstruction sites accordingly.)
304-317: Heads up:type_nameoutput isn't stable across Rust versions.The test asserts on
"alloc::string::String"which comes fromstd::any::type_name. That function's docs explicitly say the output is "best effort" and not guaranteed to be stable. It's grand for a demo, but if this test sticks around long-term, a Rust toolchain update could break it like a save file from an old patch version. Maybe worth a comment noting the fragility, or switching to acontains-based assertion.
| #[macro_use] | ||
| pub mod typestate { |
There was a problem hiding this comment.
CI blocker: Clippy module-inception — inner module has the same name as its parent.
Right, so this is a bit like naming your character the same as the game — Clippy's not having it. The file typestate.rs already is the typestate module, so wrapping everything in another pub mod typestate { ... } creates a typestate::typestate::* path (which you can see biting at line 275 with crate::typestate::typestate::Transition).
The cleanest fix is to flatten the inner module: hoist its contents to file level. The macro_rules! definitions will still be visible to the sibling chainsync and application_code modules declared later in the same file, so #[macro_use] on the inner module becomes unnecessary. External consumers would use crate::typestate::* directly — no double-nesting.
If you want the macros exported for downstream crates too, swap #[macro_use] for #[macro_export] on each macro.
🧰 Tools
🪛 GitHub Actions: Coding Practices
[error] 16-16: Clippy: module-inception. The module name 'typestate' is the same as its containing module in crates/pure-stage/src/typestate.rs:16:1.
🤖 Prompt for AI Agents
In `@crates/pure-stage/src/typestate.rs` around lines 15 - 16, The file defines an
inner `pub mod typestate { ... }` inside typestate.rs causing double nesting
(e.g. crate::typestate::typestate::Transition); remove the inner `pub mod
typestate {}` wrapper and hoist all its contents to the file root so types like
Transition and functions live at crate::typestate::Transition, remove the
`#[macro_use]` annotation on the inner module and either leave macros as normal
items (visible to sibling modules chainsync and application_code) or add
`#[macro_export]` to specific macro_rules! definitions if they must be exported
to downstream crates, then update any paths that referenced the inner module to
the flattened crate::typestate::* paths.
| pub async fn send<T: SendData, E>(self, target: &StageRef<T>, msg: T) -> Effects<M, E, S> | ||
| where | ||
| Eff: Sequence<Head = Send<T>, Tail = E>, | ||
| { | ||
| self.effects.send(target, msg).await; | ||
| Effects { | ||
| effects: self.effects, | ||
| next: self.next, | ||
| _ph: PhantomData, | ||
| } | ||
| } |
There was a problem hiding this comment.
CI blocker: Clippy future_not_send — the async send method produces a non-Send future.
This one's like trying to fast-travel with contraband — the #![deny(clippy::future_not_send)] in lib.rs won't let it through. The future captures self (which holds crate::Effects<M>), and there's no Send bound on M anywhere in the impl block at line 207.
Adding a Send bound on M (and potentially on S) should sort it:
🔧 Proposed fix
- impl<M, Eff, S: State> Effects<M, Eff, S> {
+ impl<M: Send, Eff, S: State + Send> Effects<M, Eff, S> {If crate::Effects<M> requires more than M: Send to be Send itself, you may need to check its definition and propagate the necessary bounds.
🧰 Tools
🪛 GitHub Actions: Coding Practices
[error] 216-220: Clippy: future_not_send. The async function 'send' returns a future that is not Send. This prevents crossing threads (see -D clippy::future_not_send).
🤖 Prompt for AI Agents
In `@crates/pure-stage/src/typestate.rs` around lines 216 - 226, The async method
send on the typestate impl captures self (which holds crate::Effects<M>) and
produces a non-Send future; fix by adding the appropriate Send bounds to the
impl/generic parameters (e.g., require M: Send, and if needed S: Send) so the
returned future is Send; also inspect the definition of Effects<M> to determine
and propagate any additional trait bounds (for example M must satisfy whatever
makes Effects<M> Send) and update the impl/generics for the send method and its
surrounding impl block accordingly.
| mod application_code { | ||
| use super::chainsync::*; | ||
| use crate::{StageRef, typestate::typestate::Transition}; |
There was a problem hiding this comment.
The typestate::typestate:: double-nesting is a downstream symptom of the module-inception issue.
Once you flatten the inner module (as suggested above), this import simplifies to use crate::typestate::Transition — much tidier, like finally cleaning up your inventory after a long dungeon crawl.
🤖 Prompt for AI Agents
In `@crates/pure-stage/src/typestate.rs` around lines 273 - 275, The import
currently uses the redundant nested module path
crate::typestate::typestate::Transition inside mod application_code; flatten the
inner module by removing the unnecessary inner module layer in the typestate
module and update the import in application_code to use
crate::typestate::Transition (keep StageRef and super::chainsync::* as-is) so
the code imports Transition from the flattened typestate module.
| // Some names (channels) needed to perform the Send effects | ||
| mux: &StageRef<String>, | ||
| other: &StageRef<u8>, | ||
| // THe underlying low-level effects machinery; this could be hidden as well to remove |
There was a problem hiding this comment.
Typo: "THe" → "The".
Wee typo in the doc comment, mate.
✏️ Fix
- // THe underlying low-level effects machinery; this could be hidden as well to remove
+ // The underlying low-level effects machinery; this could be hidden as well to remove📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // THe underlying low-level effects machinery; this could be hidden as well to remove | |
| // The underlying low-level effects machinery; this could be hidden as well to remove |
🤖 Prompt for AI Agents
In `@crates/pure-stage/src/typestate.rs` at line 286, Fix the typo in the doc
comment inside typestate.rs: change "THe underlying low-level effects machinery;
this could be hidden as well to remove" to "The underlying low-level effects
machinery; this could be hidden as well to remove" (look for the comment
containing "THe underlying low-level effects machinery" near the Typestate
module).
Codecov Report❌ Patch coverage is
... and 11 files with indirect coverage changes 🚀 New features to boost your workflow:
|
1 participant
based on conversations at the Dagstuhl seminar on behavioural types
I intend to use this approach in a refactoring of the miniprotocols after the relay is done, to get
better type safety with less tests, therefore I find it reasonable to add it now even though it
isn't used yet. I also want to show this to some of the researchers who were at Dagstuhl.
Signed-off-by: Roland Kuhn rk@rkuhn.info
Summary by CodeRabbit