Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better generic unmanaged structs handling ++ bidirectional F#/C# interop for 'unmanaged' constraint #12154

Merged
merged 23 commits into from
Jul 31, 2023

Conversation

vzarytovskii
Copy link
Member

@vzarytovskii vzarytovskii commented Sep 15, 2021

Pretty much a continuation of @TIHan's work on on top of main (previous PR for reference: #6064).
RFC: https://github.com/fsharp/fslang-design/blob/main/RFCs/FS-1090-Generic-struct-type-whose-fields-are-all-unmanaged-types-is-unmanaged.md

TODO/FIXES:

  • Treat type arguments with IsUnmanagedAttribute as if they have the unmanaged constraint.
  • Emit IsUnmanagedAttribute for the unmanaged constraints on params.
  • Emit UnmanagedType modreq on type arguments on types
    - This would be a binary breaking change, we should consider if we want to do it or not.
  • An unmanaged constraint cannot be used together with not struct constraint (not the case right now).
  • Fix struct tuples, and struct anonymous records - whenwe check isUnmanaged on their arguments, tuple/record hasn't been solved yet.

** TEST list ** (Tomas's braindump)

  • F# side definition options:
    • Generic custom types
    • Generic struct records
    • Struct tuples
    • Struct DU's
    • Struct anonymous records
    • Deeply nested generic scenarios ( MyStruct<MyOtherStruct<StructTuple<int,byte>>> )
    • Any nested combination of the following (e.g. AnonRecord<StructTuple<CustomType<Record>>>)
  • Test matrix dimensions for interop (missing anything?)
    • Method with unmanaged constraint defined in C#/F# (*2 test cases)
    • Data to be passed (a generic struct) defined in C#/F# (*2 test cases)
    • The T is either valid or invalid and should show error (*2 test cases)
    • The callsite is C#/F# (*2 test cases)
  • Additional test pointers:
  • If we do not put in a modreq in F# codegen for a constrained method,:
    • C# can still call it
    • C# can still typecheck it for in/correct usage
    • VB.NET is not capable of calling it incorrectly
      • Well, apparently, VB.NET does not support this feature and is not capable of calling into such methods.
    • For places where C# puts in a modreq, F# can call those methods (haven't checked deeply yet, might differ across virutal/non-vritual scenarios)

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Nov 15, 2021

Emit UnmanagedType modreq on type arguments.

I am not sure whether we should emit it just because C# does it as well, since it will change the default behaviour we already have, it will also be not easy to test.

@T-Gro T-Gro changed the title WIP: Better generic unmanaged structs handling Better generic unmanaged structs handling ++ bidirectional F#/C# interop for 'unmanaged' constraint Jul 24, 2023
@T-Gro T-Gro marked this pull request as ready for review July 24, 2023 14:45
@T-Gro T-Gro requested a review from a team as a code owner July 24, 2023 14:45
@T-Gro
Copy link
Member

T-Gro commented Jul 24, 2023

Before this PR, the unmanaged constraint defined in F# was erased for IL output, therefore any other .NET consumer could call into such defined types/methods.

With this PR, the modreq is being emitted.
Which F# and C# understands, but VB.NET does not:

dotnet/vblang#300
dotnet/vblang#604

Which means that public APIs that should also target VB.NET will need to be aware of this and provide alternatives.

@Happypig375
Copy link
Member

voidptr support? fsharp/fslang-suggestions#1160

@T-Gro
Copy link
Member

T-Gro commented Jul 25, 2023

voidptr support? fsharp/fslang-suggestions#1160

I don't see why not, I can add it in a separate PR right after this one is merged in.

@T-Gro T-Gro added this to the July-2023 milestone Jul 25, 2023
src/Compiler/FSComp.txt Outdated Show resolved Hide resolved
@vzarytovskii vzarytovskii enabled auto-merge (squash) July 25, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants