chore: migrate P/Invoke declarations to [LibraryImport] (MODERN-002)#454
Conversation
📝 WalkthroughWalkthroughThis PR modernizes Win32 P/Invoke declarations across six source files by migrating from ChangesP/Invoke to LibraryImport Modernization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@SysManager/SysManager/Helpers/KnownFolders.cs`:
- Around line 25-30: Change the SHGetKnownFolderPath P/Invoke to return the
HRESULT (int) and use an out IntPtr (pointer) instead of out string so you can
check the result and manage the native memory yourself: update the LibraryImport
signature for SHGetKnownFolderPath to return int and out IntPtr pszPath (remove
StringMarshalling for that out param), then in the caller call
SHGetKnownFolderPath, check the returned HRESULT and call
Marshal.ThrowExceptionForHR(hresult) on failure, convert the returned IntPtr to
a managed string with Marshal.PtrToStringUni on success, and free the native
buffer with Marshal.FreeCoTaskMem (CoTaskMemFree) after conversion to avoid
leaks and ensure correct fallback behavior.
In `@SysManager/SysManager/Services/PerformanceService.cs`:
- Around line 269-271: The code is currently ignoring the BOOL return from the
native calls; update the usages of SystemParametersInfoGet and the corresponding
setter (e.g., SystemParametersInfoSet/SystemParametersInfo with SPI_GETUIEFFECTS
and SPI_SETUIEFFECTS) to check their return value, and if the call fails, do not
assume the seeded/default value or log success—instead log or propagate an error
that includes Marshal.GetLastWin32Error() and return a failure/false result (or
throw) so callers know the operation did not succeed; only treat the operation
as successful when the native BOOL return is true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7118392c-aa53-4b3b-a6e4-c662af53c678
📒 Files selected for processing (7)
SysManager/SysManager/App.xaml.csSysManager/SysManager/Helpers/KnownFolders.csSysManager/SysManager/MainWindow.xaml.csSysManager/SysManager/Services/IconExtractorService.csSysManager/SysManager/Services/PerformanceService.csSysManager/SysManager/Services/ShortcutCleanerService.csSysManager/SysManager/SysManager.csproj
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build & unit tests
- GitHub Check: Analyze (csharp)
| [LibraryImport("shell32.dll", StringMarshalling = StringMarshalling.Utf16)] | ||
| private static partial void SHGetKnownFolderPath( | ||
| in Guid rfid, | ||
| uint dwFlags, | ||
| nint hToken, | ||
| out string pszPath); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does C# [LibraryImport] support automatic HRESULT-to-exception translation or a PreserveSig=false equivalent for Win32 APIs like SHGetKnownFolderPath? What is the recommended LibraryImport signature for SHGetKnownFolderPath when the native function returns HRESULT and allocates a PWSTR that must be freed by the caller?
💡 Result:
Yes. When you use source-generated P/Invoke via LibraryImport (LibraryImportAttribute),.NET performs the same kind of HRESULT-to-exception translation that DllImportAttribute does when PreserveSig=false. In that mode, failing HRESULTs are automatically converted into exceptions, and successful S_OK (0) is treated as “no exception”; the managed signature becomes void (or the type of the last [out, retval] parameter) rather than returning the HRESULT directly. [1][2][3] For SHGetKnownFolderPath, the native signature is: HRESULT SHGetKnownFolderPath(..., PWSTR ppszPath); The output string is allocated by the callee and must always be freed by the caller with CoTaskMemFree once you’re done (even on failure). [4] Recommended LibraryImport signature (HRESULT-returning native, PWSTR allocated by callee freed by caller): [LibraryImport("shell32.dll", StringMarshalling = StringMarshalling.Utf16)] internal static partial int SHGetKnownFolderPath( ref readonly Guid rfid, uint dwFlags, IntPtr hToken, out IntPtr ppszPath); Then, call it and translate HRESULT yourself (because you will typically keep the HRESULT return type as int). Example pattern: - declare IntPtr p; - int hr = SHGetKnownFolderPath(ref knownFolderId, flags, IntPtr.Zero, out p); - if (p!= IntPtr.Zero) { string path = Marshal.PtrToStringUni(p)!; Marshal.FreeCoTaskMem(p); } If you want automatic exception translation, you need the LibraryImport signature to be “PreserveSig=false equivalent” (i.e., have.NET treat the HRESULT as not directly returned). Microsoft documents that setting PreserveSig=false causes the managed signature to become void or the type of the last [out, retval] parameter, and failing HRESULTs throw automatically. [1][3] However, for SHGetKnownFolderPath specifically, because the last parameter is a pointer-out (PWSTR), there is no built-in way described in the cited docs to both (a) keep a success/failure automatic exception translation and (b) return the allocated pointer safely without also relying on correct source-generator handling of the parameter as the “retval”/translated return. The most robust, recommended approach grounded in the official allocation/freeing contract is therefore: keep an int HRESULT return and explicitly free the returned PWSTR with CoTaskMemFree/Marshal.FreeCoTaskMem. [4]
Citations:
- 1: https://msdn.microsoft.com/library/system.runtime.interopservices.dllimportattribute.preservesig
- 2: https://learn.microsoft.com/en-us/dotnet/standard/native-interop/preserve-sig
- 3: https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.dllimportattribute.preservesig?view=net-10.0
- 4: https://learn.microsoft.com/en-us/windows/win32/api/shlobj_core/nf-shlobj_core-shgetknownfolderpath
🏁 Script executed:
# Locate and examine the KnownFolders.cs file
fd -t f "KnownFolders.cs" --exec wc -l {} \;Repository: laurentiu021/SystemManager
Length of output: 121
🏁 Script executed:
# Read the file to verify the code and fallback logic
fd -t f "KnownFolders.cs" --exec cat -n {} \;Repository: laurentiu021/SystemManager
Length of output: 3948
Preserve the HRESULT from SHGetKnownFolderPath for explicit error handling.
The current declaration with StringMarshalling = StringMarshalling.Utf16 and void return type relies on automatic HRESULT-to-exception translation. However, this combination is not reliably specified for LibraryImport with out-pointer parameters like PWSTR*. The native function allocates memory that must be freed with CoTaskMemFree; using out string obscures this contract. Return the int HRESULT, explicitly check it with Marshal.ThrowExceptionForHR(), and manually free the allocated pointer to ensure correct fallback behavior and memory safety.
Suggested fix
- [LibraryImport("shell32.dll", StringMarshalling = StringMarshalling.Utf16)]
- private static partial void SHGetKnownFolderPath(
+ [LibraryImport("shell32.dll")]
+ private static partial int SHGetKnownFolderPath(
in Guid rfid,
uint dwFlags,
nint hToken,
- out string pszPath);
+ out nint pszPath); private static string GetPath(Guid folderId)
{
try
{
- SHGetKnownFolderPath(folderId, 0, nint.Zero, out var path);
- return path;
+ var hr = SHGetKnownFolderPath(folderId, 0, nint.Zero, out var pathPtr);
+ if (hr < 0)
+ Marshal.ThrowExceptionForHR(hr);
+
+ try
+ {
+ return Marshal.PtrToStringUni(pathPtr) ?? string.Empty;
+ }
+ finally
+ {
+ Marshal.FreeCoTaskMem(pathPtr);
+ }
}
catch (COMException)
{🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SysManager/SysManager/Helpers/KnownFolders.cs` around lines 25 - 30, Change
the SHGetKnownFolderPath P/Invoke to return the HRESULT (int) and use an out
IntPtr (pointer) instead of out string so you can check the result and manage
the native memory yourself: update the LibraryImport signature for
SHGetKnownFolderPath to return int and out IntPtr pszPath (remove
StringMarshalling for that out param), then in the caller call
SHGetKnownFolderPath, check the returned HRESULT and call
Marshal.ThrowExceptionForHR(hresult) on failure, convert the returned IntPtr to
a managed string with Marshal.PtrToStringUni on success, and free the native
buffer with Marshal.FreeCoTaskMem (CoTaskMemFree) after conversion to avoid
leaks and ensure correct fallback behavior.
| int enabled = 1; | ||
| SystemParametersInfoGet(SPI_GETUIEFFECTS, 0, ref enabled, 0); | ||
| return enabled != 0; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For .NET [LibraryImport] with SetLastError = true, should callers use Marshal.GetLastPInvokeError() and explicitly check the BOOL returned by SystemParametersInfoW for SPI_GETUIEFFECTS and SPI_SETUIEFFECTS?
💡 Result:
When using.NET LibraryImport with SetLastError = true, you should primarily rely on the boolean return value of SystemParametersInfoW to determine if the call succeeded [1][2]. According to the Windows API documentation, SystemParametersInfo returns a nonzero value (TRUE) if the function succeeds and zero (FALSE) if it fails [1][2]. You should check this return value first. If the function returns FALSE, you can then use Marshal.GetLastPInvokeError to retrieve the extended error information [3][4]. This is the correct, modern.NET approach for accessing the last Win32 error code after a P/Invoke call configured with SetLastError = true [3][4]. It is important to note that GetLastError (and by extension, Marshal.GetLastPInvokeError) should only be consulted when the function's return value indicates a failure [5][6]. If the function succeeds, the value returned by GetLastPInvokeError is unreliable and may contain stale data from previous operations [5][6]. Therefore, explicitly checking the boolean return value is the standard and required practice for robust error handling with this API [5][6].
Citations:
- 1: http://www.jose.it-berater.org/sysinfo/pages/systemparametersinfo.htm
- 2: https://pic.hallikainen.org/techref/os/win/api/win32/func/src/f87_17.htm
- 3: https://msdn.microsoft.com/library/system.runtime.interopservices.dllimportattribute.setlasterror
- 4: https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.marshal.getlastpinvokeerror?view=net-10.0
- 5: https://stackoverflow.com/questions/43244719/why-does-getlasterror-have-an-error-before-my-program-begins
- 6: https://groups.google.com/g/comp.os.ms-windows.programmer.win32/c/Llq2-pVUzMg
🏁 Script executed:
# Find and examine the PerformanceService.cs file
find . -name "PerformanceService.cs" -type fRepository: laurentiu021/SystemManager
Length of output: 125
🏁 Script executed:
# Once we locate it, examine the relevant lines and method signatures
cat -n SysManager/SysManager/Services/PerformanceService.cs | sed -n '250,300p'Repository: laurentiu021/SystemManager
Length of output: 2423
🏁 Script executed:
# Check the full file size to understand context
wc -l SysManager/SysManager/Services/PerformanceService.csRepository: laurentiu021/SystemManager
Length of output: 127
🏁 Script executed:
# Search for the SystemParametersInfoGet and SystemParametersInfoSet declarations
rg -n "SystemParametersInfo(Get|Set)" SysManager/SysManager/Services/PerformanceService.csRepository: laurentiu021/SystemManager
Length of output: 356
🏁 Script executed:
# Look for LibraryImport and SetLastError declarations
rg -n -B 2 "SystemParametersInfo" SysManager/SysManager/Services/PerformanceService.cs | head -50Repository: laurentiu021/SystemManager
Length of output: 1283
Check the SystemParametersInfoW return value before trusting the result.
Both call sites discard the BOOL return. If the GET call fails, Line 271 reports true from the seeded value; if the SET call fails, Line 283 still logs success even though nothing changed.
Suggested fix
internal static bool GetUiEffectsEnabled()
{
int enabled = 1;
- SystemParametersInfoGet(SPI_GETUIEFFECTS, 0, ref enabled, 0);
+ if (!SystemParametersInfoGet(SPI_GETUIEFFECTS, 0, ref enabled, 0))
+ throw new System.ComponentModel.Win32Exception(Marshal.GetLastPInvokeError());
return enabled != 0;
}
@@
public static void SetUiEffects(bool enabled)
{
- SystemParametersInfoSet(SPI_SETUIEFFECTS, 0, enabled ? 1 : 0, SPIF_SENDCHANGE);
+ if (!SystemParametersInfoSet(SPI_SETUIEFFECTS, 0, enabled ? 1 : 0, SPIF_SENDCHANGE))
+ throw new System.ComponentModel.Win32Exception(Marshal.GetLastPInvokeError());
Log.Information("System: Visual effects {Action}", enabled ? "enabled" : "reduced");
}Also applies to: 282-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@SysManager/SysManager/Services/PerformanceService.cs` around lines 269 - 271,
The code is currently ignoring the BOOL return from the native calls; update the
usages of SystemParametersInfoGet and the corresponding setter (e.g.,
SystemParametersInfoSet/SystemParametersInfo with SPI_GETUIEFFECTS and
SPI_SETUIEFFECTS) to check their return value, and if the call fails, do not
assume the seeded/default value or log success—instead log or propagate an error
that includes Marshal.GetLastWin32Error() and return a failure/false result (or
throw) so callers know the operation did not succeed; only treat the operation
as successful when the native BOOL return is true.
) ## What does this PR do? Replaces 2 remaining generic/bare catches found during the full code audit: - **LogsViewModel** — `catch (Exception ex)` replaced with specific `EventLogException`, `UnauthorizedAccessException`, `InvalidOperationException` - **ConsoleViewModel** — bare `catch { }` replaced with `ExternalException` (clipboard lock) ## Type of change - [x] Code quality / CodeQL (`fix:`) --------- Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
…454) Co-authored-by: laurentiu021 <laurentiu021@users.noreply.github.com>
1 participant
Summary
[DllImport]declarations to[LibraryImport]source generator across 6 files<AllowUnsafeBlocks>true</AllowUnsafeBlocks>to .csproj (required by LibraryImport source gen)IconExtractorService,PerformanceService,ShortcutCleanerService,KnownFoldersSystemParametersInfooverloads toSystemParametersInfoGet/SystemParametersInfoSetwithEntryPointExtractIconWtoExtractIconwithEntryPoint = "ExtractIconW"SHGetFileInfoandSHFileOperation(structs withByValTStrfields not supported by source generator)Test plan
Summary by CodeRabbit