Get rid of SysError and WinError derived classes

All we need is `SystemError`, and the various ways to construct it can
be done with static methods that are more informative. Catching the
derived classes was a footgun that is now impossible, because one can
only has `SystemError` to catch.

I did however make `SysError` and `WinError` top-level function
wrappers in order to avoid churn in the vast majority of call sites.

src/libstore/windows/pathlocks.cc

@@ -56,26 +56,24 @@ bool lockFile(Descriptor desc, LockType lockType, bool wait)
     switch (lockType) {
     case ltNone: {
         OVERLAPPED ov = {0};
-        if (!UnlockFileEx(desc, 0, 2, 0, &ov)) {
-            WinError winError("Failed to unlock file desc %s", desc);
-            throw winError;
-        }
+        if (!UnlockFileEx(desc, 0, 2, 0, &ov))
+            throw WinError("Failed to unlock file desc %s", desc);
         return true;
     }
     case ltRead: {
         OVERLAPPED ov = {0};
         if (!LockFileEx(desc, wait ? 0 : LOCKFILE_FAIL_IMMEDIATELY, 0, 1, 0, &ov)) {
-            WinError winError("Failed to lock file desc %s", desc);
-            if (winError.lastError == ERROR_LOCK_VIOLATION)
+            auto lastError = GetLastError();
+            if (lastError == ERROR_LOCK_VIOLATION)
                 return false;
-            throw winError;
+            throw WinError(lastError, "Failed to lock file desc %s", desc);
         }
 
         ov.Offset = 1;
         if (!UnlockFileEx(desc, 0, 1, 0, &ov)) {
-            WinError winError("Failed to unlock file desc %s", desc);
-            if (winError.lastError != ERROR_NOT_LOCKED)
-                throw winError;
+            auto lastError = GetLastError();
+            if (lastError != ERROR_NOT_LOCKED)
+                throw WinError(lastError, "Failed to unlock file desc %s", desc);
         }
         return true;
     }
@@ -83,17 +81,17 @@ bool lockFile(Descriptor desc, LockType lockType, bool wait)
         OVERLAPPED ov = {0};
         ov.Offset = 1;
         if (!LockFileEx(desc, LOCKFILE_EXCLUSIVE_LOCK | (wait ? 0 : LOCKFILE_FAIL_IMMEDIATELY), 0, 1, 0, &ov)) {
-            WinError winError("Failed to lock file desc %s", desc);
-            if (winError.lastError == ERROR_LOCK_VIOLATION)
+            auto lastError = GetLastError();
+            if (lastError == ERROR_LOCK_VIOLATION)
                 return false;
-            throw winError;
+            throw WinError(lastError, "Failed to lock file desc %s", desc);
         }
 
         ov.Offset = 0;
         if (!UnlockFileEx(desc, 0, 1, 0, &ov)) {
-            WinError winError("Failed to unlock file desc %s", desc);
-            if (winError.lastError != ERROR_NOT_LOCKED)
-                throw winError;
+            auto lastError = GetLastError();
+            if (lastError != ERROR_NOT_LOCKED)
+                throw WinError(lastError, "Failed to unlock file desc %s", desc);
         }
         return true;
     }

src/libutil-tests/unix/file-system-at.cc

@@ -47,16 +47,16 @@ TEST(fchmodatTryNoFollow, works)
     /* Check that symlinks are not followed and targets are not changed. */
 
     EXPECT_NO_THROW(
-        try { fchmodatTryNoFollow(dirFd.get(), CanonPath("filelink"), 0777); } catch (SysError & e) {
-            if (e.errNo != EOPNOTSUPP)
+        try { fchmodatTryNoFollow(dirFd.get(), CanonPath("filelink"), 0777); } catch (SystemError & e) {
+            if (!e.is(std::errc::operation_not_supported))
                 throw;
         });
     ASSERT_EQ(stat((tmpDir / "file").c_str(), &st), 0);
     EXPECT_EQ(st.st_mode & 0777, 0644);
 
     EXPECT_NO_THROW(
-        try { fchmodatTryNoFollow(dirFd.get(), CanonPath("dirlink"), 0777); } catch (SysError & e) {
-            if (e.errNo != EOPNOTSUPP)
+        try { fchmodatTryNoFollow(dirFd.get(), CanonPath("dirlink"), 0777); } catch (SystemError & e) {
+            if (!e.is(std::errc::operation_not_supported))
                 throw;
         });
     ASSERT_EQ(stat((tmpDir / "dir").c_str(), &st), 0);
@@ -110,14 +110,14 @@ TEST(fchmodatTryNoFollow, fallbackWithoutProc)
 
             try {
                 fchmodatTryNoFollow(dirFd.get(), CanonPath("file"), 0600);
-            } catch (SysError & e) {
+            } catch (SystemError & e) {
                 _exit(1);
             }
 
             try {
                 fchmodatTryNoFollow(dirFd.get(), CanonPath("link"), 0777);
-            } catch (SysError & e) {
-                if (e.errNo == EOPNOTSUPP)
+            } catch (SystemError & e) {
+                if (e.is(std::errc::operation_not_supported))
                     _exit(0); /* Success. */
             }
 

src/libutil/include/nix/util/error.hh

@@ -250,18 +250,15 @@ class SystemError : public Error
     std::error_code errorCode;
     std::string errorDetails;
 
-protected:
-
     /**
-     * Just here to allow derived classes to use the right constructor
-     * (the protected one).
+     * Just here to allow the static methods to use the right constructor
+     * (the private one).
      */
     struct Disambig
     {};
 
     /**
-     * Protected constructor for subclasses that provide their own error message.
-     * The error message is appended to the formatted hint.
+     * Private constructor with explicit error message string.
      */
     template<typename... Args>
     SystemError(Disambig, std::error_code errorCode, std::string_view errorDetails, Args &&... args)
@@ -284,6 +281,79 @@ public:
     {
     }
 
+    /**
+     * Construct a POSIX error using the explicitly-provided error number.
+     * `strerror` will be used to try to add additional information to the message.
+     */
+    template<typename... Args>
+    static SystemError fromPosixExplicit(int errNo, Args &&... args)
+    {
+        return SystemError(
+            Disambig{},
+            std::make_error_code(static_cast<std::errc>(errNo)),
+            strerror(errNo),
+            std::forward<Args>(args)...);
+    }
+
+    /**
+     * Construct a POSIX error using the ambient `errno`.
+     *
+     * Be sure to not perform another `errno`-modifying operation before
+     * calling this!
+     */
+    template<typename... Args>
+    static SystemError fromPosix(Args &&... args)
+    {
+        return fromPosixExplicit(errno, std::forward<Args>(args)...);
+    }
+
+#ifdef _WIN32
+    /**
+     * Construct a Windows error using the explicitly-provided error number.
+     * `FormatMessageA` will be used to try to add additional information
+     * to the message.
+     */
+    template<typename... Args>
+    static SystemError fromWindowsExplicit(DWORD lastError, Args &&... args)
+    {
+        return SystemError(
+            Disambig{},
+            std::error_code(lastError, std::system_category()),
+            renderWindowsError(lastError),
+            std::forward<Args>(args)...);
+    }
+
+    /**
+     * Construct a Windows error using `GetLastError()`.
+     *
+     * Be sure to not perform another last-error-modifying operation
+     * before calling this!
+     */
+    template<typename... Args>
+    static SystemError fromWindows(Args &&... args)
+    {
+        return fromWindowsExplicit(GetLastError(), std::forward<Args>(args)...);
+    }
+
+private:
+    static std::string renderWindowsError(DWORD lastError);
+
+public:
+#endif
+
+    /**
+     * Construct using the native error (errno on POSIX, GetLastError() on Windows).
+     */
+    template<typename... Args>
+    static SystemError fromNative(Args &&... args)
+    {
+#ifdef _WIN32
+        return fromWindows(std::forward<Args>(args)...);
+#else
+        return fromPosix(std::forward<Args>(args)...);
+#endif
+    }
+
     const std::error_code ec() const &
     {
         return errorCode;
@@ -296,53 +366,58 @@ public:
 };
 
 /**
- * POSIX system error, created using `errno`, `strerror` friends.
- *
- * Throw this, but prefer not to catch this, and catch `SystemError`
- * instead. This allows implementations to freely switch between this
- * and `windows::WinError` without breaking catch blocks.
- *
- * However, it is permissible to catch this and rethrow so long as
- * certain conditions are not met (e.g. to catch only if `errNo =
- * EFooBar`). In that case, try to also catch the equivalent `windows::WinError`
- * code.
- *
- * @todo Rename this to `PosixError` or similar. At this point Windows
- * support is too WIP to justify the code churn, but if it is finished
- * then a better identifier becomes moe worth it.
+ * Wrapper to avoid churn
  */
-class SysError : public SystemError
+template<typename... Args>
+SystemError SysError(int errNo, Args &&... args)
 {
-public:
-    int errNo;
+    return SystemError::fromPosixExplicit(errNo, std::forward<Args>(args)...);
+}
 
-    /**
-     * Construct using the explicitly-provided error number. `strerror`
-     * will be used to try to add additional information to the message.
-     */
-    template<typename... Args>
-    SysError(int errNo, Args &&... args)
-        : SystemError(
-              Disambig{},
-              std::make_error_code(static_cast<std::errc>(errNo)),
-              strerror(errNo),
-              std::forward<Args>(args)...)
-        , errNo(errNo)
-    {
-    }
+/**
+ * Wrapper to avoid churn
+ */
+template<typename... Args>
+SystemError SysError(Args &&... args)
+{
+    return SystemError::fromPosix(std::forward<Args>(args)...);
+}
 
-    /**
-     * Construct using the ambient `errno`.
-     *
-     * Be sure to not perform another `errno`-modifying operation before
-     * calling this constructor!
-     */
-    template<typename... Args>
-    SysError(Args &&... args)
-        : SysError(errno, std::forward<Args>(args)...)
-    {
-    }
-};
+#ifdef _WIN32
+
+namespace windows {
+
+/**
+ * Wrapper to avoid churn
+ */
+template<typename... Args>
+SystemError WinError(DWORD lastError, Args &&... args)
+{
+    return SystemError::fromWindowsExplicit(lastError, std::forward<Args>(args)...);
+}
+
+/**
+ * Wrapper to avoid churn
+ */
+template<typename... Args>
+SystemError WinError(Args &&... args)
+{
+    return SystemError::fromWindows(std::forward<Args>(args)...);
+}
+
+} // namespace windows
+
+#endif
+
+/**
+ * Convenience wrapper for when we use `errno`-based error handling
+ * on Unix, and `GetLastError()`-based error handling on Windows.
+ */
+template<typename... Args>
+SystemError NativeSysError(Args &&... args)
+{
+    return SystemError::fromNative(std::forward<Args>(args)...);
+}
 
 /**
  * Throw an exception for the purpose of checking that exception
@@ -382,69 +457,4 @@ int handleExceptions(const std::string & programName, std::function<void()> fun)
 #  define nixUnreachableWhenHardened std::unreachable
 #endif
 
-#ifdef _WIN32
-
-namespace windows {
-
-/**
- * Windows Error type.
- *
- * Unless you need to catch a specific error number, don't catch this in
- * portable code. Catch `SystemError` instead.
- */
-class WinError : public SystemError
-{
-public:
-    DWORD lastError;
-
-    /**
-     * Construct using the explicitly-provided error number.
-     * `FormatMessageA` will be used to try to add additional
-     * information to the message.
-     */
-    template<typename... Args>
-    WinError(DWORD lastError, Args &&... args)
-        : SystemError(
-              Disambig{},
-              std::error_code(lastError, std::system_category()),
-              renderError(lastError),
-              std::forward<Args>(args)...)
-        , lastError(lastError)
-    {
-    }
-
-    /**
-     * Construct using `GetLastError()` and the ambient "last error".
-     *
-     * Be sure to not perform another last-error-modifying operation
-     * before calling this constructor!
-     */
-    template<typename... Args>
-    WinError(Args &&... args)
-        : WinError(GetLastError(), std::forward<Args>(args)...)
-    {
-    }
-
-private:
-
-    static std::string renderError(DWORD lastError);
-};
-
-} // namespace windows
-
-#endif
-
-/**
- * Convenience alias for when we use a `errno`-based error handling
- * function on Unix, and `GetLastError()`-based error handling on on
- * Windows.
- */
-using NativeSysError =
-#ifdef _WIN32
-    windows::WinError
-#else
-    SysError
-#endif
-    ;
-
 } // namespace nix

