Rollup merge of rust-lang#126194 - ChrisDenton:winerror, r=Mark-Simulacrum

jieyouxu · web-flow · commit c5da756bc2ab · 2024-06-09T20:54:24.000+01:00
Migrate more things to `WinError`

In `library/std/src/sys/pal/windows`, we prefer to use the `WinError` type in place of using either `io::Result` or `unsafe { GetLastError }`. The latter is unsafe and weakly typed whereas the former is larger and requires unwrapping because it returns an `Option`.

I also fixed up a couple of places where we were unnecessarily defining error constants that are already defined.
diff --git a/std/src/sys/pal/windows/api.rs b/std/src/sys/pal/windows/api.rs
@@ -251,3 +251,39 @@ pub fn get_last_error() -> WinError {
 pub struct WinError {
     pub code: u32,
 }
+impl WinError {
+    const fn new(code: u32) -> Self {
+        Self { code }
+    }
+}
+
+// Error code constants.
+// The constant names should be the same as the winapi constants except for the leading `ERROR_`.
+// Due to the sheer number of codes, error codes should only be added here on an as-needed basis.
+// However, they should never be removed as the assumption is they may be useful again in the future.
+#[allow(unused)]
+impl WinError {
+    /// Success is not an error.
+    /// Some Windows APIs do use this to distinguish between a zero return and an error return
+    /// but we should never return this to users as an error.
+    pub const SUCCESS: Self = Self::new(c::ERROR_SUCCESS);
+    // tidy-alphabetical-start
+    pub const ACCESS_DENIED: Self = Self::new(c::ERROR_ACCESS_DENIED);
+    pub const ALREADY_EXISTS: Self = Self::new(c::ERROR_ALREADY_EXISTS);
+    pub const CANT_ACCESS_FILE: Self = Self::new(c::ERROR_CANT_ACCESS_FILE);
+    pub const DELETE_PENDING: Self = Self::new(c::ERROR_DELETE_PENDING);
+    pub const DIRECTORY: Self = Self::new(c::ERROR_DIRECTORY);
+    pub const FILE_NOT_FOUND: Self = Self::new(c::ERROR_FILE_NOT_FOUND);
+    pub const INSUFFICIENT_BUFFER: Self = Self::new(c::ERROR_INSUFFICIENT_BUFFER);
+    pub const INVALID_FUNCTION: Self = Self::new(c::ERROR_INVALID_FUNCTION);
+    pub const INVALID_HANDLE: Self = Self::new(c::ERROR_INVALID_HANDLE);
+    pub const INVALID_PARAMETER: Self = Self::new(c::ERROR_INVALID_PARAMETER);
+    pub const NO_MORE_FILES: Self = Self::new(c::ERROR_NO_MORE_FILES);
+    pub const NOT_FOUND: Self = Self::new(c::ERROR_NOT_FOUND);
+    pub const NOT_SUPPORTED: Self = Self::new(c::ERROR_NOT_SUPPORTED);
+    pub const OPERATION_ABORTED: Self = Self::new(c::ERROR_OPERATION_ABORTED);
+    pub const PATH_NOT_FOUND: Self = Self::new(c::ERROR_PATH_NOT_FOUND);
+    pub const SHARING_VIOLATION: Self = Self::new(c::ERROR_SHARING_VIOLATION);
+    pub const TIMEOUT: Self = Self::new(c::ERROR_TIMEOUT);
+    // tidy-alphabetical-end
+}
diff --git a/std/src/sys/pal/windows/fs.rs b/std/src/sys/pal/windows/fs.rs
@@ -18,7 +18,8 @@ use crate::sys::{c, cvt, Align8};
 use crate::sys_common::{AsInner, FromInner, IntoInner};
 use crate::thread;
 
-use super::{api, to_u16s, IoResult};
+use super::api::{self, WinError};
+use super::{to_u16s, IoResult};
 use crate::sys::path::maybe_verbatim;
 
 pub struct File {
@@ -130,10 +131,11 @@ impl Iterator for ReadDir {
             let mut wfd = mem::zeroed();
             loop {
                 if c::FindNextFileW(self.handle.0, &mut wfd) == 0 {
-                    if api::get_last_error().code == c::ERROR_NO_MORE_FILES {
-                        return None;
-                    } else {
-                        return Some(Err(Error::last_os_error()));
+                    match api::get_last_error() {
+                        WinError::NO_MORE_FILES => return None,
+                        WinError { code } => {
+                            return Some(Err(Error::from_raw_os_error(code as i32)));
+                        }
                     }
                 }
                 if let Some(e) = DirEntry::new(&self.root, &wfd) {
@@ -244,8 +246,6 @@ impl OpenOptions {
     }
 
     fn get_access_mode(&self) -> io::Result<c::DWORD> {
-        const ERROR_INVALID_PARAMETER: i32 = 87;
-
         match (self.read, self.write, self.append, self.access_mode) {
             (.., Some(mode)) => Ok(mode),
             (true, false, false, None) => Ok(c::GENERIC_READ),
@@ -255,23 +255,23 @@ impl OpenOptions {
             (true, _, true, None) => {
                 Ok(c::GENERIC_READ | (c::FILE_GENERIC_WRITE & !c::FILE_WRITE_DATA))
             }
-            (false, false, false, None) => Err(Error::from_raw_os_error(ERROR_INVALID_PARAMETER)),
+            (false, false, false, None) => {
+                Err(Error::from_raw_os_error(c::ERROR_INVALID_PARAMETER as i32))
+            }
         }
     }
 
     fn get_creation_mode(&self) -> io::Result<c::DWORD> {
-        const ERROR_INVALID_PARAMETER: i32 = 87;
-
         match (self.write, self.append) {
             (true, false) => {}
             (false, false) => {
                 if self.truncate || self.create || self.create_new {
-                    return Err(Error::from_raw_os_error(ERROR_INVALID_PARAMETER));
+                    return Err(Error::from_raw_os_error(c::ERROR_INVALID_PARAMETER as i32));
                 }
             }
             (_, true) => {
                 if self.truncate && !self.create_new {
-                    return Err(Error::from_raw_os_error(ERROR_INVALID_PARAMETER));
+                    return Err(Error::from_raw_os_error(c::ERROR_INVALID_PARAMETER as i32));
                 }
             }
         }
@@ -315,7 +315,7 @@ impl File {
             // Manual truncation. See #115745.
             if opts.truncate
                 && creation == c::OPEN_ALWAYS
-                && unsafe { c::GetLastError() } == c::ERROR_ALREADY_EXISTS
+                && api::get_last_error() == WinError::ALREADY_EXISTS
             {
                 unsafe {
                     // This originally used `FileAllocationInfo` instead of
@@ -845,7 +845,7 @@ fn open_link_no_reparse(parent: &File, name: &[u16], access: u32) -> io::Result<
             // We make a special exception for `STATUS_DELETE_PENDING` because
             // otherwise this will be mapped to `ERROR_ACCESS_DENIED` which is
             // very unhelpful.
-            Err(io::Error::from_raw_os_error(c::ERROR_DELETE_PENDING as _))
+            Err(io::Error::from_raw_os_error(c::ERROR_DELETE_PENDING as i32))
         } else if status == c::STATUS_INVALID_PARAMETER
             && ATTRIBUTES.load(Ordering::Relaxed) == c::OBJ_DONT_REPARSE
         {
@@ -1097,7 +1097,7 @@ pub fn readdir(p: &Path) -> io::Result<ReadDir> {
             //
             // See issue #120040: https://github.com/rust-lang/rust/issues/120040.
             let last_error = api::get_last_error();
-            if last_error.code == c::ERROR_FILE_NOT_FOUND {
+            if last_error == WinError::FILE_NOT_FOUND {
                 return Ok(ReadDir {
                     handle: FindNextFileHandle(find_handle),
                     root: Arc::new(root),
diff --git a/std/src/sys/pal/windows/futex.rs b/std/src/sys/pal/windows/futex.rs
@@ -1,4 +1,4 @@
-use super::api;
+use super::api::{self, WinError};
 use crate::sys::c;
 use crate::sys::dur2timeout;
 use core::ffi::c_void;
@@ -72,7 +72,7 @@ pub fn wake_by_address_all<T>(address: &T) {
 
 pub fn futex_wait<W: Waitable>(futex: &W::Atomic, expected: W, timeout: Option<Duration>) -> bool {
     // return false only on timeout
-    wait_on_address(futex, expected, timeout) || api::get_last_error().code != c::ERROR_TIMEOUT
+    wait_on_address(futex, expected, timeout) || api::get_last_error() != WinError::TIMEOUT
 }
 
 pub fn futex_wake<T>(futex: &T) -> bool {
diff --git a/std/src/sys/pal/windows/os.rs b/std/src/sys/pal/windows/os.rs
@@ -17,7 +17,8 @@ use crate::ptr;
 use crate::slice;
 use crate::sys::{c, cvt};
 
-use super::{api, to_u16s};
+use super::api::{self, WinError};
+use super::to_u16s;
 
 pub fn errno() -> i32 {
     api::get_last_error().code as i32
@@ -333,7 +334,7 @@ fn home_dir_crt() -> Option<PathBuf> {
                     buf,
                     &mut sz,
                 ) {
-                    0 if api::get_last_error().code != c::ERROR_INSUFFICIENT_BUFFER => 0,
+                    0 if api::get_last_error() != WinError::INSUFFICIENT_BUFFER => 0,
                     0 => sz,
                     _ => sz - 1, // sz includes the null terminator
                 }
@@ -358,7 +359,7 @@ fn home_dir_crt() -> Option<PathBuf> {
         super::fill_utf16_buf(
             |buf, mut sz| {
                 match c::GetUserProfileDirectoryW(token, buf, &mut sz) {
-                    0 if api::get_last_error().code != c::ERROR_INSUFFICIENT_BUFFER => 0,
+                    0 if api::get_last_error() != WinError::INSUFFICIENT_BUFFER => 0,
                     0 => sz,
                     _ => sz - 1, // sz includes the null terminator
                 }
diff --git a/std/src/sys/pal/windows/pipe.rs b/std/src/sys/pal/windows/pipe.rs
@@ -12,6 +12,7 @@ use crate::sys::c;
 use crate::sys::fs::{File, OpenOptions};
 use crate::sys::handle::Handle;
 use crate::sys::hashmap_random_keys;
+use crate::sys::pal::windows::api::{self, WinError};
 use crate::sys_common::{FromInner, IntoInner};
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -124,20 +125,19 @@ pub fn anon_pipe(ours_readable: bool, their_handle_inheritable: bool) -> io::Res
             // testing strategy
             // For more info, see https://github.com/rust-lang/rust/pull/37677.
             if handle == c::INVALID_HANDLE_VALUE {
-                let err = io::Error::last_os_error();
-                let raw_os_err = err.raw_os_error();
+                let error = api::get_last_error();
                 if tries < 10 {
-                    if raw_os_err == Some(c::ERROR_ACCESS_DENIED as i32) {
+                    if error == WinError::ACCESS_DENIED {
                         continue;
                     } else if reject_remote_clients_flag != 0
-                        && raw_os_err == Some(c::ERROR_INVALID_PARAMETER as i32)
+                        && error == WinError::INVALID_PARAMETER
                     {
                         reject_remote_clients_flag = 0;
                         tries -= 1;
                         continue;
                     }
                 }
-                return Err(err);
+                return Err(io::Error::from_raw_os_error(error.code as i32));
             }
             ours = Handle::from_raw_handle(handle);
             break;
diff --git a/std/src/sys/pal/windows/process.rs b/std/src/sys/pal/windows/process.rs
@@ -31,6 +31,8 @@ use crate::sys_common::IntoInner;
 
 use core::ffi::c_void;
 
+use super::api::{self, WinError};
+
 ////////////////////////////////////////////////////////////////////////////////
 // Command
 ////////////////////////////////////////////////////////////////////////////////
@@ -645,12 +647,12 @@ impl Process {
     pub fn kill(&mut self) -> io::Result<()> {
         let result = unsafe { c::TerminateProcess(self.handle.as_raw_handle(), 1) };
         if result == c::FALSE {
-            let error = unsafe { c::GetLastError() };
+            let error = api::get_last_error();
             // TerminateProcess returns ERROR_ACCESS_DENIED if the process has already been
             // terminated (by us, or for any other reason). So check if the process was actually
             // terminated, and if so, do not return an error.
-            if error != c::ERROR_ACCESS_DENIED || self.try_wait().is_err() {
-                return Err(crate::io::Error::from_raw_os_error(error as i32));
+            if error != WinError::ACCESS_DENIED || self.try_wait().is_err() {
+                return Err(crate::io::Error::from_raw_os_error(error.code as i32));
             }
         }
         Ok(())
diff --git a/std/src/sys/pal/windows/stdio.rs b/std/src/sys/pal/windows/stdio.rs
@@ -1,6 +1,6 @@
 #![unstable(issue = "none", feature = "windows_stdio")]
 
-use super::api;
+use super::api::{self, WinError};
 use crate::cmp;
 use crate::io;
 use crate::mem::MaybeUninit;
@@ -370,7 +370,7 @@ fn read_u16s(handle: c::HANDLE, buf: &mut [MaybeUninit<u16>]) -> io::Result<usiz
 
         // ReadConsoleW returns success with ERROR_OPERATION_ABORTED for Ctrl-C or Ctrl-Break.
         // Explicitly check for that case here and try again.
-        if amount == 0 && api::get_last_error().code == c::ERROR_OPERATION_ABORTED {
+        if amount == 0 && api::get_last_error() == WinError::OPERATION_ABORTED {
             continue;
         }
         break;
