Refactored range type handler

[YohDeadfall]

This is my idea how the range handler should be refactored to fix #2040. The only one error I encountered is shown below. Will investigate this tomorrow.

\.nuget\packages\microsoft.dotnet.ilcompiler\1.0.0-alpha-26714-01\build\Microsoft.NETCore.Native.Windows.props(95,43): error MSB4184: The expression ""System.String[]".GetValue(1)" cannot be evaluated. Index was outside the bounds of the array.

[roji]

I may be missing something, but I don't see how this PR solves the CoreRT issue (theoretical endless nested RangeHandler<RangeHandler<.....>>)... You still have CreateRangeHandler() defined on NpgsqlTypeHandler (non-generic), with RangeHandler extending NpgsqlTypeHandler and overriding that method to throw NotSupportedException. That means that any static analyzer that inspects the hierarchy still sees the theoretical possibility of calling CreateRangeHandler(), then calling CreateRangeHandler() on the returned value, and so ad infinitum... The same problem exists with CreateArrayHandler().

Some of the other changes you've done (unrelated cleanup/refactoring) could be fine though, but it's better to separate them out into another smaller PR.

[YohDeadfall]

In short, you are missing that the RangeHandler<TElement> class is not NpgsqlTypeHandler<NpgsqlRange<TElement>> anymore. It inherits directly from non-generic NpgsqlTypeHandler (:

As you described #2042, the issue was caused by an instance of the NpgsqlTypeHandler<TDefault> class which could construct a NpgsqlTypeHandler<NpgsqlRange<TDefault>> which could construct a NpgsqlTypeHandler<NpgsqlRange<NpgsqlRange<TDefault>>> and so on. But now NpgsqlTypeHandler<TDefault>.CreateRangeHandler produces a RangeHandler<TDefault> which has it's own overriden implementation of the method and no base. So there is no reference from the range handler back to NpgsqlTypeHandler<TDefault>.

[YohDeadfall]

Move it to an additional PR?

[roji]

I think it's small enough to leave in...

[roji]

OK, I now understand the idea. I'm just not sure I completely understand precisely what CoreRT is doing and precisely what makes it fail on the current code.

Can I ask you to simply test CoreRT on your PR? If the error goes away I agree that it is a much better way to solve it than my PR...

[YohDeadfall]

The code below compiles without any error.

static void Main(string[] args)
{
    using (var conn = new NpgsqlConnection("Server = localhost; User ID = npgsql_tests; Password = npgsql_tests; Database = npgsql_tests; Timeout = 0; Command Timeout = 0"))
    using (var cmd = new NpgsqlCommand("SELECT int4range(10, 20)", conn))
    {
        conn.Open();

        using (var rdr = cmd.ExecuteReader())
            if (rdr.Read()) Console.WriteLine(rdr.GetFieldValue<NpgsqlRange<int>>(0));
    }
}

PS D:\Source\Repos\npgsql\test\Npgsql.CoreRT> dotnet publish -r win-x64 -c release
Microsoft (R) Build Engine version 15.7.179.6572 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  Restoring packages for D:\Source\Repos\npgsql\src\Npgsql\Npgsql.csproj...
  Restoring packages for D:\Source\Repos\npgsql\test\Npgsql.CoreRT\Npgsql.CoreRT.csproj...
  Restore completed in 219.55 ms for D:\Source\Repos\npgsql\src\Npgsql\Npgsql.csproj.
  Installing runtime.win-x64.Microsoft.DotNet.ILCompiler 1.0.0-alpha-26718-02.
  Generating MSBuild file D:\Source\Repos\npgsql\test\Npgsql.CoreRT\obj\Npgsql.CoreRT.csproj.nuget.g.targets.
  Restore completed in 42.22 sec for D:\Source\Repos\npgsql\test\Npgsql.CoreRT\Npgsql.CoreRT.csproj.
  Npgsql -> D:\Source\Repos\npgsql\src\Npgsql\bin\release\netstandard2.0\Npgsql.dll
  Npgsql.CoreRT -> D:\Source\Repos\npgsql\test\Npgsql.CoreRT\bin\release\netcoreapp2.0\win-x64\Npgsql.CoreRT.dll
  Generating native code
  Npgsql.CoreRT -> D:\Source\Repos\npgsql\test\Npgsql.CoreRT\bin\release\netcoreapp2.0\win-x64\publish\

[roji]

OK, and just to be 100% sure you're also seeing an infinite type recursion issue when trying to build without your changes?

(FYI I actually tried to test with corert myself but there was a build failure trying to publish for linux-x64).

[YohDeadfall]

Yes, I see it without that fix.

[roji]

OK great, good to merge them!

It also means there are no other issues with corert, which is good news...

Do you think we should release this for 4.0.3? It seems like a pretty low-risk refactor...

[YohDeadfall]

For now it compiles using the AOT compiler, but it doesn't work because of heavily used reflection in Npgsql. At least we should provide a basic rd.xml file or notify about it in the documentation. I will work on it, but it's not a blocking issue to release the fix in 4.0.3, I think.

[roji]

@YohDeadfall thanks for your work on this. Maybe open an issue to track adding rd.xml?

[YohDeadfall]

Opened #2073.

[YohDeadfall]

Backported to 4.0.4 via 6487cc6.