src/libutil/include/nix/util/file-system-at.hh

@@ -97,7 +97,7 @@ namespace unix {
  * AT_SYMLINK_NOFOLLOW, since it's the best we can do without failing.
  *
  * @pre path.isRoot() is false
- * @throws SysError if any operation fails
+ * @throws SystemError if any operation fails
  */
 void fchmodatTryNoFollow(Descriptor dirFd, const CanonPath & path, mode_t mode);
 

src/libutil/include/nix/util/file-system.hh

@@ -199,7 +199,7 @@ std::filesystem::path readLink(const std::filesystem::path & path);
  *
  * @note this function will clobber `errno` (Unix) / "last error"
  * (Windows), so care must be used to get those error codes, then call
- * this, then build a `SysError` / `WinError` with the saved error code.
+ * this, then build a `SystemError` with the saved error code.
  */
 std::filesystem::path descriptorToPath(Descriptor fd);
 

src/libutil/linux/linux-namespaces.cc

@@ -40,7 +40,7 @@ bool userNamespacesSupported()
 
             auto r = pid.wait();
             assert(!r);
-        } catch (SysError & e) {
+        } catch (SystemError & e) {
             debug("user namespaces do not work on this system: %s", e.msg());
             return false;
         }
@@ -77,7 +77,7 @@ bool mountAndPidNamespacesSupported()
                 return false;
             }
 
-        } catch (SysError & e) {
+        } catch (SystemError & e) {
             debug("mount namespaces do not work on this system: %s", e.msg());
             return false;
         }

src/libutil/unix-domain-socket.cc

@@ -78,8 +78,8 @@ bindConnectProcHelper(std::string_view operationName, auto && operation, Socket
                 if (operation(fd, psaddr, sizeof(addr)) == -1)
                     throw SysError("cannot %s to socket at '%s'", operationName, path);
                 writeFull(pipe.writeSide.get(), "0\n");
-            } catch (SysError & e) {
-                writeFull(pipe.writeSide.get(), fmt("%d\n", e.errNo));
+            } catch (SystemError & e) {
+                writeFull(pipe.writeSide.get(), fmt("%d\n", e.ec().value()));
             } catch (...) {
                 writeFull(pipe.writeSide.get(), "-1\n");
             }

src/libutil/unix/file-descriptor.cc

@@ -188,7 +188,7 @@ void unix::closeExtraFDs()
             }
         }
         return;
-    } catch (SysError &) {
+    } catch (SystemError &) {
     }
 #endif
 

src/libutil/unix/file-system.cc

@@ -186,9 +186,9 @@ static void _deletePath(
         if ((st.st_mode & PERM_MASK) != PERM_MASK)
             try {
                 unix::fchmodatTryNoFollow(parentfd, CanonPath(name), st.st_mode | PERM_MASK);
-            } catch (SysError & e) {
+            } catch (SystemError & e) {
                 e.addTrace({}, "while making directory %1% accessible for deletion", PathFmt(path));
-                if (e.errNo == EOPNOTSUPP)
+                if (e.is(std::errc::operation_not_supported))
                     e.addTrace({}, "%1% is now a symlink, expected directory", PathFmt(path));
                 throw;
             }

src/libutil/unix/include/nix/util/monitor-fd.hh

@@ -60,7 +60,7 @@ inline void MonitorFdHup::runThread(int watchFd, int notifyFd)
 {
     int kqResult = kqueue();
     if (kqResult < 0) {
-        throw SysError("MonitorFdHup kqueue");
+        throw SystemError::fromPosix("MonitorFdHup kqueue");
     }
     AutoCloseFD kq{kqResult};
 
@@ -78,14 +78,14 @@ inline void MonitorFdHup::runThread(int watchFd, int notifyFd)
 
     int result = kevent(kq.get(), kevs.data(), kevs.size(), nullptr, 0, nullptr);
     if (result < 0) {
-        throw SysError("MonitorFdHup kevent add");
+        throw SystemError::fromPosix("MonitorFdHup kevent add");
     }
 
     while (true) {
         struct kevent event;
         int numEvents = kevent(kq.get(), nullptr, 0, &event, 1, nullptr);
         if (numEvents < 0) {
-            throw SysError("MonitorFdHup kevent watch");
+            throw SystemError::fromPosix("MonitorFdHup kevent watch");
         }
 
         if (numEvents > 0 && (event.flags & EV_EOF)) {
@@ -112,7 +112,7 @@ inline void MonitorFdHup::runThread(int watchFd, int notifyFd)
             if (errno == EINTR || errno == EAGAIN) {
                 continue;
             } else {
-                throw SysError("in MonitorFdHup poll()");
+                throw SystemError::fromPosix("in MonitorFdHup poll()");
             }
         }
 

src/libutil/unix/users.cc

@@ -39,7 +39,7 @@ std::filesystem::path getHome()
                 auto st = maybeStat(homeDir->c_str());
                 if (st && st->st_uid != geteuid())
                     unownedUserHomeDir.swap(homeDir);
-            } catch (SysError & e) {
+            } catch (SystemError & e) {
                 warn(
                     "couldn't stat $HOME ('%s') for reason other than not existing, falling back to the one defined in the 'passwd' file: %s",
                     *homeDir,

src/libutil/windows/current-process.cc

@@ -8,6 +8,8 @@
 
 namespace nix {
 
+using namespace nix::windows;
+
 std::chrono::microseconds getCpuUserTime()
 {
     FILETIME creationTime;
@@ -17,7 +19,7 @@ std::chrono::microseconds getCpuUserTime()
 
     if (!GetProcessTimes(GetCurrentProcess(), &creationTime, &exitTime, &kernelTime, &userTime)) {
         auto lastError = GetLastError();
-        throw windows::WinError(lastError, "failed to get CPU time");
+        throw WinError(lastError, "failed to get CPU time");
     }
 
     ULARGE_INTEGER uLargeInt;

src/libutil/windows/file-system-at.cc

@@ -13,8 +13,6 @@
 
 namespace nix {
 
-using namespace nix::windows;
-
 namespace windows {
 
 namespace {
@@ -210,6 +208,8 @@ bool isReparsePoint(HANDLE handle)
 
 } // namespace windows
 
+using namespace nix::windows;
+
 Descriptor openFileEnsureBeneathNoSymlinks(
     Descriptor dirFd, const CanonPath & path, ACCESS_MASK desiredAccess, ULONG createOptions, ULONG createDisposition)
 {
@@ -259,9 +259,10 @@ Descriptor openFileEnsureBeneathNoSymlinks(
                 FILE_TRAVERSE | SYNCHRONIZE,                  // Just need traversal rights
                 FILE_DIRECTORY_FILE | FILE_OPEN_REPARSE_POINT // Open directory, don't follow symlinks
             );
-        } catch (WinError & e) {
+        } catch (SystemError & e) {
             /* Check if this is because it's a symlink */
-            if (e.lastError == ERROR_CANT_ACCESS_FILE || e.lastError == ERROR_ACCESS_DENIED) {
+            auto err = e.ec().value();
+            if (err == ERROR_CANT_ACCESS_FILE || err == ERROR_ACCESS_DENIED) {
                 throwIfSymlink(wcomponent, pathUpTo(std::next(it)));
             }
             throw;
@@ -286,9 +287,9 @@ Descriptor openFileEnsureBeneathNoSymlinks(
             desiredAccess,
             createOptions | FILE_OPEN_REPARSE_POINT, // Don't follow symlinks on final component either
             createDisposition);
-    } catch (WinError & e) {
+    } catch (SystemError & e) {
         /* Check if final component is a symlink when we requested to not follow it */
-        if (e.lastError == ERROR_CANT_ACCESS_FILE) {
+        if (e.ec().value() == ERROR_CANT_ACCESS_FILE) {
             throwIfSymlink(finalComponent, path);
         }
         throw;

src/libutil/windows/known-folders.cc

@@ -8,8 +8,6 @@
 
 namespace nix::windows::known_folders {
 
-using namespace nix::windows;
-
 static std::filesystem::path getKnownFolder(REFKNOWNFOLDERID rfid)
 {
     PWSTR str = nullptr;

src/libutil/windows/muxable-pipe.cc

@@ -7,15 +7,17 @@
 
 namespace nix {
 
+using namespace nix::windows;
+
 void MuxablePipePollState::poll(HANDLE ioport, std::optional<unsigned int> timeout)
 {
     /* We are on at least Windows Vista / Server 2008 and can get many
        (countof(oentries)) statuses in one API call. */
     if (!GetQueuedCompletionStatusEx(
             ioport, oentries, sizeof(oentries) / sizeof(*oentries), &removed, timeout ? *timeout : INFINITE, false)) {
-        windows::WinError winError("GetQueuedCompletionStatusEx");
-        if (winError.lastError != WAIT_TIMEOUT)
-            throw winError;
+        auto lastError = GetLastError();
+        if (lastError != WAIT_TIMEOUT)
+            throw WinError(lastError, "GetQueuedCompletionStatusEx");
         assert(removed == 0);
     } else {
         assert(0 < removed && removed <= sizeof(oentries) / sizeof(*oentries));
@@ -52,12 +54,12 @@ void MuxablePipePollState::iterate(
                         // here is possible (but not obligatory) to call
                         // `handleRead` and repeat ReadFile immediately
                     } else {
-                        windows::WinError winError("ReadFile(%s, ..)", (*p)->readSide.get());
-                        if (winError.lastError == ERROR_BROKEN_PIPE) {
+                        auto lastError = GetLastError();
+                        if (lastError == ERROR_BROKEN_PIPE) {
                             handleEOF((*p)->readSide.get());
                             nextp = channels.erase(p); // no need to maintain `channels` ?
-                        } else if (winError.lastError != ERROR_IO_PENDING)
-                            throw winError;
+                        } else if (lastError != ERROR_IO_PENDING)
+                            throw WinError(lastError, "ReadFile(%s, ..)", (*p)->readSide.get());
                     }
                 }
                 break;

src/libutil/windows/windows-error.cc

@@ -6,9 +6,9 @@
 #  define WIN32_LEAN_AND_MEAN
 #  include <windows.h>
 
-namespace nix::windows {
+namespace nix {
 
-std::string WinError::renderError(DWORD lastError)
+std::string SystemError::renderWindowsError(DWORD lastError)
 {
     LPSTR errorText = NULL;
 
@@ -32,5 +32,5 @@ std::string WinError::renderError(DWORD lastError)
     return fmt("CODE=%d", lastError);
 }
 
-} // namespace nix::windows
+} // namespace nix
 #endif

src/nix/man-pages.cc

@@ -17,7 +17,7 @@ void showManPage(const std::string & name)
     setEnv("MANPATH", (getNixManDir().string() + ":").c_str());
     execlp("man", "man", name.c_str(), nullptr);
     if (errno == ENOENT) {
-        // Not SysError because we don't want to suffix the errno, aka No such file or directory.
+        // Not SystemError because we don't want to suffix the errno, aka No such file or directory.
         throw Error(
             "The '%1%' command was not found, but it is needed for '%2%' and some other '%3%' commands' help text. Perhaps you could install the '%1%' command?",
             "man",