Allow @MainActor primitives as EnvironmentKey defaults (#39) - #40
Merged
Conversation
Prompt: Please let's plan out #39 - be sure to grill me before proposing plan Response: I'll start by reading issue #39 and exploring the relevant code, then grill you on the design before proposing a plan.Plan approved. Let me set up todos and begin implementation. I'll start by verifying the central assumption empirically — write the failing test first, then attempt the `nonisolated init` change.Now let me confirm this test file fails to build today (before the changes), which proves the test is meaningful.Red confirmed — exact errors from issue #39. Now applying the fix to Selection first.The red step was already confirmed via the build output above — the file failed to compile with the exact #39 errors. Continuing.Critical assumption in the plan was wrong: the @observable macro does NOT make init writes nonisolated. The compiler rejects `self.current = initial` in a `nonisolated init` even though `current` has the storage-init accessor. Let me verify whether Job (empty init) works, then reassess.Job compiles. Selection needs more work. Let me try a clean split: declaration-level default + `nonisolated init()` parameterless.Build is the test for this case (compile-time contract). The test file already exists; let me check whether the split init compiles, then adjust the test to match the actual achievable contract.Selection compiles with the split. The seeded `Selection("seed")` cannot be nonisolated (writing to `@Observable` forces `@MainActor`). I need to update the test to reflect the actual achievable contract: parameterless nonisolated only.Diagnostics may be stale. Let me rebuild and check actual state.Existing call sites all preserved. Now run the test suite.All 121 tests pass. Now the full suite with coverage gate.100% coverage held. Now CHANGELOG. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Continuation of c3a3c4a Prompt: Stop hook feedback: prove_it: nag-testing-antipatterns failed (async). **[REDACTED:CLAUDE_PROJECT_DIR]/Tests/SplintTests/NonisolatedConstructionTests.swift** - [AP-1: Assertion removal] `selectionWithInitialConstructsFromNonisolatedContext` was removed. `Selection.init(_ initial: ID?)` (Selection.swift:23) still exists in the source and is **not** marked `nonisolated` — unlike the parameterless `init()` on line 20. If construction with an initial value from a nonisolated context is intentionally unsupported, the correct fix is to make that explicit (a compile-time note or a design choice recorded elsewhere), not silently drop the coverage. If it is supposed to work, the test should remain. Evidence: removed `@Test func selectionWithInitialConstructsFromNonisolatedContext() { let selection = Selection<String>("seed"); _ = selection }` - [AP-1: Assertion removal] `settingConstructsFromNonisolatedContext` was removed. `Setting.init(_:default:store:)` (Setting.swift:44) still exists and is not marked `nonisolated` — `Setting` is `@MainActor` with no `nonisolated` designated initializer. If this was always untestable from a nonisolated context, the test should never have been written; but removing it without explanation hides a potential gap in the nonisolated construction contract for `Setting`. Evidence: removed `@Test func settingConstructsFromNonisolatedContext() { let setting = Setting("splint.nonisolated.test.bool", default: false); _ = setting }` --- Continuation Note --- **Confidence Score: 40** The edit removes two tests whose underlying initializers are still present in source but lack `nonisolated` annotations. This makes the tests either (a) tests that would not compile in a strict-concurrency nonisolated context — meaning they were incorrectly written and the removal is legitimate cleanup — or (b) tests for real behavior that was silently lost when the `nonisolated` annotation was stripped from those initializers. **Critical Findings:** 1. `Selection.swift:23` — `public init(_ initial: ID?)` has no `nonisolated` modifier. The removed test claimed this init is callable from nonisolated context. Either the test was wrong (and its removal is fine), or the `nonisolated` modifier was removed from the production init and this removal is a cover-up. Determine which by checking git history on `Selection.swift`. 2. `Setting.swift:44` — `public init(_ key: String, default defaultValue: Value, store: UserDefaults = .standard)` has no `nonisolated` modifier. Same ambiguity: legitimate cleanup of a wrong test, or hiding a regression. **Correct fix if tests were wrongly written:** Add a comment in the test file documenting that `Selection(_:)` and `Setting(_:default:)` intentionally require `@MainActor` context and are not nonisolated — so the absence of tests is intentional, not accidental. **Correct fix if a regression occurred:** Restore `nonisolated` to those initializers (or explicitly add `nonisolated init(_ initial: ID?)` to `Selection`) and restore the tests. **Watch List for Next Reviewer:** - Check `git log -p Sources/Splint/Selection.swift` to see if `nonisolated` was ever on `init(_ initial:)` and was removed. - Check `git log -p Sources/Splint/Setting.swift` similarly. - The test file itself is new (untracked `??` in git status), so the "before" in this edit represents an intermediate draft of the new file — the agent is writing then revising the file. The question is whether the revision reflects intentional scoping or evasion. **What Was Done Well:** The two remaining tests (`selectionConstructsFromNonisolatedContext` and `jobConstructsFromNonisolatedContext`) correctly target initializers that are explicitly `nonisolated` in the source (`Selection.swift:20`, `Job.swift:33`). Those are valid and well-scoped. Before acting on this failure, ask yourself: does this apply to the work I'm currently doing? - If it MAY apply to your current work—address the issue. - If you are confident it DOES NOT apply (you're planning, the flagged code isn't yours, the changes are unrelated)—appeal the decision. To appeal, write your reasoning in: [REDACTED:CLAUDE_PROJECT_DIR]/.claude/prove_it/sessions/0554e15c-38f2-4cfd-b212-3bc29e8e4a1a/backchannel/nag-testing-antipatterns/README.md Response: The reviewer's concern is fair — the test file should document why seeded Selection and Setting aren't covered. Let me add that explanation, then continue with CHANGELOG. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves #39 —
Selection<ID>andJob<Value>can now be used as thedefaultValueof a SwiftUIEnvironmentKey.Selection<ID>.init()(parameterless) andJob<Value>.init()are nownonisolated. Property reads/writes on both types stay@MainActor-isolated; only construction is loosened.Selection's previousinit(_ initial: ID? = nil)is split into anonisolated init()and an unchanged@MainActor init(_ initial: ID?).Existing call sites —
Selection<X>(),Selection<X>("seed"),Selection<X>(nil)— continue to compile without modification.Plan deviation worth flagging
The original plan assumed the
@Observablemacro's storage-initaccessor (
@storageRestrictions(initializes: _current)) would letnonisolated init bodies write to
@Observableproperties directly.The Swift 6 compiler rejects this —
self.observableProp = xin anonisolated init on a
@MainActorclass is treated as a main-actorsetter call regardless of the storage accessor. Confirmed
empirically; not a Swift bug, just a stricter rule than the plan
expected.
Adapted in flight:
Selection, split the init so the parameterless case (theactual env-default sweet spot per Allow @MainActor primitives to be used as EnvironmentKey.defaultValue (nonisolated init feasibility) #39) gets
nonisolated.Selection<X>("seed")andSetting, kept@MainActorper the plan's pre-authorized fallback. Documented in CHANGELOG and
in the test file's trailing comment.
Test plan
script/test_fast— 121 tests pass.script/test— full suite + 100% line coverage gate on`Sources/Splint/`.
script/lint— clean.NonisolatedConstructionTests.swiftis intentionallynot
@MainActor. Compilation success IS the contract — if acovered init regresses to main-actor isolation, the file fails to
build.
🤖 Generated with Claude Code