fix(dummies): gate distinct collections by the effective element domain - #199
Conversation
A distinct collection (SetOf, ListOf(...).Distinct(), dictionary keys)
gated its element count against the element generator's cardinality
alone, so it rejected satisfiable requests whose Containing(...) values
lie outside that domain -- e.g. Any.SetOf(Any.Int32().OneOf(1, 2))
.Containing(3).WithCount(3), where {1, 2, 3} is reachable.
Model the effective distinct domain instead. A new internal
IDomainMembership<T> lets a generator answer whether a value is one it
could produce; every finite-cardinality generator now implements it.
CollectionState counts only the elements that must come from the
generator: it subtracts the contained values proven outside the domain
(and each opaque ContainingAny draw) from the required count, and raises
the resolution cap by the same out-of-domain values. A value already
inside the domain still does not inflate capacity, so genuinely
impossible requests keep failing eagerly; an unprovable overlap defers
to the bounded generation-time draw rather than a false conflict.
The check is written in subtractive form so it cannot overflow for a
near-long.MaxValue element domain, and membership is decided under the
generator's own equality so it stays a sound upper bound under a custom
comparer.
Refine the still-Proposed ADR-0013 (its context and the rationale claim
that the eager check "never rejects a satisfiable request", which this
case disproved) and the user-facing wording, and add tests across SetOf,
ListOf().Distinct(), custom comparers and order-independent declaration.
Refs: #188
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1JGj1wj5C55UE9vxQFWoF
The comment claimed the out-of-domain scenario was exercised "for dictionary-shaped keys", but no dictionary is constructed and none can be: AnyDictionary exposes no Containing surface, so its keys are gated purely by count. Say instead that dictionaries share the same CollectionState path and are therefore covered transitively. Refs: #188 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1JGj1wj5C55UE9vxQFWoF
A distinct collection (SetOf, ListOf(...).Distinct(), dictionary keys)
gated its element count against the element generator's cardinality
alone, so it rejected satisfiable requests whose Containing(...) values
lie outside that domain -- e.g. Any.SetOf(Any.Int32().OneOf(1, 2))
.Containing(3).WithCount(3), where {1, 2, 3} is reachable.
Model the effective distinct domain instead. A generator now answers
both "how many distinct values can I produce" and "is this one of them"
through a single unified interface ICardinalityHint<T> (cardinality plus
membership), so a fixed value proven outside the domain extends the
effective cardinality while one already inside it does not.
CollectionState counts only the elements that must come from the
generator: it subtracts the contained values proven outside the domain
(and each opaque ContainingAny draw) from the required count, and raises
the resolution cap by the same out-of-domain values. Genuinely
impossible requests still fail eagerly; an unprovable overlap defers to
the bounded generation-time draw rather than a false conflict. The check
is subtractive so it cannot overflow for a near-long.MaxValue domain,
and membership is decided under the generator's own equality so it stays
a sound upper bound under a custom comparer.
Merging cardinality and membership into one interface makes the pairing
compiler-enforced: a finite-cardinality generator cannot drift out of
the eager perimeter without failing to compile. Hold that promise across
the whole knowable perimeter by bringing every finite generator into it,
including the six that silently sat outside before -- Int128, UInt128,
Decimal, Double, Single and Half -- whose specs now advertise a finite
cardinality (allow-lists, and narrow 128-bit ranges) and answer
membership; continuum ranges and strings stay out, deferring to the
bounded draw as before.
Refine the still-Proposed ADR-0013 to the unified capability, and add
ADR-0023 (Proposed) asking whether the exotic-width generators
(Int128/UInt128 and their dedicated engine, and Half) should remain in a
dummies library at all -- a breaking product decision left to the
maintainer. Add tests across SetOf, ListOf().Distinct(), custom
comparers, order-independent declaration, near-maximum cardinality, and
the newly-gated decimal, floating-point and 128-bit generators.
Refs: #188
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01L1JGj1wj5C55UE9vxQFWoF
2876bca to
5794263
Compare
|
@codex Please perform an in-depth review of the code introduced or modified by this pull request, which implements a new feature. The objective is to determine whether the feature is correctly designed, correctly implemented, sufficiently tested, and consistently integrated into the existing project. Review scopeStart by reading:
Do not review the diff in isolation. Verify that the changes are consistent with the abstractions, behaviors, contracts, and conventions already established in the project. Review areas1. Requirements and expected behaviorVerify that the implementation actually satisfies the described requirements. Look specifically for:
Do not treat a behavior as incorrect merely because a different design would also have been possible. Report an issue only when there is a demonstrable contradiction, omission, defect, or concrete risk. 2. Design and architectureEvaluate:
Also verify that the feature does not introduce a second competing way to achieve an existing operation without an explicit and justified reason. 3. Public API and developer experienceFor every public API, fluent API, extension method, overload, or generic abstraction added or modified, review:
Mentally validate several realistic usage scenarios, including edge cases and invalid usages. 4. Code correctnessLook for concrete defects, including:
Do not report micro-optimizations without a realistic impact. 5. TestsAssess whether the tests genuinely demonstrate the intended behavior of the feature. Verify coverage of:
Identify tests that may pass without actually proving the announced behavior, as well as tests that are excessively coupled to implementation details. Do not mechanically request a test for every line or method. Recommend tests only when they protect an important behavior or an identified risk. 6. DocumentationVerify that:
Do not criticize writing style unless it creates a real technical ambiguity or communicates incorrect behavior. 7. Regressions and integrationAnalyze the impact on existing code, including:
Do not perform a general audit of the entire repository. Inspect existing code only when it is necessary to evaluate the changes introduced by this pull request. Expected outputStart with a concise summary containing:
Then report only genuinely actionable findings, classified using these severity levels:
For each finding, include:
Group findings that share the same root cause instead of reporting multiple variations of the same issue. After the findings, add a separate section for open questions. Do not present a question as a defect unless the answer establishes that an actual issue exists. Finish with:
Review rules
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5794263d6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
DecimalIntervalSpec and ContinuousIntervalSpec only reported a finite cardinality for their allow-list branch, so a validated pin such as Any.Decimal().Zero() or Any.Double().Between(1, 1) -- a singleton domain of exactly one value -- was left outside the eager perimeter: Any.SetOf(Any.Decimal().Zero()).WithCount(2) passed declaration and failed only later during the bounded draw. Report 1 for a validated pin (_min == _max) so the fully knowable contradiction conflicts eagerly, and cover it with a distinct-collection test. Refs: #188 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1JGj1wj5C55UE9vxQFWoF
Codex noted that finite floating-point ranges (a narrow Half/Single sub-range, Half's ~63k full domain) sit outside the eager perimeter. That is intentional (Option A): counting a narrow float type's representable values in a range is type-specific bit-arithmetic disproportionate to the dummy use case, the case is pathological, and the bounded seed-named draw already covers it. Make the boundary explicit in ADR-0013's two-groups paragraph (EN + FR): pools, narrow integer/time ranges, allow-lists and single-value pins are counted cheaply and gated eagerly; a floating-point range defers, so a decimal or floating-point generator is gated only through an allow-list or a pin. Refs: #188 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1JGj1wj5C55UE9vxQFWoF
Flip ADR-0013 (gate distinct collections by cardinality, otherwise by a bounded draw) from Proposed to Accepted at the maintainer's decision, and set its date to the day it reached that status, per the index convention. Update the ADR index row to match. Refs: #188 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1JGj1wj5C55UE9vxQFWoF
The maintainer decided to keep Any.Int128, Any.UInt128, and Any.Half rather than prune them, preserving the "Any covers every numeric primitive" orthogonality. The proposal never reached main, so remove the never-accepted draft and its index row; no generator code changes. Refs: #188 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L1JGj1wj5C55UE9vxQFWoF
Rebased onto main after PR #199 merged a substantive rewrite of this ADR's decision content; folds that content into the editorial trim so neither PR's work is lost.
Rebased onto main after PR #199 merged a substantive rewrite of this ADR's decision content; folds that content into the editorial trim so neither PR's work is lost.
Summary
Fix issue #188: a distinct collection (
SetOf,ListOf(...).Distinct(), dictionary keys) gated its element count against the element generator's cardinality alone, so it rejected satisfiable requests whoseContaining(...)values lie outside that domain — e.g.Any.SetOf(Any.Int32().OneOf(1, 2)).Containing(3).WithCount(3), where{1, 2, 3}is reachable. This models the effective distinct domain instead, and completes the "hold the eager promise everywhere" work started in ADR-0013 by bringing every finite-domain generator into the same cardinality contract.Type of change
ICardinalityHint+IDomainMembershipinto one interfaceChanges
Fix the false rejection (Dummies: Distinct collection cardinality rejects valid contained values outside the item domain #188).
CollectionStatenow counts only the elements that must be drawn from the generator: it subtracts theContaining(...)values proven outside the domain (and each opaqueContainingAny(...)draw) from the required count, and raises the resolution cap by the same out-of-domain values. The check is written subtractively so it cannot overflow for a near-long.MaxValuedomain, and membership is decided under the generator's own equality so it stays a sound upper bound under a custom comparer. A contained value already inside the domain still does not inflate capacity, so genuinely impossible requests keep failing eagerly; an unprovable overlap defers to the bounded generation-time draw rather than a false conflict.Unify cardinality and membership into one interface.
ICardinalityHintbecomes genericICardinalityHint<T>, carrying bothDistinctCardinalityandContains(T). A finite-cardinality generator is now compiler-forced to answer membership, so the pairing cannot silently drift out of the eager perimeter. The interimIDomainMembership<T>is removed.Hold the promise across the whole knowable perimeter. The six generators that silently sat outside it are brought in —
AnyDecimal,AnyDouble,AnySingle,AnyHalf,AnyInt128,AnyUInt128. Their specs (DecimalIntervalSpec,ContinuousIntervalSpec,WideIntervalSpec) now advertise a finite cardinality (allow-lists, and narrow 128-bit ranges) and answer membership; continuum ranges and strings stay out, deferring to the bounded draw as before.OrdinalIntervalSpecgains the matchingContains.Tests in
AnyCollectionTests: out-of-domain values extending the effective domain; in-domain values not inflating it; order-independence ofDistinct/Containing/count; near-maximum cardinality without overflow; conservativeContainingAnydeferral; a merging comparer collapsing an out-of-domain value back into the domain; and the eager perimeter now reaching the decimal, floating-point and 128-bit generators.Documentation:
ICardinalityHintandCollectionStateXML docs updated to the unified contract and the effective-domain model;README.nuget.mdwording clarified.Testing
dotnet build FirstClassErrors.slndotnet test FirstClassErrors.slnFirstClassErrors.Analyzers.UnitTests)All green — 0 warnings;
Dummies.UnitTests162 passed, and the rest of the solution (analyzers 85, core 423, request-binder, gendoc 169, cli 64, …) with 0 failures.Documentation
ICardinalityHintandCollectionStatedoc/updated —README.nuget.md; ADR-0013 refined and accepteddoc/handwritten/for-users/README.fr.md) updated if user-facing behavior changed — n/a: that user page does not cover distinct collections; the ADR-0013 French mirror is kept in syncArchitecture decisions
Proposed: ADR-____ADR-0013 (gate distinct collections by cardinality, otherwise by a bounded draw) is refined and accepted in this PR: the rationale is corrected — the earlier draft claimed the eager check "never rejects a satisfiable request", which #188 disproved — and the maintainer accepted it here (
Status: Accepted).ADR-0023 (prune the exotic-width numeric generators — 128-bit and
Half) was drafted during this work as aProposedquestion, but the maintainer decided to keepAny.Int128/Any.UInt128/Any.Half. The draft has been withdrawn; no generator code changed.Related issues
Closes #188