Skip to content

Commit c683496

Browse files
electricessenceclaude
electricessence
and
claude
committed
Fix five selection defects; add regression test project. (v2.5.4)
Five defects, none previously covered by any test (the repo had no test project): 1. Multi-value exclusions were silently ignored on every enumerable/ params path. Root cause: DeferredHashSet fills itself lazily during Contains, but every call site consulted .Count BEFORE anything had been pumped, so a freshly created deferred set always looked empty and the code took the "no exclusions" fast path. Compounding it, the locals were typed ISet<T>, and DeferredHashSet.Contains hides (new) rather than overrides, so the pumping Contains could never bind through that type. Call sites now take Count fast paths only for already-materialized sets and consult the deferred set through its concrete type. Affected: RandomSelectIndex/RandomSelectOne/ TryRandomSelectOne with non-ISet exclusions, all *Except overloads with 2+ exclusions, and NextExcluding with enumerable exclusions. 2. The span-based single-exclusion path (RandomSelectIndexExcept) rented ArrayPool.Rent(others.Length) -- zero on that path -- and Rent(0) returns an empty array, so any non-empty source threw IndexOutOfRangeException. 3. LinkedList.TryRandomPluck walked `i <= r` from First, advancing r+1 times: the first element could never be selected, and drawing the last index threw NullReferenceException -- including on every draw from a single-element list. 4. Two selection paths drew from the global Random even when a caller supplied a seeded instance, breaking deterministic replay. 5. NextExcluding(int, uint, params uint[]) used LINQ Cast<int>() on the uint values; a boxed uint cannot unbox to int, so any 'others' threw InvalidCastException. Now Select(v => (int)v). Tests: new tests/Open.RandomizationExtensions.Tests (xunit, net10.0), 23 regression tests covering each defect plus the previously-working ISet paths. The library csproj now excludes tests/** from its default globs (the project lives at the repo root). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 134fdfe commit c683496

7 files changed

Lines changed: 463 additions & 43 deletions

‎Open.RandomizationExtensions.csproj‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@
1818
<RepositoryUrl>https://github.com/Open-NET-Libraries/Open.RandomizationExtensions</RepositoryUrl>
1919
<PackageLicenseExpression>MIT</PackageLicenseExpression>
2020
<RepositoryType>git</RepositoryType>
21-
<Version>2.5.3</Version>
21+
<Version>2.5.4</Version>
22+
<PackageReleaseNotes>Fixes: multi-value exclusions were silently ignored (deferred exclusion sets reported Count 0 before being pumped); span-based single-exclusion selection threw IndexOutOfRangeException (zero-length pooled rent); LinkedList TryRandomPluck could never select the first element and threw on the last; two selection paths ignored a supplied Random; NextExcluding uint overload threw InvalidCastException.</PackageReleaseNotes>
2223
<PackageReleaseNotes></PackageReleaseNotes>
2324
<PackageLicenseExpression>MIT</PackageLicenseExpression>
2425
<PublishRepositoryUrl>true</PublishRepositoryUrl>
@@ -36,9 +37,16 @@
3637
<PackageReference Include="System.Memory" Version="4.5.4" />
3738
</ItemGroup>
3839

40+
<ItemGroup>
41+
<!-- The library project lives at the repo root, so the default SDK globs would
42+
otherwise sweep the nested test project's sources into this compilation. -->
43+
<Compile Remove="tests\**" />
44+
<None Remove="tests\**" />
45+
</ItemGroup>
46+
3947
<ItemGroup>
4048
<None Remove=".gitattributes" />
41-
<None Remove=".gitignore" />
49+
<None Remove=".gitignore" />
4250
<None Include="logo.png">
4351
<Pack>True</Pack>
4452
<PackagePath></PackagePath>

‎Open.RandomizationExtensions.sln‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,20 +5,51 @@ VisualStudioVersion = 16.0.29009.5
55
MinimumVisualStudioVersion = 10.0.40219.1
66
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Open.RandomizationExtensions", "Open.RandomizationExtensions.csproj", "{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}"
77
EndProject
8+
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "tests", "tests", "{0AB3BF05-4346-4AA6-1389-037BE0695223}"
9+
EndProject
10+
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Open.RandomizationExtensions.Tests", "tests\Open.RandomizationExtensions.Tests\Open.RandomizationExtensions.Tests.csproj", "{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}"
11+
EndProject
812
Global
913
GlobalSection(SolutionConfigurationPlatforms) = preSolution
1014
Debug|Any CPU = Debug|Any CPU
15+
Debug|x64 = Debug|x64
16+
Debug|x86 = Debug|x86
1117
Release|Any CPU = Release|Any CPU
18+
Release|x64 = Release|x64
19+
Release|x86 = Release|x86
1220
EndGlobalSection
1321
GlobalSection(ProjectConfigurationPlatforms) = postSolution
1422
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
1523
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Debug|Any CPU.Build.0 = Debug|Any CPU
24+
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Debug|x64.ActiveCfg = Debug|Any CPU
25+
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Debug|x64.Build.0 = Debug|Any CPU
26+
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Debug|x86.ActiveCfg = Debug|Any CPU
27+
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Debug|x86.Build.0 = Debug|Any CPU
1628
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Release|Any CPU.ActiveCfg = Release|Any CPU
1729
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Release|Any CPU.Build.0 = Release|Any CPU
30+
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Release|x64.ActiveCfg = Release|Any CPU
31+
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Release|x64.Build.0 = Release|Any CPU
32+
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Release|x86.ActiveCfg = Release|Any CPU
33+
{E1B5CC84-FBF4-43AF-AF65-26F40C0429D2}.Release|x86.Build.0 = Release|Any CPU
34+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
35+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Debug|Any CPU.Build.0 = Debug|Any CPU
36+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Debug|x64.ActiveCfg = Debug|Any CPU
37+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Debug|x64.Build.0 = Debug|Any CPU
38+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Debug|x86.ActiveCfg = Debug|Any CPU
39+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Debug|x86.Build.0 = Debug|Any CPU
40+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Release|Any CPU.ActiveCfg = Release|Any CPU
41+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Release|Any CPU.Build.0 = Release|Any CPU
42+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Release|x64.ActiveCfg = Release|Any CPU
43+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Release|x64.Build.0 = Release|Any CPU
44+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Release|x86.ActiveCfg = Release|Any CPU
45+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E}.Release|x86.Build.0 = Release|Any CPU
1846
EndGlobalSection
1947
GlobalSection(SolutionProperties) = preSolution
2048
HideSolutionNode = FALSE
2149
EndGlobalSection
50+
GlobalSection(NestedProjects) = preSolution
51+
{F9CEEB2B-B64E-4BEB-AA2B-928E205E4E7E} = {0AB3BF05-4346-4AA6-1389-037BE0695223}
52+
EndGlobalSection
2253
GlobalSection(ExtensibilityGlobals) = postSolution
2354
SolutionGuid = {03122CA3-616D-446E-A2A7-CEF0FA66AD4C}
2455
EndGlobalSection

‎Randomizer.cs‎

Lines changed: 85 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,13 @@ public static bool TryRandomPluck<T>(this LinkedList<T> source, out T value, Ran
3636
}
3737

3838
var r = (random ?? R.Value).Next(source.Count);
39-
var node = source.First;
40-
for (var i = 0; i <= r; i++)
41-
node = node.Next;
39+
var node = source.First!;
40+
// Advance exactly r times: node starts AT index 0, so r advances lands on index r.
41+
// (Previously `i <= r` advanced r+1 times, which could never select the first
42+
// element and threw NullReferenceException whenever the last index was drawn --
43+
// including on every draw from a single-element list.)
44+
for (var i = 0; i < r; i++)
45+
node = node.Next!;
4246
value = node.Value;
4347
source.Remove(node);
4448
return true;
@@ -134,17 +138,25 @@ public static int RandomSelectIndex<T>(this in ReadOnlySpan<T> source, Random? r
134138
if (source.Length == 0)
135139
return -1;
136140

137-
DeferredHashSet<T>? setCreated = null;
141+
if (exclusion is null || exclusion is ICollection<T> c && c.Count == 0)
142+
return (random ?? R.Value).Next(source.Length);
143+
144+
// A DeferredHashSet fills itself lazily during Contains, so its Count is
145+
// meaningless until it has been pumped: the Count==0/Count==1 fast paths below
146+
// may only be taken for an already-materialized set. (Previously every lazily
147+
// created set reported Count==0 here and the entire exclusion was ignored.)
148+
var materialized = exclusion as ISet<T>;
149+
DeferredHashSet<T>? deferred = materialized is null ? new DeferredHashSet<T>(exclusion) : null;
138150
try
139151
{
140-
var exclusionSet = exclusion == null || exclusion is ICollection<T> c && c.Count == 0 ? default
141-
: exclusion as ISet<T> ?? (setCreated = new DeferredHashSet<T>(exclusion));
142-
143-
if (exclusionSet == null || exclusionSet.Count == 0)
144-
return (random ?? R.Value).Next(source.Length);
152+
if (materialized is not null)
153+
{
154+
if (materialized.Count == 0)
155+
return (random ?? R.Value).Next(source.Length);
145156

146-
if (exclusionSet.Count == 1)
147-
return RandomSelectIndexExcept(in source, random, exclusionSet.Single());
157+
if (materialized.Count == 1)
158+
return RandomSelectIndexExcept(in source, random, materialized.Single());
159+
}
148160

149161
var count = source.Length;
150162
var pool = ArrayPool<int>.Shared;
@@ -154,7 +166,13 @@ public static int RandomSelectIndex<T>(this in ReadOnlySpan<T> source, Random? r
154166
var indexCount = 0;
155167
for (var i = 0; i < count; ++i)
156168
{
157-
if (!exclusionSet.Contains(source[i]))
169+
// The deferred set must be called through its concrete type:
170+
// its Contains hides (new) rather than overrides the base method,
171+
// so an ISet<T>-typed call would bypass the lazy pump entirely.
172+
bool excluded = deferred is null
173+
? materialized!.Contains(source[i])
174+
: deferred.Contains(source[i]);
175+
if (!excluded)
158176
indexes[indexCount++] = i;
159177
}
160178

@@ -168,7 +186,7 @@ public static int RandomSelectIndex<T>(this in ReadOnlySpan<T> source, Random? r
168186
}
169187
finally
170188
{
171-
setCreated?.Dispose();
189+
deferred?.Dispose();
172190
}
173191
}
174192

@@ -202,7 +220,10 @@ public static int RandomSelectIndexExcept<T>(this in ReadOnlySpan<T> source, Ran
202220
return RandomSelectIndex(in source, random, others.Prepend(excluding));
203221

204222
var pool = ArrayPool<int>.Shared;
205-
var indexes = pool.Rent(others.Length);
223+
// Rent enough for every candidate index. (Previously this rented
224+
// others.Length -- zero on this path -- and ArrayPool.Rent(0) returns an
225+
// empty array, so any non-empty source threw IndexOutOfRangeException.)
226+
var indexes = pool.Rent(source.Length);
206227
try
207228
{
208229
var i = -1;
@@ -215,7 +236,7 @@ public static int RandomSelectIndexExcept<T>(this in ReadOnlySpan<T> source, Ran
215236
indexes[indexCount++] = i;
216237
}
217238

218-
return indexCount == 0 ? -1 : indexes[R.Value.Next(indexCount)];
239+
return indexCount == 0 ? -1 : indexes[(random ?? R.Value).Next(indexCount)];
219240
}
220241
finally
221242
{
@@ -288,17 +309,23 @@ static int RandomSelectIndex<T>(Random? random, int count, IEnumerable<T> source
288309
if (count == 0)
289310
return -1;
290311

291-
DeferredHashSet<T>? setCreated = null;
312+
if (exclusion is null || exclusion is ICollection<T> c && c.Count == 0)
313+
return (random ?? R.Value).Next(count);
314+
315+
// See the span overload: Count fast paths are only valid for a materialized set;
316+
// a DeferredHashSet must be consulted through its concrete type.
317+
var materialized = exclusion as ISet<T>;
318+
DeferredHashSet<T>? deferred = materialized is null ? new DeferredHashSet<T>(exclusion) : null;
292319
try
293320
{
294-
var exclusionSet = exclusion == null ? default
295-
: exclusion as ISet<T> ?? (setCreated = new DeferredHashSet<T>(exclusion));
296-
297-
if (exclusionSet == null || exclusionSet.Count == 0)
298-
return (random ?? R.Value).Next(count);
321+
if (materialized is not null)
322+
{
323+
if (materialized.Count == 0)
324+
return (random ?? R.Value).Next(count);
299325

300-
if (exclusionSet.Count == 1)
301-
return RandomSelectIndexExcept(random, count, source, exclusionSet.Single());
326+
if (materialized.Count == 1)
327+
return RandomSelectIndexExcept(random, count, source, materialized.Single());
328+
}
302329

303330
var pool = ArrayPool<int>.Shared;
304331
var indexes = pool.Rent(count);
@@ -309,7 +336,10 @@ static int RandomSelectIndex<T>(Random? random, int count, IEnumerable<T> source
309336
foreach (var value in source)
310337
{
311338
++i;
312-
if (!exclusionSet.Contains(value))
339+
bool excluded = deferred is null
340+
? materialized!.Contains(value)
341+
: deferred.Contains(value);
342+
if (!excluded)
313343
indexes[indexCount++] = i;
314344
}
315345

@@ -322,7 +352,7 @@ static int RandomSelectIndex<T>(Random? random, int count, IEnumerable<T> source
322352
}
323353
finally
324354
{
325-
setCreated?.Dispose();
355+
deferred?.Dispose();
326356
}
327357
}
328358

@@ -348,7 +378,7 @@ static int RandomSelectIndexExcept<T>(Random? random, int count, IEnumerable<T>
348378
indexes[indexCount++] = i;
349379
}
350380

351-
return indexCount == 0 ? -1 : indexes[R.Value.Next(indexCount)];
381+
return indexCount == 0 ? -1 : indexes[(random ?? R.Value).Next(indexCount)];
352382
}
353383
finally
354384
{
@@ -771,13 +801,16 @@ public static ushort NextExcluding(this Random source,
771801
if (range == 0)
772802
throw new ArgumentOutOfRangeException(nameof(range), range, "Must be a number greater than zero.");
773803

774-
DeferredHashSet<ushort>? setCreated = null;
804+
if (exclusion is null)
805+
return (ushort)source.Next(range);
806+
807+
// See RandomSelectIndex: Count fast paths are only valid for a materialized set,
808+
// and a DeferredHashSet must be consulted through its concrete type.
809+
var materialized = exclusion as ISet<ushort>;
810+
DeferredHashSet<ushort>? deferred = materialized is null ? new DeferredHashSet<ushort>(exclusion) : null;
775811
try
776812
{
777-
var exclusionSet = exclusion == null ? null
778-
: exclusion as ISet<ushort> ?? (setCreated = new DeferredHashSet<ushort>(exclusion));
779-
780-
if (exclusionSet == null || exclusionSet.Count == 0)
813+
if (materialized is not null && materialized.Count == 0)
781814
return (ushort)source.Next(range);
782815

783816
var pool = ArrayPool<ushort>.Shared;
@@ -787,7 +820,10 @@ public static ushort NextExcluding(this Random source,
787820
var indexCount = 0;
788821
for (ushort i = 0; i < range; ++i)
789822
{
790-
if (!exclusionSet.Contains(i))
823+
bool excluded = deferred is null
824+
? materialized!.Contains(i)
825+
: deferred.Contains(i);
826+
if (!excluded)
791827
indexes[indexCount++] = i;
792828
}
793829

@@ -802,7 +838,7 @@ public static ushort NextExcluding(this Random source,
802838
}
803839
finally
804840
{
805-
setCreated?.Dispose();
841+
deferred?.Dispose();
806842
}
807843
}
808844

@@ -819,13 +855,16 @@ public static int NextExcluding(this Random source,
819855
if (range <= 0)
820856
throw new ArgumentOutOfRangeException(nameof(range), range, "Must be a number greater than zero.");
821857

822-
DeferredHashSet<int>? setCreated = null;
858+
if (exclusion is null)
859+
return source.Next(range);
860+
861+
// See RandomSelectIndex: Count fast paths are only valid for a materialized set,
862+
// and a DeferredHashSet must be consulted through its concrete type.
863+
var materialized = exclusion as ISet<int>;
864+
DeferredHashSet<int>? deferred = materialized is null ? new DeferredHashSet<int>(exclusion) : null;
823865
try
824866
{
825-
var exclusionSet = exclusion == null ? null
826-
: exclusion as ISet<int> ?? (setCreated = new DeferredHashSet<int>(exclusion));
827-
828-
if (exclusionSet == null || exclusionSet.Count == 0)
867+
if (materialized is not null && materialized.Count == 0)
829868
return source.Next(range);
830869

831870
var pool = ArrayPool<int>.Shared;
@@ -835,7 +874,10 @@ public static int NextExcluding(this Random source,
835874
var indexCount = 0;
836875
for (var i = 0; i < range; ++i)
837876
{
838-
if (!exclusionSet.Contains(i))
877+
bool excluded = deferred is null
878+
? materialized!.Contains(i)
879+
: deferred.Contains(i);
880+
if (!excluded)
839881
indexes[indexCount++] = i;
840882
}
841883

@@ -850,7 +892,7 @@ public static int NextExcluding(this Random source,
850892
}
851893
finally
852894
{
853-
setCreated?.Dispose();
895+
deferred?.Dispose();
854896
}
855897
}
856898

@@ -893,7 +935,9 @@ public static int NextExcluding(this Random source,
893935
var exInt = excluding > int.MaxValue ? -1 : (int)excluding;
894936
return others.Length == 0
895937
? NextExcluding(source, range, exInt)
938+
// Select, not Cast: LINQ's Cast<int>() unboxes, and a boxed uint cannot
939+
// unbox to int, so Cast threw InvalidCastException for any 'others'.
896940
: NextExcluding(source, range,
897-
others.Where(v => v <= int.MaxValue).Cast<int>().Prepend(exInt));
941+
others.Where(v => v <= int.MaxValue).Select(v => (int)v).Prepend(exInt));
898942
}
899943
}

0 commit comments

Comments
 (0)