Generate BigInteger literals in CSharpHelper

[roji]

Needed for code literal generation of NodaTime types. Hopefully this can still go into 2.2?

(if merged into release/2.2, this automatically gets merged into master too, right?)

[ajcvickers]

@Eilon What's the status on 2.2 builds? Is there time for this to go in? It's very low risk and needed for the Npgsql provider.

[Eilon]

Let's take it if we can get it in today.

[Eilon]

Wut, no trailing comma??,

[roji]

Just followin' the existing style :)

[roji]

Thanks for reviewing and approving so quickly - I amended to use the invariant culture (thanks).

Just to make sure: BigInteger resides in System.Numerics. I can see namespace discovery & inclusion logic in MigrationsCodeGenerator, and I assume it's the user's responsibility to reference System.Numerics from the project containing the migrations - so it seem fine, but I just want to make sure...

If this creates any issues, it's only necessary for generating code literals for NodaTime objects - not very critical (but would definitely be nice to have).

[ajcvickers]

@bricelam is going to try to get this in today. Thanks @roji.

[roji]

Thanks for the super quick handling guys....

[roji]

After trying to put this to use, it seems we went a bit too quickly with this...

When generating a migration with seeding data for a NodaTime Instant, I get the following expression, which has two problems:

NodaTime.Instant.InstantFromUnixTimeNanoseconds(BigInteger.Parse("1519909261000000000", NumberFormatInfo.InvariantInfo)), "foo" }

In the generated migration and model snapshot, BigInteger and NumberFormatInfo are missing their using statements (System.Numerics, System.Globalization). This is relatively minor, and users can easily add the missing directives as a workaround (but ideally we may need some way to inject them).

A stranger issue is the call to NodaTime.Instant.InstantFromUnixTimeNanoseconds(). My mapping's GenerateCodeLiteral() actually looks like this:

public override Expression GenerateCodeLiteral(object value)
    => Expression.Call(
        typeof(NpgsqlNodaTimeUtils).GetMethod(nameof(NpgsqlNodaTimeUtils.InstantFromUnixTimeNanoseconds), new[] { typeof(BigInteger) }),
        Expression.Constant(NpgsqlNodaTimeUtils.ToUnixTimeNanoseconds((Instant)value)));

A static method is called because some arithmetic needs to be done, and the appropriate expression types aren't supported in CSharpHelper. In any case, InstantFromUnixTimeNanosec() is defined on NpgsqlNodaTimeUtils, but CSharpHelper generates a a call on NodaTime.Instant instead. This seems to come from the following code:

case ExpressionType.Call:
{
    var callExpression = (MethodCallExpression)expression;
    if (callExpression.Method.IsStatic)
    {
        builder
            .Append(Reference(expression.Type, useFullName: true));
    }
    else
    {
        if (!HandleExpression(callExpression.Object, builder))
        {
            return false;
        }
    }

    builder
        .Append('.')
        .Append(callExpression.Method.Name);

For static methods, the reference is generated for the expression type, instead of for the method's declaring type. Unless I'm missing something, this looks like a bug (I can submit a fix).

/cc @bricelam

[ajcvickers]

@roji For the namespace issue, see #13799. For 2.2 RTM we should namespace-qualify everything in these expressions--but maybe because we added BigInteger directly to CSharp helper with this PR it still needs the the using statement? I added a reference to that issue.

The second issue looks like a clear bug. Could you file an issue for it?

Also, how important is this for the Npgsql provider? We can't get anything in for RTM, but low-risk patch fixes (probably in 2.2.2 at this point) may be possible.

[roji]

@ajcvickers opened #13985 for the second bug, will submit a PR soon. At the moment is it blocking NodaTime code literal generation, so NodaTime values cannot be seeded. I've had a couple questions about this, but I can't say it's a total showstopper either. On the other hand, the fix is probably trivial enough for inclusion in a patch (I don't know much about how high you set the bar...).

For the namespace issue, yep, #13799 seems to cover it. It would be nice to add using directives after going through all the literals, rather than fully-qualifying everything and bloating the migration files (although at least in theory we could have the same name in two namespaces, causing a conflict...).

[ajcvickers]

@roji Thanks. The patch bar has a number of elements, but the one I think might be missing here is high enough customer impact. If we have reports of people hitting this issue it will be more compelling.