Null comparer consistency

[jnm2]

Fixes https://github.com/dotnet/corefx/issues/18528

Please wait for my PR adding tests in corefx before merging.

[jnm2]

Not sure on this, but I went with Comparer.Default just in Array to be consistent with everything else in array uses it. However, I used Comparer<object>.Default in Tuple and ValueTuple to be consistent with everything else in the BCL.
/cc @jcouv

[jcouv]

null [](start = 60, length = 4)

Why now pass null? I'm tempted to leave as before. Maybe BCL reviewers can comment. #Resolved

[jnm2]

@jcouv Because there is a chance that the comparer is never used, and because it's not DRY. I asked the same thing when I did ConcurrentDictionary and I'm following @hughbe's answer. #Resolved

[jcouv]

@jnm2 Looks good to me, with one question.
You should also update the tests (which live in coreFX) and run them against your private build of coreCLR (as previously discussed).

@stephentoub @weshaggard @AlexGhiondea for review. Thanks

[jnm2]

I am having an issue building corefx. I had set CoreCLROverridePath as a user env variable and rebooted, then deleted my corefx copy and recloned and ran build from VS2017 developer command prompt:

C:\Users\Joseph\Source\Repos\corefx>build
Installing dotnet cli...
Restoring BuildTools version 1.0.27-prerelease-01514-02...
Initializing BuildTools...
Done initializing tools.
Running: C:\Users\Joseph\Source\Repos\corefx\src\Native\build-native.cmd   x64  Debug  Windows_NT  --numproc 8    toolSetDir=c:\tools\clr
Tools are already initialized.
Running: C:\Users\Joseph\Source\Repos\corefx\Tools\msbuild.cmd /nologo /verbosity:minimal /clp:Summary /maxcpucount /nodeReuse:false /l:BinClashLogger,Tools\net46\Microsoft.DotNet.Build.Tasks.dll;LogFile=binclash.log /p:ConfigurationGroup=Debug /p:BuildPackages=false /p:GenerateNativeVersionInfo=true  /flp:v=normal  /flp2:warningsonly;logfile=msbuild.wrn  /flp3:errorsonly;logfile=msbuild.err  C:\Users\Joseph\Source\Repos\corefx\src\Native\..\../build.proj /t:GenerateVersionHeader

Build succeeded.
    0 Warning(s)
    0 Error(s)

Time Elapsed 00:00:00.21
Command execution succeeded.
**********************************************************************
** Visual Studio 2017 Developer Command Prompt v15.0.26403.7
** Copyright (c) 2017 Microsoft Corporation
**********************************************************************
[vcvarsall.bat] Environment initialized for: 'x86_x64'
Commencing build of native components

-- The C compiler identification is MSVC 19.0.24218.2
-- The CXX compiler identification is MSVC 19.0.24218.2
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/x86_amd64/cl.exe
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/x86_amd64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/x86_amd64/cl.exe
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/x86_amd64/cl.exe -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR - Failed
-- Performing Test COMPILER_HAS_DEPRECATED
-- Performing Test COMPILER_HAS_DEPRECATED - Success
-- Configuring done
-- Generating done
-- Build files have been written to: C:/Users/Joseph/Source/Repos/corefx/bin/obj/Windows_NT.x64.Debug/native
Tools are already initialized.
The specified config file 'C:\Users\Joseph\Source\config.json' does not exist.
Error: Could not load Json configuration file.
Failed to generate native component build project!
Command execution failed with exit code 1.

[weshaggard]

The specified config file 'C:\Users\Joseph\Source\config.json' does not exist.
Error: Could not load Json configuration file.
Failed to generate native component build project!

Where is your corefx repo root? Is it at C:\Users\Joseph\Source? It looks like it is picking up a different config.json file for the run command.

It looks like we explicitly pass it at https://github.com/dotnet/corefx/blob/master/run.cmd#L43 so I'm not sure why it would be picking up a config.json not in the root of your repo.

[jcouv]

