Remove IVT from System.Collections.Immutable

[stephentoub]

System.Collections.Immutable currently has InternalsVisibleTo to its tests. This is something we try hard to avoid. We should remove the reliance on internals from the tests and switch the tests over to referencing the reference assembly. Some of the validation can be moved to being debug-only asserts in the actual implementation. Other use of internals are purely for convenience. Other uses could be achieved via either reflection or more preferably by separating out the small amount of internals use into a separate unit tests project that builds the relevant product src into the test, ala what's done in networking tests, in regex, etc.

Fixing this will shrink the size of the product assembly (the linker will be able to trim it), will avoid future issues like #107840, will enable our analyzers that bail on IVT to work for S.C.I, and will make it more consistent with the rest of dotnet/runtime.

[dotnet-policy-service]

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

[eiriktsarpalis]

I went through the internal APIs being used in testing and apart from the ones that can be fixed trivially, there are roughly two categories of tests that make comprehensive use of internals:

• KeyAnalyzerTests.cs which I think could be addressed by including KeyAnalyzer.cs in the test project.
• Tests consuming the IBinaryTree and IImmutableListQueries internal interfaces. These effectively provide a view of the implementation details of certain collection types and they're not trivial to replace with reflection.

[stephentoub]

Tests consuming the IBinaryTree and IImmutableListQueries internal interfaces. These effectively provide a view of the implementation details of certain collection types and they're not trivial to replace with reflection.

Most if not all of the IImmutableListQueries ones can be addressed by just having a separate interface used in the tests. It's generally used to abstract over multiple types in order to test APIs they have in common but aren't part of a public abstraction, like ImmutableList<T> and ImmutableList<T>.Builder. The tests can instead just provide its own implementation on top of each of those instead of as part of. In fact, that interface isn't actually used by the product src, so bonus, it can be deleted from the source entirely.

The IBinaryTree ones are harder. Though it looks like many of the things being validated could instead be moved to be Debug.Asserts in the actual implementation, rather than being validated separately by tests. For example, one of the main uses is validate that the AVL trees are correctly shaped; that can be checked with asserts in debug builds.

Reflection might need to be used for a few things, or possibly just remove some tests: it's great to be thorough, but in general the vast majority of our testing is done in terms of public surface area, and then if we want to validate internals, we build the relevant source into a separate unit tests project.

[julealgon]

Fixing this will shrink the size of the product assembly (the linker will be able to trim it)...

@stephentoub could you elaborate a bit more on this particular point? Would this also affect an enterprise API solution enough that you'd recommend also avoiding InternalsVisibleTo outside of the runtime repo as a more broadly applicable best practice?

I'm particularly interested in understanding whether we should also avoid the use of InternalsVisibleTo in our own projects.

[stephentoub]

Different teams make different choices about InternalsVisibleTo. We try not to use it, for a few reasons.

First and foremost, we want our API testing to be against public surface area, so that it's being used as real code would use it. When we go against internals instead of public, we end up having gaps, like #107840.

Secondarily, there's various kinds of static analysis that isn't as robust when IVT is involved. For example, there's an analyzer rule that recommends sealing types that have no derivations... but if some other assembly has access to internals, the analyzer can't reliably flag such types, because it doesn't know if that other assembly might be deriving from the type in question. That's true for the analyzers involved in trimming as well. We run the trimmer over every assembly, in order to get rid of non-public cruft that's left in the build but no one uses, e.g. code from shared files where only some of the functionality in the shared file is used, or internal/private const fields, which don't need to exist at all, since their values are propagated to the use site. That's where my comment about size comes from.