fix: performance, correctness, and test hygiene improvements

[timonkrebs]

Summary

Comprehensive code review covering all 6 project directories. Changes fall into four categories: a correctness bug fix, performance improvements, immutability enforcement, and test hygiene.

---

Bug Fix

PipeEx.StructuredConcurrency/StructuredConcurrency.cs — double-completion crash in Let overload

The Let<TSource, TSource2, TDeferred> overload called TaskCompletionSource.SetResult, SetCanceled, and SetException on deferredCompletionSource. These non-Try variants throw InvalidOperationException if the TCS has already been moved to a terminal state (possible under concurrent execution paths). Changed all three to TrySetResult, TrySetCanceled, and TrySetException.

---

Performance: eliminate allocations in conditional chains

PipeEx.ConditionalExpressions/GuardExpressions.cs

ConditionalExecutionResult<TSource> was a class, causing a heap allocation on every Guard call. Changed to readonly struct — zero heap allocation per conditional chain step.

PipeEx.ConditionalExpressions/IfExpressions.cs

IfExpression<TSource, TResult> was a sealed class, causing a heap allocation on every If/ElseIf chain step. Changed to readonly struct for the same reason.

---

Immutability

PipeEx.StructuredConcurrency/StructuredTask.cs

deferredTask1 and deferredTask2 fields in StructuredDeferredTask<T, TDeferred> and StructuredDeferredTask<T, TDeferred1, TDeferred2> were not readonly. They are set only in the constructor and should not be reassignable afterward. Made both fields readonly.

---

Test hygiene

PipeEx.Tests/StructuredConcurrencyTests.cs

Test6 and Test7 had identical bodies (both arranged a TaskCompletionSource, piped through an async lambda, called SetCanceled, then asserted). Removed the duplicate; renamed the remaining test to clarify intent; renumbered downstream tests (old Test8–12 → new Test7–11).

PipeEx.Tests/GeneratedTests.cs

• Added missing namespace PipeEx.Tests; declaration (file was in the global namespace).
• Removed Assert.NotNull(result) calls on ConditionalExecutionResult<TSource> values — now that the type is a readonly struct, the assertion is trivially always true and adds noise without value.

---

Documentation

Added XML doc comments throughout StructuredConcurrency.cs and StructuredTask.cs (all public/internal API surface: I, Let, Await, constructor, AsyncStructuredTaskMethodBuilder<T> members) so IntelliSense surfaces intent at call sites.

---

Files changed

File	Change
PipeEx.ConditionalExpressions/GuardExpressions.cs	class → readonly struct
PipeEx.ConditionalExpressions/IfExpressions.cs	sealed class → readonly struct
PipeEx.StructuredConcurrency/StructuredTask.cs	readonly on deferredTask1/deferredTask2; XML docs
PipeEx.StructuredConcurrency/StructuredConcurrency.cs	Set* → TrySet* bug fix; XML docs
PipeEx.Tests/StructuredConcurrencyTests.cs	Remove duplicate test; renumber
PipeEx.Tests/GeneratedTests.cs	Add namespace; remove redundant Assert.NotNull on struct

Test plan

• dotnet test passes with no regressions
• Verify StructuredConcurrencyTests still covers cancellation chaining (Test5 + new Test6)
• Spot-check ConditionalExpressionsTests — all Guard/If paths still exercise the struct constructors correctly
• Confirm GeneratedTests compile cleanly under PipeEx.Tests namespace

🤖 Generated with Claude Code

---

Generated by Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[timonkrebs]

Code review — PR #52

CI is green on all four platforms. The doc additions, the TrySet* bug fix, and the readonly field enforcement are solid improvements. A few issues need attention before merge:

Severity	File	Short description
Breaking / Critical	IfExpressions.cs:385	sealed class → readonly struct is a binary-breaking change; introduces default-value correctness hole
Breaking / Major	GuardExpressions.cs:171	Same binary-breaking concern for ConditionalExecutionResult
Correctness / Major	StructuredConcurrency.cs:296	OperationCanceledException filter too narrow — third-party cancellations treated as faults
Minor	StructuredConcurrency.cs:184,305	Fire-and-forget tasks not discarded with _ = — inconsistent with line 264
Minor	StructuredConcurrency.cs:241,247	/// <inheritdoc/> on [Obsolete] stubs that have no sensible base to inherit from
Minor	StructuredConcurrency.cs:331,360	Await carries already-completed task as new deferred slot — needs a comment explaining intent

See inline comments for details.

---

Generated by Claude Code

[timonkrebs]

Breaking public API change — needs a major version bump or a compatibility note.

ConditionalExecutionResult<TSource> is a public type. Changing it from class to readonly struct is both a source-breaking and binary-breaking change:

