Fixed string Join method

[AlexRadch]

Fixed string Join(string separator, params object[] values) method.
Calling string.Join(",", null, 1, 2, 3); return empty string but should ",1,2,3".

[danmoseley]

@AlexRadch unfortunately compatibility with the .NET Framework is very important to us, and that returns "" also. I don't think there is good enough reason to break compatibility here even though it presumably isn't doucmented behavior.

[AlexRadch]

@danmosemsft how many programs use this unusual behavior as useful? I think no one.
I think many programs use this Join and in some rare cases can return empty string when should not do it. In other words many programs have erroneous behavior because this method have error and no one use this error as future.
So why .NET Framework still support this bug? And do not fix it?

[jnm2]

@danmosemsft I was about to follow this up with, how soon until this fix makes it into full framework? If it has to be quirked, so be it; I seriously hope desktop .NET vNext ships this fix.

[danmoseley]

@jnm2 it looks like the object[] overload of Join has had this bug since it was introduced in 4.0. It seems to me indeed a bug since it's inconsistent with the original string[] overload of Join. Given that I think we might take this change but backporting might take a while. @stephentoub thoughts?

[jnm2]

I've already identified two places in the codebase I happen to have open where this bug could cause some very interesting debugging sessions. Even more if I inadvertently switch from an IEnumerable overload to the object[] overload.

[dnfclas]

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

Thanks, DNFBOT;

[dnfclas]

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

Thanks, DNFBOT;

[danmoseley]

@stephentoub are you comfortable with this change?

@AlexRadch it would need a follow up change in corefx to add a test

[stephentoub]

@stephentoub are you comfortable with this change?

See my comment at https://github.com/dotnet/coreclr/issues/4506#issuecomment-261016448. As long as we port the fix back to desktop (under a quirk I guess, in particular since the behavior is documented) and fix the documentation, I'm ok with it.

[danmoseley]

@AlexRadch in a day or two (supposed to be less) corefx will receive this coreclr change. When that happens could you please submit a PR to add tests to protect this? They will go in here corefx\src\System.Runtime\tests\System\StringTests.cs.

[jamesqo]

Nice to see this finally fixed! 🎉

[danmoseley]

We have a PR to update our breaking change guidelines here dotnet/corefx#13718

[AlexRadch]

@danmosemsft I created pull request dotnet/corefx#13747 with fixed tests.