fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

`outref` parameter from an interface method is compiled as `byref` parameter

Open auduchinok opened this issue 3 years ago • 4 comments

Consider this F# code:

type I =
    abstract M: param: outref<int> -> unit

type T() =
    interface I with
        member this.M(param) = failwith "todo"

The param parameter is compiled as out parameter:

[CompilationMapping(SourceConstructFlags.ObjectType)]
[Serializable]
public interface I1
{
  void M(out int param);
}

[CompilationMapping(SourceConstructFlags.ObjectType)]
[Serializable]
public class T1 : Program.I1
{
  public T1()
  {
    Program.T1 t1 = this;
  }

  void Program.I1.M(out int param) => throw Operators.Failure("todo");
}

Consider that the interface is coming from an IL assembly (e.g. from C# project):

public interface I2
{
    void M(out int i);
}
type T2() =
    interface I2 with
        member this.M(i) = failwith "todo"

Now the parameter is being compiled as ref instead:

[CompilationMapping(SourceConstructFlags.ObjectType)]
[Serializable]
public class T2 : I2
{
  public T2()
  {
    Program.T2 t2 = this;
  }

  void I2.M(ref int i) => throw Operators.Failure("todo");
}

This doesn't seem to break anything at runtime, but the changed method signature is unexpected when working with FCS symbols and breaks a piece of analysis in our case.

auduchinok avatar Jul 08 '22 10:07 auduchinok

This is by design per RFC https://github.com/fsharp/fslang-design/blob/main/FSharp-4.5/FS-1053-span.md

An explicit Out attribute is needed

Will change to a diagnostic suggestion

dsyme avatar Jul 08 '22 13:07 dsyme

@dsyme Could maybe FSharpSymbol produced for IL parameter somehow report it's out?

auduchinok avatar Jul 08 '22 13:07 auduchinok

I thought about this again and agree we should do something here to make this consistent between F# and IL definitions.

dsyme avatar Jul 08 '22 15:07 dsyme

I've updated our analysis logic to be inline with how F# sees the things and it's not a problem for us anymore.

I agree it would be great to have this more consistent between F# and IL definitions in a future F# language version, but I've just realised how important it's that FCS stays consistent with F# point of view and now I'm grateful it provides the info it does (it could maybe allow seeing both IL and F# representations here, but that could be a different discussion).

auduchinok avatar Jul 08 '22 16:07 auduchinok