FYI @marek-safar
Tuple, ValueTuple and Array are getting fixed in .NET Core 2.0 (use a default comparer instead of NRE on null comparer). My understanding is that Mono will pick the ValueTuple changes automatically (via source-level pull), but I'm not sure about Tuple or Array.

[jnm2]

@weshaggard

Where is your corefx repo root?

C:\Users\Joseph\Source\Repos\corefx

The source is https://github.com/jnm2/corefx/tree/null_comparer_consistency. I could rebase on upstream master, but the same build was working on a different machine.

[weshaggard]

@jnm2 can you look to see if you have this line locally https://github.com/jnm2/corefx/blob/null_comparer_consistency/run.cmd#L43? It should be telling run.exe to load the config.json file in root of your repo.

[marek-safar]

@jcouv we pick them all automatically when they land in CoreRT repo, which means you need to fix CoreRT version of Array as well.

[jnm2]

@weshaggard Ah, thank you! That's my problem. Line 43 is a blank line. I don't know why git checkout * is not updating my run.cmd. I'll make sure that gets fixed.

[jnm2]

Work in progress for the corefx PR: dotnet/corefx@master...jnm2:null_comparer_consistency

I'm a little unsure how to make the Tuple and ValueTuple tests netfx-specific since they are several calls deep from the test entry point and the SkipOnTargetFramework attribute will not apply to just those calls.
Ideally I'd be able to replicate SkipOnTargetFramework inside a conditional, but xUnit appears to have no TestContext of any sort.

[jnm2]

I just realized that System.Runtime is the only one that needs netfx-specific tests. System.Collections.lmmutable and System.ValueTuple tests fail if I give them netfx tests.

[jnm2]

No, it's just the System.ValueTuple tests. Because I fixed the corefx ValueTuple source too, I guess.

[jnm2]

Think everything is straightened out. Corefx PR ready at dotnet/corefx#19442

[jnm2]

To keep things in sync, I just rebased both PRs on master. Otherwise unrelated tests fail.

[jcouv]

LGTM Thanks!

[jcouv]

@dotnet-bot test Windows_NT x64 CoreCLR Perf Tests Correctness please

[jcouv]

@dotnet-bot test Windows_NT x64 CoreCLR Perf Tests Correctness please

[jcouv]

@stephentoub I saw there are discussions on the corefx sibling PR. Ok to merge this one?

[jcouv]

@dotnet-bot test Windows_NT x86 CoreCLR Perf Tests Correctness please

[jcouv]

@jnm2
After much debate back and forth, I've been convinced in discussion with @KevinRansom (F#), @weshaggard and compat council that the benefits (slightly more usable and consistent API) do not outweigh the costs (less stable API, creating a more complicated compat/test matrix for library developers using tuples, need for quirking on desktop) in the case of Tuple and ValueTuple.
I know this is different than other types that previously received a similar fix, I was not involved in those compat discussions, so I'm not commenting on those.

Although it's painful for me to not accept a PR, because the change looks good technically and I much appreciated your effort and patience, I think it's the better course of action for those types (Tuple and ValueTuple).
As far as Array, I am not responsible for that type, so if you still want to pursue that one, that will be a different discussion with whoever owns the type and with reviewers.
Sorry again for the late decision and thanks for your contributions.

CC @karelz

[KevinRansom]

This is a low value change. Consistency with other comparers in the framework has some merit ... but not very much ... Most developers have learned not to not pass null, to this API, indeed I'm guessing that those that tried it saw the exception, and fixed their code straight away, with a mild curse that it didn't do what they first typed. Contrast that with a future developer who typed in a null, and it worked because he was testing on Net47 and then deployed to his customers machines a which were a mix of 4.5, 4.6 and 4.61 where it didn't work, and his customer says "Did you even test this thing?".

[jnm2]

@jcouv @KevinRansom I appreciate the consideration, agree with the conclusion, and have no hard feelings. I'm happy to simply facilitate the possibility of an ideal.

[jnm2]

@jcouv I can't imagine Array being in a different boat than Tuple, but can you loop the owners of Array in here just to confirm that?