Design record for ticket #1782 (REMED-COLL-SORTEDSET-VIEW-DESIGN), audit
finding SR-AUD-361. Recorded 2026-07-27 before any production change. No
production or test source changed under this ticket. SR-AUD-361 remains
confirmed (design-complete), not remediated.
System::Collections::Generic::SortedSet<T>::GetViewBetween must return a
live, bounded, bidirectionally write-through view over the same underlying
tree state, matching .NET's TreeSubSet, instead of the independent snapshot
copy it returns today.
The selected architecture is Alternative D — one public type with a tagged representation over independently owned, reference-counted tree state:
SortedSet<T>stops holdingstd::set<T>by value and instead holdsstd::shared_ptr<State>, whereStateowns thestd::set<T>and the single version counter.- A
SortedSet<T>object is either an owning full set (no bounds) or a bounded view (std::optional<T>lower and upper bounds over the sameState). One public type, one public return type, no new public class. GetViewBetweenkeeps returningSortedSet<T>by value; the returned object is a handle onto the parent's state, not a copy of its elements.std::shared_ptrreproduces .NET's GC lifetime rule exactly: a view keeps the state alive, so a view that outlives the set it came from is well-defined rather than a dangling reference.
One public signature change is required and is not yet approved.
GetViewBetween must lose its const qualifier, because a live view returned
from a const SortedSet<T>& would be a write-through handle onto a const
object. That is a source-breaking change of the same category as ticket
#1770/#1771's ICollection::CopyTo removal and ticket #1779/#1780's Empty()
return-type change, so implementation ticket #1783 is created blocked
pending explicit user approval (§28).
Alternative E (retain snapshot semantics and document the divergence) is rejected: it is what the code does today, it leaves a documented .NET contract permanently violated, and it does not fix the four adjacent defects this design work newly measured (§4.6).
The owning per-file report is
audit/modules/collections/include/System/Collections/Generic/SortedSet.hpp.audit.md.
Its finding text, preserved verbatim and unaltered by this ticket:
SR-AUD-361 — medium — GetViewBetween returns a detached snapshot rather than the required live bounded view
The implementation returns a separate
SortedSetcopy and explicitly documents the divergence. The direct probe reportsview-add-visible-in-source=0andsource-add-visible-in-view=0: mutations do not flow in either direction. .NET returns a range-enforced, write-through live view, so callers can silently mutate the wrong object.Missing assertions and diagnostics:
- Tests exercise range membership but not bidirectional write-through, live updates, or out-of-range view mutation.
- A future implementation needs view-bound diagnostics for source/view updates and violations.
The finding is medium in audit/AUDIT_FINDINGS_INDEX.md and is not a member
of any CCF-* cross-cutting cause.
Two earlier planning statements about this finding are partly superseded by this design and are corrected here rather than rewritten in place, per this repository's practice of preserving historical narrative:
NEXT.mdandplan.md(recorded under ticket #1779) state that SR-AUD-361 "would require replacingSortedSet<T>'sstd::setbacking with a custom tree structure supporting live, bounded, write-through sub-range views — .NET's ownTreeSubSetnested class is 378 lines — before any bounded implementation ticket could even be written". That premise does not hold.std::setis already an ordered associative container withlower_bound,upper_bound, and stable iterators; a bounded view needs only a shared owner for the container plus a pair of bounds. §7 and §10 give the full design and §11 the working prototype evidence; no hand-rolled red-black tree is needed. .NET'sTreeSubSetis 378 lines mainly because it must re-implementInOrderTreeWalk,BreadthFirstTreeWalk,FindNode,MinInternal, andMaxInternalagainst rawNodepointers — workstd::setalready does.SortedSet.hpp's own@warningdoc-comment makes the same claim ("not achievable on top ofstd::set… without replacing this type's entire internal representation with a hand-rolled tree structure matching .NET's own"). It is wrong for the same reason and must be replaced by ticket #1783.
Correcting a wrong reason for deferring the work does not make the work
smaller: the ownership model, copy/move semantics, and the required const
removal are the real cost, and they are why this is a design-first ticket.
All probes live in the repository-local, gitignored build-probe-sortedset/
tree (matched by the build* .gitignore entry). No production or test source
was modified. Build helper: build-probe-sortedset/build.sh <probe> <mode>,
which compiles with
-std=c++23 -Wall -Wextra -Wpedantic plus -fsanitize=address,undefined
(asan), -Werror (werror), or neither (none), against
modules/core/include and modules/collections/include and the six
modules/core/src/System/*Exception*.cpp support sources, so every frame in a
sanitizer report is instrumented.
./build-probe-sortedset/build.sh probe1_current_behavior asan
ASAN_OPTIONS=detect_leaks=1 UBSAN_OPTIONS=print_stacktrace=1 \
./build-probe-sortedset/probe1_current_behavior
Result: exit 0, failures=0, no ASan/UBSan diagnostic and no leak. The
current implementation is memory-safe; it is semantically wrong. Full log:
build-probe-sortedset/probe1_current_behavior.log. The load-bearing lines,
mapped to the seventeen required reproduction steps:
| # | Scenario | Observed | .NET |
|---|---|---|---|
| 1–2 | View shape over {1..10}, range [3,7] |
view-count=5, view-min=3, view-max=7, excludes 2 and 8 |
same |
| 3–4 | Parent Add(5) in range after view creation |
source-add-visible-in-view=0 |
visible |
| 5–6 | Parent Remove(4) in range |
source-remove-visible-in-view=0 |
visible |
| 7–8 | view.Add(5) |
view-add-returned=1, view-add-visible-in-source=0 |
visible in source |
| 9–10 | view.Remove(4) |
view-remove-returned=1, view-remove-visible-in-source=0, parent-still-contains-4=1 |
removed from source |
| 11 | view.Add(99), far out of range |
out-of-range-add-threw=0, out-of-range-add-returned=1, out-of-range-value-now-in-view=1, view-max-after-out-of-range-add=99 |
ArgumentOutOfRangeException("item") |
| 11b | view.Remove(10), out of range |
out-of-range-remove-threw=0, out-of-range-remove-returned=0 |
same (false, no throw) |
| 12 | view.Clear() |
clear-view-count-after=0, clear-parent-count-after=7 (unchanged from 7) |
parent loses exactly the in-range elements |
| 13 | Nested outer[3,7].GetViewBetween(4,6) |
nested-inner-count=3 — correct by accident |
same |
| 13b | Nested view that widens a bound | nested-widen-lower-threw=0, nested-widen-lower-count=4; nested-widen-upper-threw=0 |
ArgumentOutOfRangeException("lowerValue"/"upperValue") |
| 13c | inner.Remove(5) |
nested-inner-remove-visible-in-outer=0, nested-inner-remove-visible-in-parent=0 |
visible in both |
| 14 | Parent destroyed, returned object survives | after-parent-destruction-view-count=5, sum 25, still mutable, no ASan report |
view stays valid (GC keeps the parent alive) |
| 15 | Parent copy / move / copy-assign / move-assign | returned object completely unaffected in every case (view-count-after-parent-move=2, viewMoved-count-after-parent-copy-assign=2, …) |
no C++ equivalent; view tracks the object |
| 16 | Mutating the parent during iteration of the returned object | parent-mutation-during-view-iteration-throws=0, all 3 elements visited |
InvalidOperationException |
| 16b | Mutating the returned object during iteration of the parent | view-mutation-during-parent-iteration-throws=0, all 7 visited |
InvalidOperationException |
| 16c | Mutating the returned object during its own iteration | view-self-mutation-during-iteration-throws=1 |
same |
| 17 | Element type whose ordering reverses operator< |
descending-view-count=3, descending-inverted-threw=1 — works only because the probe type defines both operator< and operator> consistently |
ordering comes from IComparer<T> alone |
| — | Inverted range GetViewBetween(7, 3) |
ArgumentException, message lowerValue is greater than upperValue. (Parameter 'lowerValue') |
Must be less than or equal to upperValue. (Parameter 'lowerValue') |
| — | Equal bounds [3,3] |
equal-bounds-count=1 |
same |
| — | Disjoint bounds [100,200] |
disjoint-range-count=0, disjoint-range-min=0 (T{}) |
same |
| — | view.UnionWith({5,6,99}) |
union-with-out-of-range-view-contains-99=1, parent unaffected |
ArgumentOutOfRangeException; in-range items write through |
| — | view.IntersectWith, view.ExceptWith |
operate on the copy only; intersect-parent-count=7 unchanged |
write through to the parent |
The two boldface lines in rows 3–4 and 7–8 reproduce the audit's own
view-add-visible-in-source=0 / source-add-visible-in-view=0 evidence exactly.
./build-probe-sortedset/build.sh probe2_iterator_lifetime asan
./build-probe-sortedset/probe2_iterator_lifetime safe # exit 0
ASAN_OPTIONS=detect_leaks=0 ./build-probe-sortedset/probe2_iterator_lifetime copy-assign
ASAN_OPTIONS=detect_leaks=0 ./build-probe-sortedset/probe2_iterator_lifetime move-assign
ASAN_OPTIONS=detect_leaks=0 ./build-probe-sortedset/probe2_iterator_lifetime outlive
Logs: probe2_safe.log, probe2_unsafe.log.
safe:guard-add-fires=1,guard-remove-fires=1,guard-clear-fires=1,guard-duplicate-add-fires=0(a rejected duplicate correctly does not bump the version). The guard works for same-object structural modification.copy-assign:assign-guard-fired=0, thenassign-stale-dereference-value=60. Whole-object copy assignment destroys every node the outstanding iterator points into, butversion_is a plain member that assignment overwrites with the source's counter instead of bumping, so the guard cannot fire. libstdc++'s_Rb_treenode-recycling assignment then reuses the storage, so ASan reports nothing and the iterator silently yields an element of the new tree. No diagnostic at all.move-assign:move-assign-guard-fired=0, then ASanheap-use-after-free,READ of size 4, freed by_Rb_tree::_M_eraseduring the assignment.outlive: an iterator outliving its set produces ASanstack-use-after-scopeinsideIterator::checkVersion()itself — the guard's own rawconst SortedSet* owner_read is the unsafe access.
outlive is ordinary C++ iterator-lifetime UB and is not a defect. The
copy-assign and move-assign results are a real gap in the guard this
class advertises, and are treated in §4.6 and §28.
./build-probe-sortedset/build.sh probe3_comparer_requirement werror
./build-probe-sortedset/build.sh probe3_comparer_requirement werror \
-DSORTEDSET_PROBE_INSTANTIATE_VIEW
Without the macro the type works (less-only-count=3, less-only-contains-5=1,
less-only-min=1, less-only-max=9). With it, instantiating GetViewBetween
for a T that provides operator< and nothing else — exactly the contract
SortedSet.hpp's own class doc-comment states — is a hard compile error:
SortedSet.hpp:297:19: error: no match for 'operator>' (operand types are 'const LessOnly' and 'const LessOnly')
297 | if (lower > upper)
SortedSet.hpp:300:77: error: no match for 'operator>' (operand types are 'const LessOnly' and 'const LessOnly')
300 | for (auto it = data_.lower_bound(lower); it != data_.end() && !(*it > upper); ++it)
GetViewBetween is the only member of the class that spells its comparisons
with operator>; every other ordering decision is delegated to std::set,
which uses operator< through std::less<T>.
modules/collections/include/System/Collections/Generic/SortedSet.hpp:296-303:
[[nodiscard]] SortedSet<T> GetViewBetween(const T& lower, const T& upper) const {
if (lower > upper)
throw System::ArgumentException("lowerValue is greater than upperValue.", "lowerValue");
SortedSet<T> view;
for (auto it = data_.lower_bound(lower); it != data_.end() && !(*it > upper); ++it)
view.Add(*it);
return view;
}The return is SortedSet<T> by value — not a reference, not another public
type, not a private proxy. It is a wrapper around freshly copied storage: a
default-constructed SortedSet<T> whose own std::set receives copies of the
in-range elements one Add at a time. Nothing links it to the source.
The member is const, so it is callable on a const SortedSet<T>& today.
template<typename T>
class SortedSet {
std::set<T> data_;
intcs version_ = 0;
// ... no base classes, no virtual members
};Measured (probe5_layout_symbols, GCC 14.2.0, x86-64):
sizeof(SortedSet<int>) = 56, alignof = 8, sizeof(std::set<int>) = 48,
sizeof(SortedSet<std::string>) = 56, sizeof(SortedSet<int>::Iterator) = 24,
is_polymorphic = 0, is_trivially_copyable = 0,
is_nothrow_move_constructible = 1, is_copy_assignable = 1.
Against the fourteen questions the ticket poses:
| Question | Current answer |
|---|---|
Copies values into a new SortedSet? |
Yes — element-by-element Add. |
| Shares comparer state? | No comparer state exists; both objects use std::less<T>. |
| Shares mutation state? | No. Separate std::set and separate version_. |
| Sees later parent insertions? | No (source-add-visible-in-view=0). |
| Sees later parent removals? | No (source-remove-visible-in-view=0). |
| Forwards view mutations to the parent? | No (view-add-visible-in-source=0). |
| Restricts additions to the bounds? | No. view.Add(99) succeeds and Max becomes 99. Bounds exist only for the instant of the copy. |
| Valid after parent copy/move/assign/clear/destruction? | Yes, trivially — it is independent. Probe 1 confirms with no sanitizer diagnostic. |
| Independent versioning? | Yes, and that is the defect: parent mutation cannot invalidate view enumerators. |
Correct Count, Min, Max, enumeration? |
Correct at the instant of the call, stale from the next parent mutation onward. |
| Reverse enumeration? | The port has no Reverse() at all (.NET has IEnumerable<T> Reverse()). |
Nested GetViewBetween? |
Compiles and returns another snapshot; widening is silently accepted where .NET throws. |
| Set operations within the bounds? | Present but bounds-unaware and non-write-through. |
| Iterator invalidation matching the parent? | No — three-way divergence, probe 1 rows 16/16b/16c. |
Preserves comparer equivalence rather than operator< assumptions? |
No — it uses operator>, which is neither std::set's ordering predicate nor the documented element contract (§3.3). |
SortedSet.hpp:277-290 carries an explicit @warning KNOWN DIVERGENCE FROM .NET block describing the snapshot behavior and asserting the fix is not
achievable on std::set. §2 corrects that assertion; ticket #1783 must replace
the block.
Three tests, all asserting only snapshot-instant range membership:
| File:line | Assertions |
|---|---|
modules/collections/tests/System/Collections/Generic/LinkedListSortedSetTests.cpp:465 |
SortedSet<int> view = ss.GetViewBetween(3, 7); then count/min/max and two negative Contains. |
modules/collections/tests/System/Collections/Generic/SortedStackTests.cpp:43 |
auto view = s.GetViewBetween(2, 4); then count and three Contains. |
modules/collections/tests/System/Collections/Generic/Ticket1713VersionTrackingTests.cpp:108 |
auto view = s.GetViewBetween(2, 3); count and two Contains; its comment explicitly documents the snapshot implementation and becomes stale under the fix. |
Focused validation of the current behavior for this ticket:
./build/SharpRuntimeTests_Collections_Core --gtest_filter="SortedSetTests.*:GenSortedSetTest.*:SortedSetVersionTrackingTests.*"
→ 41/41 passed; --gtest_filter="*GetViewBetween*" → 3/3 passed.
None of the three tests asserts a snapshot property that the live-view fix
would break: all three only read the view immediately after creating it. The
existing test suite therefore requires no assertion change under the selected
design; only the stale comment at Ticket1713VersionTrackingTests.cpp:109 must
be corrected.
These are not SR-AUD-361 and are not new SR-AUD-* identifiers — the
audit numbering is frozen at SR-AUD-364. They are recorded here because they
live inside the surface ticket #1783 rewrites and would otherwise be silently
carried forward. They are folded into #1783's scope (§28), not spun out as
separate tickets.
GetViewBetweenrequiresoperator>although the class documents and otherwise needs onlyoperator<(§3.3). A conforming element type fails to compile. Fixed for free by taking the predicate fromstd::set::key_comp().- Bounds are not enforced after construction.
view.Add(99)succeeds (probe 1 row 11). Even under snapshot semantics this contradicts the@return/@paramdocumentation of a "range" object. - Nested views may silently widen (probe 1 row 13b) where .NET throws
ArgumentOutOfRangeException. - Whole-object assignment defeats the fail-fast version guard, producing a
silently wrong dereference on copy-assign and an ASan-confirmed
heap-use-after-freeon move-assign (§3.2). The selected ownership model eliminates both as a consequence, not as a special case (§14, §11.4).
Additionally, the exception message for an inverted range diverges from
.NET (lowerValue is greater than upperValue. vs Must be less than or equal to upperValue.); the type and parameter name already match.
Read from the local current .NET sources, not from memory:
/rv/tmp/runtime/src/libraries/System.Collections/src/System/Collections/Generic/SortedSet.cs(2,015 lines)/rv/tmp/runtime/src/libraries/System.Collections/src/System/Collections/Generic/SortedSet.TreeSubSet.cs(379 lines)/rv/tmp/runtime/src/libraries/System.Collections/src/Resources/Strings.resx/rv/tmp/runtime/src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
| Question | .NET answer | Source |
|---|---|---|
| Public return type | public virtual SortedSet<T> GetViewBetween(T? lowerValue, T? upperValue) |
SortedSet.cs:1508 |
| Cached or new per call | New per call: return new TreeSubSet(this, lowerValue, upperValue, true, true); |
SortedSet.cs:1514 |
| How the view references the tree | TreeSubSet : SortedSet<T> holds private readonly SortedSet<T> _underlying; and re-roots via root = _underlying.FindRange(_min, _max, _lBoundActive, _uBoundActive) |
TreeSubSet.cs:17,46,318 |
| Bound inclusivity | Inclusive both ends: IsWithinRange returns false only when Compare(_min, item) > 0 or Compare(_max, item) < 0 |
TreeSubSet.cs:112-122 |
| Lower/upper validation | if (Comparer.Compare(lowerValue, upperValue) > 0) throw new ArgumentException(SR.SortedSet_LowerValueGreaterThanUpperValue, nameof(lowerValue)); |
SortedSet.cs:1510-1513 |
lowerValue > upperValue message |
"Must be less than or equal to upperValue.", parameter lowerValue |
Strings.resx:138-140 |
| Parent → view propagation | VersionCheckImpl: when version != _underlying.version, the subset re-roots and adopts the parent's version |
TreeSubSet.cs:313-328 |
| View → parent propagation | AddIfNotPresent calls _underlying.AddIfNotPresent(item); DoRemove calls _underlying.Remove(item) |
TreeSubSet.cs:59,84 |
Out-of-range Add |
throw new ArgumentOutOfRangeException(nameof(item)) — parameter name item, default message |
TreeSubSet.cs:54-57 |
Out-of-range Remove |
return false — no throw, parent untouched |
TreeSubSet.cs:79-82 |
Clear |
Breadth-first collects the in-range items and calls _underlying.Remove for each; the parent keeps everything outside the range |
TreeSubSet.cs:92-110 |
Count caching |
Count calls VersionCheck(updateCount: true); the subset recomputes by InOrderTreeWalk only when _countVersion != _underlying.version |
SortedSet.cs:266-273, TreeSubSet.cs:322-327 |
Min / Max |
MinInternal / MaxInternal are overridden to walk within the bounds and return default(T) when the view is empty |
TreeSubSet.cs:124-183, SortedSet.cs:1457,1478 |
| Nested views | A view may only narrow: widening either bound throws ArgumentOutOfRangeException(nameof(lowerValue)) / (nameof(upperValue)); otherwise it delegates to _underlying.GetViewBetween, so nesting is always flattened to depth 1 |
TreeSubSet.cs:342-353 |
| Set operations | Non-virtual base methods routed through virtual Add/Remove/Contains, so on a view they enforce bounds and write through; UnionWith/IntersectWith call VersionCheck() first when this is TreeSubSet |
SortedSet.cs:843-851,983-1000,1065,1103 |
| Enumeration and version checks | Enumerator captures _tree and _version; MoveNext calls _tree.VersionCheck() then throws InvalidOperationException(SR.InvalidOperation_EnumFailedVersion) on mismatch. Initialize/MoveNext skip out-of-range nodes via _tree.IsWithinRange |
SortedSet.cs:1829-1930 |
| Enumerator message | "Collection was modified; enumeration operation may not execute." |
Strings.resx:2647-2649 |
| Reverse enumeration | public IEnumerable<T> Reverse() builds new Enumerator(this, reverse: true); on a view it is bounded like the forward one |
SortedSet.cs:1499-1506 |
| Synchronization / thread safety | None: bool ICollection.IsSynchronized => false, object ICollection.SyncRoot => this |
SortedSet.cs:279-282 |
| Owner no longer referenced | The view's _underlying field is a strong reference, so the GC cannot collect the parent while any view is reachable; the view stays fully functional |
TreeSubSet.cs:17,41 |
| Comparer propagation | TreeSubSet constructs : base(Underlying.Comparer) — the view always uses the parent's comparer |
TreeSubSet.cs:39 |
| Copy construction from a view | new SortedSet<T>(collection) explicitly excludes TreeSubSet from its DeepClone fast path and falls back to enumerate-sort-dedupe |
SortedSet.cs:88 |
| Serialization on a view | GetObjectData / OnDeserialization throw PlatformNotSupportedException |
TreeSubSet.cs:365-375 |
| Exception ordering | Range validity is checked before any allocation; on a view, bound checks precede narrowing | SortedSet.cs:1510, TreeSubSet.cs:344-351 |
(1) Directly reproducible. Bound inclusivity; the invalid-range exception
type, message, and parameter name; bidirectional write-through; out-of-range
Add throwing ArgumentOutOfRangeException("item"); out-of-range Remove
returning false; range-scoped Clear; lazy version-gated Count; bounded
Min/Max returning T{} when empty; nested-view narrowing-only validation
with flattening; bounds-enforcing write-through set algebra; a single shared
version counter driving fail-fast enumeration; comparer propagation; and the
"no synchronization" contract.
(2) Relies on managed GC or object identity. Only one behavior: a view
keeps its parent alive. std::shared_ptr<State> reproduces it exactly, with
one deliberate refinement — what stays alive is the tree state, not the
parent SortedSet object. In .NET those are the same thing; in C++ the
object is a value that can be copied, moved, assigned, and destroyed
independently of its storage. §12 defines the consequences.
(3) Requires a different safe C++ ownership model. Copy, move, assignment,
and destruction of a SortedSet<T> object have no .NET counterpart at all,
because .NET SortedSet<T> is a reference type with no copy or assignment
operator. §12 and §13 define them from first principles rather than by
analogy. Likewise, TreeSubSet's virtual-override mechanism cannot be used:
GetViewBetween returns by value, and returning a derived type by a base value
would slice it. The tagged representation in §10 is the C++ equivalent.
(4) Intentional sharp-runtime deviations. Four, all recorded in §26:
serialization hooks (absent by permanent project deviation, so
PlatformNotSupportedException on a view has nothing to attach to);
Reverse() (absent from the port and deliberately not added here);
IComparer<T> construction (absent from the port — ordering is
std::less<T>, so "comparer propagation" reduces to "the view uses the same
std::set and therefore the same key_comp()"); and T?/default(T) nullable
bound arguments (C++ references cannot be null, so both bounds are always
active, matching GetViewBetween's own lowerBoundActive: true, upperBoundActive: true).
SortedSet<T> is a standalone class template: no base classes and no virtual
members, so the "collection interfaces implemented by SortedSet<T>" list is
empty. It implements none of ICollection, IEnumerable<T>, ISet<T>, or
IReadOnlyCollection<T>, unlike .NET's
SortedSet<T> : ISet<T>, ICollection<T>, ICollection, IReadOnlyCollection<T>, IReadOnlySet<T>, ISerializable, IDeserializationCallback. That absence is
what makes a tagged single-type representation feasible at all.
| Surface | Line | Change under the selected design |
|---|---|---|
SortedSet() |
61 | Body changes: allocate the shared State. Signature unchanged. |
explicit SortedSet(std::initializer_list<T>) |
67 | Body changes. Signature unchanged. |
| Copy constructor | implicit | Becomes user-declared. Owning set → deep clone (today's behavior); view → another handle. |
| Move constructor | implicit | Becomes user-declared, noexcept; leaves the source a valid empty owning set. |
| Copy assignment | implicit | Becomes user-declared. Rebinds this handle; never mutates state another handle observes. |
| Move assignment | implicit | Becomes user-declared, noexcept. |
| Destructor | implicit | Stays implicit; shared_ptr releases the state, which survives while any view or iterator holds it. |
GetViewBetween |
296 | Loses const; returns a live bounded handle; validates via key_comp(); rejects nested widening. |
Add |
106 | On a view, rejects out-of-range with ArgumentOutOfRangeException("item"); writes to shared state. |
Remove |
119 | On a view, returns false for out-of-range; writes to shared state. |
Clear |
141 | On a view, erases only [lower, upper] from the shared state. |
Contains |
132 | On a view, returns false for out-of-range. |
getCountProperty |
75 | O(1) for an owning set; version-cached O(k) for a view. |
getIsEmptyProperty |
81 | Delegates to getCountProperty() == 0. |
getMinProperty / getMaxProperty |
89 / 97 | Range-scoped; T{} when the view is empty. |
| Lower/upper bound operations | — | No public member exists; internally std::set::lower_bound/upper_bound become the range primitives. |
Iterator (nested class) |
39 | Holds shared_ptr<const State> + current + end + version; no raw owner pointer. |
begin() / end() |
320 / 322 | Range-scoped for a view. Signatures unchanged. |
Reverse |
— | Does not exist. Explicitly out of scope (§26). |
UnionWith, IntersectWith, ExceptWith, SymmetricExceptWith |
149–201 | Bounds-enforcing and write-through on a view; existing &other == this self-aliasing guards must be strengthened to shared-state comparison (§18). |
IsSubsetOf, IsSupersetOf, IsProperSubsetOf, IsProperSupersetOf, SetEquals, Overlaps |
210–270 | Must compare in-range elements only; SetEquals's data_ == other.data_ must become an element-wise range comparison. |
ToVector |
311 | Range-scoped for a view. |
| Comparer access | — | No public accessor exists; not added. |
| Serialization hooks | — | None exist; not added (permanent project deviation). |
ToSortedSet() |
— | New, additive: materializes an independent owning set (§10). |
getIsViewProperty() |
— | New, additive: lets callers and tests distinguish the two roles. |
System/Collections/Generic/SortedSet.hpp is included by exactly three
files, all tests (§4.5). There is no production consumer anywhere in this
repository, no test/consumer/ fixture, and no other module header. The
module owner is Collections.Core (modules/collections/CMakeLists.txt),
whose only public dependency is Core.Base; the design adds no new include and
therefore no new dependency edge — the graph stays at 41 modules / 90 edges.
System::Collections::Immutable::ImmutableSortedSet is unrelated: it uses
std::set directly and does not include this header.
GetViewBetweenreturns by value. Any design in which the view is a derived type is impossible without changing the return type, because returning a derived object through a base value slices it. This alone rules out a literal port ofTreeSubSet.SortedSet<T>is a value type today — copyable, movable, assignable, destructible, and stored by value in every existing call site. Turning the whole type into a reference/handle type would silently convert every existingSortedSet<T> b = a;into an alias. Unacceptable.- Elements must be owned independently of any one object. For a view to
remain valid while its parent object is copied, moved, assigned, or
destroyed, the
std::set<T>cannot live inside the parent object. This is the single structural change every live-view alternative shares, and it is why no live-view design can preserve object layout (§17). std::setiterators are stable across insert and across erase of other elements, so a bounded range can be expressed as a pair of positions recomputed on demand; no custom tree is required.std::setowns its ordering predicate, retrievable by value viakey_comp(). Any comparison the view performs must use that predicate, notoperator<oroperator>written by hand, or the view's notion of "in range" can disagree with the container's notion of "sorted".- There is no GC. Lifetime must be explicit.
std::shared_ptrgives exactly the reachability rule .NET's strong_underlyingfield gives, at the cost of one control block per set. - No virtual members may be added.
SortedSet<T>is non-polymorphic today (is_polymorphic = 0); adding a vptr would change layout further, break value semantics under slicing, and violate the project's "no broad public header refactor" rule for no benefit.
Every SortedSet<T> holds shared_ptr<State>; copying always shares.
Views are the same object with bounds.
Rejected. It converts SortedSet<T> from a value type into a handle type for
every existing user. SortedSet<int> b = a; b.Add(x); would mutate a — a
silent, un-diagnosable semantic break in code that never mentions
GetViewBetween. It is also further from .NET than the selected design: .NET
has no copy operation at all, so there is no .NET behavior that A reproduces
and D does not. Its only advantage over D is that copy semantics become
uniform, which is not worth breaking every unrelated consumer.
The parent owns a shared_ptr<State>; views hold weak_ptr<State> and throw
InvalidOperationException (or ObjectDisposedException) once the parent dies.
Can it be safe? Yes — weak_ptr::lock() makes parent death deterministically
detectable with no dangling pointer, and the moved-from/reassigned cases are
detectable too. B is memory-safe; it is not, however, simpler or more faithful.
Rejected for three reasons. (i) It diverges from .NET precisely where D
matches: in .NET a view keeps its parent alive and never becomes invalid, and
returning a view from a factory whose set is a local is legal, idiomatic managed
code. Under B that pattern throws at first use. (ii) Every single member —
Count, Min, Max, Contains, Add, Remove, Clear, begin, end,
every set operation — needs a liveness check and a new failure mode, roughly
doubling the exception matrix for a state .NET cannot even reach. (iii) It buys
nothing D lacks: D never dangles either, because the state outlives every
handle. B trades a strictly-safe behavior for a throwing one.
GetViewBetween returns a distinct SortedSetView<T> (or an internal proxy).
Genuine advantages. Roles are explicit in the type system; copy semantics are
unambiguous (a view type is documented as a handle, a set type as a value); no
if (isView()) branch inside SortedSet<T>; and a view can be made
non-assignable or non-default-constructible if desired.
Rejected. (i) It does not avoid the layout change — the view still needs
the set's storage to be independently owned, so SortedSet<T>'s data members
change anyway. C pays D's whole compatibility cost and adds a return-type break
on top. (ii) The return type changes, breaking
SortedSet<int> view = ss.GetViewBetween(3, 7); — one in-repository test uses
exactly that spelling, and downstream usage cannot be inspected. (iii) It
breaks .NET parity structurally: in .NET a view is a SortedSet<T> and can
be passed to UnionWith, IsSubsetOf, SetEquals, or any SortedSet<T>
parameter. SortedSetView<T> would need either an implicit conversion (which
re-materializes a snapshot at every boundary, reintroducing the very defect) or
a duplicated set-algebra API on a second type. (iv) It adds a new public header
and public type to Collections.Core.
SortedSet<T> holds shared_ptr<State> plus optional bounds and is either an
owning full set or a bounded view. GetViewBetween keeps its return type.
Costs, stated honestly. (i) Copy semantics depend on the object's role. This
is stated as one rule — copying preserves the role: an owning set copies its
elements, a view copies its reference — but it is still a runtime-dependent
behavior, and it is the single most surprising thing about the design. (ii)
About ten members gain an isView() branch. (iii) sizeof changes, up for
large T (§17). (iv) One pointer indirection on every operation.
Benefits. It is the only alternative that keeps the public return type, keeps
every existing call site compiling and every existing assertion passing (§4.5),
keeps a view usable everywhere a SortedSet<T> is expected, and matches .NET's
own model of "the view is a SortedSet<T>". It also fixes all four adjacent
defects of §4.6 as a by-product.
Evaluated honestly, and rejected. Its cost is not zero, and it is not merely "the finding stays open":
- Ported C# that relies on write-through —
set.GetViewBetween(a,b).Add(x),view.Clear()to delete a range,foreachover a view while the set is mutated — compiles unchanged and produces a silently different result. There is no compile error, no exception, and no diagnostic. This is exactly the failure mode ticket #1771 rejected when it declined to keep a throwingCopyToshim ("removal makes each call a compile error naming the replacement"). - The header already documents the divergence and has done so for the whole life of the finding; documentation demonstrably has not prevented the audit from classifying it as a confirmed medium defect.
- It leaves the four adjacent defects of §4.6 in place, including one
(
operator>) that makes a documented-conforming element type fail to compile and one (assignment defeating the version guard) with an ASan-confirmed use-after-free. - The cost of deferring rises: every new consumer written against snapshot behavior increases the eventual migration burden.
A reduced variant — E′: keep snapshot semantics but fix the four adjacent
defects and sharpen the documentation — is a legitimate fallback if the
const removal in §28 is refused. It closes none of SR-AUD-361 but is strictly
better than the status quo. It is recorded as the rollback target in §21.
Ratings: ✅ preserved /
| Criterion | A (uniform shared) | B (weak views) | C (view type) | D (tagged) | E (snapshot) |
|---|---|---|---|---|---|
| Memory safety | ✅ | ✅ | ✅ | ✅ | ✅ |
| Ownership / lifetime model | ✅ explicit | ✅ explicit, role-based | ✅ trivial | ||
| Parent → view propagation | ✅ | ✅ | ✅ | ✅ | ❌ |
| View → parent propagation | ✅ | ✅ | ✅ | ✅ | ❌ |
| Bounds enforcement | ✅ | ✅ | ✅ | ✅ | ❌ |
| Copy behavior | ❌ every set becomes an alias | ✅ | ✅ | ||
| Move behavior | ✅ | ✅ | ✅ | ✅ | |
| Iterator validity | ✅ | ✅ | ✅ (also fixes §4.6.4) | ||
| Comparer support | ✅ | ✅ | ✅ | ✅ (fixes §4.6.1) | ❌ §4.6.1 unfixed |
| Public source compatibility | ✅ | const removal |
❌ return type and const |
const removal only |
✅ |
| ABI / mangled symbols | ❌ | ✅ | |||
| Object layout | ❌ | ❌ | ❌ | ❌ | ✅ |
| Implementation complexity | Low | High | High (two types) | Medium | None |
| Performance | Same | +lock() per op |
Same as D | +1 indirection; O(k) view Count |
Best per-op, O(k) per call |
| Module dependencies | none added | none added | none added | none added | none |
| Testability | Poor (aliasing hard to pin) | Medium | Good | Good | Good |
| Migration burden | Catastrophic | High | High | Medium | None |
| .NET parity | Partial | Partial | Partial | Full | None |
SortedSet<T> becomes a handle onto reference-counted tree state, tagged by
the presence of bounds.
┌──────────────────────────────┐
SortedSet<T> parent│ shared_ptr<State> ───────────┼──┐
(owning: no bounds)│ optional<T> lower_ = {} │ │
│ optional<T> upper_ = {} │ │ ┌────────────────┐
└──────────────────────────────┘ ├──▶│ State │
┌──────────────────────────────┐ │ │ std::set<T> │
SortedSet<T> view │ shared_ptr<State> ───────────┼──┤ │ intcs version │
(bounded: [3,7]) │ optional<T> lower_ = 3 │ │ └────────────────┘
│ optional<T> upper_ = 7 │ │ ▲
└──────────────────────────────┘ │ │
┌──────────────────────────────┐ │ │
SortedSet<T> view2 │ shared_ptr<State> ───────────┼──┘ │
(bounded: [4,6], │ optional<T> lower_ = 4 │ │
nested → flattened│ optional<T> upper_ = 6 │ Iterator ───┘
to depth 1) └──────────────────────────────┘ (also holds a
shared_ptr)
- One shared
version. Any structural modification through the parent or through any view bumps it, so every outstanding iterator on any of them fail-fasts — reproducing .NET's single_underlying.version. - Views are flattened. A view of a view refers to the same root
Statewith intersected bounds, exactly asTreeSubSet::GetViewBetweendelegates to_underlying.GetViewBetween. Nesting depth is always 1. - Iterators hold their own
shared_ptr<const State>, so an iterator can outlive the object it came from without reading freed memory.
Precise enough that ticket #1783 does not redesign anything. Doc-comments are
elided here; #1783 must supply full Doxygen blocks per CLAUDE.md §3.
namespace System::Collections::Generic {
using SharpRuntime::intcs;
template<typename T>
class SortedSet {
struct State {
std::set<T> data;
intcs version = 0;
};
std::shared_ptr<State> state_;
std::optional<T> lower_; // absent => lower bound inactive
std::optional<T> upper_; // absent => upper bound inactive
mutable intcs cachedCount_ = -1; // .NET TreeSubSet::count
mutable intcs cachedCountVersion_ = -1; // .NET TreeSubSet::_countVersion
using SetIterator = typename std::set<T>::const_iterator;
// std::set::key_comp() returns BY VALUE. Binding it to a const reference
// returns a reference to a temporary (-Wreturn-local-addr, observed while
// prototyping). Copy the predicate; never alias it.
[[nodiscard]] typename std::set<T>::key_compare comparer() const;
[[nodiscard]] SetIterator rangeBegin() const; // lower_bound(*lower_) or begin()
[[nodiscard]] SetIterator rangeEnd() const; // upper_bound(*upper_) or end()
SortedSet(std::shared_ptr<State> state,
std::optional<T> lower,
std::optional<T> upper); // private view constructor
public:
class Iterator {
std::shared_ptr<const State> state_;
SetIterator it_;
SetIterator end_;
intcs version_ = 0;
void checkVersion() const;
public:
Iterator(std::shared_ptr<const State> state, SetIterator it, SetIterator end);
const T& operator*() const;
const T* operator->() const;
Iterator& operator++();
bool operator==(const Iterator& other) const;
bool operator!=(const Iterator& other) const;
};
SortedSet();
explicit SortedSet(std::initializer_list<T> items);
SortedSet(const SortedSet& other); // role-preserving
SortedSet(SortedSet&& other) noexcept;
SortedSet& operator=(const SortedSet& other); // rebinds
SortedSet& operator=(SortedSet&& other) noexcept;
~SortedSet() = default;
// --- unchanged signatures, view-aware bodies -------------------------
[[nodiscard]] intcs getCountProperty() const;
[[nodiscard]] bool getIsEmptyProperty() const;
[[nodiscard]] T getMinProperty() const;
[[nodiscard]] T getMaxProperty() const;
bool Add(const T& item);
bool Remove(const T& item);
[[nodiscard]] bool Contains(const T& item) const;
void Clear();
void UnionWith(const SortedSet<T>& other);
void IntersectWith(const SortedSet<T>& other);
void ExceptWith(const SortedSet<T>& other);
void SymmetricExceptWith(const SortedSet<T>& other);
[[nodiscard]] bool IsSubsetOf(const SortedSet<T>& other) const;
[[nodiscard]] bool IsSupersetOf(const SortedSet<T>& other) const;
[[nodiscard]] bool IsProperSubsetOf(const SortedSet<T>& other) const;
[[nodiscard]] bool IsProperSupersetOf(const SortedSet<T>& other) const;
[[nodiscard]] bool SetEquals(const SortedSet<T>& other) const;
[[nodiscard]] bool Overlaps(const SortedSet<T>& other) const;
[[nodiscard]] std::vector<T> ToVector() const;
Iterator begin() const;
Iterator end() const;
// --- THE ONE BREAKING SIGNATURE CHANGE: `const` is removed -----------
[[nodiscard]] SortedSet<T> GetViewBetween(const T& lower, const T& upper);
// --- additive, non-breaking -----------------------------------------
[[nodiscard]] bool getIsViewProperty() const;
[[nodiscard]] bool IsWithinRange(const T& item) const;
[[nodiscard]] SortedSet<T> ToSortedSet() const; // materialize, detached
};
} // namespace System::Collections::GenericToSortedSet() is the C++ spelling of .NET's new SortedSet<T>(view) — the
supported way to obtain the old snapshot behavior deliberately (§24). A
collection constructor SortedSet(const SortedSet&) cannot express it because
it collides with the copy constructor.
New standard includes required: <memory>, <optional>, <iterator>, plus
System/ArgumentOutOfRangeException.hpp. All are Core.Base or standard
library; no new module dependency edge (§6).
- The
Stateis the owner of the elements. NoSortedSet<T>object owns elements directly. Every object — owning set or view — holds ashared_ptr<State>. - A
Statelives exactly as long as at least one handle references it, where a handle is an owning set, a view, or anIterator. This is the C++ expression of .NET's rule thatTreeSubSet._underlyingis a strong reference. - An owning set is a
SortedSet<T>with no active bounds; a view is one with at least one active bound.GetViewBetweenalways activates both, so in practice a view has both. The inactive-bound representation is retained so a futureHead()/Tail()(which .NET's_lBoundActive/_uBoundActivealso anticipates) needs no representation change. - A view never owns a distinct copy of any element, so there is no synchronisation problem, no staleness window, and no reconciliation step.
- Destroying an owning set does not destroy the elements while a view or iterator survives. The state is simply no longer reachable through that object. This is well-defined and matches .NET. It also means an orphaned state can exist — reachable only through views — which is correct, not a leak: measured leak-free at 100k elements (§15).
- There are no ownership cycles.
Stateholds only elements; it never holds aSortedSet<T>.shared_ptralone is sufficient; noweak_ptris required anywhere.
The single governing rule:
Copying preserves the object's role; assignment rebinds the assigned handle and never mutates state that another handle observes.
| Operation | Behavior | Effect on existing views | Effect on existing iterators |
|---|---|---|---|
| Copy-construct an owning set | Deep clone into a fresh State (today's value semantics, preserved exactly) |
none — they still observe the original state | none |
| Copy-construct a view | Shares the same State and copies the bounds → another handle onto the same range |
none | none |
| Move-construct (either role) | Transfers the shared_ptr and bounds; the source becomes a valid, empty owning set (matching today's observed parent-after-move-count=0) |
none — the State is untouched, so views keep working and now observe mutations made through the destination object |
none |
| Copy-assign | Equivalent to destroying this handle and copy-constructing it from other (copy-and-move-assign idiom, self-assignment guarded) |
none — views onto the previous state keep that state alive and keep observing it | iterators into the previous state remain valid and continue to observe its pre-assignment elements |
| Move-assign | Same rebinding, noexcept; the source becomes a valid empty owning set |
none | as above |
| Destructor | Releases one reference to the State |
none | none |
Two deliberate decisions inside that table:
(a) Assignment rebinds rather than mutating in place. The alternative —
overwriting the existing State's contents so views follow the parent's new
value — was considered and rejected: it makes a = b silently change what an
unrelated view observes (action at a distance), and it has no .NET counterpart
to justify the surprise.
(b) Iterators into a reassigned object keep working on the previous contents rather than fail-fasting. This is a strict improvement over today, where the same code silently yields an element of the new tree (copy-assign) or is an ASan-confirmed use-after-free (move-assign) — §3.2. Bumping the outgoing state's version to force a fail-fast was considered and rejected: the outgoing state's contents did not change, so it would spuriously invalidate enumerations held by unrelated views of that same state. Fail-fast stays scoped to genuine structural modification of the state being enumerated.
- Copying a view yields another handle onto the same
Statewith the same bounds. Mutating either handle is visible through the other and through the parent. This is the C++ equivalent ofvar v2 = view;in C#. - Moving a view transfers the handle and bounds; the moved-from view
becomes a valid, empty owning set (it is no longer a view), matching
today's
view-after-move-count=0. - Assigning to a view rebinds it by the same rule as §13:
view = othermakesviewbehave likeother(a handle ifotheris a view, an independent owning set ifotheris an owning set). It does not attempt to replace the viewed range's contents, and it never throws. - Nested and overlapping views are ordinary additional handles. Overlapping
views agree instantly because there is only one
State(measured:overlap-b-sees-a-removal=1,overlap-a-sees-b-add=1). - A view's bounds are immutable after construction. There is no setter, so a view cannot silently widen.
Ordering is always state_->data.key_comp() — never operator< or
operator> spelled by hand. cmp(a, b) means "a orders before b".
| Operation | Condition | Result |
|---|---|---|
GetViewBetween(lower, upper) on any object |
cmp(upper, lower) |
ArgumentException("Must be less than or equal to upperValue.", "lowerValue") — .NET's exact message; the base appends (Parameter 'lowerValue') exactly once (post-#1776) |
GetViewBetween on a view |
cmp(lower, *lower_) (widens the lower bound) |
ArgumentOutOfRangeException("lowerValue") |
GetViewBetween on a view |
cmp(*upper_, upper) (widens the upper bound) |
ArgumentOutOfRangeException("upperValue") |
GetViewBetween |
valid, including lower == upper and a range disjoint from the contents |
a live view; a disjoint range is a valid empty view that still enforces its bounds |
SortedSet.cs:1510 / TreeSubSet.cs:344 |
||
IsWithinRange(item) |
lower_ active and cmp(item, *lower_) → false; upper_ active and cmp(*upper_, item) → false; else true |
inclusive both ends, TreeSubSet.cs:112-122 |
Add(item) on a view |
!IsWithinRange(item) |
ArgumentOutOfRangeException("item"), nothing written |
Add(item) on a view |
in range, absent | inserts into the shared state, bumps the version, returns true |
Add(item) on a view |
in range, present | returns false, version unchanged |
Add(item) on an owning set |
— | unchanged from today |
Remove(item) on a view |
!IsWithinRange(item) |
returns false, no throw, parent untouched |
Remove(item) on a view |
in range | erases from the shared state, bumps the version |
Contains(item) on a view |
!IsWithinRange(item) |
false |
Clear() on a view |
— | erases exactly [rangeBegin, rangeEnd) from the shared state; elements outside the bounds are untouched; version bumped only if something was erased |
Clear() on an owning set |
— | clears the shared state; version bumped only if it was non-empty |
getCountProperty() on a view |
— | std::distance(rangeBegin(), rangeEnd()), cached against the shared version (.NET's _countVersion) |
getMinProperty() / getMaxProperty() on an empty view |
— | T{} (default(T)), never throws |
Exception ordering on GetViewBetween: invalid range first, then lower
widening, then upper widening. No state is observed or allocated before the
first check.
Superseded by ticket #1785 (§33). The order above is what #1782 selected and #1783 shipped; it is preserved here as the historical record. The order in force since #1785 is lower widening, then upper widening, then invalid range — .NET's. Only a nested call that is simultaneously widening and inverted can tell the two apart; every other row of this table is unchanged. The "no state observed or allocated before the first check" property still holds.
P = owning set, V1/V2 = views over P's state, N = a view nested
inside V1. ≡ means the same underlying State.
| Mutation | Seen by P |
Seen by V1 |
Seen by V2 |
Seen by N |
Invalidates outstanding iterators of |
|---|---|---|---|---|---|
P.Add(x), x in V1's range |
yes | yes | if in range | if in range | P, V1, V2, N — all |
P.Add(x), x outside every view range |
yes | no | no | no | all (single shared version, exactly like .NET) |
P.Remove(x) |
yes | if was in range | if was in range | if was in range | all |
P.Clear() |
yes | yes (becomes empty) | yes | yes | all |
V1.Add(x), in range |
yes | yes | if in range | if in range | all |
V1.Add(x), out of range |
— | — | — | — | none (throws, no mutation) |
V1.Remove(x), in range |
yes | yes | if was in range | if was in range | all |
V1.Remove(x), out of range |
no | no | no | no | none (returns false) |
V1.Clear() |
yes, loses only V1's range |
yes | partially, where ranges overlap | partially | all |
N.Remove(x) |
yes | yes | if in range | yes | all |
P = other (assignment) |
P rebinds |
no change | no change | no change | none (§13(b)) |
P destroyed |
— | no change, state survives | no change | no change | none |
P moved |
destination observes it | no change | no change | no change | none |
The "invalidates all" column is the deliberate .NET behavior: SortedSet.cs's
enumerator compares against a single version shared through
TreeSubSet.VersionCheckImpl, so a mutation anywhere in the tree fail-fasts
every enumeration of the tree or any of its views.
begin()returnsIterator(state_, rangeBegin(), rangeEnd());end()returnsIterator(state_, rangeEnd(), rangeEnd()). For an owning set these aredata.begin()/data.end(), so today's behavior is unchanged.Iteratorcapturesstate_->versionat construction.operator*,operator->, andoperator++compare it and throwSystem::InvalidOperationException("Collection was modified; enumeration operation may not execute.")on mismatch — the exact current message, which is also .NET'sSR.InvalidOperation_EnumFailedVersion.operator==/operator!=compare only the underlyingstd::setiterator, as today, so range-fortermination is unchanged.- One version counter per
State. Mutation through the parent invalidates view enumerations and vice versa (probe 4:parent-mutation-during-view-iteration-throws=1,view-mutation-during-parent-iteration-throws=1), closing the two divergences of probe 1 rows 16/16b. - A rejected duplicate
Addand aRemoveof an absent element do not bump the version, so they do not invalidate an in-flight enumeration. This preserves today'sguard-duplicate-add-fires=0behavior. - Because
Iteratorholdsshared_ptr<const State>, an iterator that outlives the object it came from is safe and well-defined, and assignment to that object detaches rather than dangles (§13(b)). Both are strict improvements over §3.2's measured behavior. - Reverse iteration.
Reverse()does not exist in this port and is not added by ticket #1783 (§26). Should it ever be added, the rule is fixed here: reverse enumeration of a view walks[rangeBegin, rangeEnd)backwards fromstd::prev(rangeEnd()), under the same single-version guard.
-
On a view, every element mutation routes through the bounds-enforcing
Add/Remove, soUnionWithwith an out-of-range element throwsArgumentOutOfRangeException("item")and in-range elements write through — matching .NET, which routes the non-virtual base methods through virtualAdd/Remove/Contains. -
Read-only predicates (
IsSubsetOf,IsSupersetOf,IsProperSubsetOf,IsProperSupersetOf,SetEquals,Overlaps) consider only in-range elements on both sides.SetEquals's currentdata_ == other.data_whole-container comparison must become an element-wise comparison of the two ranges usingkey_comp()equivalence (!cmp(a,b) && !cmp(b,a)), notoperator==. -
A new self-aliasing hazard is created by this design and must be handled. Today's
ExceptWith/SymmetricExceptWithguard&other == this— object identity. With shared state,p.ExceptWith(viewOfP)aliases at the state level while the two objects differ, so iteratingother's range while erasing from the samestd::setis the exact iterator-invalidation UB ticket 324 fixed forHashSet<T>. The rule for #1783:- If
other.state_ == this->state_and the bounds are equal, apply the existing identity shortcut (Clear()forExceptWithandSymmetricExceptWith; no-op forIntersectWith). - Otherwise, whenever
other.state_ == this->state_, materializeother's in-range elements into astd::vector<T>before mutating. - The prototype takes the conservative route of always materializing
other's range first, which is correct for every case at the cost of one vector; #1783 may narrow it to the aliasing case only.
Measured working:
except-with-own-view-parent-count=2with elements 1 and 5 retained,symmetric-except-own-view-count=0,except-self-count=0,symmetric-except-self-count=0. - If
-
UnionWith/IntersectWithon a view have noVersionCheck()analogue to call: the port re-reads the shared state on every access, so .NET's explicitif (treeSubset != null) VersionCheck();is unnecessary rather than omitted.
Unchanged and explicitly restated: SortedSet<T> offers no thread-safety
guarantee, matching .NET (ICollection.IsSynchronized => false). Two
clarifications the shared representation makes necessary:
- A set and every view derived from it are one collection for concurrency purposes. Concurrent access to a parent and one of its views is exactly as unsafe as concurrent access to a single set.
shared_ptrreference-count updates are atomic, so lifetime management is race-free even if contents are not. Copying or destroying handles on different threads does not corrupt the control block. Nothing stronger is claimed.
Because the design claims no concurrency property beyond the existing contract, no ThreadSanitizer campaign is required for #1783 (§23).
Ticket #1783 should land in this order, keeping the build green at each step:
- Representation. Introduce
State, movedata_/version_into it, addstate_,lower_,upper_, and the count cache. Implement the five special members. KeepGetViewBetweenreturning a materialized set for now. Gate: the existing 41 SortedSet tests pass unchanged. - Range primitives.
comparer(),rangeBegin(),rangeEnd(),IsWithinRange,getIsViewProperty(). RoutegetCountProperty,getMinProperty,getMaxProperty,ToVector,begin,endthrough them. Gate: unchanged behavior for owning sets. Iteratorrework.shared_ptr<const State>plus a range end. Gate: the fourSortedSetVersionTrackingTestscases pass unchanged.- Live view. Drop
constfromGetViewBetween, return the bounded handle, add the invalid-range and nested-widening validation with .NET's messages and parameter names. Gate: the three existingGetViewBetweentests pass unchanged (§4.5). - Bounds-enforcing mutation. View-aware
Add,Remove,Contains,Clear. - Set algebra. View-aware set operations plus the strengthened shared-state self-aliasing guards of §18.
ToSortedSet()and the migration documentation.- Documentation. Replace the
@warning KNOWN DIVERGENCEblock; correct the stale comment atTicket1713VersionTrackingTests.cpp:109; update the class doc-comment's element requirement; addREADME.mdbreaking-change guidance (taking care not to add a second markdown link to an already-linked document, which measurably adds a Doxygen warning). - Permanent tests, consumer fixture, full gate (§21–§23).
A new dedicated file
modules/collections/tests/System/Collections/Generic/SortedSetLiveViewTests.cpp
(the pattern established by LinkedListNodeLifetimeTests.cpp and
CopyToBoundaryTests.cpp), keeping the existing three GetViewBetween tests in
place unchanged as the "still works" baseline. Required cases, one assertion
group each:
- Parent → view: in-range
AddandRemoveon the parent are visible in the view, includingCount,Min,Max, and enumeration. - View → parent: in-range
AddandRemoveon the view are visible in the parent. - Out-of-range invisibility: a parent mutation outside the bounds changes nothing observable through the view.
- Out-of-range
Add: throwsArgumentOutOfRangeException, parameter nameitem, and writes nothing. - Out-of-range
Remove: returnsfalse, throws nothing, leaves the parent intact. - Out-of-range
Contains:falseeven when the parent holds the element. Clearon a view: removes exactly the range from the parent and nothing else.- Bounds inclusivity: both endpoints are members;
[x,x]is a valid one-element range. - Invalid range: exact exception type, parameter name, and the .NET message (asserted as an exact string, since #1776 made the suffix single).
- Disjoint range: a valid empty view that still enforces its bounds, and
whose in-range
Addwrites through. - Nested narrowing: correct contents and write-through.
- Nested widening:
ArgumentOutOfRangeExceptionwith parameter namelowerValueandupperValuerespectively. - Overlapping views: mutation through one is visible through the other.
- Owner destruction: a view outliving its parent stays readable and mutable.
- Iterator outliving its set: safe and yields the pre-existing contents.
- Parent copy is a deep clone; the copy's mutations are invisible to the original and to its views.
- View copy is a handle; its mutations are visible through the original.
- Parent move: views follow the state; the moved-from object is a valid empty owning set.
- View move: keeps viewness; the moved-from object is a valid empty owning set.
- Copy-assign and move-assign a parent that has views: views and iterators are undisturbed, and the reassigned parent is detached.
- Enumeration invalidation, all three directions: self, parent-during-view, view-during-parent.
- Rejected duplicate
Add/ absentRemovedo not invalidate an in-flight enumeration. - Set algebra through a view:
UnionWithbounds violation;IntersectWith/ExceptWith/SymmetricExceptWithwrite-through sparing out-of-range elements. - Shared-state self-aliasing:
p.ExceptWith(viewOfP),view.SymmetricExceptWith(sameView), plus the existingExceptWith(self)andSymmetricExceptWith(self)regressions. - Range-aware
SetEquals/Overlaps/IsSubsetOf. ToSortedSet()produces a detached owning set.getIsViewProperty()is false for every constructor and true for everyGetViewBetweenresult, including nested.- Element type with
operator<only instantiatesGetViewBetween— a compile-level regression for §4.6.1, expressed as a file-scope instantiation plus a runtime assertion. - Non-trivial element type (
std::string) across the whole matrix. - Scale: 100,000 elements, a 20,001-element view, cached
Count, enumeration sum, and rangeClear.
Existing suites that must keep passing unchanged: SortedSetTests.*,
GenSortedSetTest.*, SortedSetVersionTrackingTests.* (41 today).
Each phase in §20 is independently revertable, and phases 1–3 are behavior
preserving. If a defect is found after phase 4, reverting phases 4–6 restores
snapshot semantics while keeping the safer representation — which is exactly
fallback E′ of §8: the adjacent defects of §4.6 stay fixed, SR-AUD-361
reopens. Because the whole change is one header plus one test file, git revert
of the implementation commit is a complete rollback with no data or schema
migration.
| Scenario | Sanitizers | Why |
|---|---|---|
| The full new test file | ASan + UBSan + LeakSanitizer | Ownership change; the state may be reachable only through views |
| Owner destroyed while views and iterators survive | ASan + LSan | The central lifetime claim of §12 |
| Parent copy / move / copy-assign / move-assign with live views and iterators | ASan + LSan | The §13 rebinding rules, and the §3.2 regressions |
| Iterator outliving its set | ASan | Replaces today's measured stack-use-after-scope |
| Nested and overlapping views mutating the same state | ASan + UBSan | Iterator invalidation across handles |
| Set algebra with shared-state aliasing | ASan | §18's new hazard — the ticket-324 failure mode |
100,000-element view: build, enumerate, range-Clear, teardown |
ASan + LSan | Orphaned-state teardown at scale |
| ThreadSanitizer | not required | §19 claims no concurrency property beyond the existing contract, and the design adds no shared mutable global. Per the repository's rule, TSan is run only when such a property is claimed. |
Verify LeakSanitizer is actually active with a deliberate-leak self-test, as
ticket #1775 did — under this sandbox's ptrace policy LSan has previously
failed to initialise silently.
Add test/consumer/collections_sorted_set_view.cpp, a standalone fixture
linking only SharpRuntime::Collections.Core, compiled
-Wall -Wextra -Wpedantic -Werror through the existing
test/consumer/CMakeLists.txt harness. It must construct a set, take a view,
mutate in both directions, take a nested view, outlive the parent, and exit 0 —
proving the header is self-sufficient and that a narrow consumer needs no new
component.
Add a companion negative fixture
test/consumer/collections_sorted_set_view_negative.cpp, following the
collections_object_model_readonlydictionary_negative.cpp precedent, asserting
that GetViewBetween on a const SortedSet<T>& fails to compile — the
visible face of the one approved signature change.
Run scripts/check_selective_components.sh with a repository-local TMPDIR
(the script's mktemp build trees otherwise land in /tmp, violating the
build policy).
For consumers of this repository, including CNA and mobile-eggbert, neither of which is in this checkout and neither of which has been inspected:
- Full rebuild is mandatory.
SortedSet<T>'s data members change, so object files compiled against the old header are layout-incompatible with new ones (§25). - Audit every
GetViewBetweencall site for a snapshot assumption. The dangerous patterns are: mutating the result and expecting the source to be unaffected; holding the result and expecting it to remain a point-in-time copy; and adding out-of-range elements to the result. - To keep snapshot behavior deliberately, replace
auto snap = set.GetViewBetween(a, b);withauto snap = set.GetViewBetween(a, b).ToSortedSet();. This is the exact analogue of .NET'snew SortedSet<T>(view). - A
constset can no longer produce a view.constSet.GetViewBetween(...)becomes a compile error naming the non-constoverload. Take a non-constreference, or copy the set first and take the view from the copy. - Copying the result of
GetViewBetweencopies the handle, not the elements.SortedSet<int> v2 = view;gives a second handle. UseToSortedSet()for an independent set. - Enumerating a view now fail-fasts when the source is mutated, matching
.NET. Code that mutated the source while walking a view previously
"worked"; it will now throw
InvalidOperationException. - Ticket #1773 is unrelated and stays blocked. It covers the
ICollection::CopyToABI sweep from ticket #1771 only. A downstream sweep for this change is a separate future item and is not created by ticket #1782.
Separated into the five layers the ticket requires. Returning the same public type is explicitly not treated as implying no impact.
GetViewBetween's parameter list and return type are unchanged.- It loses its
constqualifier. Every call on a non-constset still compiles; a call on aconst SortedSet<T>&becomes a compile error. All three in-repository call sites use non-constsets (§4.5), so no in-repository source break. Downstream cannot be inspected. - The five special members become user-declared. No call site changes, but the type stops being aggregate-initializable in any new way and copy semantics change for views (§14).
- Two additive members (
getIsViewProperty,ToSortedSet) and one additive query (IsWithinRange); additions cannot break existing code. SortedSet<T>is not explicitly instantiated anywhere; it is a header-only class template in theCollections.CoreINTERFACEtarget.
Measured with nm/c++filt on probe5_layout_symbols.o
(build-probe-sortedset/probe5_symbols_mangled.log):
W _ZNK6System11Collections7Generic9SortedSetIiE14GetViewBetweenERKiS5_ (today, const)
W _ZN18SortedSetPrototype9SortedSetIiE14GetViewBetweenERKiS3_ (proposed, non-const)
The Itanium C++ ABI encodes cv-qualification of the implicit object parameter,
so dropping const changes _ZNK… to _ZN…. Unlike ticket #1780's Empty()
— whose mangled name was byte-identical because return types are not encoded —
this is a genuine mangled-name change. It is not a link break in practice:
the class is header-only with weak/COMDAT emission per translation unit and the
Collections.Core target produces no archive, so every translation unit that
uses the member emits the new symbol when recompiled. It is a link break for
any pre-built object file that references the old symbol.
Measured (build-probe-sortedset/probe5_layout_symbols.log, GCC 14.2.0,
x86-64):
| Type | Today | Proposed |
|---|---|---|
sizeof(SortedSet<int>) |
56 | 40 |
sizeof(SortedSet<std::string>) |
56 | 104 |
sizeof(SortedSet<int>::Iterator) |
24 | 40 |
alignof |
8 | 8 |
is_polymorphic |
0 | 0 (unchanged — no vptr added) |
is_trivially_copyable |
0 | 0 |
is_nothrow_move_constructible |
1 | 1 (preserved) |
is_copy_assignable |
1 | 1 (preserved) |
The size for int shrinks (the 48-byte inline std::set is replaced by a
16-byte shared_ptr) and for std::string grows (two
std::optional<std::string> bounds cost 80 bytes). The growth scales with
sizeof(T); storing the bounds behind a single shared_ptr<const Bounds>
would make the size T-independent at the cost of one allocation per view and
one indirection per bounds check. That optimization is deferred, not
selected: views are comparatively rare, and allocation-free bounds checks are
on every hot path.
Any object file compiled against the old header is layout-incompatible with one compiled against the new header. Mixing them is an ODR violation with no diagnostic.
This is the point of the change. Existing code that relies on snapshot
independence compiles unchanged and behaves differently. §24 lists the
patterns; ToSortedSet() is the documented replacement. No in-repository
caller relies on snapshot independence — the only three call sites read the
view immediately and assert nothing that changes (§4.5).
Full clean rebuild of every consumer, plus the §24 call-site audit. This is the same rebuild expectation ticket #1771's ABI break already established for this release line; consumers still on the pre-#1771 revision must rebuild anyway.
| Operation | Today | Proposed |
|---|---|---|
Add/Remove/Contains on an owning set |
O(log n) | O(log n) + one pointer indirection |
Add/Remove/Contains on a view |
O(log k) on the copy | O(log n) + 1–2 comparator calls |
getCountProperty() on an owning set |
O(1) | O(1) |
getCountProperty() on a view |
O(1) on the copy | O(k) on first call per version, then cached (matches .NET's _countVersion) |
getMinProperty()/getMaxProperty() on a view |
O(1) on the copy | O(log n) — better than .NET's tree walk |
GetViewBetween itself |
O(k log k) — copies every in-range element | O(1) — no traversal, no allocation of elements |
| Enumerating a view | O(k) | O(log n) to position + O(k) |
| Memory per view | k elements | 1 shared_ptr + 2 bounds |
| Construction of an owning set | no allocation beyond the tree | +1 control-block allocation |
Net: GetViewBetween goes from O(k log k) with k allocations to O(1) with
none; the only regression is one heap allocation per owning set and O(k) for
the first Count of a view after each mutation. Measured at scale: a
100,000-element set, a 20,001-element view, cached Count, full enumeration,
and range Clear all run clean under ASan+UBSan+LSan.
Reverse()is not added. It is absent from the port today; adding it is unrelated API breadth. §17.7 fixes its semantics if it is ever added.IComparer<T>construction is not added. Ordering staysstd::less<T>; "comparer propagation" is satisfied because the view uses the parent'sstd::setand therefore itskey_comp().- Serialization hooks are not added (permanent project deviation).
CopyTois not added;ToVector()remains the copy-out route. The ticket #1771/#1774ICollectioncopy boundary is untouched.- Collection interfaces (
ISet<T>,ICollection<T>,IEnumerable<T>) are not implemented. Doing so would require virtual members, which §7.7 rules out. SortedDictionary,SortedList,ImmutableSortedSet,HashSetare not touched.ImmutableSortedSetin particular does not use this header.- SR-AUD-362 production behavior and its conservative correction note are not touched.
- Ticket #1773 stays blocked and untouched; CNA and mobile-eggbert are not inspected.
- No new third-party dependency, no module split, no CI matrix change, no repository-wide formatting.
| # | Risk | Severity | Mitigation |
|---|---|---|---|
| 1 | The const removal breaks an unknown downstream call site |
Medium | It is a compile error naming the replacement, never a silent behavior change — the same rationale ticket #1771 used to refuse a throwing shim. Gated on explicit approval (§28). |
| 2 | Downstream code silently relies on snapshot independence | High | The one genuinely silent risk. No compile error is possible. Mitigated by §24's call-site audit instruction, ToSortedSet(), and a README.md breaking-change entry — not eliminated. |
| 3 | Role-dependent copy semantics surprise a reader | Medium | Stated as one rule (§13), exposed by getIsViewProperty(), and pinned by tests 16–19 (§21). |
| 4 | The new shared-state self-aliasing hazard in set algebra (§18) | Medium | Explicitly designed, prototyped, and covered by test 24. It is the ticket-324 failure mode in a new guise; missing it would be a real regression. |
| 5 | sizeof(SortedSet<std::string>) grows 56 → 104 |
Low | Measured, documented, and reversible via the deferred shared_ptr<Bounds> variant (§25.3). |
| 6 | One extra heap allocation per owning set | Low | Measured; negligible next to the std::set node allocations that follow. |
| 7 | Orphaned state (reachable only through views) looks like a leak | Low | It is correct behavior; LeakSanitizer coverage at 100k elements proves it is released (§22). |
| 8 | The lazy Count cache goes stale under an unforeseen mutation path |
Medium | The cache is keyed on the single shared version, which every mutating path bumps; test 30 exercises it across an invalidation. |
| 9 | Scope creep into Reverse(), IComparer<T>, or the collection interfaces |
Medium | Explicitly excluded in §26. |
| 10 | Iterators surviving reassignment of their set observe pre-assignment data | Low | Deliberate (§13(b)), well-defined, and strictly better than today's silent-wrong-value / use-after-free. Documented. |
#1783 — REMED-COLL-SORTEDSET-LIVE-VIEW, P2, size L, status blocked.
Title: Implement live SortedSet GetViewBetween views. Finding: SR-AUD-361.
Size L, not M: one header is substantially rewritten, ~30 permanent
regressions and two consumer fixtures are added, and the change is semantically
breaking — comparable to ticket #1769 (REMED-COLL-LINKED-NODE, size L), which
made the same class of ownership change to LinkedListNode<T>. Priority stays
P2, inherited from SR-AUD-361's medium severity and matching the P2 used
for the other medium Collections contract findings (#1778, #1779, #1780).
Approve removing the
constqualifier fromSystem::Collections::Generic::SortedSet<T>::GetViewBetween(const T&, const T&), and approve the accompanying semantic change from a detached snapshot to a live, bidirectionally write-through bounded view, together with theSortedSet<T>object-layout change (sizeof(SortedSet<int>)56 → 40,sizeof(SortedSet<std::string>)56 → 104) that requires every consumer, including CNA and mobile-eggbert, to be rebuilt.
This is the same approval category as ticket #1770/#1771's
ICollection::CopyTo removal and ticket #1779/#1780's Empty() return-type
change. Ticket #1783 must not begin until it is granted.
If the approval is refused, the fallback is E′ (§8): keep snapshot
semantics, fix the four adjacent defects of §4.6, and sharpen the header
documentation. E′ needs no approval — it changes no signature and no layout —
but it closes none of SR-AUD-361, which would stay confirmed indefinitely.
Everything in §11 and §20, the permanent tests of §21, the sanitizer plan of §22, the consumer fixtures of §23, the documentation updates of §20.8, and the four adjacent defects of §4.6 (which live inside the rewritten surface and are fixed as a consequence of the design, not as separate work).
Everything in §26.
Warning-free cmake --build build --parallel 4;
scripts/run_component_tests.sh build with no regression below the 13,022-test
floor; python3 scripts/validate_module_boundaries.py --root . at 41 modules /
90 edges; python3 test/validate_module_boundaries_test.py;
python3 scripts/generate_component_catalog.py --check;
python3 scripts/db_consistency_check.py --db plan.sqlite3; git diff --check;
scripts/check_doxygen_warnings.sh at or below 1,942;
scripts/check_selective_components.sh with a repository-local TMPDIR; and a
network-permitted scripts/local_ci_check.sh build.
Every command and its result, for reproduction. All artifacts are in the
gitignored build-probe-sortedset/ tree; none is a tracked file.
| Probe | Command | Result |
|---|---|---|
probe1_current_behavior.cpp |
build.sh probe1_current_behavior asan then ASAN_OPTIONS=detect_leaks=1 UBSAN_OPTIONS=print_stacktrace=1 ./probe1_current_behavior |
exit 0, failures=0, no diagnostic, no leak. Full pre-fix matrix → §3.1 |
probe2_iterator_lifetime.cpp |
build.sh probe2_iterator_lifetime asan; then safe, copy-assign, move-assign, outlive |
safe exit 0; copy-assign silently wrong value 60, no diagnostic; move-assign ASan heap-use-after-free; outlive ASan stack-use-after-scope in checkVersion() → §3.2 |
probe3_comparer_requirement.cpp |
build.sh probe3_comparer_requirement werror and again with -DSORTEDSET_PROBE_INSTANTIATE_VIEW |
without: compiles -Werror, runs, exit 0. with: two no match for 'operator>' errors at SortedSet.hpp:297 and :300 → §3.3 |
SortedSetPrototype.hpp + probe4_prototype.cpp |
build.sh probe4_prototype asan -I build-probe-sortedset then ASAN_OPTIONS=detect_leaks=1 UBSAN_OPTIONS=print_stacktrace=1 ./probe4_prototype |
exit 0, failures=0, no diagnostic, no leak, including the 100,000-element scale case. Every §15/§16/§17/§18 rule verified → §11–§18 |
probe5_layout_symbols.cpp |
build.sh probe5_layout_symbols werror -I build-probe-sortedset; then g++ -c … && nm -C |
Layout and is_* trait table of §25.3; mangled-name comparison of §25.2 |
probe6_public_header_standalone.cpp |
build.sh probe6_public_header_standalone werror |
The production header compiles standalone under -Wall -Wextra -Wpedantic -Werror and runs, exit 0 — the baseline #1783 must preserve |
The prototype found one real design defect during development that the
implementation must avoid: std::set::key_comp() returns by value, so
binding it to a const reference is -Wreturn-local-addr (a reference to a
temporary). Recorded inline in §11.
Added by implementation ticket #1783 (REMED-COLL-SORTEDSET-LIVE-VIEW, P2,
size L) on local branch feature/remediation-coll-sortedset-live-view.
Sections 1–29 above are the design record of ticket #1782 and are preserved
unaltered, including the pre-fix measurements; nothing in them is rewritten to
read as though the defect never existed. This section records what was actually
built, where it matched the design, and the two places where it deliberately or
necessarily did not.
The user granted the exact approval §28 required — the const removal, the
snapshot-to-live-view semantic change, and the object-layout change — scoped to
ticket #1783 only. SR-AUD-361 moves from confirmed (design-complete) to
remediated. Ticket #1773 remains blocked and untouched; CNA and
mobile-eggbert were not inspected, searched, configured, built, or modified.
modules/collections/include/System/Collections/Generic/SortedSet.hpp was
rewritten to §11's declarations. The final public signature is
[[nodiscard]] SortedSet<T> GetViewBetween(const T& lower, const T& upper);and the final representation is exactly §11's: std::shared_ptr<State> (the
State owning std::set<T> data and intcs version), std::optional<T> lower_/upper_, and the mutable intcs cachedCount_/cachedCountVersion_
pair. Iterator holds std::shared_ptr<const State>, the current position, the
range end, and a version snapshot. The five special members, getIsViewProperty,
IsWithinRange, ToSortedSet, the range primitives, the bounds and exception
matrix of §15, the propagation matrix of §16, the enumeration rules of §17, and
the set-operation rules of §18 all landed as specified.
The nine phases of §20 were implemented as one header rewrite rather than nine
commits, because phases 1–3 alone leave GetViewBetween materializing a set
from state it no longer owns — an intermediate that builds but has no
independent value. The phase gates were still honoured in order: the 41
pre-existing SortedSetTests.* / GenSortedSetTest.* /
SortedSetVersionTrackingTests.* cases, including the three GetViewBetween
tests, passed unchanged on the first build of the new header, and the complete
SharpRuntimeTests_Collections_Core executable passed 1,736/1,736 before the
new suite was added.
- Bound equality uses comparer equivalence, not
operator==. The prototype's shared-state self-aliasing guard compared bounds withother.lower_ == lower_, which instantiatesstd::optional<T>::operator==and therefore requiresT::operator==— reintroducing exactly the class of defect §4.6.1 records. The shipped code routes bound equality through a privatesameBoundsAs, which uses!cmp(a,b) && !cmp(b,a)on the container's own predicate, so the element contract stays "whateverstd::set<T>orders with" for every member, not just for the ones the prototype exercised. Iterator::operator++stops at the range end. §11 givesIteratoranend_member that the prototype stored but never read. Rather than carry a write-only field,operator++now clamps atend_, turning an increment past the end from undefined behavior into a no-op. This changes no defined behavior:operator==/operator!=still compare positions only, so range-fortermination is untouched.IsSubsetOf,IsSupersetOf,IsProperSubsetOf,IsProperSupersetOf, andOverlapswere implemented from §18.2 rather than from the prototype, which omitted them. They now walk[rangeBegin, rangeEnd)on both sides and route membership through the range-awareContains.
§15's exception-ordering row claims that checking the invalid range before
the nested-widening bounds "matches SortedSet.cs:1510 / TreeSubSet.cs:344".
Re-reading both sources during implementation shows that is not what .NET
does on a view: TreeSubSet.GetViewBetween checks widening first and only then
delegates to _underlying.GetViewBetween, which performs the invalid-range
check. The two orders are observable only when a nested call is both inverted
and widening — e.g. view[3,7].GetViewBetween(2, 1), where .NET throws
ArgumentOutOfRangeException("lowerValue") and this port throws
ArgumentException("Must be less than or equal to upperValue.", "lowerValue").
The shipped code follows the design's order, since #1783's brief is to
implement §11/§15 rather than to redesign them, and validating an argument pair
for mutual consistency before validating it against object state is the more
defensible rule. Recorded here as an intentional, bounded deviation rather than
silently corrected in §15.
Design §19 says a set and its views are one collection for concurrency purposes
and that "nothing stronger is claimed". Implementation adds one fact §19 did not
anticipate and that did not exist before this revision: a view's
getCountProperty() is const but fills the per-object lazy Count cache, so two
threads calling it on the same view object race on that cache even though
every call is const. The pre-#1783 header's const members wrote nothing, so
this is a genuine change.
Measured with ThreadSanitizer (build-probe-sortedset/probe9_tsan_readonly.cpp,
three modes, all with TSan confirmed active by a deliberate-race self-test):
| Mode | Result |
|---|---|
known-race (self-test) |
2 data races — TSan is active, not silently inert |
distinct-handles — 8 threads reading through their own handles onto one shared state, creating and destroying 400 view handles |
0 data races; §19's control-block claim holds |
shared-view-count — 8 threads calling getCountProperty() on one view object |
1 data race: Read of size 4 … Previous write of size 4 … in getCountProperty() const |
This is not a defect against the type's contract — SortedSet<T> claims no
thread safety, and .NET's TreeSubSet caches count/_countVersion from its
own Count getter in exactly the same way. It is documented in the header's
thread-safety paragraph, and no thread-safety guarantee is added. Making the
cache atomic was considered and rejected: it would claim a guarantee the type
does not offer, for one member only, while element reads stayed unsynchronized.
build-probe-sortedset/probe8_postfix_layout_symbols.cpp re-measures the
shipped type (probe 5 no longer compiles: its production call site takes a view
from a const set, which is the approved break). Every §25.3 prediction is
confirmed exactly:
| Measurement | §25.3 predicted | Shipped |
|---|---|---|
sizeof(SortedSet<int>) |
40 | 40 |
sizeof(SortedSet<std::string>) |
104 | 104 |
sizeof(SortedSet<int>::Iterator) |
40 | 40 |
alignof |
8 | 8 |
is_polymorphic |
0 | 0 |
is_trivially_copyable |
0 | 0 |
is_nothrow_move_constructible |
1 | 1 |
is_copy_assignable |
1 | 1 |
The mangled name changed as §25.2 predicted:
W _ZNK6System11Collections7Generic9SortedSetIiE14GetViewBetweenERKiS5_ (before)
W _ZN6System11Collections7Generic9SortedSetIiE14GetViewBetweenERKiS5_ (after)
One accepted cost of the noexcept move operations. §11 specifies
noexcept move construction and move assignment, and §13 specifies that the
moved-from object is a valid, empty owning set. Meeting both means allocating
a fresh State for the source inside a noexcept function, so an allocation
failure there terminates rather than propagating. This is deliberate: the
alternative — a null state_ in a moved-from object — would put a liveness check
on every member of the class, and the allocation is one small control block.
is_nothrow_move_constructible stays 1 as §25.3 requires.
| Check | Result |
|---|---|
cmake --build build --parallel 4 |
0 errors, 0 warnings |
SharpRuntimeTests_Collections_Core |
1,783 passed (1,736 before, +47 new) |
| 41 pre-existing SortedSet cases | pass unchanged, no assertion edited |
scripts/local_ci_check.sh build |
13,069 tests across 37 executables (floor 13,022) |
scripts/validate_module_boundaries.py --root . |
41 modules / 90 edges — no new edge |
test/validate_module_boundaries_test.py |
7 tests OK |
scripts/generate_component_catalog.py --check |
catalogue current |
scripts/db_consistency_check.py --db plan.sqlite3 |
no problems |
git diff --check |
clean |
scripts/check_doxygen_warnings.sh |
1,937 warnings (ceiling 1,942): -6 from documenting the Iterator members, +1 from README.md's new link into docs/, which Doxyfile's INPUT does not scan |
scripts/check_selective_components.sh (repo-local TMPDIR) |
all 10 components pass |
Positive consumer fixture, compile-only -Werror and linked |
compiles, exits 0 |
Negative consumer fixture (const caller) |
rejected, as designed |
probe3_comparer_requirement -DSORTEDSET_PROBE_INSTANTIATE_VIEW |
now compiles -Werror and runs (§4.6.1 closed) |
probe6_public_header_standalone |
still compiles standalone -Werror, exits 0 |
probe7_postfix_behavior under ASan+UBSan+LSan |
exit 0, failures=0, 82 assertions, no diagnostic, no leak |
probe2_iterator_lifetime copy-assign |
value 1 (the pre-assignment element), was a silently wrong 60 |
probe2_iterator_lifetime move-assign |
exit 0, no report — was ASan heap-use-after-free |
probe2_iterator_lifetime outlive |
exit 0, no report — was ASan stack-use-after-scope |
| Full permanent suite under ASan+UBSan+LSan | 47/47 pass, no diagnostic, no leak |
| LeakSanitizer activity | confirmed by deliberate-leak self-test (232 bytes in 5 allocations reported) |
probe1_current_behavior is deliberately not re-run to green: it asserts the
pre-fix contract and now aborts on its first view.Add(99), because that call
correctly throws. It is preserved as #1782's evidence;
probe7_postfix_behavior.cpp is its post-fix counterpart.
Unchanged from §21's rollback strategy, and now concrete: git revert of the
implementation commit restores the previous header exactly. Reverting only the
live-view behavior while keeping the safer representation (fallback E′) is still
possible but is no longer a single revert, since the change landed as one
rewrite.
Added by ticket #1784 (REMED-COLL-SORTEDSET-VIEW-COUNT-RACE, P1, size S) on
local branch feature/remediation-coll-sortedset-count-race. Sections 1–30 are
the design record of ticket #1782 and the implementation record of ticket
#1783 and are preserved unaltered — including §30.5, which is where this defect
was first measured and reported. Nothing below rewrites history to read as
though the lazy cache was never implemented: the cache is still there, still
lazy, still per-view, and still mirrors .NET's TreeSubSet.count/_countVersion.
Only the way its two fields are written changed.
This ticket does not reopen SR-AUD-361, which stays remediated. It
corrects a defect introduced by that finding's own remediation.
§30.5 recorded, as a newly found consequence rather than a defect, that a
view's getCountProperty() is const but fills the per-object lazy Count
cache, so two threads calling it on the same view object race. #1783 classified
that as acceptable because "SortedSet<T> claims no thread safety".
That classification was wrong, and this ticket reverses it. The reasoning has three steps:
- A C++ data race is undefined behaviour, not merely an unhelpful result. Two conflicting non-atomic accesses to the same scalar with no happens-before edge make the whole program ill-formed, no diagnostic required — the compiler may assume it cannot happen and optimize accordingly. "The type promises nothing" does not downgrade UB into a documented limitation.
- The operation is observationally read-only.
getCountProperty()isconst, takes no lock, returns a number, and changes nothing a caller can see. Nothing in its signature or documentation warns that calling it is a write. Every otherconstmember of this class is genuinely read-only, so the hazard is invisible at the call site. - It is a regression, not an inherited limitation. The pre-#1783 header's
constmembers wrote nothing at all, so concurrent read-only use of oneSortedSet<T>object was race-free before this remediation and stopped being race-free because of it. §30.5 says exactly this and then declines to act on it.
The .NET comparison #1783 relied on does not carry over either. .NET's
TreeSubSet really does cache count/_countVersion from its Count getter
(SortedSet.TreeSubSet.cs, VersionCheckImpl), but in the CLR a torn or
racing int write is not undefined behaviour — int writes are atomic by
specification, so the worst case there is a stale-but-valid number. The C++
port inherits the design and not that guarantee. Moreover, .NET documents the
opposite of what #1783 assumed for its collections: multiple concurrent
readers are supported as long as nobody writes. #1783's cache broke that
half of the contract while claiming to match .NET.
Repository-local, gitignored probe
build-probe-sortedset/probe10_tsan_count_race.cpp, built by
build-probe-sortedset/build_tsan.sh with
-std=c++23 -Wall -Wextra -Wpedantic -g -O1 -fsanitize=thread. Every mode is
read-only once its worker threads start; concurrent mutation is never
exercised, because it is unsupported before and after this ticket and a report
produced by it would say nothing about this defect.
| Mode | What it does | Pre-fix | Post-fix |
|---|---|---|---|
known-race |
TSan self-test: unsynchronized int increment |
2 races | 2 races |
same-view-count |
8 threads, getCountProperty() on one view object, no mutation |
1 race | 0 |
readonly-enumeration |
Count + Contains + iteration + Min/Max + ToVector on one view object |
1 race | 0 |
nested-views |
Count on a nested view object and on its parent view | 2 races | 0 |
overlapping-views |
Count on two overlapping view objects over one state | 2 races | 0 |
copied-handles-count |
each thread owns a distinct copied view handle | 0 | 0 |
independent-sets |
each thread owns an independent full-set copy | 0 | 0 |
fullset-count |
8 threads, Count on one owning full set object | 0 | 0 |
sequential-count |
the identical call sequence, single-threaded | 0 | 0 |
view-churn |
repeated view creation and destruction, no mutation | 0 | 0 |
The self-test reporting 2 races in both columns is what makes the zeroes
evidence: ThreadSanitizer is instrumenting the post-fix binary, not silently
inert. Every mode also asserts its exact expected Count on every observation
(inconsistent-observations=0 throughout, before and after).
fullset-count being clean pre-fix pins the defect precisely: the owning-set
path returns state_->data.size() and never touches the cache, so only the
view path was affected. #1783's own probe,
build-probe-sortedset/probe9_tsan_readonly.cpp, was re-run unmodified against
the corrected header for direct before/after continuity: its shared-view-count
mode goes from 1 race to 0, with known-race still reporting 2.
The exact pre-fix diagnostic
(build-probe-sortedset/probe10_prefix_same-view-count.log):
WARNING: ThreadSanitizer: data race (pid=515048)
Read of size 4 at 0x7ffdaeef08f4 by thread T2:
#0 System::Collections::Generic::SortedSet<int>::getCountProperty() const
modules/collections/include/System/Collections/Generic/SortedSet.hpp:315
Previous write of size 4 at 0x7ffdaeef08f4 by thread T1:
#0 System::Collections::Generic::SortedSet<int>::getCountProperty() const
modules/collections/include/System/Collections/Generic/SortedSet.hpp:317
Line 315 is if (cachedCountVersion_ != state_->version); line 317 is
cachedCountVersion_ = state_->version. The racing object is the view's own
cache pair, exactly as §30.5 described.
All five were measured, not argued. build-probe-sortedset/probe11_cache_alternatives.cpp
reproduces SortedSet<T>'s exact member sequence and varies only the cache
representation, so sizeof/alignof of each shape is what a consumer's object
layout would actually see:
| Alternative | SortedSet<int> |
SortedSet<std::string> |
Layout vs #1783 |
|---|---|---|---|
current (#1783), two plain intcs |
40 | 104 | — (the racing baseline) |
| A remove the cache | 32 | 96 | ❌ broken |
| B two same-width atomics (SELECTED) | 40 | 104 | ✅ preserved |
| B′ one packed 64-bit atomic | 40 | 104 | ✅ preserved |
C std::mutex per view |
80 | 144 | ❌ broken (+100%) |
C′ std::shared_mutex per view |
96 | 160 | ❌ broken (+140%) |
E immutable snapshot via shared_ptr |
48 | 112 | ❌ broken |
Alternative A — remove the cache. Eliminates the race by construction and
is the simplest possible code. Rejected on two independent grounds. It breaks
the object layout the user approved under #1783 (40 → 32, 104 → 96), and
keeping the now-dead fields to hold the layout would leave two unread private
members — a -Wunused-private-field warning under Clang, against a repository
rule of zero warnings, and precisely the "misleading active-cache" residue this
ticket was told not to leave behind. It also makes every view Count an O(k)
walk of the k in-range elements with no amortization, so a Count in a loop
over a large view becomes quadratic, and getIsEmptyProperty() — which routes
through Count — degrades from amortized O(1) to O(k). .NET keeps the cache
for exactly this reason.
Alternative B — atomic cache fields (selected). std::atomic<intcs> is
4 bytes with 4-byte alignment on every supported toolchain (measured:
sizeof=4 alignof=4 is_always_lock_free=1), so it occupies the same storage
as the plain field it replaces and the object layout is preserved byte for
byte. The concern the brief raised — can a pair of independent atomics
publish a consistent version/count pair? — is real, and the answer is yes
only with an ordered publication protocol, which §31.4 specifies. Two
relaxed atomics would not be enough: a reader could observe the new version
before the matching count store became visible and return the previous count.
ABA and version wrap are unchanged by this alternative and are analysed
separately in §31.6. Copy, move, and assignment are unaffected because the
class already declares all five special members explicitly and never
copy-constructs the cache; the atomics are reset, not copied. This does not
make the collection thread-safe and is not intended to: it removes an internal
write from a read path, restoring the pre-#1783 property that concurrent
readers do not race.
Alternative B′ — one packed 64-bit atomic. Equally layout-preserving
(measured 40/104, because a single 8-byte-aligned member and two 4-byte
members occupy the same tail slot in an 8-aligned object) and it makes pair
consistency structural rather than protocol-dependent — a single atomic can
never be read torn, so §31.4's ordering argument would be unnecessary. Rejected
narrowly, on reviewability: it replaces two fields whose names map one-to-one
onto .NET's count and _countVersion with one bit-packed integer plus
shift/mask accessors, losing the direct correspondence to the reference
implementation for a correctness margin the release/acquire protocol already
provides. Recorded here as a viable fallback if the protocol ever proves
fragile in review.
Alternative C — synchronized per-view cache. A std::mutex doubles the
object (40 → 80) and a std::shared_mutex more than doubles it (40 → 96),
both breaking the approved layout. Worse, both make the class
non-copy-assignable and non-move-constructible unless the special members are
rewritten to skip the lock, and locking inside a const getter would advertise
a synchronization guarantee that the surrounding class — whose element reads,
Add, and Remove remain entirely unlocked — does not honour. Callers would
reasonably infer more safety than exists. Rejected.
Alternative D — cache in the shared State. Structurally wrong for this
type. The cache is keyed by range, and arbitrary views over one state have
arbitrary, overlapping, nested bounds, so a single shared count cache would
thrash between views and answer the wrong range unless it were keyed. Keying it
means an unbounded map from bounds-pair to count living in State, growing
without limit as views are created, needing its own synchronization for the
same reason the per-view field did, and requiring T to be hashable or
further ordered. That is a large amount of new machinery, new allocation on the
Count path, and a new element-type requirement, to solve a problem two
atomics solve for free. Rejected; no keyed cache is justified by any measured
evidence here.
Alternative E — immutable published snapshot. Publishing a
shared_ptr<const CountSnapshot> gives structural pair consistency like B′,
but grows the object (40 → 48, measured) and allocates a fresh control block
plus snapshot on every recomputation — that is, once per version per view,
which for the alternating mutate/read pattern is once per mutation. It
therefore violates the brief's "no new allocation on the Count path" constraint
without buying anything B′ does not already give for free. Rejected.
The two cache fields become same-width atomics, and the class documents a two-step protocol that the writer and the reader must obey together:
static constexpr intcs kCountNotCached = -1;
mutable std::atomic<intcs> cachedCount_{0};
mutable std::atomic<intcs> cachedCountVersion_{kCountNotCached};
[[nodiscard]] intcs getCountProperty() const {
if (!getIsViewProperty()) return static_cast<intcs>(state_->data.size());
const intcs currentVersion = state_->version;
if (cachedCountVersion_.load(std::memory_order_acquire) == currentVersion)
return cachedCount_.load(std::memory_order_relaxed);
const auto computed = static_cast<intcs>(std::distance(rangeBegin(), rangeEnd()));
cachedCount_.store(computed, std::memory_order_relaxed);
cachedCountVersion_.store(currentVersion, std::memory_order_release);
return computed;
}The version is the publishing field: it is written last, with release,
and read first, with acquire. A reader that observes the new version
therefore also observes the count store that happened before the release, so
the (count, version) pair can never be read torn. The count's own accesses are
relaxed because the version's ordering already sequences them.
Why duplicate publication is harmless: within the supported model no thread is
mutating during a concurrent read window, so state_->version is fixed and
every racing thread computes the same count for it. Two threads may both
recompute and both store, but they store identical values. The protocol exists
to order the pair, not to arbitrate between conflicting results, so no
compare-exchange, no retry loop, and no lock is needed.
state_->version itself deliberately stays a plain intcs. Making it atomic
would be pure cost — nothing writes it during a legal concurrent read window —
and would falsely suggest that concurrent mutation had become defined.
The three assignment paths that must not inherit a value computed for state the
handle no longer refers to (move construction, copy assignment, move
assignment) route through one new private helper, invalidateCountCache(),
which follows the same ordering rule: count first, version last with release.
Two static_asserts in the header pin the layout claim rather than trusting
it, so a platform that ever pads its atomics fails to compile instead of
silently re-breaking the ABI:
static_assert(sizeof(std::atomic<intcs>) == sizeof(intcs));
static_assert(alignof(std::atomic<intcs>) == alignof(intcs));Lock-freedom is not static_asserted. It is measured
(std::atomic<intcs>::is_always_lock_free == 1 on this toolchain) and
documented, but a hypothetical platform with a locked 32-bit atomic would still
be correct — merely slower — and would allocate nothing, so failing the
build there would cost portability for no correctness gain.
This is now written in SortedSet.hpp's class doc-comment and is the
authoritative statement. It has two deliberately unequal halves:
- Concurrent mutation is unsupported and undefined. If any thread mutates,
no other thread may touch the collection at all. An owning set and every view
derived from it are one collection for this purpose, so mutating through
a view while reading through the parent is exactly as undefined as mutating a
single set while reading it.
state_->dataandstate_->versionare non-atomic and deliberately stay that way. No new promise of concurrent mutation safety is introduced by this ticket, and none is planned. - Concurrent read-only access is race-free. No
constmember of the class writes to an unsynchronized field, so any number of threads may callgetCountProperty(),Contains(),getMinProperty(),getMaxProperty(),ToVector(), the read-only set predicates, or iterate — on the same object or on distinct handles over the same shared state — for as long as nobody mutates. This is the guarantee .NET documents for its own collections, it is the property the pre-#1783 header had, and restoring it is the entire content of this ticket.
Reference-count updates on the shared state remain atomic, so copying and
destroying handles on different threads cannot corrupt the control block —
§19's claim, unchanged and re-verified by the view-churn mode.
The type is therefore still not thread-safe. It is merely free of internal races when read, which is a strictly weaker and much more ordinary guarantee.
Requested explicitly, and bounded deliberately.
intcs is int32_t (SharpRuntimeHelper.hpp:50). State::version starts at
0 and is only ever incremented, by ++state_->version, from Add, Remove,
and Clear when they actually change the set. Three consequences:
- Signed overflow. After
INT32_MAXeffective mutations,++versionis signed-integer overflow — undefined behaviour in C++, where the sameversion++in .NET wraps in a defined way. This is pre-existing: the counter, its type, and its increment all predate #1783 (they arrived with ticket 1713's fail-fast enumerator work) and this ticket changes none of them. - Equality is the only comparison, so uniqueness matters. Both the Count
cache (
cachedCountVersion_ == currentVersion) and theIteratorguard (version_ != state_->version) test equality alone. If the counter ever wrapped to a previously observed value, a stale cache or a stale iterator would be silently accepted as current. Reaching that needs 2^32 mutations on one state between the two observations. - The sentinel.
kCountNotCachedis-1, which the counter cannot hold without first overflowing; the same assumption the #1783 code made.
The chosen fix does not change this risk in either direction. It reads and
writes the identical values with the identical equality test; only the memory
ordering of the accesses changed. Widening or redesigning the counter would be
a general version-counter change touching Iterator, every mutator, and the
enumerator contract — explicitly out of this ticket's scope. It is recorded as
inactive ticket #1786 (REMED-COLL-VERSION-COUNTER-OVERFLOW, P3), with no
new SR-AUD-* identifier, and was not begun.
Re-measured with build-probe-sortedset/probe8_postfix_layout_symbols.cpp, the
same probe #1783 used, and diffed against #1783's stored output:
| Measurement | #1783 | #1784 |
|---|---|---|
sizeof(SortedSet<int>) |
40 | 40 |
sizeof(SortedSet<std::string>) |
104 | 104 |
sizeof(SortedSet<int>::Iterator) |
40 | 40 |
alignof (both) |
8 | 8 |
is_polymorphic |
0 | 0 |
is_trivially_copyable |
0 | 0 |
is_nothrow_move_constructible |
1 | 1 |
is_copy_assignable |
1 | 1 |
The two log files are byte-identical. The mangled GetViewBetween symbol is
unchanged as well —
_ZN6System11Collections7Generic9SortedSetIiE14GetViewBetweenERKiS5_, matching
#1783's recorded symbol exactly.
- Public source compatibility: unaffected. No signature, return type,
parameter, or
constqualification changed. The permanent suite asserts the exact pointer-to-member type of fourteen public members, so a change is a compile error rather than a silent break. - Object layout / ABI: unaffected. Identical
sizeof,alignof, traits, and symbols. Unlike #1783, this revision needs no consumer rebuild on its own account, and it required no new user approval. - Semantic: unaffected. Every Count value, propagation path, exception, and
iterator-invalidation result is unchanged; the 47
SortedSetLiveViewTestscases and all 41 pre-existing SortedSet cases pass with no assertion edited. - Performance: unchanged in complexity, negligible in constant. Count stays
O(1) for an owning set and O(k) once per version for a view, amortized O(1)
across repeated reads. The added cost is one acquire load on the hit path and
one release store on the miss path; on x86-64 both compile to plain
movinstructions, since the architecture provides acquire/release ordering for aligned loads and stores without a fence. No allocation is added anywhere. - One new include,
<atomic>— a standard header, no new module or component dependency. Boundaries stay at 41 modules / 90 edges.
| Check | Result |
|---|---|
cmake --build build --parallel 4 |
0 errors, 0 warnings |
SharpRuntimeTests_Collections_Core |
1,812 passed (1,783 before, +29 new) |
47 SortedSetLiveViewTests + 41 pre-existing SortedSet cases |
pass unchanged, no assertion edited |
| ThreadSanitizer, 10 modes | 0 reports in all nine real modes; self-test still reports 2 |
#1783's own probe9 shared-view-count, unmodified |
1 race → 0 |
| ASan + UBSan + LeakSanitizer over both permanent suites | 76/76 pass, no diagnostic, no leak |
| LeakSanitizer activity | confirmed by deliberate-leak self-test (4,112 bytes in 102 allocations reported, exit 1) |
| ABI/layout probe vs #1783 baseline | byte-identical, symbols unchanged |
Consumer fixture collections_sorted_set_view.cpp, -Wall -Wextra -Wpedantic -Werror |
compiles, exits 0 |
Negative fixture (const caller) |
still correctly rejected |
check_selective_components.sh Collections.Core collections_sorted_set_view.cpp |
isolated check passed, 1,812 tests |
check_selective_components.sh (full 10-component matrix) |
all 10 pass |
- SR-AUD-361 stays
remediated. This corrects a post-remediation defect; it does not reopen the live-view finding. - The §30.4 nested-view exception-ordering divergence from .NET is unchanged. It is a deliberate semantic decision from #1782's design and needs its own decision before any change. Recorded as inactive ticket #1785; not begun.
GetViewBetweensemantics, sharedStateownership, inclusive bounds, nested-view narrowing, shared mutation versioning, and #1783's copy/move/assignment behaviour are all preserved exactly.- No general thread safety was added, no mutation path was synchronized, and no other collection was touched.
Reverting the single implementation commit restores #1783's plain-intcs cache
and, with it, the data race. The permanent suite would still pass — a data race
is invisible to an uninstrumented run — so a revert must be validated by
re-running build-probe-sortedset/probe10_tsan_count_race.cpp under
ThreadSanitizer, not by CTest alone. Alternative B′ (§31.3) is the drop-in
replacement if the two-atomic protocol is ever judged too subtle.
Added by ticket #1786 (REMED-COLL-VERSION-COUNTER-OVERFLOW, P3, size S) on
local branch feature/remediation-coll-sortedset-version-overflow. Sections
1–31 are preserved unaltered. This is a pointer, not a restatement: the
whole analysis lives in its own document, because it is about the counter's
arithmetic rather than about the live-view contract this file records.
§31.6 above analysed State::version's type and overflow at #1784's request,
concluded correctly that #1784 "does not change this risk in either direction",
and deferred the counter itself to inactive ticket #1786. That ticket has now
been completed, and its record is
docs/SortedSetVersioningDesign.md.
What changed, in one paragraph: State::version and Iterator::version_ became
SharpRuntime::ulongcs (64-bit unsigned), so the increment is defined for every
representable prior value and a repeat needs 2^64 mutations rather than 2^32.
The Count cache's tag stayed 32 bits — widening it is the one change that would
break the object layout §31.3 fixed — and is instead stored biased by one and
compared widened, which identifies a counter value exactly, cannot be produced
by a never-filled cache, and stops the cache being written once the counter
outgrows it.
Two corrections to §31.6, recorded here rather than edited into it:
- §31.6 lists three consequences. There is a fourth, found while
tracing the code for #1786 and the most serious of them:
kCountNotCachedwas-1, which the counter itself reaches after 2^32 − 1 effective mutations, so a view that had never computed its Count read its cache as warm and answered0. Unlike the ABA cases it needs no prior observation. - §31.6 calls the third consequence "the same assumption the #1783 code made",
which is accurate, but the sentence it refers to in
SortedSet.hpp— "the shared version counter starts at 0 and only increments, so it never legitimately holds this value" — was false, not merely optimistic. It is now true by construction: no counter value can produce the sentinel.
What is unchanged and was deliberately not touched: GetViewBetween's
semantics, shared State ownership, inclusive bounds, nested-view narrowing,
the shared single-counter invalidation rule of §17, the copy/move/assignment
behaviour of §13 and §14, and #1784's release/acquire Count-cache publication
protocol, which §9.2 of the new document reproduces verbatim. sizeof,
alignof, every member offset, and the mangled GetViewBetween symbol are
byte-identical to #1784's measurements. SR-AUD-361 stays remediated and was
not reopened; #1785 stays inactive and no exception ordering changed.
Added by ticket #1785 (REMED-COLL-SORTEDSET-NESTED-EXCEPTION-ORDER, P3, size
XS, category design) on local branch
feature/remediation-coll-sortedset-nested-order. Sections 1–32 are preserved
unaltered except for two explicit supersession markers inside §15, which point
here. This section records a decision that reverses §15's, taken under
explicit user approval; it does not rewrite the earlier design as though it had
always matched .NET.
§30.4 measured, during #1783's implementation, that §15's claim — checking the
invalid range before the nested-widening bounds "matches SortedSet.cs:1510
/ TreeSubSet.cs:344" — is false. #1783 nevertheless shipped §15's order, on
§15's stated rule that an argument pair should be validated for mutual
consistency before being validated against object state, and recorded the
divergence honestly rather than silently correcting it.
That left a genuine choice, which is why #1785 was opened as a design ticket
and not as a bug: exact .NET parity, or the design's own rule. Both orders
throw, neither loses data, and neither corrupts state, so the divergence is
observable only in the exception type and parameter of a doubly-invalid
nested call. The user explicitly approved adopting .NET's order, which is
acceptance-criteria branch (b) of the ticket. This is a parity correction, not a
reopening of SR-AUD-361, which stays remediated; no new SR-AUD-* identifier
was created, the audit numbering staying frozen at 364.
Two methods are involved, and the order only becomes visible when both run.
SortedSet.TreeSubSet.cs:342-353 — the method a view dispatches to:
public override SortedSet<T> GetViewBetween(T? lowerValue, T? upperValue)
{
if (_lBoundActive && Comparer.Compare(_min, lowerValue) > 0)
{
throw new ArgumentOutOfRangeException(nameof(lowerValue));
}
if (_uBoundActive && Comparer.Compare(_max, upperValue) < 0)
{
throw new ArgumentOutOfRangeException(nameof(upperValue));
}
return (TreeSubSet)_underlying.GetViewBetween(lowerValue, upperValue);
}SortedSet.cs:1508-1515 — the method it then delegates to, and the only one an
owning set ever runs:
public virtual SortedSet<T> GetViewBetween(T? lowerValue, T? upperValue)
{
if (Comparer.Compare(lowerValue, upperValue) > 0)
{
throw new ArgumentException(SR.SortedSet_LowerValueGreaterThanUpperValue, nameof(lowerValue));
}
return new TreeSubSet(this, lowerValue, upperValue, true, true);
}Three facts follow, and all three are load-bearing:
- The widening checks come first, because they are in the caller. The inverted-range check is unreachable until both bounds are narrowing-or-equal.
- The lower bound is checked before the upper bound, so a request that
widens both selects
lowerValue. #1783 already matched this. _underlyingis the root set, not the parent view —TreeSubSet's constructor stores the set it was created from, and a view is always created from the root — so nesting flattens to depth 1 and the delegated call runs the base method, neverTreeSubSet's override recursively. #1783 already matched this too; it is the reason this port's nested view is a bounded handle on the sameStaterather than a chain.
SR.SortedSet_LowerValueGreaterThanUpperValue is "Must be less than or equal to upperValue." (System.Collections/src/Resources/Strings.resx:138-140),
which is the message #1783 already used verbatim.
[[nodiscard]] SortedSet<T> GetViewBetween(const T& lower, const T& upper) {
const auto cmp = comparer();
if (lower_.has_value() && cmp(lower, *lower_))
throw System::ArgumentOutOfRangeException("lowerValue");
if (upper_.has_value() && cmp(*upper_, upper))
throw System::ArgumentOutOfRangeException("upperValue");
if (cmp(upper, lower))
throw System::ArgumentException("Must be less than or equal to upperValue.", "lowerValue");
return SortedSet<T>(state_, std::optional<T>(lower), std::optional<T>(upper));
}The change is the movement of one if, nothing else. lower_/upper_
being std::optional is this port's spelling of _lBoundActive/_uBoundActive,
so an owning full set skips both widening checks and reaches exactly the base
method's single check — .NET's behaviour for an owning set, and unchanged from
#1783. Ordering is decided by state_->data.key_comp() only; no
operator>, operator<=, operator>=, or natural-order comparison was
introduced, so §15's element-type contract is intact.
build-probe-sortedset/probe18_nested_exception_order.cpp prints the whole
matrix — outcome, exception type, parameter name, exact message, and a check
that a failed call left the shared state and the shared version untouched. It
was run against the working tree before the edit
(probe18_prefix.log) and after it (probe18_postfix.log). It is a single
translation unit built through the existing build-probe-sortedset/build.sh, so
no job count is involved.
Diffing the two logs, exactly 7 of the 32 outcome rows change, and every one of them is a doubly-invalid nested call:
| Probe case | Pre-fix (#1783) | Post-fix (#1785) |
|---|---|---|
7 — view[3,7].GetViewBetween(2, 1) |
ArgumentException / lowerValue |
ArgumentOutOfRangeException / lowerValue |
8 — view[3,7].GetViewBetween(12, 9) |
ArgumentException / lowerValue |
ArgumentOutOfRangeException / upperValue |
16d — inner[4,6].GetViewBetween(3, 2) |
ArgumentException / lowerValue |
ArgumentOutOfRangeException / lowerValue |
14e — Descending view[7,3].GetViewBetween(9, 11) |
ArgumentException / lowerValue |
ArgumentOutOfRangeException / lowerValue |
14f — Descending view[7,3].GetViewBetween(0, 1) |
ArgumentException / lowerValue |
ArgumentOutOfRangeException / upperValue |
15e — LessOnly view[3,7].GetViewBetween(2, 1) |
ArgumentException / lowerValue |
ArgumentOutOfRangeException / lowerValue |
15f — LessOnly view[3,7].GetViewBetween(12, 9) |
ArgumentException / lowerValue |
ArgumentOutOfRangeException / upperValue |
Every other row — every success, every widening-only failure, every
inverted-only failure, every top-level call, and every
state-unchanged=yes version-stable=yes line — is byte-identical before and
after. The permanent suite additionally covers the eighth shape the probe does
not print, an upper-widening inversion at nesting depth two
(inner[4,6].GetViewBetween(9, 7) → ArgumentOutOfRangeException("upperValue")).
P = {1..10}, V = P.GetViewBetween(3, 7), N = V.GetViewBetween(4, 6).
"Widens lower" means cmp(lower, *lower_); "widens upper" means
cmp(*upper_, upper); "inverted" means cmp(upper, lower).
| # | Case | Example | Widens lower | Widens upper | Inverted | Result |
|---|---|---|---|---|---|---|
| 1 | narrower | V(4,6) |
no | no | no | live view, Count == 3 |
| 2 | identical bounds | V(3,7) |
no | no | no | live view, Count == 5 |
| 3 | lower widens | V(2,6) |
yes | no | no | ArgumentOutOfRangeException("lowerValue") |
| 4 | upper widens | V(4,9) |
no | yes | no | ArgumentOutOfRangeException("upperValue") |
| 5 | both widen | V(2,9) |
yes | yes | no | ArgumentOutOfRangeException("lowerValue") — lower is checked first |
| 6 | inverted, strictly inside | V(6,4) |
no | no | yes | ArgumentException("Must be less than or equal to upperValue.", "lowerValue") |
| 7 | inverted and lower widens | V(2,1) |
yes | no | yes | ArgumentOutOfRangeException("lowerValue") — changed by #1785 |
| 8 | inverted and upper widens | V(12,9) |
no | yes | yes | ArgumentOutOfRangeException("upperValue") — changed by #1785 |
| 9 | inverted, both bounds outside the range but non-widening | V(9,2) |
no | no | yes | ArgumentException(…, "lowerValue") |
| — | inverted and both widen | — | yes | yes | yes | arithmetically unreachable, see below |
| 10 | equal bounds | V(5,5) |
no | no | no | live one-element view |
| 11 | empty result | V(5,5) over a set without 5 |
no | no | no | live empty view that still enforces [5,5] |
| 12 | one-element result | V(4,4) |
no | no | no | live view, Count == 1 |
| 13 | natural ascending ordering | rows 1–12 with T = int |
as above | |||
| 14 | custom comparer (std::less sorts descending) |
view[7,3] |
identical precedence, decided in comparer order | |||
| 15 | operator<-only element type |
LessOnly |
identical precedence | |||
| 16 | nested view of a nested view | N(3,6), N(5,7), N(3,2), N(9,7), N(6,5) |
validated against N's bounds [4,6], not V's |
Row "inverted and both widen" is empty because it cannot occur. A view's
bounds always satisfy !cmp(*upper_, *lower_) — construction rejects anything
else — so widening both ends gives lower < *lower_ <= *upper_ < upper, which
is ordered, not inverted. SortedSetNestedViewOrderTests proves this
exhaustively over a grid rather than asserting it in prose.
Both messages are fully determined by .NET's own resources and by this
repository's ArgumentException composition (which appends the (Parameter 'x') suffix exactly once since ticket #1776), so the tests pin the complete
text rather than a prefix. Nothing here is intentionally unstable, so no
"prefix-only" exemption is claimed.
| Selected by | C++ type | getParamNameProperty() |
what() |
HResult |
|---|---|---|---|---|
| lower widening | System::ArgumentOutOfRangeException |
lowerValue |
Specified argument was out of the range of valid values. (Parameter 'lowerValue') |
0x80131502 (COR_E_ARGUMENTOUTOFRANGE) |
| upper widening | System::ArgumentOutOfRangeException |
upperValue |
Specified argument was out of the range of valid values. (Parameter 'upperValue') |
0x80131502 |
| inverted range | System::ArgumentException |
lowerValue |
Must be less than or equal to upperValue. (Parameter 'lowerValue') |
0x80070057 (COR_E_ARGUMENT) |
ArgumentOutOfRangeException derives from ArgumentException, so a caller that
already caught the base type keeps catching every case; only a caller that
discriminates between the two, or reads getParamNameProperty(), can observe
the change at all. Every test and the consumer fixture catch the derived type
first so the two are never conflated.
| Layer | Verdict |
|---|---|
| Public signatures | unchanged — one statement moved inside one existing inline body |
Return type, const qualification, [[nodiscard]] |
unchanged |
| Mangled symbols | unchanged — no declaration was touched |
sizeof / alignof / member offsets |
unchanged — no member added, removed, reordered, or retyped |
| Vtable / virtual ABI | unchanged — SortedSet<T> has no virtual members |
Iterator layout |
unchanged |
| Ownership model, live-view semantics, write-through | unchanged |
| Count caching | unchanged |
| Iterator/enumerator invalidation | unchanged — a rejected call bumps no version, as before |
| Thread-safety contract | unchanged |
| Allocation behaviour | unchanged — still O(1) in element copies; a rejected call allocates nothing |
| Semantics | changed, deliberately and narrowly — the exception type and parameter of a nested call that is simultaneously widening and inverted |
| Consumer rebuild | ordinary recompilation of the changed header only; no ABI break, so no relink-only hazard |
In-repository callers. Every GetViewBetween call site in this repository
was reviewed: modules/collections/tests/System/Collections/Generic/
(SortedSetLiveViewTests.cpp, SortedSetCountCacheTests.cpp,
SortedSetVersionOverflowTests.cpp, LinkedListSortedSetTests.cpp,
SortedStackTests.cpp, Ticket1713VersionTrackingTests.cpp), and
test/consumer/collections_sorted_set_view{,_negative}.cpp. None asserted a
doubly-invalid nested call, so none relied on the old precedence and none needed
changing; the three assertions that come closest —
SortedSetLiveViewTests.cpp:849, :892, and :894 — are widening-only or
inverted-only and are unaffected. No production (src/) code calls
GetViewBetween at all. Downstream repositories were not inspected, per this
ticket's scope; CNA and mobile-eggbert remain on an older revision and are
tracked by blocked ticket #1773.
modules/collections/tests/System/Collections/Generic/SortedSetNestedViewOrderTests.cpp
adds 23 tests: the full §33.5 matrix with exact type, parameter, message,
and HResult; an exhaustive (lower, upper) grid over [-2, 12]² compared
against .NET's decision procedure transcribed independently as an oracle; the
unreachability proof for "inverted and both widen"; the custom-comparer,
operator<-only, and std::string element types; nesting to depth three; and
the no-op guarantees (nothing mutated, no version bumped, every view still fully
usable after 1,500 consecutive failed constructions). SortedSetLiveViewTests.cpp's
47 live-view regressions are deliberately not duplicated; only the
nested-view behaviour that had to survive this reordering is re-asserted.
| Gate | Result |
|---|---|
cmake --build build --parallel 4 |
0 warnings, 0 errors |
SharpRuntimeTests_Collections_Core |
2,252 passed (was 2,229; +23) |
scripts/local_ci_check.sh build |
13,538 tests across 37 executables (was 13,515) |
scripts/validate_module_boundaries.py --root . |
41 modules, 90 edges |
test/validate_module_boundaries_test.py |
7 tests OK |
scripts/generate_component_catalog.py --check |
catalogue current |
scripts/db_consistency_check.py --db plan.sqlite3 |
no consistency problems |
scripts/check_selective_components.sh |
passed, including Collections.Core collections_sorted_set_view.cpp in isolation |
scripts/check_doxygen_warnings.sh |
1,939 warnings (ceiling 1,942) |
Consumer fixture, -Wall -Wextra -Wpedantic -Werror |
compiles clean, exits 0 |
| ASan + UBSan + LSan, four SortedSet suites | 128 tests, 0 diagnostics, 0 leaks |
git diff --check |
clean |
Sanitizers. build-asan-sortedset/build_1785.sh builds
SortedSetCountCacheTests, SortedSetLiveViewTests, the new
SortedSetNestedViewOrderTests, and SortedSetVersionOverflowTests against a
locally built ASan/UBSan GoogleTest — the same bounded configuration #1784 and
#1786 used, extended by one file, and still not a whole-repository sanitizer
tree. Every exception path, including 1,500 consecutive failed nested
constructions, ran with zero AddressSanitizer, UndefinedBehaviorSanitizer, and
LeakSanitizer findings. The configuration was proved live rather than inert by
re-running the deliberate-leak self-test
(build-asan-sortedset/lsan_selftest_1785.log: 4112 byte(s) leaked in 102 allocation(s)). ThreadSanitizer was deliberately not run: this ticket adds
no shared mutable state, no const write, and no new field — it moves one if
inside an existing body — so #1784's TSan campaign remains the governing
evidence, and §19's contract is unchanged.
GetViewBetween's signature and constness; top-level (owning-set) behaviour;
the shared State ownership model; live write-through; bounds inclusivity;
nested-view flattening; the Count cache and its publication protocol; the
mutation counter and iterator invalidation; the thread-safety contract; and
SR-AUD-361, which stays remediated. Tickets #1788, #1789, #1791, and #1794
remain blocked, and #1773 remains blocked and out of repository scope.
Move the cmp(upper, lower) check back above the two has_value() checks in
SortedSet.hpp and delete SortedSetNestedViewOrderTests.cpp. Nothing else
depends on the order: no signature, symbol, layout, or allocation changed, so a
revert is a one-hunk header edit plus one test file, and the floors return to
2,229 / 13,515.