fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

Default use of implicit conversion creates non-working code (using Pulumi libraries)

Open marklam opened this issue 2 years ago • 8 comments

The code generated by the new (12.0.1.0 compiler for F# 6) which implicitly uses op_Implicit is generating code that doesn't work the same way that calling op_Implict (or using C#'s implicits) does.

This doesn't work

    let conditionImplicit =
        Cdn.Inputs.DeliveryRuleRequestSchemeConditionArgs(
            Name = "RequestScheme1",
            Parameters =
                Cdn.Inputs.RequestSchemeMatchConditionParametersArgs(
                    MatchValues = matchValues,
                    Operator = Input.op_Implicit "Equal",
                    OdataType = Input.op_Implicit "#Microsoft.Azure.Cdn.Models.DeliveryRuleRequestSchemeConditionParameters"
                )
        )

Not using implicit conversions does work

    let conditionExplicit =
        Cdn.Inputs.DeliveryRuleRequestSchemeConditionArgs(
            Name = "RequestScheme2",
            Parameters =
                Input.op_Implicit (Cdn.Inputs.RequestSchemeMatchConditionParametersArgs(
                    MatchValues = matchValues,
                    Operator = Input.op_Implicit "Equal",
                    OdataType = Input.op_Implicit "#Microsoft.Azure.Cdn.Models.DeliveryRuleRequestSchemeConditionParameters"
                ))
        )

C#'s implicit conversions do work

            var conditionImplicit =
                new DeliveryRuleRequestSchemeConditionArgs {
                    Name = "RequestScheme1",
                    Parameters =
                        new RequestSchemeMatchConditionParametersArgs {
                            MatchValues = matchValues,
                            Operator = "Equal",
                            OdataType = "#Microsoft.Azure.Cdn.Models.DeliveryRuleRequestSchemeConditionParameters"
                        }
                };

The repro case is at https://github.com/marklam/ImplicitsProblem

It looks like the code generation (as disassembled with ILspy) is different for the F# implicit case. This means that when this code is used within a Pulumi app, errors like https://github.com/pulumi/pulumi-azure-native/issues/1569 occur.

It seems like this is being produced by the implicit calls:

 call class [Pulumi]Pulumi.Input`1<!0> class [Pulumi]Pulumi.Input`1<class [Pulumi.AzureNative]Pulumi.AzureNative.Cdn.Inputs.RequestSchemeMatchConditionParametersArgs>::op_Implicit(!0)  

instead of

newobj instance void [Pulumi.AzureNative]Pulumi.AzureNative.Cdn.Inputs.DeliveryRuleRequestSchemeConditionArgs::.ctor()

Known workarounds

Enable warnings for implicit conversions, and use the old way (call op_Implicit yourself)

<WarnOn>3388;3391;3395</WarnOn>

Related information

Provide any related information (optional):

  • Windows 10
  • Visual Studio 2022
  • F# 6
  • .NET Runtime 3.1

marklam avatar Apr 05 '22 18:04 marklam

I've also encountered this bug on multiple occasions now when working with Pulumi and F# 😞 The Pulumi SDK for dotnet relies heavily on implicit conversions and without them, the code looks unusable.

For example

// using implicit conversions (does not work)
Bucket("my-bucket", BucketArgs (Website = BucketWebsiteArgs (IndexDocument = "index.html")))

// explicit input calls (works but extremely noisy)
Bucket("my-bucket", BucketArgs (Website = input (BucketWebsiteArgs (IndexDocument =  input "index.html"))))

In the snippet above, input is just an alias for Input.op_Implicit (value: 't) : Input<'t>

Zaid-Ajaj avatar Aug 16 '22 10:08 Zaid-Ajaj

@Zaid-Ajaj I did some work in that area in https://github.com/dotnet/fsharp/pull/13673 so I might be able to fix this. Do you have a small repro that I can test against?

NinoFloris avatar Aug 16 '22 13:08 NinoFloris

@NinoFloris here you go https://github.com/Zaid-Ajaj/fsharp-implicit-call-bug-repro this repro doesn't need the Pulumi CLI and it is just a console app that demonstrates the problem. I hope this helps 🙏

Zaid-Ajaj avatar Aug 16 '22 14:08 Zaid-Ajaj

Thanks!

I've minimized the repro further and analyzed the IL.

type Input<'T>(_v: 'T) =
    static member op_Implicit(value: 'T): Input<'T> = Input<'T>(value)

type OtherArgs() =
    member val Name: string = Unchecked.defaultof<_> with get,set
type SomeArgs() =
    member val OtherArgs: Input<OtherArgs> = Unchecked.defaultof<_> with get, set
    
let test() =
    SomeArgs(OtherArgs = OtherArgs(Name = "test"))

The il of test looks like this:

    .locals init (
      [0] class Program/SomeArgs V_0,
      [1] class Program/OtherArgs V_1
    )

    // [18 5 - 18 51]
    IL_0000: newobj       instance void Program/SomeArgs::.ctor()
    IL_0005: stloc.0      // V_0
    IL_0006: ldloc.0      // V_0
    IL_0007: newobj       instance void Program/OtherArgs::.ctor()
    IL_000c: call         class Program/Input`1<!0/*class Program/OtherArgs*/> class Program/Input`1<class Program/OtherArgs>::op_Implicit(!0/*class Program/OtherArgs*/)
    IL_0011: stloc.1      // V_1
    IL_0012: ldloc.1      // V_1
    IL_0013: ldstr        "test"
    IL_0018: callvirt     instance void Program/OtherArgs::set_Name(string)
    IL_001d: nop
    IL_001e: ldloc.1      // V_1
    IL_001f: call         class Program/Input`1<!0/*class Program/OtherArgs*/> class Program/Input`1<class Program/OtherArgs>::op_Implicit(!0/*class Program/OtherArgs*/)
    IL_0024: callvirt     instance void Program/SomeArgs::set_OtherArgs(class Program/Input`1<class Program/OtherArgs>)
    IL_0029: nop
    IL_002a: ldloc.0      // V_0
    IL_002b: ret

Following the lines, starting from IL_0011 the compiler went astray:

  1. It emitted a stloc.1 for the result of the implicit conversion from OtherArgs to Input<OtherArgs>. Meaning it now holds a value of Program/Input`1<!0/class Program/OtherArgs/> in local [1] which has the expected type Program/OtherArgs, this is obviously not correct.
  2. Next we load the same Input`1 again onto the stack.
  3. We load the constant string "test" I used for Name onto the stack.
  4. Then at IL_0018 it uses these two stack entries, the Input`1 as the supposed OtherArgs instance and the string as the first argument to set_Name.

Pushing this all into ilverify confirms my conclusion:

[IL]: Error [StackUnexpected]: [FsImplicitTest.dll : .Program::test()][offset 0x00000011][found ref '[FsImplicitTest]Program+Input`1<Program+OtherArgs>'][expected ref '[FsImplicitTest]Program+OtherArgs'] Unexpected type on the stack.

Now at this point if this was a virtual property the runtime would have to look at the instance reference it has on the stack to run set_Name for (in this case a reference to an Input). It would get the method table from the object header and try to find set_Name which doesn't exist on Input. The runtime would then throw some invalid program exception killing the process, which would have made this a bit easier :)

Here though, because it's a non virtual method, the instructions were even inlined (and the F# compiler might also do this if Pulumi's Input class was written in F#). As a result this runtime work does not happen and code just overwrote what was there (in whatever instance reference given) at the offset the backing field for Name would normally live. For Input this offset corresponds with the underlying/wrapped value. And indeed navigating the value in the debugger shows that SomeArgs.OtherArgs now directly points to an Input storing Name instead of OtherArgs. Heap corruption!

Fix is in https://github.com/dotnet/fsharp/pull/13673

NinoFloris avatar Aug 16 '22 20:08 NinoFloris

Really awesome stuff @NinoFloris 🤯 it will be huge for F# Pulumi programs when this lands 🚀

Zaid-Ajaj avatar Aug 16 '22 21:08 Zaid-Ajaj

@vzarytovskii this one is also fixed by https://github.com/dotnet/fsharp/pull/13673

NinoFloris avatar Sep 05 '22 15:09 NinoFloris

@NinoFloris Any idea when it will ship in the next dotnet SDK?

Zaid-Ajaj avatar Sep 05 '22 15:09 Zaid-Ajaj

Not sure, might have made it for rc1 but otherwise rc2 maybe?

NinoFloris avatar Sep 05 '22 23:09 NinoFloris

@Zaid-Ajaj can you test once more now that 7.0 is out?

NinoFloris avatar Nov 08 '22 22:11 NinoFloris

Ok, this should be fixed in the rtm, going to close it, please feel free to reopen otherwise.

vzarytovskii avatar Nov 14 '22 12:11 vzarytovskii