• Any external consumer code that does if (result == null) or result?.Value will now produce a compile error (structs are non-nullable).
• The default value changes from null (class) to default(ConditionalExecutionResult<TSource>) — i.e. Value = default(TSource), Skip = false — which represents "guard passed on the default value", a semantically meaningful but possibly unintended sentinel.
• Equality semantics change from reference equality (class) to auto-generated member-wise equality (struct via ValueType.Equals), which for generic structs uses reflection by default (slow) and produces a different result for code that previously relied on distinct object identity.
• Any external code that stored the value in an object, dynamic, or IConditionalResult-style variable will now silently box the struct.

If this library is versioned with semver, this should be a major-version increment. If it's internal-only, please document the breakage in the PR.

---

Generated by Claude Code

[timonkrebs]

Supplementary inline findings (inline comments couldn't attach to unchanged context lines in the diff — posting here so nothing is lost):

---

IfExpressions.cs line 385 — Breaking / Critical

IfExpression<TSource, TResult> converted from sealed class to readonly struct.

This is a binary-breaking API change: any assembly compiled against the old version will fail at runtime with a MissingMethodException or type-load error.

It also introduces a correctness hole: default(IfExpression<TSource, TResult>) is now a valid, non-null value with IsMatched = false and Source = default(TSource). If a caller somehow ends up with a default-initialized instance (uninitialized array element, Unsafe, etc.) and calls ElseIf, the predicate fires on default(TSource) — null for reference types — causing a NullReferenceException inside the predicate rather than at the call site.

The old sealed class with an internal constructor made such states impossible; the struct removes that invariant.

Suggested fix: If the allocation saving is measured to matter, keep the struct but bump the major version and document the break. Otherwise revert to sealed class and profile first.

---

StructuredConcurrency.cs line 296 — Correctness / Major

} catch (OperationCanceledException ex) when (
    ex.CancellationToken == cts.Token ||
    ex.CancellationToken == innerDeferredTask.CancellationTokenSource.Token) {
    deferredCompletionSource.TrySetCanceled(cts.Token);

The when filter is too narrow. If innerDeferredTask.deferredTask1 was cancelled with any third token (e.g., an HttpClient timeout token, or CancellationToken.None on an unlinked source), the clause evaluates to false and the exception falls through to catch (Exception ex) — completing the TCS as faulted rather than cancelled.

This is inconsistent with RunInnerStructured (line 117), which catches all OperationCanceledException unconditionally.

Suggested fix:

} catch (OperationCanceledException) {
    deferredCompletionSource.TrySetCanceled(cts.Token);
}

---

StructuredConcurrency.cs lines 184 and 305 — Minor

impl();        // line 184 in ChainInnerStructured
wrapperTask(); // line 305 in Let<TSource,TSource2,TDeferred>

Both return a Task that is silently discarded. The intent is fire-and-forget, which is correct, but the pattern is inconsistent with line 264 in the same file:

_ = RunInnerStructured(innerStructuredTask, cts, tcs);

Suggested fix: Use _ = impl(); and _ = wrapperTask(); to match the existing convention and make the intent explicit.

---

StructuredConcurrency.cs lines 241 and 247 — Minor

/// <inheritdoc/>
[OverloadResolutionPriority(3)]
[Obsolete("async-let carries at most two deferred values; Await the chain before adding another Let.", true)]
public static StructuredDeferredTask<...> Let<..., TDeferred3>(...) => throw new NotSupportedException();

/// <inheritdoc/> has no sensible base member to inherit from — these stubs have a different type-parameter arity than any non-stub overload. IntelliSense will show empty or misleading documentation.

Suggested fix:

/// <summary>Not supported. Async-let carries at most two deferred values; call <c>Await</c> before adding another <c>Let</c>.</summary>

---

StructuredConcurrency.cs lines 331 and 360 — Minor (design clarity)

return new StructuredDeferredTask<TResult, TDeferredSource>(impl(), source.deferredTask1, source.CancellationTokenSource);

After Await, the returned StructuredDeferredTask carries source.deferredTask1 — a task already awaited inside impl() — as the new deferredTask1. This is intentional (enables another Let to be chained afterward), but a reader of the return type expects the deferred slot to represent a pending workload.

Suggested fix: Add a brief comment:

// deferredTask1 is the already-joined deferred from the prior step, carried forward
// so a subsequent Let can attach a new parallel workload to this CTS scope.

---

Generated by Claude Code

[timonkrebs]

Continuing the inline review — all issues described in the previous review summary are now anchored to specific lines. The most important items to address before merging:

