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

syscall: add DLLError.Unwrap() method [freeze exception] #42584

Closed
zx2c4 opened this issue Nov 13, 2020 · 10 comments
Closed

syscall: add DLLError.Unwrap() method [freeze exception] #42584

zx2c4 opened this issue Nov 13, 2020 · 10 comments

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Nov 13, 2020

These older structs embed a wrapped error. But because they're old, they haven't defined an Unwrap() method. We should do this, so that users can check for issues such as if errors.Is(err, windows.ERROR_PROC_NOT_FOUND) { ... }, which is becoming more important as Microsoft keeps adding APIs.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/269761 mentions this issue: syscall: add DLLError.Unwrap function

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/269937 mentions this issue: windows: add DLLError.Unwrap function

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 13, 2020

cc @alexbrainman

@cagedmantis cagedmantis changed the title syscall.DLLError/windows.DLLError: needs Unwrap() method syscall: DLLError/windows.DLLError: needs Unwrap() method Nov 13, 2020
@cagedmantis cagedmantis added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows labels Nov 13, 2020
@cagedmantis cagedmantis added this to the Backlog milestone Nov 13, 2020
gopherbot pushed a commit to golang/sys that referenced this issue Nov 13, 2020
Because we're expecting for future functions to be unavailable, we
should add an Unwrap() function to the DLLError struct, so that people
can test for this situation easily via:

    if errors.Is(err, windows.ERROR_PROC_NOT_FOUND) { ... }

DLLError already was wrapping the underlying Errno error, but never got
the Go 1.13 helper method.

Update golang/go#42584

Change-Id: Ib916ddd55a2de29f988edaaf82f2ae0ce1b18e3b
Reviewed-on: https://go-review.googlesource.com/c/sys/+/269937
Trust: Jason A. Donenfeld <[email protected]>
Run-TryBot: Jason A. Donenfeld <[email protected]>
TryBot-Result: Go Bot <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
@zx2c4 zx2c4 changed the title syscall: DLLError/windows.DLLError: needs Unwrap() method syscall: DLLError: needs Unwrap() method Nov 13, 2020
@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 13, 2020

Changing the title, as things are now handled with x/sys/windows.

@alexbrainman alexbrainman changed the title syscall: DLLError: needs Unwrap() method proposal: syscall: add DLLError.Unwrap() method Nov 14, 2020
@alexbrainman alexbrainman modified the milestones: Backlog, Proposal Nov 14, 2020
@alexbrainman alexbrainman removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 14, 2020
@alexbrainman
Copy link
Member

@zx2c4 thank you for creating this issue.

I adjusted your issue title so it looks like proposal

https://github.com/golang/proposal#the-proposal-process

I also fiddled with the labels. Hopefully I did correct thing.

Now we wait for proposal decision. Even if this proposal is accepted, I doubt we will be able to implement this until Go 1.16 is released.

Alex

@rsc
Copy link
Contributor

rsc commented Nov 18, 2020

This seems OK and it would be better to be consistent with x/sys.
It is also consistent with our adding Unwrap to every error implementation that wraps another error.
I'm not sure how we missed this one to start with.

Looking in the API files, I see these remaining without Unwrap methods:

go1.txt:230:pkg compress/flate, type ReadError struct, Err error
go1.txt:236:pkg compress/flate, type WriteError struct, Err error

And those are marked "Deprecated, no longer returned".
So somehow we missed only DLLError (and maybe x509.SystemRootsError, which was recently fixed).

Let's fix it.

@rsc
Copy link
Contributor

rsc commented Nov 18, 2020

Because it is trivial and consistent with everything else, we can just approve this. Accepted.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Nov 18, 2020

Great. I'll submit https://go-review.googlesource.com/c/go/+/269761/ whenever you want to +2 it.

@rsc rsc changed the title proposal: syscall: add DLLError.Unwrap() method syscall: add DLLError.Unwrap() method Nov 19, 2020
@rsc rsc modified the milestones: Proposal, Backlog Nov 19, 2020
@alexbrainman
Copy link
Member

alexbrainman commented Nov 20, 2020

@rsc @golang/release

There is https://go-review.googlesource.com/c/go/+/269761/ that implements this change. The CL was submitted after Go 1.16 freeze started. I propose we submit the CL now for Go 1.16.

The change is trivial.

And according to @rsc #42584 (comment) the change was somehow missed. Let's fix this problem in Go 1.16 rather than delaying it for Go 1.17.

What do you think?

Alex

@dmitshur
Copy link
Contributor

@alexbrainman Thanks for making this freeze exception request and providing a rationale.

I am approving this for Go 1.16. The change is trivial and should not add significant delay or increase risk for the release. (It's also easy to revert if we learn of serious problems that cannot be resolved in time.)

@dmitshur dmitshur modified the milestones: Backlog, Go1.16 Nov 20, 2020
@dmitshur dmitshur changed the title syscall: add DLLError.Unwrap() method syscall: add DLLError.Unwrap() method [freeze exception] Nov 20, 2020
@golang golang locked and limited conversation to collaborators Nov 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants