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

Consider using structs to wrap simple native typedefs #2069

Closed
JeremyKuhne opened this issue Oct 11, 2019 · 8 comments
Closed

Consider using structs to wrap simple native typedefs #2069

JeremyKuhne opened this issue Oct 11, 2019 · 8 comments
Assignees
Labels
design-discussion Ongoing discussion about design without consensus enhancement Product code improvement that does NOT require public API changes/additions

Comments

@JeremyKuhne
Copy link
Member

We've been using enums to wrap typedefs from Windows. For example:

typedef int BOOL;

We're wrapping as:

public enum BOOL : int
{
    FALSE = 0,
    TRUE = 1
}

While enums are more directly aligned with typedef, they don't provide other advantages that C# can provide, such as implicit operators. This can be useful in the case of BOOL, where checking if (b == BOOL.TRUE) is not correct as anything other than BOOL.FALSE is true.

We mitigate this somewhat with extension methods and copious comments, but it still makes for less than ideal consumption. if (SomeMethod().IsTrue()) isn't natural.

Using a struct with a single internal field is functionally equivalent from an interop perspective. It allows you to use operators with can make code much easier to write:

public readonly struct BOOL
{
    public int RawValue { get; }
    public BOOL(bool b) => RawValue = b ? 1 : 0;
    public BOOL(int b) => RawValue = b;
    public bool IsTrue => RawValue != 0;
    public bool IsFalse => RawValue == 0;
    public static implicit operator bool(BOOL b) => b.IsTrue;
    public static implicit operator BOOL(bool b) => new BOOL(b);
}

[DllImport]
BOOL SomeMethod(BOOL value);

// Can now be used like
if (SomeMethod(true))

// Instead of
if (SomeMethod(BOOL.TRUE).IsTrue())

The downsides are:

  1. It isn't technically correct, but .NET treats it the way we want
  2. It isn't as easy to optimize as enum, but the difference is trivial
  3. Core turned down the struct pattern, but doesn't have the volume of APIs WinForms does

Upsides are:

  1. Natural code flow
  2. Less risk of misuse

Another thing that is possible with structs that isn't with enums is wrapping pointers. Notably things like HWND, HBITMAP, etc. Using structs can allow us to strongly type our APIs:

public readonly struct HWND
{
    public IntPtr Value { get; }
    public HWND(IntPtr handle) => Value = handle;
}

With pointer size objects added to the mix I'd argue that using structs for typedef is the best pattern to be using in WinForms.

Discussed here.

cc: @hughbe, @zsd4yr, @RussKie, @weltkante

@JeremyKuhne JeremyKuhne self-assigned this Oct 11, 2019
@merriemcgaw merriemcgaw added this to the 5.0 milestone Oct 11, 2019
@merriemcgaw merriemcgaw added the enhancement Product code improvement that does NOT require public API changes/additions label Oct 11, 2019
@weltkante
Copy link
Contributor

weltkante commented Oct 11, 2019

First off, I'm in favor of using wrapper structs if using an enum is no longer good enough for the needs of the WinForms project. WPF already uses them so there is precedence in the framework. However WinForms has a much larger interop surface than WPF and I'd like to ensure that they actually will work in all relevant cases.

Using a struct with a single internal field is functionally equivalent from an interop perspective.

Technically this isn't true, which was discovered after a bugfix was made, which started crashing WPF usage of struct wrappers. This has lead to a technical discussion which highlighted that the native calling conventions for struct returns differ depending on whether the method has an implicit this parameter.

Primitive(__stdcall SomeInterface::*SomeMethod)(); // returns value in register
Primitive(__stdcall *SomeMethod)(SomeInterface*); // returns value in register
SomeStruct(__stdcall SomeInterface::*SomeMethod)(); // returns struct by writing to caller provided reference
SomeStruct(__stdcall *SomeMethod)(SomeInterface*); // returns struct in register

This means if CoreCLR were implemented correctly then struct wrappers don't work in COM interfaces, only in P/Invokes. It was not investigated in depth if struct wrappers in other positions, other calling conventions, or other platforms than Windows x86/x64 would cause any incompatibilities, since all those cases were out of scope as far as the WPF usage was concerned.

So while the particular scenario of struct wrappers as return values in Windows x86/x64 has ensured compatibility the assumption that struct wrappers are equivalent is based on a bug, it is unclear if it holds in other scenarios.

I'd like to point out two particular risks going forward without checking this:

  • While it is common to use BOOL or HRESULT as return value there also the occasional cases where they are passed or returned as a parameter. You'd want to ensure that wrapper structs work in both positions, being usable only in return positions is very easy to forget. (Alternatively you could create an analyzer which detects and prevents such usage, but I don't know if that is worth the effort.)

  • When Windows moves on to new platforms, in particular Windows ARM64 is already being considered, it is unclear whether these assumptions will still hold up.

    From a first test (C++ project with assembly output) ARM64 has the same difference and returns structs by reference when an implicit this parameter is present. However I cannot test how CoreCLR behaves as I have no hardware/VM to setup an ARM64 CoreCLR.

@JeremyKuhne
Copy link
Member Author

Technically this isn't true, which was discovered after a bugfix was made

It is true from the .NET Framework perspective. As you pointed out, even though it is technically incorrect, the runtime could never change their treatment of these as it would essentially break the world. What would likely happen is that they would add additional modifiers that allow you to get structs to pass per spec. For the record I was the source of the break you mentioned as I stumbled across the fact that I couldn't wrap some COM interfaces correctly and requested the original "fix". :) (COM interfaces that returned a struct rather than an HRESULT).

While it is common to use BOOL or HRESULT as return value there also the occasional cases where they are passed or returned as a parameter

They work fine as parameters (by ref or otherwise). I use structs in my WInterop library, which has several hundred P/Invokes.

When Windows moves on to new platforms, in particular Windows ARM64 is already being considered, it is unclear whether these assumptions will still hold up.

It would break a ton of apps (as you've pointed out with WPF). The runtime will keep compatibility- there really is no choice.

@RussKie RussKie added the design-discussion Ongoing discussion about design without consensus label Oct 15, 2019
@JeremyKuhne
Copy link
Member Author

I'm going to assert that using structs would be the best choice as it would improve encapsulation and "type safety". This came up again with #2218, where we really should have a COLORREF with conversion methods hanging off of it. For example (I give this code freely to .NET without need for attribution, in headers or otherwise).

@weltkante
Copy link
Contributor

Generally I'm ok with this, but I'd like to point out that its not been clearly answered on whether this will work for ARM64. Would be nice if this could be verified. I only could check the native side, the C/C++ ABIs seem the have the same difference of how structs are passed, I can't test if CoreCLR will generate the right assembly code for everything to work out. The workaround in CoreCLR doesn't look like it would be x86/x64 specific but it would be nice to make sure.

That said its not something to block on, if it can't be tested now I'm fine with moving it either into #2053 or a separate issue.

@RussKie
Copy link
Member

RussKie commented Oct 28, 2019

We will be getting access to ARM64 hardware in near to medium term future, so we should be able to test the hypothesis.

@JeremyKuhne
Copy link
Member Author

Another valuable thing we could get out of "structifying" type definitions is that we could guard against sign extension problems with handles. See #2004 (comment) and the bug fix with #2736.

An OLE_HANDLE and HBITMAP struct could handle conversions properly and with context.

@RussKie RussKie modified the milestones: 5.0 Previews 1-4, 5.0 Apr 20, 2020
@merriemcgaw merriemcgaw modified the milestones: 5.0, 6.0 Sep 8, 2020
@RussKie RussKie modified the milestones: 6.0, 7.0 Aug 27, 2021
@merriemcgaw merriemcgaw modified the milestones: .NET 7.0, .NET 8.0 Aug 11, 2022
@elachlan
Copy link
Contributor

Should we close this since winforms is now moving to CsWin32?

@RussKie
Copy link
Member

RussKie commented Oct 23, 2022

Yes, thank you.

@RussKie RussKie closed this as completed Oct 23, 2022
@ghost ghost removed this from the .NET 8.0 milestone Oct 23, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Nov 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
design-discussion Ongoing discussion about design without consensus enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

5 participants