Skip to content

Resolve Screenplay options once, at the entry point that sees them first - #2452

Closed
woksin wants to merge 1 commit into
mainfrom
fix/screenplay-nullable-emission-and-options-resolution
Closed

Resolve Screenplay options once, at the entry point that sees them first#2452
woksin wants to merge 1 commit into
mainfrom
fix/screenplay-nullable-emission-and-options-resolution

Conversation

@woksin

@woksin woksin commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Fixed

WithDefaults ran twice on the way through a generation - once in the
generator against the assembly being analyzed, once again in the emitter
against the domain of the model - and the two fallbacks are different
questions that only happen to agree today. A host calling Emit directly
therefore behaved subtly differently from the same model reached through
a generation.

Resolved options now answer with themselves, so whichever entry point
sees them first decides the fallback and the other half reads that
decision rather than working one out again.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JfEAq3cyHk3mrWpo8QA9db
@woksin woksin added the patch label Aug 5, 2026
@woksin

woksin commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Reviewer notes.

What changed. ScreenplayOptions.WithDefaults carries a private Resolved flag and returns this once set. ScreenplayGenerator.Generate resolves against AnalyzedCompilations.NameOf(ordered); ScreenplayEmitter.Emit still resolves against model.Domain, because a host can emit a model it already holds without ever generating — but on the path through a generation the second call is now a no-op, so the fallback the analysis half used is the one the document is named after.

Why the flag rather than deleting one call site. The two fallbacks answer different questions and are not interchangeable: Emit has nothing but the model's domain to fall back on, Generate knows the assembly it read. Deleting either resolution would change behaviour for one of the two entry points. The flag makes the resolution idempotent instead, which is the property the issue actually wants.

Note on record equality. Resolved is a private auto-property, so it participates in the synthesized Equals/GetHashCode (private members do) but not in ToString/PrintMembers (which only prints public ones). Two otherwise-identical options that differ in whether they have been resolved therefore compare unequal. That is intended, but worth a reviewer's eye in case anything compares options across the resolve boundary.

Verification. Screenplay.Specs 1173 passed / 0 failed. Debug build zero warnings. The SA1101 warnings visible in the spec project's build output are pre-existing on main and unrelated (they reproduce on a clean checkout).

Scope correction — item 6 of #2401 needed no work. My triage comment on the issue claimed no spec covered optional/nullable emission end to end. That was wrong: for_ScreenplayGenerator/when_generating/from_source_declaring_values_that_may_be_absent.cs (commit da2b5711, 2026-07-28, on main) covers exactly that — nullable value type, nullable reference type, collection of optionals, query parameter, printed Type? assertions, a round-trip reprint, and an assertion that Nullable is never named. I missed it because it sits under for_ScreenplayGenerator rather than for_ApplicationModelAnalyzer and is named for absence rather than nullability. This PR therefore closes out #2401 on its own; I have corrected the issue.

@woksin

woksin commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

CI note — the red specs (Source/DotNET/MongoDB.Specs) run in this PR's history is an unrelated pre-existing flake.

It failed once, then passed on a re-run of the identical commit with no changes (failed run). The failing spec was for_MongoDBReadModelForCommandResolver.when_resolving.and_the_instance_is_present.should_resolve_the_document, with:

MongoDB.Bson.BsonSerializationException : There is already a serializer registered for type Object.

Why it cannot be this change:

The flake is real and will recur. for_MongoDBReadModelForCommandResolver/when_resolving/given/a_resolver.cs calls new ServiceCollection().AddCratisMongoDB(); in Establish(), which runs once per spec class in the assembly, and its own comment notes the Cratis serializers "cannot be registered once the driver has cached its own for a Guid". 230c671f ("Establish MongoDB the way an application does in the resolver specs") introduced that call to fix the opposite symptom, recording that the specs "passed only when some other specification in the assembly had run AddCratisMongoDB() first, and failed the moment they ran alone" — so the fixture is order-dependent in both directions rather than in neither.

Worth its own issue; I have not filed one.

@woksin

woksin commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Folded into #2451 rather than shipped separately.

Both changes are in the Screenplay generator, and this repository ships related work as one pull request — #2418 landed twelve issues at once, and #2425 is titled for exactly that. Splitting them also split one release across two PRs carrying different semver labels, which is the opposite of what the label is for.

The commit is unchanged (f6dbd59e), merged into feat/screenplay-specification-held-values with no rewriting. #2451 now carries both bullets and stays minor, which dominates this one's patch.

The CI note above still applies and is repeated on #2451: the red MongoDB.Specs run was an unrelated pre-existing flake, now filed separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant