Removed CA1838 warning suppressions on p/invokes for StringBuilders.

[AraHaan]

Fixes #3983

Proposed changes

• Fixes rule violations on CA1838 on the p/invokes.

Customer Impact

• Minimal, the changed code should be to the p/invokes only.

Regression?

• Yes / No
  No

Risk

• Minimal

Test methodology

TODO: Test.

Test environment(s)

.NET SDK (reflecting any global.json):
 Version:   6.0.100-preview.2.21155.3
 Commit:    1a9103db2d

Runtime Environment:
 OS Name:     Mac OS X
 OS Version:  11.3
 OS Platform: Darwin
 RID:         osx.11.0-x64
 Base Path:   /usr/local/share/dotnet/sdk/6.0.100-preview.2.21155.3/

Host (useful for support):
  Version: 6.0.0-preview.2.21154.6
  Commit:  3eaf1f316b

.NET SDKs installed:
  6.0.100-preview.2.21155.3 [/usr/local/share/dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 6.0.0-preview.2.21154.6 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 6.0.0-preview.2.21154.6 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]

To install additional .NET runtimes or SDKs:
  https://aka.ms/dotnet-download

(used my browser on my mac this time to make changes)

Note: the code to the StringBuilders was attempted to try to be a minimal change, however it may require changing all the callsites too and simply not having StringBuilder parameters for the methods that wrap the actual p/invokes, this is due to the fact that StringBuilder was used for the buffers instead of string.

Also VB still needs done on it however.

This is opened as a draft because I think it may require a bit more work and testing on a windows system.

Microsoft Reviewers: Open in CodeFlow

[weltkante]

You cannot change StringBuilder to string in the interop signatures - it is not marshaled to a mutable buffer.

[RussKie]

None of these API must have a StringBuilder as an input parameter, instead it's likely be a char*.
Each callsite must be reviewed individually, and updated to correctly allocate buffers either by stackalloc char[...] or by ArrayPool<char>.Shared.Rent(...).

[RussKie]

Please consider how each of the changes will be tested.

[AraHaan]

Alright

[weltkante]

Might or might not be related to test failures, but I don't like the usage of Trim here

• its used in cases where the Windows API should be returning the exact size, why is it trimming in those cases?
• Trim also removes leading zero bytes which makes no sense at all, maybe use TrimEnd instead?
  • In practice this probably only leads to errors when there is garbage behind an "empty" string, you'd trim the leading zero byte indicating the empty string and return the garbage behind it instead. Which leads to the next point:
• it has massively different semantics from C, which looks for the first zero byte. Removing trailing (not leading) zero bytes often is equivalent if you can guarantee that there is no garbage beyond the first zero byte. I'm not sure if all Windows APIs do guarantee that (its not in the contract and if Windows write a longer string into the buffer, then shortens it, there may be garbage beyond the first zero byte, which you would expose because you only trim trailing zero bytes)
• the way you're using it allocates a string and then allocates another string if trimming is necessary - this could be optimized by trimming the span before converting it into a string

I know I'm being nitpicky here, since in practice it probably always works, but why take risks on edge case failures by performing an operation which is more convenient but having the wrong semantics? If the cost of doing it "correctly" is too high I'd suggest doing a TrimEnd('\0') on the span (not the string) as compromise.

That said, fixing any bugs triggering test failures is probably more important than going for style issues about Trim

[AraHaan]

So all of these including FormatMessage returns the actual size of the stuff?

(well except for SHLoadIndirectString)

[weltkante]

So all of these including FormatMessage returns the actual size of the stuff?

yes:

FormatMessage:

If the function succeeds, the return value is the number of TCHARs stored in the output buffer, excluding the terminating null character.

DragQueryFileW:

When the function copies a file name to the buffer, the return value is a count of the characters copied, not including the terminating null character.

[AraHaan]

Yeah, I think I should migrate this to use the DllImport generator added in .NET 7 so It can help me with passing spans into the signatures of the p/invokes.

[elachlan]

Yeah, I think I should migrate this to use the DllImport generator added in .NET 7 so It can help me with passing spans into the signatures of the p/invokes.

@RussKie would the team be okay using: https://github.com/microsoft/CsWin32 ? It reduces the amount of code considerably.

Related: #4136

[RussKie]

IIRC, we'd discussed https://github.com/microsoft/CsWin32 and https://github.com/terrafx/terrafx.interop.windows (and maybe some other option) with @JeremyKuhne, and arrived to a conclusion that it was undesirable for few reasons. One is that it'd introduce external dependencies, which we'd have to ship (with all related issues such as payload size, packaing, signing, etc). Also, we strive to import only those definitions that we use (i.e., less code - less to compile - less to jit, etc.). And lastly, some of our imports are optimised (e.g., we use IHandle instead of SafeHandle).

As per #4136 we do plan to use COM source generators when those are ready (.NET 8?), and replace the manual COM wrappers that @kant2002 has kindly built for us in .NET 7.
@gpetrou is kindly helping us to migrate to LibraryImport definitions from DllImport, and this is the recommended route for now.

[AraHaan]

Technically what is unused in TerraFX.Interop.Windows can be trimmed, however how it is currently the WindowsDesktop profiles both do not support trimming at all yet to provide such a space savings.

Also as for external dependencies, wasn't TerraFX made a member of the DNF despite it staying in it's own github organization (it might be me misremembering and it being ClangSharp instead)?

If all else fails, winforms and wpf could manually use clangsharp instead (for the COM if that is supported in it) if that route is desired as well.

[elachlan]

IIRC, we'd discussed https://github.com/microsoft/CsWin32 and https://github.com/terrafx/terrafx.interop.windows (and maybe some other option) with @JeremyKuhne, and arrived to a conclusion that it was undesirable for few reasons. One is that it'd introduce external dependencies, which we'd have to ship (with all related issues such as payload size, packaing, signing, etc). Also, we strive to import only those definitions that we use (i.e., less code - less to compile - less to jit, etc.). And lastly, some of our imports are optimised (e.g., we use IHandle instead of SafeHandle).

@AArnott - does using cswin32 result in a shippable dependency or is it just design/build time? Also would adding optimisations such as IHandle be possible?

@RussKie - as far as I can tell cswin32 only generates the definitions you use. which would keep the size down. Only problem I see would be relying on the CSWin32 team to update any bad definitions. But I think that they are keen to keep it updated.

[AArnott]

@elachlan CsWin32 generates code directly into your compilation. The only (recommended) runtime dependency it adds is to System.Memory, which most compilations already have anyway.
As for optimizations, I'm not familiar with IHandle. But I'll tell you that the code we emit it first and foremost designed to be as close to 'bare metal' perf as you can get. No one should ever have to avoid using CsWin32 for perf reasons. In addition to the extern methods themselves which don't shy away from pointers and using native handle types (avoiding SafeHandles), we also generate what I call 'friendly overloads' which use SafeHandle, const char* with string, etc. Most folks favor the friendly overloads, but the extern methods themselves are always there for those perf-critical paths.

CsWin32 is opt-in per-API. If you ask for CreateFile the only thing you get is CreateFile and the enums to support its method signature. If you don't like how a particular supporting enum or struct type is generated, you can copy and paste it from generated code into your own source controlled code and change it all you want. CsWin32 will suppress generation of types that you declare yourself and emit references to yours. That is, if you don't use the partial modifier. If you do use the partial modifier, we assume you want to contribute more members to our generated type.

I'd love to see WinForms replace their interop code with CsWin32-generated code and would be happy to support you in doing so. There are bugs in some of the generated code, but we're working them down in a priority order and I think there are only a few left that don't block adoption except for those APIs that are impacted. I recommend folks start migration by adding a CsWin32 package reference and replace just a few APIs with generated ones to get a feel for it, then migrate gradually over time, so if you do need any of the APIs that may still be generated incorrectly, it only blocks adoption for those particular APIs, and we'd be happy to hear feedback on anything blocking.

CC: @RussKie

[kant2002]

I'm personally for using CsWin32 instead of manually looking at the docs and writing PInvokes. I think that work may not start earlier then .NET 8 unless somebody really motivated can reduce risks by explaining how that would not be an issue.

Practical matters:

1. In this project interop declarations are split by DLLs in which function declared. Does CsWin32 support that way of code grouping?
2. To have buy-in from the team, would be interesting to see examples of codegen and compare with current LibraryImport code. LibraryImportGenreator gives ability to use very low overhead PInvoke. Also that maybe interesting for broader community as a whole.
3. IHandle is internal pattern which provide lightweight alternative to SafeHandle. See

   winforms/src/System.Windows.Forms.Primitives/src/Interop/User32/Interop.SendMessageW.cs

   Lines 18 to 27 in 86eff93

   	public static nint SendMessageW(
   	IHandle hWnd,
   	WM Msg,
   	nint wParam = default,
   	nint lParam = default)
   	{
   	nint result = SendMessageW(hWnd.Handle, Msg, wParam, lParam);
   	GC.KeepAlive(hWnd);
   	return result;
   	}

   for the usage. Does CsWin32 support this kind of patterns. LibraryImportGenerator most likely would support that.

[JeremyKuhne]

Tagging @tannergooding as we're talking about TerraFX here as well.

We have a few design principles we're trying to achieve with our interop code:

• Going close to bare metal to avoid performance issues (everything is blittable, no SafeHandle, etc.).
• Using stack based scopes (ref structs) to lightly and safely close out resources (if def'ed to classes in debug for tracking of proper usage).
• Adding wrappers (like the IHandle ones) to handle keeping objects alive and other safe usage related.
• Avoiding verbosity by removing redundancy in Windows "enums" (COPY.COPY_FILE_NO_OFFLOAD -> COPY.FILE_NO_OFFLOAD). This isn't a deal-breaker, but nice-to-have.
• No legacy COM interop (in progress)
• Expressing the documented dll location in the model Interop.User32

We're trying to find the right balance between making things fast and pushing contributors to the pit of success. If we could generate nearly the same interop surface area with a library that falls under Microsoft's ownership or the .NET Foundation I'd be very keen on making the jump for .NET 8. For CsWin32 I think that would mean:

1. Being able to put the methods in the same layout we currently have (Interop static partial class with dll name partial classes within).
2. Being able to turn off "easy" wrapper generation, preferably per API.
3. Being able to say that we want generation with private so we can enforce using our wrappers in the main assembly.

@AArnott I'm happy to talk more about it with you whenever you want.

cc: @merriemcgaw

[AArnott]

In this project interop declarations are split by DLLs in which function declared. Does CsWin32 support that way of code grouping?

No. That is how the dotnet/pinvoke project divided it too, but CsWin32 uses namespaces from win32metadata that the SDK PM (@mikebattista) worked on for quite a while. All the pinvoke extern methods go into a single PInvoke class, and the types that support them are divided across the namespaces based on use case. This made the functions much easier to find for many customers that couldn't otherwise figure out that, for example, when they asked for CreateFile that would go off to some namespace and class while another method would go elsewhere. Folks in C++ don't tend to have to think about what module a function is exported from, so in C# we didn't require them to think about that either.

To have buy-in from the team, would be interesting to see examples of codegen and compare with current LibraryImport code. LibraryImportGenreator gives ability to use very low overhead PInvoke. Also that maybe interesting for broader community as a whole.

It's there for you to try out. Or we could set up a meeting to review it together. As I mentioned earlier, we already have fairly low overhead, and I suspect the perf is comparable with the source generator you mentioned except perhaps when using AOT, and I don't know whether that is applicable to WinForms.

IHandle is internal pattern which provide lightweight alternative to SafeHandle... LibraryImportGenerator most likely would support that

I'd have to see IHandle, and understand how it strikes a better balance between using native handles and SafeHandle. CsWin32 already supports both of these ends, so if you can help me understand how IHandle is better than either end, I can tell you what it would take to do so with CsWin32. That said, I'd be very surprised if LibraryImportGenerator could generate code that would work with a custom handle type that you defined.

Going close to bare metal to avoid performance issues (everything is blittable, no SafeHandle, etc.).

CsWin32 can do that with a switch. In which case absolutely no marshaling occurs at all, and there are no SafeHandles. I would expect it to have better performance than LibraryImportGenerator which as I understand it still does marshaling... just in your own library code instead of leaving it to the runtime.

Using stack based scopes (ref structs) to lightly and safely close out resources

That's an interesting idea that we've only spent a few minutes talking about. I'd like to learn more about where/when/how you would use this. Maybe CsWin32 can emit types to help you with this.

Avoiding verbosity by removing redundancy in Windows "enums"

We made a design decision to emit APIs that exactly match those found in the docs and header files. This is optimized to make it easy to translate C++ code to C# and so that win32 API docs match your code, allowing you to translate and jump either direction. Trimming prefixes from enum values for example would make it much more difficult to jump around since symbols would have different names.

No legacy COM interop

I'm not sure what you mean by this. When you turn marshaling off in CsWin32, any COM interfaces that CsWin32 generates are struct pointers instead of interfaces, which of course is much less convenient but completely avoids RCWs and the GC/finalization pressure that comes from that. Is that what you're looking for?

Being able to turn off "easy" wrapper generation

You mean you want to suppress the friendly overloads? I could certainly see us adding that as an option if that's important to you.

Being able to say that we want generation with private so we can enforce using our wrappers in the main assembly.

We offer internal by default with an option for public. I could see private being offered for the methods, but not the interop types to support them and still be a useful code generator. I guess you'd have to add your internal wrappers via partial class to the generated code in order to access the private members.

[AArnott]

I'm playing around with your repo and seeing how LibraryImport works. Very interesting. And I can see how IHandle could potentially be supported by it.

[AArnott]

Here's an idea of what CsWin32 use in WinForms would look like.

 eng/Versions.props                                 |  3 +-
 .../src/Interop/Gdi32/Interop.CombineRgn.cs        | 14 -------
 .../src/Interop/Gdi32/Interop.CreateRectRgn.cs     | 14 -------
 .../src/Interop/Gdi32/Interop.GetClipRgn.cs        | 14 -------
 .../src/Interop/Gdi32/Interop.GetRgnBox.cs         |  1 +
 .../src/Interop/Gdi32/Interop.HRGN.cs              | 47 ----------------------
 .../src/Interop/Gdi32/Interop.RegionScope.cs       | 22 +++++-----
 .../src/Interop/Gdi32/Interop.SelectClipRgn.cs     | 14 -------
 .../src/Interop/User32/Interop.GetUpdateRgn.cs     | 14 -------
 .../src/Interop/User32/Interop.GetWindowRgn.cs     | 21 ----------
 .../src/Interop/User32/Interop.InvalidateRgn.cs    | 21 ----------
 .../src/Interop/User32/Interop.RedrawWindow.cs     | 21 ----------
 .../src/Interop/User32/Interop.SetWindowRgn.cs     |  5 ++-
 .../UxTheme/Interop.GetThemeBackgroundRegion.cs    | 33 ---------------
 .../src/NativeMethods.json                         |  3 ++
 .../src/NativeMethods.txt                          |  9 +++++
 .../src/System.Windows.Forms.Primitives.csproj     |  5 ++-
 .../System/Windows/Forms/DeviceContextHdcScope.cs  |  5 ++-
 18 files changed, 36 insertions(+), 230 deletions(-)

Not bad. Just a few APIs moving over drops 194 lines of code from your repo, and deletes several files.
This example generates a bit more than is used (e.g. HDC). In order to curtail the otherwise explosive growth of adoption due to interop types, I cut it off with a few casts or adaptations to make your own structs or enums work with the CsWin32-generated ones. A more complete migration would drop your hand-written types altogether in favor of the CsWin32-generated ones.

[AraHaan]

Here's an idea of what CsWin32 use in WinForms would look like.

 eng/Versions.props                                 |  3 +-
 .../src/Interop/Gdi32/Interop.CombineRgn.cs        | 14 -------
 .../src/Interop/Gdi32/Interop.CreateRectRgn.cs     | 14 -------
 .../src/Interop/Gdi32/Interop.GetClipRgn.cs        | 14 -------
 .../src/Interop/Gdi32/Interop.GetRgnBox.cs         |  1 +
 .../src/Interop/Gdi32/Interop.HRGN.cs              | 47 ----------------------
 .../src/Interop/Gdi32/Interop.RegionScope.cs       | 22 +++++-----
 .../src/Interop/Gdi32/Interop.SelectClipRgn.cs     | 14 -------
 .../src/Interop/User32/Interop.GetUpdateRgn.cs     | 14 -------
 .../src/Interop/User32/Interop.GetWindowRgn.cs     | 21 ----------
 .../src/Interop/User32/Interop.InvalidateRgn.cs    | 21 ----------
 .../src/Interop/User32/Interop.RedrawWindow.cs     | 21 ----------
 .../src/Interop/User32/Interop.SetWindowRgn.cs     |  5 ++-
 .../UxTheme/Interop.GetThemeBackgroundRegion.cs    | 33 ---------------
 .../src/NativeMethods.json                         |  3 ++
 .../src/NativeMethods.txt                          |  9 +++++
 .../src/System.Windows.Forms.Primitives.csproj     |  5 ++-
 .../System/Windows/Forms/DeviceContextHdcScope.cs  |  5 ++-
 18 files changed, 36 insertions(+), 230 deletions(-)

Not bad. Just a few APIs moving over drops 194 lines of code from your repo, and deletes several files. This example generates a bit more than is used (e.g. HDC). In order to curtail the otherwise explosive growth of adoption due to interop types, I cut it off with a few casts or adaptations to make your own structs or enums work with the CsWin32-generated ones. A more complete migration would drop your hand-written types altogether in favor of the CsWin32-generated ones.

That looks great, I wonder what all the other apis used would look like (except the ones that are currently bugged).

[elachlan]

Not bad. Just a few APIs moving over drops 194 lines of code from your repo, and deletes several files. This example generates a bit more than is used (e.g. HDC). In order to curtail the otherwise explosive growth of adoption due to interop types, I cut it off with a few casts or adaptations to make your own structs or enums work with the CsWin32-generated ones. A more complete migration would drop your hand-written types altogether in favor of the CsWin32-generated ones.

That is an impressive reduction. I think the positive here is that Winforms offloads a lot of the pinvoke interop effort to a project which is being used by many other projects. This would hopefully help improve the interop code more than doing it ourselves.

[elachlan]

@JeremyKuhne CsWin32 can help with #4135 as well.

[elachlan]

I suspect the perf is comparable with the source generator you mentioned except perhaps when using AOT

@AArnott, @kant2002 is very interested in NativeAOT for performance and all their work is centred around enabling that. Would you be able to expand on the performance implications you were eluding to here?

[AArnott]

Well, in the default CsWin32 mode where marshaling is allowed, CsWin32 can generate functions that take structs as parameter types, which .NET will then potentially need to marshal, and do so within its own interop layer that I guess doesn't work in all runtime modes (e.g. AOT + trimming). It may be slower too, I don't know.
You can turn CsWin32's setting to generate functions and structs that never require marshaling, but the user experience downgrades (e.g. struct fields are blittable char* instead of string).
CsWin32 can certainly address that concern in the future, since it can potentially generate the same kind of code that you get today with [LibraryImport], but that'll be some work.

[kant2002]

@AArnott form what I read bringing CsWin32 will negate small perf gains here and there across codebase. Each bullet points of @JeremyKuhne response is small decision which maintained across codebase for perf reasons, so that's unlikely they can be reversed.

that I guess doesn't work in all runtime modes (e.g. AOT + trimming). It may be slower too, I don't know.

AOT works just fine with most marshaling except marshaling COM interfaces and marshaling COM objects inside VARIANT and SAFEARRAY. Would be curious to know does some primitives for VARIANT and SAFEARRAY provided so. Also while we talk about AOT, reflection in AOT supported, that's common misconception to not talk about it.

but the user experience downgrades (e.g. struct fields are blittable char* instead of string).

For this repo, that's explicit decision which maintained by the team and contributors as well. That allow WinForms easier jump on board with runtime innovations/refactoring like https://docs.microsoft.com/en-us/dotnet/standard/native-interop/disabled-marshalling and being close to supporting BuiltInComInteropSupport from https://docs.microsoft.com/en-us/dotnet/core/deploying/trimming/trimming-options#framework-features-disabled-when-trimming.

And obviously perf reasons plays big role here. so I would not dismiss these concerns.

CsWin32 can certainly address that concern in the future, since it can potentially generate the same kind of code that you get today with [LibraryImport], but that'll be some work.

I think item with "some work" is what I can help you with, at least I would like evaluate options, and take easiest task which will help this project.

Also if you have some simple, or not so simple showcase project which I can git clone and play with it, that would help me to better understand on what I sighing up for. 😄

Folks in C++ don't tend to have to think about what module a function is exported from, so in C# we didn't require them to think about that either.

At least for me, having all native dependencies listed in single folder help with my ComWrappers work massively, since I can learn about dependencies easier. C# for me is all about easier learning of code structure, and having one big class looks like not very appealing. I do agree that knowing that CreateFile will go to some magic namespace is probably unintuitive, but what is namespace in which generate code can be derived from folder where TXT file store, or via additional Item metadata. That's just throwing ideas without really knowing how CsWin32 work.

[AArnott]

@kant2002: thanks for your offer to help and your thoughts on AOT, COM, etc. I'm learning here and your experience is helping.

having one big class looks like not very appealing.

Fortunately the only thing that "one big class" is relatively simple (extern methods, friendly overloads, and constants). That tends to me a minority of the generated code, since structs, enums and interfaces get generated into separate types and files. Also, I'm pretty sure we at least generate each module's methods into different files via partial classes, so that may help with your review of the code as well. We tried supporting splitting this out on a class-per-module basis initially, but that design conflicted with some other very real requirements so I don't think we'll backtrack on that decision unless there's a firm requirement that isn't being met with the current design.

if you have some simple, or not so simple showcase project

Are you asking to play with a project that is already using CsWin32 so you can get a feel for what it's like? If so, here's a PR I found of another repo adopting CsWin32: microsoft/vs-extension-testing#92. Does that help? Or if you wanted to try actually adopting CsWin32 in a small repo, I might be able to find a good candidate for you.

[elachlan]

@AraHaan did you want to reroll this against the current branch?

[AraHaan]

I could yeah. However I will need to reclone as the original clone on my pc got deleted automatically.

[AraHaan]

Since this was implemented, I have closed this.