microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
2k stars 84 forks source link

Changes from .NET7 to .NET8 #1022

Closed msedi closed 10 months ago

msedi commented 11 months ago

Obviously some changes either in the generator or in C# has taken place so that the current generated code does not compile anymore.

Just to name two example:

  1. Windows.Win32.PInvoke.EnumDisplayMonitors(null, null, proc, 0); and
  2. bool isConnected = Windows.Win32.PInvoke.InternetGetConnectedState(out var x, 0);

where valid before. Now the are reporting

For 1: Argument 1: cannot convert from '' to 'Windows.Win32.Graphics.Gdi.HDC'

For 2: Argument 1 may not be passed with the 'out' keyword

These are the two methods that are generated, obviously the reserved was there before but now is gone. Is there a reason why this has changed?

internal static unsafe winmdroot.Foundation.BOOL InternetGetConnectedState(out winmdroot.Networking.WinInet.INTERNET_CONNECTION lpdwFlags)
internal static extern unsafe winmdroot.Foundation.BOOL InternetGetConnectedState(winmdroot.Networking.WinInet.INTERNET_CONNECTION* lpdwFlags, uint dwReserved);

Sure, I know the reason why the code does not compile anymore, but this means something must have changed in the generated code, but I couldn't figure out what has changed.

Just a few side notes: I do not have NativeMethods.json. I'm using version 0.3.18-beta on .NET 8, language version is not explicitly set (in the UI it shows latest, which for .NET8 means C#11.

Any help would be nice because it seems this is the only obstacle that prevents from going to .NET8.

AArnott commented 11 months ago

See also #1014

Nuklon commented 10 months ago

Trying out 8.0 RC1 and inline arrays are no longer building.

internal unsafe readonly ReadOnlySpan<char> AsReadOnlySpan() => MemoryMarshal.CreateReadOnlySpan(ref Unsafe.AsRef(Value[0]), SpanLength);
Microsoft.Windows.CsWin32\Microsoft.Windows.CsWin32.SourceGenerator\Windows.Win32.char.InlineArrays.g.cs(117,117,117,125): error CS1620: Argument 1 must be passed with the 'ref' keyword
msedi commented 10 months ago

I'm not exatcly sure if it has to do with .NET8. I saw the problem when switching from version 0.2.46-beta to version 0.3.18-beta on NET8. So it worked with version 0.2.46-beta.

Update: Its seems to be a problem of one of the newer version of CsWin32. It also does not work on .NET7.

Nuklon commented 10 months ago

I'm not exatcly sure if it has to do with .NET8. I saw the problem when switching from version 0.2.46-beta to version 0.3.18-beta on NET8. So it worked with version 0.2.46-beta.

Update: Its seems to be a problem of one of the newer version of CsWin32. It also does not work on .NET7.

It works OK for me in .NET 7.0 and lower. With .NET 8.0 it doesn't work as Unsafe.AsRef has changed.

msedi commented 10 months ago

It works OK for me in .NET 7.0 and lower. With .NET 8.0 it doesn't work as Unsafe.AsRef has changed.

@Nuklon: Ok, that might be a different problem then.

AArnott commented 10 months ago

When I target net8.0 using the 8.0.100-rc.1.23455.8 SDK, CsWin32 0.3.18-beta seems to work fine when NativeMethods.txt has this content:

EnumDisplayMonitors
InternetGetConnectedState

This is with LangVersion set to 11 as well as 12.

I'm going to need more specifics about how to repro the problem in order to fix it.

These are the two methods that are generated, obviously the reserved was there before but now is gone. Is there a reason why this has changed?

We remove reserved parameters from friendly overloads when they must always be 0.

Nuklon commented 10 months ago

@AArnott It seems to work now with VS 17.8 Preview 2, with Preview 1 I was unable to compile WIN32_FIND_DATAW. VS does warn for inline arrays now that it should be passed with the "in" keyword (CS9195), but it seems tracked in https://github.com/microsoft/CsWin32/issues/1014.

msedi commented 10 months ago

@AArnott: Thanks for looking into this.

EnumDisplayMonitors could before called this way:

Windows.Win32.PInvoke.EnumDisplayMonitors(null, null, proc, 0);

Now the compiler complains:

Argument 1: cannot convert from '<null>' to 'Windows.Win32.Graphics.Gdi.HDC'

I checked the two version and it seems 0.3.18-beta does not create the SafeHandle version anymore (see below the generated code for the two version). Is this a bug or was this intended?

0.2.46-beta

internal static unsafe winmdroot.Foundation.BOOL EnumDisplayMonitors(SafeHandle hdc, winmdroot.Foundation.RECT? lprcClip, winmdroot.Graphics.Gdi.MONITORENUMPROC lpfnEnum, winmdroot.Foundation.LPARAM dwData)

internal static extern unsafe winmdroot.Foundation.BOOL EnumDisplayMonitors(winmdroot.Graphics.Gdi.HDC hdc, [Optional] winmdroot.Foundation.RECT* lprcClip, winmdroot.Graphics.Gdi.MONITORENUMPROC lpfnEnum, winmdroot.Foundation.LPARAM dwData);

0.3.18-beta

internal static unsafe winmdroot.Foundation.BOOL EnumDisplayMonitors(winmdroot.Graphics.Gdi.HDC hdc, winmdroot.Foundation.RECT? lprcClip, winmdroot.Graphics.Gdi.MONITORENUMPROC lpfnEnum, winmdroot.Foundation.LPARAM dwData)

internal static extern unsafe winmdroot.Foundation.BOOL EnumDisplayMonitors(winmdroot.Graphics.Gdi.HDC hdc, [Optional] winmdroot.Foundation.RECT* lprcClip, winmdroot.Graphics.Gdi.MONITORENUMPROC lpfnEnum, winmdroot.Foundation.LPARAM dwData);

Regarding the InternetGetConnectedState, you are right, the exclusion of reserved parameters was the problem. It would be nice though if there would be a list of breaking changes, because it was not quite obvious from the (or is there some list and I haven't seen it?).

AArnott commented 10 months ago

@msedi: we do publish release notes at https://github.com/microsoft/CsWin32/releases But sometimes I forget to publish a release so feel free to remind me. We could also do a better job of highlighting changes that would change the shape of the generated APIs, but then, most CsWin32 changes do represent some API change... since that's most of what it generates. So I guess consider everything on that list to be a 'potentially breaking change' list.

That's not to say that every change is intentional. And in your case of EnumDisplayMonitors, I don't recall trying to remove use of the SafeHandle from that method. I'll take a look at that. But as that isn't a .NET 8 specific change, I'll file a separate issue to track that and close this one.