Remove unwanted String.Split methods from Reference assembly

[joperezr]

When merging #7142 I added some Split overloads to string that we really didn't wanted to so this change is removing them. We caught this because I only added them to the reference assembly and didn't add them to model.xml, so these methods were not found at runtime. This is the reason why tests in dotnet/corefx#11706 are failing.

cc: @tarekgh

[tarekgh]

@weshaggard these overloads Jose removing are not existing in Xamarin, but do you think we need to have them in Net Standard 2.0?

[tarekgh]

@joperezr LGTM if Wes don't want to include these into .net standard 2.0. thanks.

[joperezr]

Windows_NT arm Cross failures are not related to my change, It looks like every PR is hitting that now. @jkotas is this a known issue?

[weshaggard]

These string overloads are brand new (see #895) and aren't part of our .NET Standard 2.0 work. We shouldn't be removing them though. Can you help me understand what is breaking @joperezr

[joperezr]

Can you help me understand what is breaking @joperezr

Sure, with my last PR I added them to the ref project, but I didn't add them to model.xml so they are being removed from System.Private.CoreLib via BclRewriter. This caused that assemblies in corefx side like System.Runtime.Extensions who are also referencing the coreclr targetting pack to think that these methods exist at compile time, but they don't exist at runtime. One class in particular, System.Runtime.Versioning.FrameworkName calls String.Split(char) on it's constructor, and before I added these methods it was resolving to the String.Split(params char[]) method, but now it finds a better overload in String.Split(char), so it uses that which doesn't exist at runtime, so all of the tests that use FrameworkName are now broken.

Two ways to fix this, either add these methods to the implementation by adding them to model.xml, or remove them from the ref assembly given that they are not part of the set we want to add now. I thought the latter, hence this PR. I could also add them to model.xml if you want me to instead.

[justinvp]

either add these methods to the implementation by adding them to model.xml

These are already in model.xml.

But none of these should be exposed from any reference assembly yet.

As an aside, FrameworkName should be updated to use a cached static readonly char[] separator. I'll submit a PR for that.

[justinvp]

So, I believe the correct fix is to update this PR to remove the four new Split methods from src/mscorlib/ref/mscorlib.cs altogether.

[justinvp]

FYI, FrameworkName change: dotnet/corefx#11778

[weshaggard]

LGTM. @justinvp this mscorlib ref is only for corefx consumption and should match what is in S.P.CoreLib so it is fine for the new APIs to be in here.

[justinvp]

this mscorlib ref is only for corefx consumption and should match what is in S.P.CoreLib so it is fine for the new APIs to be in here.

Oh, I see. In that case, could we start calling the new Split methods from CoreFX assemblies, and start getting rid of all the static readonly char[] separator fields that are used throughout the repo?

[weshaggard]

@justinvp We can only do that for things that are directly building against .NET Core/CoreCLR here. There is a lot of shared code in corefx that builds for other targets as well so it isn't something we can move to holistically just yet.

[justinvp]

@weshaggard Got it. Thanks! 😄