Skip to content

Fix ImmutableHashSet<T>.Builder.ExceptWith when enumeration throws - #136790

Open
Hatr828 wants to merge 1 commit into
dotnet:mainfrom
Hatr828:fix-hashset-exceptwith
Open

Hatr828 wants to merge 1 commit into
dotnet:mainfrom
Hatr828:fix-hashset-exceptwith

Conversation

@Hatr828

@Hatr828 Hatr828 commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

ImmutableHashSet.Builder.ExceptWith can leave the builder in an inconsistent state if the input enumerable throws during enumeration.

For example:

var builder = ImmutableHashSet.CreateBuilder<int>();
builder.UnionWith(new[] { 1, 2, 3, 4 });

try
{
    builder.ExceptWith(ThrowingEnumerable());
}
catch (InvalidOperationException)
{
}

int count = 0;
foreach (int item in builder) count++;

// Before the fix: count is 1, but builder.Count is 4.

IEnumerable<int> ThrowingEnumerable()
{
    yield return 1;
    throw new InvalidOperationException();
}

If enumeration throws, updates are skipped leaving the builder with an incorrect count and potentially losing unrelated elements. Added a regression test that failed before the fix and passed afterward.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Nov 1, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

@Manishram-ai Manishram-ai left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reproduced this on .NET 8.0.8 with the example from the description. After the throw, Count is still 4 but the builder only contains 2. Removing items one at a time like the fix does keeps Count and the contents in sync.

UnionWith has the same problem though. If the enumerable throws after it yields a new item, the item ends up in the set but Count doesn't go up.

var builder = ImmutableHashSet.CreateBuilder<int>();
builder.UnionWith(new[] { 1, 2, 3, 4 });

try { builder.UnionWith(ThrowingEnumerable()); } catch (InvalidOperationException) { }

// builder.Contains(5) is true, but builder.Count is still 4

IEnumerable<int> ThrowingEnumerable()
{
    yield return 5;
    throw new InvalidOperationException();
}

ImmutableDictionary's builder AddRange does the same thing. Not sure if you'd rather fix those here or in a separate PR.

@Hatr828

Hatr828 commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor Author

@Manishram-ai I checked ImmutableHashSet.Builder.UnionWith and issue is more severe than just an incorrect count. For example:

var builder = ImmutableHashSet.CreateBuilder<int>();
builder.UnionWith(new[] { 1, 2, 3, 4 });

try { builder.UnionWith(ThrowingEnumerable()); } catch (InvalidOperationException) { }

foreach (int item in builder)  Console.WriteLine(item); // prints only "1  2  3" on .NET 10.0.10

IEnumerable<int> ThrowingEnumerable()
{
    yield return 5;
    yield return 6;
    throw new InvalidOperationException();
}

The original element 4 is no longer enumerated and elements are skipped only if UnionWith has 2+ elements (in this example). I suspect this is related to tree rebalancing and the builder retaining the old root when the final update is skipped.

I think I will fix UnionWith and AddRange in separate PRs.

@Manishram-ai

Copy link
Copy Markdown

Thanks for digging into it. I see the same on .NET 8.0.8: it enumerates 1 2 3, Contains(4) is false, and Count is still 4. ToImmutable() returns that same broken set (3 items, Count 4), so the damage outlives the builder. Separate PRs for UnionWith and AddRange sound good, happy to review them when they're up.

This branch has not been deployed

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

Labels

area-System.Collections community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants