Turn SystemError.from_errno into a macro#15874
Merged
straight-shoota merged 6 commits intomasterfrom Jun 10, 2025
Merged
Conversation
Contributor
|
The actual method that resolves #15496 is |
HertzDevil
reviewed
Jun 4, 2025
ysbaddaden
approved these changes
Jun 5, 2025
This ensures we capture `Errno.value` right away and it cannot be influenced by other code producing the arguments.
Co-authored-by: Quinton Miller <nicetas.c@gmail.com>
e7a4962 to
bfe0758
Compare
bfe0758 to
e48f1d1
Compare
ysbaddaden
reviewed
Jun 6, 2025
src/system_error.cr
Outdated
| # The system message corresponding to the OS error value amends the *message*. | ||
| # Additional keyword arguments are forwarded to the exception initializer `.new_from_os_error`. | ||
| macro from_errno(message, **opts) | ||
| # This is a macro in order to retrieve `Errno.value` first before evaluating `message` and `opts`. |
Collaborator
There was a problem hiding this comment.
Polish: maybe move this comment outside of the macro, and be more general:
The following are macros so we can get the errno or winerror before
evaluating message and opts that may change the errno or winerror value.
ysbaddaden
approved these changes
Jun 6, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SystemError.from_errnocallsErrno.valueto retrieve the system error value from the last lib call.But this happens in the body of the method which only executes after its arguments are evaluated (for example building the error message). This can lead to a change in
Errno.valuewhich.from_errnowould then mistake for the original error.In order to fix this, we must query
Errno.valueimmediately after the lib call.Changing
.from_errnointo a macro achieves that because it expands into a call toErrno.valuebefore constructing the actual error object and its arguments.We need to change the deprecated
from_errnomethod which receives anErrnovalue as well because the macro would shadow the method.One downside of the macro approach is that it won't work with file-private error types (#15496 (comment)). But this is probably not a very relevant use case.
If this is accepted, I'll do the same change to
SystemError.from_winerrorandSystemError.from_wsa_error.Resolves #15496