1. Breaking API changes (GuardExpressions.cs:171, IfExpressions.cs:385) — class-to-struct on public types is binary-breaking; needs a major version bump or explicit scoping note.
2. Missing IEquatable<T> on both new structs — needed for correct, performant equality semantics.
3. Removed test coverage gap (StructuredConcurrencyTests.cs) — the unique TaskCompletionSource-cancel-then-chain path is not clearly covered by remaining tests.
4. OperationCanceledException filter too narrow (StructuredConcurrency.cs:296) — third-party cancellations are silently converted to faults.

Lower-priority items (doc quality, _ = consistency, clarifying comment on Await) can be addressed in a follow-up if preferred.

---

Generated by Claude Code

[timonkrebs]

Missing IEquatable<T> on the new struct.

The auto-generated equality for a generic readonly struct uses ValueType.Equals, which for structs containing generic fields falls back to reflection-based field comparison — slower than an explicit implementation. Since the struct is public and used in the result of every Guard call, consider adding:

public readonly struct ConditionalExecutionResult<TSource>
    : IEquatable<ConditionalExecutionResult<TSource>>
{
    // ... existing members ...

    public bool Equals(ConditionalExecutionResult<TSource> other)
        => Skip == other.Skip && EqualityComparer<TSource>.Default.Equals(Value, other.Value);

    public override bool Equals(object? obj)
        => obj is ConditionalExecutionResult<TSource> other && Equals(other);

    public override int GetHashCode() => HashCode.Combine(Value, Skip);

    public static bool operator ==(ConditionalExecutionResult<TSource> left, ConditionalExecutionResult<TSource> right) => left.Equals(right);
    public static bool operator !=(ConditionalExecutionResult<TSource> left, ConditionalExecutionResult<TSource> right) => !left.Equals(right);
}

Without this, the compiler will also emit CS0661/CS0660 warnings if == is added later.

---

Generated by Claude Code

[timonkrebs]

Inaccurate <returns> XML doc on every Else overload.

The <returns> tag says "Unwrapped ConditionalExecutionResult" but the return type is TSource — the actual unwrapped value, not the result wrapper. This affects IntelliSense for all six Else overloads.

Suggested fix:

/// <returns>The source value carried through the chain.</returns>

---

Generated by Claude Code

[timonkrebs]

Breaking public API change — sealed class → readonly struct.

IfExpression<TSource, TResult> is public. Changing it from sealed class to readonly struct is a source-breaking and binary-breaking change for external consumers:

• Null-checks (if (expr == null)) become compile errors.
• Every async method that returns Task<IfExpression<TSource, TResult>> now boxes the struct inside the Task<T> state machine, adding an allocation that the original sealed class avoided (it was already a heap object and just needed a reference).
• The struct carries TSource + bool isMatched + TResult? — potentially three fields. Passing by value through every ElseIf/Else overload copies all three fields per call vs. copying a single pointer (class reference). For reference-type TSource/TResult this is three pointer copies per step vs. one; for large value-type generics it could be significantly worse.
• Default equality changes from reference equality (class) to auto-generated member-wise equality (struct).

Additionally, this struct is also missing IEquatable<T> (same as ConditionalExecutionResult).

---

Generated by Claude Code

[timonkrebs]

Fragile null-forgiving on a TResult? field (Else overloads, lines 319/331/342).

All three Else overloads contain:

source.IsMatched ? source.Result! : ...

Result is declared as TResult?. When TResult is a value type (e.g. int), TResult? is Nullable<int>. The ! operator only suppresses the nullable-reference-type warning — for Nullable<T>, the implicit conversion to T calls .Value, which throws InvalidOperationException if the nullable has no value.

In practice this is safe today because the only way to get IsMatched = true is via the internal constructor that always passes a real result. The concern is that the invariant ("Result is non-null whenever IsMatched is true") is not encoded in the type, making future refactors fragile.

Suggested defensive pattern or at minimum a Debug.Assert:

=> source.IsMatched
    ? source.Result ?? throw new InvalidOperationException("Matched IfExpression has a null Result.")
    : elseTransform(source.Source);

---

Generated by Claude Code

[timonkrebs]

Fire-and-forget async task not discarded with _ = — inconsistent with line 264.

impl();   // line 184 — Task returned by async lambda is dropped

vs.

_ = RunInnerStructured(innerStructuredTask, cts, tcs);  // line 264 — explicitly discarded

Both are intentional fire-and-forget patterns (the result is channeled through tcs), but the inconsistency can confuse readers and analyzers. Some Roslyn analyzers (e.g. CA2012) treat the undiscarded impl() call as an accidental discard. Please make all fire-and-forget calls consistent:

_ = impl();

---

Generated by Claude Code

[timonkrebs]

