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

Jump to bottom

proposal: x/sys/windows: let HRESULT implement the error interface #57278

Open
hajimehoshi opened this issue Dec 13, 2022 · 2 comments
Open

proposal: x/sys/windows: let HRESULT implement the error interface #57278

hajimehoshi opened this issue Dec 13, 2022 · 2 comments
Labels
OS-Windows Proposal
Milestone
Proposal

Comments

@hajimehoshi
Copy link
Member

hajimehoshi commented Dec 13, 2022 •
edited

Now error values from APIs in the x/sys/windows package are syscall.Errno, even though the original API returns HRESULT value. For example, CoInitializeEx originally returns HRESULT, but windows.CoInitializeEx wraps the HRESULT value as syscall.Errno and returns it. This package doesn't distinguish Errno and HRESULT as error values, and this seems odd in terms of semantics. My understanding is Errno and HRESULT are different things. There might be a historical reason why this package does so, but in this case I'd like to know.

My proposal is to let HRESULT implement the error interface and let the APIs return it (when the value is not S_OK) instead of wrapping as Errno. This simple implementation would be enough:

func (h HRESULT) Error() string {
    return fmt.Sprintf("HRESULT(%08x)", h)
}

This is a breaking change and existing error checkings might no longer work, after the APIs return HRESULT instead of Errno. However, as x/sys/windows is still v0, a breaking change should be acceptable.

The controversial point is that there are some non-zero values which can be used in a successful case (e.g. S_FALSE for CoInitializeEx). In this case, implementing error might be odd. IMO my proposal is not a perfect solution but should be better than wrapping HRESULT as Errno without conditions, and improve the current semantics confusion.
@gopherbot gopherbot added this to the Proposal milestone Dec 13, 2022
@ianlancetaylor
Copy link
Contributor

CC @golang/windows

@apparentlymart
Copy link

There seems to be some general disagreement in this package about how it represents the various conventions for representing errors in different parts of the Windows API. 🤔

The script which generates all of the constants representing different errors seems to generate syscall.Errno constants for the System Error Codes, like ERROR_FILE_NOT_FOUND (syscall.Errno(2)).

That script uses Handle to represent anything that is typed as HRESULT in the Windows platform headers, such as E_ABORT (Handle(0x80004004)). That seems to disagree with the decision made in the wrapping stubs for functions that return HRESULT, such as CoInitializeEx returning syscall.Errno as discussed in the initial issue writeup. Presumably this means that the results of these functions will not compare using == with the associated constants, because the types won't match. 😬

I don't see the "errno constants" in there at all, but I think that's because they are part of the C runtime library rather than part of the platform headers, and so they aren't visible to or relevant to x/sys/windows (which isn't exposing the C runtime API at all) although apparently (per #23468) CGo on Windows is using syscall.Errno for these ones too.

x/sys/windows does use syscall.Errno to represent the Winsock equivalents of those error states, such as WSAENAMETOOLONG (syscall.Errno(10063)).

It seems like there was some discussion about the types used by this generator script in https://go-review.googlesource.com/c/sys/+/170918 .

I'd guess most or all of these inconsistencies are unfixable without breaking existing callers. 😖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
OS-Windows Proposal
Projects
Proposals
Status: Incoming
Milestone
Proposal
Development

No branches or pull requests
4 participants
@hajimehoshi @apparentlymart @ianlancetaylor @gopherbot