feat(sdk): 💥 add HasWorkflowDefinition trait - #1173
Conversation
6afa706 to
63e9cd7
Compare
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review |
63e9cd7 to
d4e54ff
Compare
d4e54ff to
63a5f2e
Compare
| time::{Duration, SystemTime}, | ||
| }; | ||
| use temporalio_common::{ | ||
| WorkflowDefinition, | ||
| HasWorkflowDefinition, | ||
| data_converters::{DataConverter, SerializationContextData}, | ||
| protos::{ | ||
| coresdk::IntoPayloadsExt, |
There was a problem hiding this comment.
🟡 HasWorkflowDefinition is introduced as the required trait bound on the new public Client::start_workflow and Client::get_workflow_handle methods, but is only privately imported in temporalio_client and never re-exported. Users who want to write generic code bounded on HasWorkflowDefinition must add temporalio_common as a direct dependency with no compiler guidance. Fix: add pub use temporalio_common::HasWorkflowDefinition; to crates/client/src/lib.rs.
Extended reasoning...
HasWorkflowDefinition not re-exported from temporalio_client
What the bug is and how it manifests
The PR adds HasWorkflowDefinition as a required trait bound on the two newly-public methods Client::start_workflow<W: HasWorkflowDefinition> and Client::get_workflow_handle<W: HasWorkflowDefinition>. However, in crates/client/src/lib.rs (around line 69), HasWorkflowDefinition is only pulled in via a private use temporalio_common::{ HasWorkflowDefinition, ... }. The pub use workflow_handle::{ ... } block re-exports only handle-related types and does not include HasWorkflowDefinition. The trait is publicly available from temporalio_common but not from temporalio_client.
The specific code path that triggers it
A user adding temporalio_client as their only dependency and writing a helper function like fn start<W: HasWorkflowDefinition>(client: &Client, w: W, ...) will see error[E0405]: cannot find trait HasWorkflowDefinition in this scope. The compiler suggests nothing useful about temporalio_common. The user must discover on their own that the trait lives in a separate crate.
Why existing code does not prevent it
The primary use case — calling client.get_workflow_handle::<MyWorkflow>(wfid) where MyWorkflow is macro-generated — works perfectly without importing the trait. Type inference resolves the bound automatically, so most users never encounter the missing re-export. It only surfaces when writing generic code. Additionally, several other traits (WorkflowDefinition, SignalDefinition, etc.) also lack re-exports from temporalio_client, establishing a pre-existing pattern. The integration tests themselves import HasWorkflowDefinition directly from temporalio_common, confirming this is the current expected path.
Impact and severity
The practical impact is limited. For the primary ergonomics scenario this PR introduces — client.get_workflow_handle::<MyWorkflow>(wfid) — users never need to name HasWorkflowDefinition explicitly. The issue only matters for writing generic utility functions bounded on HasWorkflowDefinition, which is an edge case. The omission is also consistent with the pre-existing treatment of WorkflowDefinition and related traits. Severity is accordingly nit.
Step-by-step proof
- A user depends only on
temporalio_client(nottemporalio_common). - They write:
fn start_my_wf<W: HasWorkflowDefinition>(client: &Client, w: W, ...) { ... } - The compiler produces
error[E0405]: cannot find trait HasWorkflowDefinition in this scope. - The user searches
temporalio_clientre-exports and finds noHasWorkflowDefinition. - They must inspect SDK source or docs to discover it lives in
temporalio_common. - Adding
pub use temporalio_common::HasWorkflowDefinition;tocrates/client/src/lib.rsresolves the issue.
How to fix
Add to the pub use section in crates/client/src/lib.rs:
pub use temporalio_common::HasWorkflowDefinition;| ) -> Result<(), WorkflowInteractionError> | ||
| where | ||
| CT: WorkflowService + NamespacedClient + Clone, | ||
| S: SignalDefinition<Workflow = W>, | ||
| S: SignalDefinition<Workflow = W::Run>, | ||
| S::Input: Send, | ||
| { | ||
| let payloads = self |
There was a problem hiding this comment.
🟡 After this PR, explicit type annotation UntypedSignal::<MyWorkflow>::new("name") fails to compile on a WorkflowHandle<CT, MyWorkflow> because the signal/query/update bounds now require SignalDefinition<Workflow = W::Run>, but UntypedSignal<T> implements SignalDefinition<Workflow = T>. Type inference handles the bare UntypedSignal::new(...) case fine, but users who write explicit annotations or generic helpers parameterized by the outer workflow type (W) rather than the internal marker type (W::Run) will get a compile error.
Extended reasoning...
The Mismatch Between UntypedSignal<T> and the New W::Run Bounds
This PR changes the signal, query, execute_update, and start_update method bounds on WorkflowHandle<CT, W> from S: SignalDefinition<Workflow = W> to S: SignalDefinition<Workflow = W::Run>. The intent is correct: typed handles now store W (the user-facing outer type like InteractionWorkflow), but signals/queries/updates are defined against W::Run (the macro-generated inner marker type like interaction_workflow::Run). However, the UntypedSignal<T> family was not updated accordingly.
The Specific Code Path That Triggers It
UntypedSignal<W> (line ~195 in workflow_handle.rs) implements SignalDefinition as:
impl<W: WorkflowDefinition> SignalDefinition for UntypedSignal<W> {
type Workflow = W; // T maps directly to Workflow
...
}Meanwhile, WorkflowHandle::signal at line 405 now requires S: SignalDefinition<Workflow = W::Run>. For a handle typed as WorkflowHandle<CT, InteractionWorkflow>, W = InteractionWorkflow and W::Run = interaction_workflow::Run (the private macro-generated struct). So UntypedSignal::<InteractionWorkflow> satisfies SignalDefinition<Workflow = InteractionWorkflow>, not SignalDefinition<Workflow = interaction_workflow::Run>, causing a compile error on explicit annotation.
Why Existing Code Does Not Prevent This
The type inference case works because the compiler can unify the unconstrained type parameter T in UntypedSignal<T> with W::Run directly. But once the user writes UntypedSignal::<InteractionWorkflow>, the type parameter is fixed at InteractionWorkflow, which diverges from the required interaction_workflow::Run. The same problem surfaces in generic helpers: fn send_untyped<W: HasWorkflowDefinition>(h: &WorkflowHandle<Client, W>, name: &str) — UntypedSignal::<W>::new(name) fails because W ≠ W::Run for macro-generated types. The user is forced to write UntypedSignal::<W::Run>::new(name), which is unintuitive when the handle is typed as WorkflowHandle<Client, W>.
Step-by-Step Proof
- User calls
client.get_workflow_handle::<InteractionWorkflow>(wfid)→ returnsWorkflowHandle<Client, InteractionWorkflow>, soW = InteractionWorkflow. - The macro generates
interaction_workflow::Runas the inner marker, andimpl HasWorkflowDefinition for InteractionWorkflow { type Run = interaction_workflow::Run; }. handle.signal(...)bound:S: SignalDefinition<Workflow = interaction_workflow::Run>.- User writes
UntypedSignal::<InteractionWorkflow>::new("increment"):S = UntypedSignal<InteractionWorkflow>, which hasWorkflow = InteractionWorkflow. - Compiler checks:
InteractionWorkflow == interaction_workflow::Run? No → compile errorE0277. - Bare
UntypedSignal::new("increment"): compiler infersT = interaction_workflow::Runautomatically → compiles fine.
Impact
The common case (no explicit type param) works via inference and covers all the test cases added in this PR. The failure only surfaces when: (a) a user explicitly annotates the type parameter on UntypedSignal/UntypedQuery/UntypedUpdate, or (b) a user writes a generic helper function parameterized over W: HasWorkflowDefinition and tries to construct UntypedSignal::<W>::new(name). The workaround — using UntypedSignal::<W::Run>::new(name) — is accessible even though W::Run is a private module type, but it is semantically confusing when the handle is publicly typed as WorkflowHandle<CT, W>.
How to Fix
The cleanest fix is to add an impl<W: HasWorkflowDefinition> SignalDefinition for UntypedSignal<W> that sets type Workflow = W::Run, alongside the existing impl, or change UntypedSignal<W> to be generic over any W: HasWorkflowDefinition and set type Workflow = W::Run. This would make UntypedSignal::<InteractionWorkflow> satisfy SignalDefinition<Workflow = interaction_workflow::Run> as required, matching user intuition that the outer workflow type is the right one to use.
| /// Indicates that a type is associated with a [`WorkflowDefinition`], enabling typed | ||
| /// signal, query, and update methods on `WorkflowHandle`. | ||
| /// | ||
| /// Structs annotated with `#[workflow_methods]` implement this trait automatically. | ||
| pub trait HasWorkflowDefinition: WorkflowDefinition { | ||
| /// The [`WorkflowDefinition`] type that [`SignalDefinition`], [`QueryDefinition`], and | ||
| /// [`UpdateDefinition`] are associated with. |
There was a problem hiding this comment.
🟡 The doc comment on HasWorkflowDefinition says only "Structs annotated with #[workflow_methods] implement this trait automatically", but the macro actually generates two distinct impls — one for the Run marker struct (with type Run = Self) and one for the workflow impl type itself (e.g., InteractionWorkflow, with type Run = interaction_workflow::Run). The second impl is the entire point of this PR and is absent from the docs; without it, readers cannot understand why signal/query/update bounds use W::Run rather than W, or how get_workflow_handle::<InteractionWorkflow> works.
Extended reasoning...
Documentation gap on HasWorkflowDefinition
What the bug is and how it manifests
The doc comment on HasWorkflowDefinition (lines 16–22 of crates/common/src/workflow_definition.rs) reads:
Structs annotated with
#[workflow_methods]implement this trait automatically.
This is incomplete in a way that obscures the trait's design. The macro generates two separate HasWorkflowDefinition impls per annotated type — but the doc mentions neither impl precisely, and critically omits the one that is the central feature of this PR.
The specific code path
In crates/macros/src/workflow_definitions.rs, the codegen_with_options function emits:
// Impl 1: the generated Run marker struct
impl ::temporalio_common::HasWorkflowDefinition for #module_ident::#struct_ident {
type Run = Self;
}
// Impl 2: the outer workflow type (e.g., InteractionWorkflow)
impl ::temporalio_common::HasWorkflowDefinition for #impl_type {
type Run = #module_ident::#struct_ident;
}Impl 2 is what enables client.get_workflow_handle::<InteractionWorkflow>(wfid). The entire PR motivation (issue #1161) is to allow typed handles without referencing the generated interaction_workflow::Run module type. The doc says nothing about this second impl.
Why existing code does not prevent it
This is a documentation issue only — the code compiles and runs correctly. There is no runtime or compile-time mechanism to enforce doc accuracy. The omission simply leaves users who read the trait docs without the information needed to understand the dual-impl design or why signal/query/update bounds use W::Run rather than W.
Addressing the refutation
The refutation argues that the doc is "not incorrect" because it does accurately state that #[workflow_methods] causes the trait to be implemented automatically. This is true — the comment is not wrong. However, it covers only the second case (the impl type) and omits the first (the Run marker struct with type Run = Self). More significantly, it provides no guidance on the semantics of type Run: a reader would not understand that when W = InteractionWorkflow, W::Run = interaction_workflow::Run, and that this is why all signal/query/update definitions use W::Run as their Workflow associated type. The missing context is central to correct manual implementation and to understanding the API surface introduced by this PR.
Step-by-step proof of the confusion
- A user reads the
HasWorkflowDefinitiondoc and sees: "Structs annotated with#[workflow_methods]implement this trait automatically." - They conclude: only the
Runmarker struct (which is the "struct annotated with#[workflow_methods]" in their mental model) implements the trait. - They wonder why
client.get_workflow_handle::<InteractionWorkflow>compiles —InteractionWorkflowis the impl type, not a struct annotated with#[workflow_methods]. - They wonder why
handle.signal(InteractionWorkflow::increment, ...)works, given thatSignalDefinitionis bounded onWorkflow = W::Run, notW. - None of this is explained because the doc omits: (a) that the impl type also gets a
HasWorkflowDefinitionimpl, and (b) whattype Runmeans in each case.
How to fix
Expand the doc comment to explicitly mention both impls and the semantics of type Run:
/// Indicates that a type is associated with a [`WorkflowDefinition`], enabling typed
/// signal, query, and update methods on `WorkflowHandle`.
///
/// The `#[workflow_methods]` macro implements this trait on two types:
/// - The generated `Run` marker struct (e.g., `interaction_workflow::Run`), where `type Run = Self`.
/// This allows existing `.start_workflow(MyWorkflow::run, ...)` calls to continue working.
/// - The workflow impl type itself (e.g., `InteractionWorkflow`), where `type Run = interaction_workflow::Run`.
/// This is what enables `client.get_workflow_handle::<InteractionWorkflow>(wfid)`, and explains
/// why signal/query/update bounds use `W::Run` rather than `W` on `WorkflowHandle<CT, W>`.
What was changed
Add a new trait to act as a bridge between user defined workflow types and the run marker struct. This allows for users to get typed workflow handles in a straighforward manner:
or
We implement this on the existing
my_workflow::Runtype so existingstart_workflowcontinue to work e.g..start_workflow(MyWorkflow::run, (), options).See #1174 for this approach.Why?
See #1161
TL;DR constructing a typed workflow handle requires referencing the generated type
my_workflow::Run. This is bad ergonomics prevents constructing typed workflow handles in a different module.This definitely has a trade off of adding an additional public trait just to make a jump between types, but it provides typed handlers in the way I think most users would expect.
Checklist
Closes sdk version 0.1.0-alpha.1: Can't use typed execute_update due to type mismatch of Workflow expected #1161
How was this tested:
Added client interaction tests that leverage a typed handle from client instead of the one from starting a workflow.
Any docs updates needed?