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

GetMessage returns a bool when it should return a BOOL #362

Closed
sylveon opened this issue Jul 23, 2021 · 7 comments · Fixed by #363
Closed

GetMessage returns a bool when it should return a BOOL #362

sylveon opened this issue Jul 23, 2021 · 7 comments · Fixed by #363
Assignees
Labels
bug Something isn't working

Comments

@sylveon
Copy link

sylveon commented Jul 23, 2021

Actual behavior

Currently, GetMessage is defined to return a bool, which can be compared to true or false only. This incorrect according to the documentation:

If the function retrieves a message other than WM_QUIT, the return value is nonzero.

If the function retrieves the WM_QUIT message, the return value is zero.

If there is an error, the return value is -1. For example, the function fails if hWnd is an invalid window handle or lpMsg is an invalid pointer. To get extended error information, call GetLastError.

This means, that from C#, it's impossible to handle errors for this function.

Because of the bool return, it actually ends up being that ignoring errors is encouraged, when the docs actually call this out as wrong:

Because the return value can be nonzero, zero, or -1, avoid code like this:

while (GetMessage( lpMsg, hWnd, 0, 0))

Expected behavior

GetMessage should return an int, so that it can be compared to -1 and 0.

Repro steps

  1. NativeMethods.txt content:
GetMessage
  1. NativeMethods.json content (if present):
    Nothing

  2. Any of your own code that should be shared?
    No

Context

  • CsWin32 version: 0.1.506-beta
  • Win32Metadata version (if explicitly set by project): Not explicitly set
  • Target Framework: net5.0-windows
  • LangVersion (if explicitly set by project): Not explicitly set
@AArnott AArnott transferred this issue from microsoft/CsWin32 Jul 23, 2021
@AArnott
Copy link
Member

AArnott commented Jul 23, 2021

This is an interesting one. The Windows headers and docs type the return value as BOOL but then explicitly document as returning at least 3 values (oddly considering "nonzero" to evidently not include -1, which if I may point out is also a nonzero value).

Anyway, I think we may need to special case this in the metadata. Should it be int instead? At least in CsWin32 we consider uses of BOOL in method signatures to be fair game to express as C# bool. If we keep BOOL in the metadata, can we get at least a hint that more values than two may be expressed by the method?

@marler8997
Copy link

As I understand, in the metadata the return value of GetMessage* is defined as BOOL (not bool), which itself is defined as a 32-bit signed integer. Is CsWin32 translating BOOL to a .NET bool instead?

@sylveon
Copy link
Author

sylveon commented Jul 29, 2021

Yes, cswin32 is writing out the pinvoke with a .NET bool as return value.

@weltkante
Copy link

This has been discussed before (#308 and microsoft/win32metadata#301) and I'm surprised it hasn't been acted upon.

@marler8997
Copy link

It looks like @AArnott said he would find a way to preserve the full 32-bit signed BOOL value and allow it to be implicitly convertible to a bool (microsoft/win32metadata#308 (comment)). I don't know how that's possible in C#?

@mikebattista
Copy link
Contributor

It looks like @AArnott said he would find a way to preserve the full 32-bit signed BOOL value and allow it to be implicitly convertible to a bool (#308 (comment)). I don't know how that's possible in C#?

@AArnott ?

@AArnott
Copy link
Member

AArnott commented Aug 17, 2021

I have already made BOOL provide access to the underlying Int32 and is implicitly convertible to and from C# bool. See this definition:

internal readonly partial struct BOOL
{
    private readonly int value;

    internal int Value => this.value;
    internal BOOL(bool value) => this.value = value ? 1 : 0;
    internal BOOL(int value) => this.value = value;
    public static implicit operator bool(BOOL value) => value.Value != 0;
    public static implicit operator BOOL(bool value) => new BOOL(value);
    public static explicit operator BOOL(int value) => new BOOL(value);
}

The remaining problem seems to be that although the metadata declares GetMessageW as returning BOOL, CsWin32's projection still declares it as returning bool. I'll move the issue to CsWin32.

@AArnott AArnott changed the title GetMessage returns a bool when it should return an int GetMessage returns a bool when it should return an BOOL Aug 17, 2021
@AArnott AArnott changed the title GetMessage returns a bool when it should return an BOOL GetMessage returns a bool when it should return a BOOL Aug 17, 2021
@AArnott AArnott transferred this issue from microsoft/win32metadata Aug 17, 2021
@AArnott AArnott added the bug Something isn't working label Aug 17, 2021
AArnott added a commit that referenced this issue Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants