ImmutableHashSet properties allows collection initializer usage without error but initializer has no effect

[ionite34]

Description

(System.Collections.Immutable) ImmutableHashSet currently allow collection initializer usage when it is a property, and causes no compilation errors, however the resulting property retains the original assignment.

Reproduction Steps

using System.Collections.Immutable;

var x = new Thing
{
    Set =
    {
        "Abc",
        "123"
    }
};

Console.WriteLine(x.Set.Count); // 0

public class Thing
{
    public ImmutableHashSet<string> Set { get; set; } = ImmutableHashSet<string>.Empty;
}

Expected behavior

One would either expect the collection initializer to make the assignments or, like in the case of ImmutableDictionary, cause a Error CS0200 : Property or indexer 'ImmutableDictionary<string, string>.this[string]' cannot be assigned to -- it is read only error, instead of allowing the initializer while not having an effect.

Actual behavior

Collection retains the default value assigned and the collection initializer has no effect.

Regression?

No response

Known Workarounds

No response

Configuration

.NET 9 and .NET 8, Windows x64

Other information

No response

[dotnet-policy-service]

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

[KalleOlaviNiemitalo]

The collection initializer calls the public ImmutableHashSet<T> Add(T item) method and discards the result.

I don't think it will be feasible to change C# to assign the result of the Add method back to the property; that would be a breaking change. But an analyzer could warn about this kind of incorrect usage. CA2009 is already specific to immutable collections.

dotnet/roslyn-analyzers#5685 looks related, but it talks about PureAttribute, and ImmutableHashSet<T>.Add(T) does not have PureAttribute. See also #34098 (comment).

[ionite34]

An analyzer warning would probably be good for this case, though I guess it would be waiting on whatever new Pure/MustUseReturnValue attribute ends up getting added to ImmutableHashSet.Add(T)? Not sure how feasible it would be to specifically add an analyzer warning for detecting collection initializer usages of immutable collection types.

[KalleOlaviNiemitalo]

https://github.com/dotnet/roslyn-analyzers/blob/6.0.0/src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/DoNotIgnoreMethodResults.cs already recognizes several methods of System.String, despite them not having PureAttribute. I think it would be easy to add something similar for ImmutableHashSet<T>.Add. The ImmutableHashSet<T> type is already listed in https://github.com/dotnet/roslyn-analyzers/blob/6.0.0/src/Utilities/Compiler/WellKnownTypeNames.cs#L146.

In the meantime though, the DoNotIgnoreMethodResults analyzer also has "UserDefinedMethodRule", which can be configured in .editorconfig. Perhaps this can be set up to warn about ignoring the result of ImmutableHashSet<T>.Add.

[KalleOlaviNiemitalo]

Alas, no, UserDefinedMethodRule in DoNotIgnoreMethodResults in .NET 9 does not support this case. I tried it like this:

Runtime110279.csproj

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <Nullable>enable</Nullable>
  </PropertyGroup>

</Project>

Program.cs

using System;
using System.Collections.Immutable;

var x = new Thing
{
    Set =
    {
        "Abc",
        "123"
    }
};
x.Set.Add("huh");

Console.WriteLine(x.Set.Count); // 0

public class Thing
{
    public ImmutableHashSet<string> Set { get; set; } = ImmutableHashSet<string>.Empty;
}

.editorconfig

root = true

[*.cs]
dotnet_diagnostic.CA1806.severity = warning
dotnet_code_quality.CA1806.additional_use_results_methods = Add

# dotnet_code_quality.CA1806.additional_use_results_methods = M:System.Collections.Immutable.ImmutableHashSet<T>.Add(T)

and got a CA1806 warning for the x.Set.Add("huh") call but no warnings for the collection initializer.

The analyzer searches for user-specified methods in OperationKind.Invocation but SharpLab shows Operation: ObjectOrCollectionInitializer; I guess that doesn't match then.

But it should be easy to make the analyzer look for OperationKind.ObjectOrCollectionInitializer too and check if the type is ImmutableHashSet<T>.

[ionite34]

If there is some way to have it attribute based on the eventual Add method it compiles to, would probably be easier to maintain and allow usages by libraries, though yeah it doesn't seem Roslyn analyzers are currently able to get that stage of information?

I do feel this is a necessary analyzer warning though, since methods on immutable collection types already have attribute based warnings. Scenarios where the initializer has already been defined and a refactor changes a collection type from HashSet / List to ImmutableHashSet / ImmutableList could easily create this kind of silent error. Additionally an analyzer fix could offer to change the collection initializer to a C# 12 collection expression instead if init/set accessibility is available.

Currently it seems an analyzer could target warnings for initializers with these types for coverage of System.Collections.Immutable:

using System.Collections.Immutable;
using System.Diagnostics;

var x = new Thing
{
    ISet = {"a", "b"},
    Set = {"a", "b"},
    SortedSet = {"a", "b"},
    IList = {"a", "b"},
    List = {"a", "b"}
};

Debug.Assert(x.ISet.Count == 1);
Debug.Assert(x.Set.Count == 1);
Debug.Assert(x.SortedSet.Count == 1);
Debug.Assert(x.IList.Count == 1);
Debug.Assert(x.List.Count == 1);

public class Thing
{
    public IImmutableSet<string> ISet { get; set; } = ["a"];
    public ImmutableHashSet<string> Set { get; set; } = ["a"];
    public ImmutableSortedSet<string> SortedSet { get; set; } = ["a"];
    public IImmutableList<string> IList { get; set; } = ["a"];
    public ImmutableList<string> List { get; set; } = ["a"];
}

ImmutableArray<T> does not support collection initializers as a property due to being a value type, but is unable to have a private constructor like the other immutable collections so causes a runtime NullReferenceException when doing new ImmutableArray<string>() {"a", "b"}, would benefit from this analyzer warning as well to prevent that.

[KalleOlaviNiemitalo]

methods on immutable collection types already have attribute based warning

What attribute do you mean?

[ionite34]

methods on immutable collection types already have attribute based warning

What attribute do you mean?

I may have confused the Jetbrains IDE warning as a compiler provided one, apparently it works using a JetBrains.Annotations.PureAttribute attribute externally added to the immutable collection types by them https://github.com/JetBrains/ExternalAnnotations/blob/master/Annotations/Misc/System.Collections.Immutable/Pure.xml