HashSet<T> constructor with capacity

[MarkusSintonen]

Issue #382
-Added constructor with capacity parameter to HashSet
-Added two unit tests for HashSet capacity constructor

[dnfclas]

Hi @MarkusSintonen, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla2.dotnetfoundation.org.

TTYL, DNFBOT;

[dnfclas]

@MarkusSintonen, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

[svick]

I take it this is the implementation for #382?

[MarkusSintonen]

Yes, I updated the comment

[mellinoe]

Note: this is the error in the CI build:

Generic\HashSet\HashSet_ConstructorTests.cs(291,36): error CS1502: The best overloaded method match for 'System.Collections.Generic.HashSet.HashSet(System.Collections.Generic.IEnumerable)' has some invalid arguments

This is expected because this new method hasn't actually been added to the contract assembly we build internally, which is what the test compiles against.

[KrzysztofCwalina]

How does it verify that the default comparer is used?

[stephentoub]

@mellinoe, the way we currently build our test projects for partial facades makes it impossible to add tests for new APIs, since they're not part of the contract but we build the tests against the contract:
https://github.com/MarkusSintonen/corefx/blob/future/src/System.Collections/tests/System.Collections.Tests.csproj#L18
Do we have a plan for solving this?

[mellinoe]

Other than bringing out our contract generation code to GitHub, I don't know that we have a plan for this necessarily. Traditionally we've always built our tests against the contract, so a change like this would force us to update the contract internally along with the tests. Unfortunately it's not really possible for people adding new APIs to do that until the contract generation stuff is out here.

Technically it should be possible to compile these tests against the partial facade itself, but that will require also compiling against mscorlib.dll, as well as potentially other partial facades that are used by the library. For example, System.Diagnostics.Debug's tests are compiled against the library project file, because it uses internal types, and System.Diagnostics.Debug is a partial facade.

https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.Debug/tests/project.json
https://github.com/dotnet/corefx/blob/master/src/System.Diagnostics.Debug/tests/System.Diagnostics.Debug.Tests.csproj

The basic process is to add this to project.json to pull in mscorlib:

"Microsoft.DotNet.CoreCLR": "1.0.0-prerelease",

Additionally you need a few project properties to force-resolve mscorlib.dll from that NuGet package if you aren't a partial facade project:

<NuGetTargetFrameworkMoniker>DNXCore,Version=v5.0</NuGetTargetFrameworkMoniker>
<IgnoreArchitectureMismatches>true</IgnoreArchitectureMismatches>
<PostFilterNugetReferences>true</PostFilterNugetReferences>

[stephentoub]

Technically it should be possible to compile these tests against the partial facade

@mellinoe, separate from (and before) this change, should we update our projects as you suggest? Or is the stance "no new APIs in partial facades" for now until the tooling issues are sorted?

[MarkusSintonen]

I wondered why the test compilation failed on my machine. Well at least it wasn't a problem with my own setup. I think it would be valuable to get the full tooling available since this would allow test coverage for future brach API additions.

Should I remove the two unit tests related to the new API for now?

[ghost]

Using C# 6, we can say:

throw new ArgumentOutOfRangeException(nameof(capacity)); 

[akoeplinger]

@jasonwilliams200OK corefx doesn't use C#6 yet.

[stephentoub]

@MarkusSintonen, it looks like you may have accidentally pushed your concurrent bag changes to this PR. Would you be able to fix that? We're still blocked in taking this change on the infrastructure issues around tests, but hopefully that will be stored out soon and then this can be merged. Thanks!

[stephentoub]

@mellinoe, I tried your suggested workaround, but it then fails to compile due to lots of duplicated types between mscorlib and the various contracts. Could you see if there's an easy way to unblock this?

[stephentoub]

@MarkusSintonen, sorry for the delay on this. Let's go ahead on this without the test for the new ctor, and when we have the infrastructure in place, we can then add the test. Can you squash this down to a single commit? Thanks!

[stephentoub]

Replaced by #2862.