OperationCanceledException filter is too narrow — third-party cancellations are treated as faults.

} catch (OperationCanceledException ex)
    when (ex.CancellationToken == cts.Token
       || ex.CancellationToken == innerDeferredTask.CancellationTokenSource.Token) {
    deferredCompletionSource.TrySetCanceled(cts.Token);
} catch (Exception ex) {
    deferredCompletionSource.TrySetException(ex);
    cts.Cancel();
}

If innerDeferredTask.deferredTask1 faults with an OperationCanceledException carrying a third token (e.g. a CancellationToken passed into the user's own factory), the when filter fails and the exception falls into catch (Exception ex), completing deferredCompletionSource as faulted rather than cancelled. Callers awaiting the result will see a faulted Task containing an OperationCanceledException, which is surprising.

Consider broadening the filter to accept any OperationCanceledException:

} catch (OperationCanceledException) {
    cts.TryCancel();   // or cts.Cancel() if not already cancelled
    deferredCompletionSource.TrySetCanceled(cts.Token);
}

or at minimum document the current narrow-filter behaviour in a comment so it's an explicit design decision rather than a silent gotcha.

---

Generated by Claude Code

[timonkrebs]

/// <inheritdoc/> on an [Obsolete] error-trap stub that has no sensible base member to inherit from.

/// <inheritdoc/>
[OverloadResolutionPriority(3)]
[Obsolete("async-let carries at most two deferred values; Await the chain before adding another Let.", true)]
public static StructuredDeferredTask<TSource, TDeferred1, TDeferred2> Let<TSource, TDeferred1, TDeferred2, TDeferred3>(...)
    => throw new NotSupportedException();

The <inheritdoc/> tag makes the doc tool look for a base member with the same signature to copy the description from. These overloads have a different type-parameter count than any existing Let overload, so there is no matching base member. XML documentation generators will either emit an empty <summary> or silently copy documentation from an unrelated overload, which would be misleading.

These stubs exist purely to produce a compile-time error. Their documentation should be explicit about that, for example:

/// <summary>
/// Not supported. An async-let chain can carry at most two deferred values.
/// Await the chain before adding a third <c>Let</c>.
/// </summary>

Same issue applies to the second [Obsolete] stub immediately below (line ~250).

---

Generated by Claude Code

[timonkrebs]

Await carries the original deferredTask1 as the new result's deferred slot — needs a clarifying comment.

return new StructuredDeferredTask<TResult, TDeferredSource>(
    impl(), source.deferredTask1, source.CancellationTokenSource);

The single-deferred Await returns a new StructuredDeferredTask whose deferredTask1 is the same already-awaited source.deferredTask1. This allows further chaining (e.g. another .Let or .Await downstream), but a reader who doesn't know the design may wonder why a completed task is being carried forward as a "deferred" slot, or whether it's a mistake.

A brief comment explaining the intent would help:

// Carry deferredTask1 forward so the result can be used as a source for
// further Let/Await chains; the task is already awaited above and will
// complete immediately for any subsequent Await call.

---

Generated by Claude Code

[timonkrebs]

Removed test may have covered a unique execution path.

According to the PR description, old Test7 had an identical body to Test6. However, looking at the diff, the removed test was:

// (removed) Test7_Ensure_Cancellation_Chaining_MultipleStages
var structuredTask = x.Task.I(val => Task.FromResult(val * 2));
x.SetCanceled();
return structuredTask.I(val => val * 2);

This is distinct from the surviving Test6_Ensure_Cancellation_Chaining_ChainedAfterCancel in one subtle way: the surviving test chains .I off a StructuredTask<T> whose source CTS was cancelled, while the removed test cancelled the original TaskCompletionSource<int> and then chained a second .I. These exercise different code paths in CheckedChain vs. the Task<TSource> source overloads.

The remaining Test5 and Test6 do not clearly cover the case "cancel the raw TaskCompletionSource after creating a StructuredTask, then chain another .I on top of the already-running StructuredTask."

Please either restore the test with a unique name, or add a comment confirming the scenario is covered elsewhere with a pointer to the test that covers it.

---

Generated by Claude Code

[timonkrebs]

Namespace addition may break test-filter strings that use bare class names.

Adding namespace PipeEx.Tests; is the correct fix for the missing namespace, but it changes the fully-qualified class names from CoreTests → PipeEx.Tests.CoreTests (and similarly for TupleDestructuringTests, IfExpressionsTests, etc.).

Any CI dotnet test --filter "ClassName=CoreTests" invocations or IDE run configurations that reference bare names will silently stop matching. Consider updating CI scripts / README examples if they reference these test class names by their old unqualified form.

---

Generated by Claude Code