Code Quality: Replace directory enumeration P/Invokes with CsWin32 - #18854
Code Quality: Replace directory enumeration P/Invokes with CsWin32#188540x5bfa wants to merge 2 commits into
Conversation
|
Validated locally. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7b9141e62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR replaces handwritten Win32/COM interop with CsWin32-generated APIs and safe handles.
Changes:
- Migrates filesystem, search, watcher, metadata, shell, signature, and device operations.
- Removes obsolete manual P/Invoke declarations.
- Adds required CsWin32 APIs and custom interop definitions.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Final review status |
|---|---|
src/Files.App/ViewModels/UserControls/NavigationToolbarViewModel.cs |
No final comment. |
src/Files.App/ViewModels/ShellViewModel.cs |
Critical (1 vote): Remove ref from FileTimeToSystemTime calls. |
src/Files.App/ViewModels/Settings/AdvancedViewModel.cs |
No final comment. |
src/Files.App/ViewModels/Properties/Items/FolderProperties.cs |
No final comment. |
src/Files.App/ViewModels/Properties/Items/BaseProperties.cs |
No final comment. |
src/Files.App/Utils/Storage/StorageItems/VirtualStorageItem.cs |
Critical (1 vote): Remove ref from the FileTimeToSystemTime input. |
src/Files.App/Utils/Storage/Search/FolderSearch.cs |
Critical (1 vote): Remove ref from FileTimeToSystemTime calls at lines 501 and 634. |
src/Files.App/Utils/Storage/Helpers/StorageHelpers.cs |
No final comment. |
src/Files.App/Utils/Storage/Helpers/FolderHelpers.cs |
Moderate (3 votes): Iterate from the first result and explicitly skip . and ... |
src/Files.App/Utils/Storage/Enumerators/Win32StorageEnumerator.cs |
Critical (1 vote): Remove ref from FileTimeToSystemTime calls at lines 183, 260, and 266. |
src/Files.App/Utils/Signatures/DigitalSignaturesUtil.cs |
No final comment. |
src/Files.App/Utils/Shell/ShellWindowsAutomation.cs |
No final comment. |
src/Files.App/Utils/Serialization/Implementation/DefaultSettingsSerializer.cs |
No final comment. |
src/Files.App/Services/Windows/WindowsSecurityService.cs |
No final comment. |
src/Files.App/Services/SizeProvider/CachedSizeProvider.cs |
No final comment. |
src/Files.App/MainWindow.xaml.cs |
No final comment. |
src/Files.App/Helpers/Win32/Win32PInvoke.Structs.cs |
No final comment. |
src/Files.App/Helpers/Win32/Win32PInvoke.Methods.cs |
No final comment. |
src/Files.App/Helpers/Win32/Win32PInvoke.Enums.cs |
No final comment. |
src/Files.App/Helpers/Win32/Win32PInvoke.Consts.cs |
No final comment. |
src/Files.App/Helpers/Win32/Win32Helper.Storage.cs |
No final comment. |
src/Files.App/Helpers/ShareItemHelpers.cs |
No final comment. |
src/Files.App/Helpers/Navigation/NavigationHelpers.cs |
No final comment. |
src/Files.App/Extensions/Win32FindDataExtensions.cs |
No final comment. |
src/Files.App/Data/Models/RemovableDevice.cs |
No final comment. |
src/Files.App.CsWin32/NativeMethods.txt |
No final comment. |
src/Files.App.CsWin32/Extras.cs |
Critical (3 votes): Declare dwReserved as uint, not nuint, to preserve the native structure layout. |
Suppressed comments (9)
src/Files.App/Helpers/Win32/Win32Helper.Storage.cs:913
- This method returns
bool, and callers usefalseto show the write error and restore file metadata.FileStream.Writenow throws on an I/O failure instead of returningfalse, so a failed ADS/tag write can exit the caller before its read-only and timestamp restoration code runs. Catch the expected write exceptions and returnfalse(or check the native write result).
stream.Write(Encoding.UTF8.GetBytes(str));
src/Files.App/Helpers/Win32/Win32Helper.Storage.cs:925
FILE_ID_INFO.FileIdis a 128-bit identifier, but this conversion silently keeps only the first 64 bits. These values are persisted asulongand later passed toOpenFileByIdasFILE_ID_TYPE.FileIdType(seeWin32Helper.Storage.cs:661), so volumes that use 128-bit IDs can produce collisions or fail to resolve layout data. Preserve the existing 64-bitFILE_ID_BOTH_DIR_INFO.FileIdbehavior or update all consumers/storage to use extended IDs; do not silently truncate.
return BitConverter.ToUInt64(fileId.FileId.Identifier.AsReadOnlySpan());
src/Files.App/Helpers/Win32/Win32Helper.Storage.cs:978
- The symbolic-link name offsets are relative to the beginning of
PathBuffer, which is already the slice decoded at offset 16. Adding+ 2therefore drops the first two UTF-16 characters; for an absolute target such asC:\target, the print name becomes\targetand is incorrectly treated as relative, resolving against the link's parent. Use the offsets directly and only strip the\??\prefix when falling back to the substitute name.
var subsString = pathBuffer.Substring((subsNameOffset / 2) + 2, subsNameLength / 2);
var printString = pathBuffer.Substring((printNameOffset / 2) + 2, printNameLength / 2);
src/Files.App/Utils/Storage/Enumerators/Win32StorageEnumerator.cs:266
- This
refargument has the same generated-signature mismatch as the preceding timestamp calls:FileTimeToSystemTimeaccepts the FILETIME byin/value. Removerefso this call compiles.
PInvoke.FileTimeToSystemTime(ref findData.ftLastAccessTime, out SYSTEMTIME systemLastAccessOutput);
src/Files.App/Utils/Storage/Enumerators/Win32StorageEnumerator.cs:263
- CsWin32 exposes the
FileTimeToSystemTimeinput as anin/value argument, so theserefarguments do not bind to the generated API and cause compilation to fail. Pass the FILETIME values withoutreffor the modified and creation timestamps; the access timestamp below needs the same correction.
PInvoke.FileTimeToSystemTime(ref findData.ftLastWriteTime, out SYSTEMTIME systemModifiedDateOutput);
itemModifiedDate = systemModifiedDateOutput.ToDateTime();
PInvoke.FileTimeToSystemTime(ref findData.ftCreationTime, out SYSTEMTIME systemCreatedDateOutput);
src/Files.App/Utils/Storage/Search/FolderSearch.cs:635
- CsWin32 exposes the
FileTimeToSystemTimeinput as anin/value argument, so theserefarguments do not bind to the generated API and cause compilation to fail. Pass both FILETIME values withoutref.
PInvoke.FileTimeToSystemTime(ref findData.ftLastWriteTime, out SYSTEMTIME systemModifiedTimeOutput);
PInvoke.FileTimeToSystemTime(ref findData.ftCreationTime, out SYSTEMTIME systemCreatedTimeOutput);
src/Files.App/ViewModels/Settings/AdvancedViewModel.cs:216
- This
using varkeeps the newly created export file handle open across all subsequentStorageFileand ZIP operations. The old code closed the handle before those awaits; with noFILE_SHARE_DELETE, the new scope can cause sharing violations when the export is moved or deleted while it is being processed. Dispose the handle immediately after CreateFile returns, before callingToStorageItem.
using var handle = PInvoke.CreateFile(
src/Files.App/ViewModels/ShellViewModel.cs:2614
- The
fixedblock now encloses the infinite watcher loop, permanently pinning this 4 KB managed array for every active watcher. The buffer only needs to stay pinned until the current overlapped read completes; keep the pin inside each loop iteration to avoid long-lived pins and managed-heap fragmentation.
// The buffer must remain pinned until each overlapped read completes.
fixed (byte* pinnedBuffer = buff)
{
while (x.Status != AsyncStatus.Canceled)
src/Files.App/ViewModels/ShellViewModel.cs:2713
- The
fixedblock now encloses the infinite watcher loop, permanently pinning this 4 KB managed array for every active Git watcher. The buffer only needs to stay pinned until the current overlapped read completes; keep the pin inside each loop iteration to avoid long-lived pins and managed-heap fragmentation.
// The buffer must remain pinned until each overlapped read completes.
fixed (byte* pinnedBuffer = buff)
{
while (x.Status != AsyncStatus.Canceled)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Would it be possible to split this PR into multiple smaller ones? |
0x5bfa
changed the title
|
Done. This is the first PR. |
|
There're another 4 PRs |
|
Thank you, will take a look when I have a chance. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| @@ -59,9 +66,9 @@ async Task<ulong> Calculate(string path, int level = 0) | |||
| { | |||
| size += (ulong)findData.GetSize(); | |||
| } | |||
| else if (findData.cFileName is not "." and not "..") | |||
| else if (fileName is not "." and not "..") | |||
| { | |||
There was a problem hiding this comment.
Small allocation nit while this is being touched: fileName is only read in the directory branch below, but it's materialized for every entry — including files, which are the majority in most folders, and including reparse points that continue on the very next line. Moving it into the else branch skips the allocation for everything but subdirectories.
There was a problem hiding this comment.
Fixed locally. fileName is now materialized only in the directory branch, after reparse points are skipped. This will be included in the next push.
| do | ||
| { | ||
| if (findData.cFileName is "." or "..") | ||
| string fileName = findData.cFileName.ToString(); |
There was a problem hiding this comment.
Same as above, this allocates a string for every entry, then discards it two lines later for anything that isn't a directory. . and .. always carry FILE_ATTRIBUTE_DIRECTORY, so the attribute check can move above the name check with no behavior change.
There was a problem hiding this comment.
Fixed locally. The directory attribute is now checked before materializing fileName. This will be included in the next push.
| string fileName = findData.cFileName.ToString(); | ||
| if (fileName is "." or "..") | ||
| continue; | ||
| var attrs = (FileAttributes)findData.dwFileAttributes; | ||
| if ((attrs & FileAttributes.Directory) != FileAttributes.Directory) | ||
| continue; |
There was a problem hiding this comment.
Same reorder applies here — the directory filter at line 80 discards most of these strings. ./.. are always directories, so hoisting the attribute check above the name check is safe.
There was a problem hiding this comment.
Fixed locally. The directory attribute is now checked before materializing fileName. This will be included in the next push.
|
|
||
| IntPtr hFile = Win32PInvoke.FindFirstFileExFromApp($"{path}{Path.DirectorySeparatorChar}*.*", Win32PInvoke.FINDEX_INFO_LEVELS.FindExInfoBasic, | ||
| out Win32PInvoke.WIN32_FIND_DATA findData, Win32PInvoke.FINDEX_SEARCH_OPS.FindExSearchNameMatch, IntPtr.Zero, Win32PInvoke.FIND_FIRST_EX_LARGE_FETCH); | ||
| WIN32_FIND_DATAW findData = default; |
There was a problem hiding this comment.
Calculate is async and findData is written at line 90, after the awaits at 72 and 79 — so it's hoisted into the state machine and &findData at 47 points into a heap object with no pinning. The unsafe block only exists because the method is async; FileSizeCalculator does the same call from a sync local function without one. Can this move into a small non-async helper returning (FindCloseSafeHandle, WIN32_FIND_DATAW)?
There was a problem hiding this comment.
This concern is valid. Holding a pointer to locals across await is memory unsafe.
There was a problem hiding this comment.
Fixed locally. FindFirstFileEx now writes to a scoped initialFindData, which is copied to findData before the async flow. This avoids taking a pointer to the state-machine local and will be included in the next push.
Resolved / Related Issues
Steps used to test these changes