Files icon indicating copy to clipboard operation
Files copied to clipboard

Code Quality: Replace Vanara with CsWin32

Open gumbarros opened this issue 1 year ago • 11 comments

Description

We're working on utilizing Native Aot in the application, however, runtime marshaler is one of the blocker against this. Vanara uses loads of runtime marshaler via DllImport and ComImport and we're working on replacing it with CsWin32, ultimately removing its reference completely.

Concerned code

  • Vanara references
  • DllImport we explicitly use within the codebase

Gains

  • AoT compliant codebase
  • Less 3rd party references

Requirements

  • Replace DllImport with CsWin32

Comments

CsWin32 requires you to add interfaces, functions, constants, enums, snd etc in NativeMethods.txt placed in Files.App.CsWin32. #define'd constants also need to be written in that text file and use like "PInvoke.CMF_DEFAULT" (They don't get placed in a particular namespace). For more info, download ILSpy and win32metadata.winmd in order to see the full projections.

gumbarros avatar Mar 19 '24 21:03 gumbarros

https://learn.microsoft.com/en-us/dotnet/standard/native-interop/pinvoke-source-generation

Another reference explaining the benefits. I think this is an important issue.

gumbarros avatar Mar 22 '24 01:03 gumbarros

I like this idea. Feel free to work on it with @yaira2’s approval.

0x5bfa avatar Mar 26 '24 12:03 0x5bfa

Sounds good, it might be worth splitting this into multiple PRs to make it easier to test and review.

yaira2 avatar Mar 26 '24 14:03 yaira2

Thanks for the initial try. Do you know CsWin32? If you know, you can replace them with API generated by CsWin32. Using DllImport or LibraryImport is deprecation in Files app because we plan to use CsWin32 instead of those code and Vanara.

0x5bfa avatar Mar 27 '24 04:03 0x5bfa

Just checked about CsWin32, looks promising because of source generator usage like LibraryImportAttribute. Will make a try tonight at NativeFindStorageItemHelper.cs

gumbarros avatar Mar 27 '24 11:03 gumbarros

Don't know if this is expected behavior when migrating: https://github.com/microsoft/CsWin32/issues/1167

gumbarros avatar Mar 28 '24 21:03 gumbarros

I'm not sure, but this function in C language uses void*. I assume that whether adding in or out and even ref is defined in win32metadata or CsWin32 repo.

#include <fileapifromapp.h>

WINSTORAGEAPI HANDLE FindFirstFileExFromAppW(
  LPCWSTR            lpFileName,
  FINDEX_INFO_LEVELS fInfoLevelId,
  LPVOID             lpFindFileData, // HERE
  FINDEX_SEARCH_OPS  fSearchOp,
  LPVOID             lpSearchFilter,
  DWORD              dwAdditionalFlags
) noexcept;

You can use Marshal.PtrToStructure:

using System.Runtime.InteropServices;

Marshal.PtrToStructure((nint)lpFindFileData, typeof(WIN32_FIND_DATA));

And you don't need to migrate all of inside of NativeFindStorageItemHelper because it would make us unreviewable so why not migrate step by step. Also, if you're struggling with some issues you can skip, or you can pick functions up from Win32PInvoke that I made in my PR #15075 after merged because I gathered methods into single file and I think it's a good play ground as well for you.

0x5bfa avatar Mar 29 '24 04:03 0x5bfa

Now you can see Files.App.Helpers.Win32 Win32PInvoke class. We can replace them with CsWin32 piece by piece (starting from about 10 functions).

0x5bfa avatar Apr 01 '24 11:04 0x5bfa

[!IMPORTANT]

  • Please Use CsWin32 instead of replacing with LibraryImport. This Win32PInvoke class should be going to contain undocumented native functions defined through LibraryImport or DllImport when possible.
  • Don't use "*FromApp" functions. They are intended to be used on UWP. Instead, use native ones.
  • E is easy, M is medium, D is difficult. If anyone else can take a look, it'd be good to start from ones marked as E.

Documented native methods (targets to convert to CsWin32)

  • [ ] M: RmRegisterResources
  • [ ] M: RmStartSession
  • [ ] E: RmEndSession
  • [ ] D: RmGetList
  • [ ] E: SetPropW
  • [ ] E: CreateEvent
  • [ ] E: SetEvent
  • [ ] E: GetDpiForWindow
  • [ ] M: CoWaitForMultipleObjects
  • [x] E: SetWindowLongPtr32
  • [x] E: SetWindowLongPtr64
  • [ ] E: SHBrowseForFolder
  • [ ] E: SHGetPathFromIDList
  • [ ] E: CloseHandle
  • [x] M: GetOverlappedResult
  • [x] E: CancelIo
  • [ ] E: CancelIoEx
  • [x] M: WaitForMultipleObjectsEx
  • [x] E: ResetEvent
  • [ ] E: WaitForSingleObjectEx
  • [ ] M: ReadDirectoryChangesW
  • [ ] E: CreateFileFromAppW
  • [ ] M: DeviceIoControl
  • [x] M: ToUnicode
  • [ ] M: ToUnicodeEx
  • [x] E: GetKeyboardState
  • [x] E: GetKeyboardLayout
  • [x] E: MapVirtualKey
  • [x] M: TranslateMessage
  • [ ] E: CreateFileFromApp
  • [ ] E: GetFileAttributesExFromApp
  • [ ] E: SetFileAttributesFromApp
  • [x] E: SetFilePointer
  • [ ] M: ReadFile
  • [ ] M: WriteFile
  • [ ] M: WriteFileEx
  • [ ] M: GetFileTime
  • [ ] M: SetFileTime
  • [ ] E: GetFileInformationByHandleEx
  • [x] E: GetFileInformationByHandleEx
  • [ ] E: FindFirstStreamW
  • [ ] E: FindNextStreamW
  • [x] E: GetDpiForMonitor
  • [x] E: OpenProcessToken
  • [x] E: GetCurrentProcess
  • [x] E: GetTokenInformation
  • [x] E: GetLengthSid
  • [x] M: CryptUnprotectData
  • [ ] E: IsWow64Process2
  • [ ] E: FindNextFile
  • [ ] E: FindClose
  • [ ] M: FileTimeToSystemTime
  • [ ] E: FindFirstFileExFromApp
  • [x] E: CompareStringEx
  • [ ] E: SHCreateStreamOnFileEx
  • [ ] M: SHCreateItemFromParsingName
  • [ ] M: CoCreateInstance
  • [ ] E: RegisterApplicationRestart
  • [ ] M: SHGetKnownFolderPath

Undocumented native functions

  • [x] ~~IsElevationRequired~~
  • [x] ~~SHUpdateRecycleBinIcon~~

0x5bfa avatar Oct 10 '24 04:10 0x5bfa

I've replaced all the PInvoke code that doesn't rely on pointers and unsafe in #17118 and #17119 I'm still working on replacing code that does rely on pointers in #17086, #17107, and #17108 (+ later PRs), although since I'm not as experienced with working with pointers and unsafe they'll take quite a bit longer to replace 🙂

Lamparter avatar May 17 '25 10:05 Lamparter

The reason I opened #17120 was to demonstrate how close the codebase is to complete removal of hardcoded native functions and I was surprised that there isn't as much work in that area left as I thought there was. Nice work!

Lamparter avatar May 17 '25 10:05 Lamparter