Improve optional out interface arguments (e.g. IWbemServices.GetObject) and other minor tweaks (#1544)

Author: jevansaks

src/CsWin32Generator/Program.cs

@@ -503,7 +503,7 @@ private unsafe bool ProcessNativeMethodsFile(SuperGenerator superGenerator, File
             {
                 lineNumber++;
                 string name = line.Trim();
-                if (string.IsNullOrWhiteSpace(name) || name.StartsWith("//", StringComparison.Ordinal) || name.StartsWith("-", StringComparison.Ordinal))
+                if (string.IsNullOrWhiteSpace(name) || name.StartsWith("#", StringComparison.Ordinal) || name.StartsWith("//", StringComparison.Ordinal) || name.StartsWith("-", StringComparison.Ordinal))
                 {
                     skippedCount++;
                     continue;

src/Microsoft.Windows.CsWin32/Generator.FriendlyOverloads.cs

@@ -215,7 +215,16 @@ private IEnumerable<MethodDeclarationSyntax> DeclareFriendlyOverload(
                 mustRemainAsPointer = true;
             }
 
-            // For compat with how out/ref parameters used to be generated, leave out/ref parameters as pointers if we're not tring to improve them.
+            if (isOptional && isIn && !isOut && externParam.Type is PointerTypeSyntax { ElementType: QualifiedNameSyntax elementTypeSyntax } && elementTypeSyntax.Right.Identifier.ValueText == "NativeOverlapped")
+            {
+                // OVERLAPPED struct must always be passed by pointer. Currently "in" optional parameters are promoted to nullable which
+                // means the structs get copied. Normally this is fine since these struct addresses don't matter, but in the case of OVERLAPPED
+                // it does. Trying to change "in" optional parameters to not be wrapped in nullable is a lot of work and impact for unclear value
+                // so just adding special handling for OVERLAPPED for now.
+                mustRemainAsPointer = true;
+            }
+
+            // For compat with how out/ref parameters used to be generated, leave out/ref parameters as pointers if we're not trying to improve them.
             if (isOptional && isOut && !isComOutPtr && !improvePointersToSpansAndRefs)
             {
                 mustRemainAsPointer = true;
@@ -311,7 +320,7 @@ private IEnumerable<MethodDeclarationSyntax> DeclareFriendlyOverload(
             else if (omittableOptionalParam && omitOptionalParams)
             {
                 // Remove the optional out parameter and supply the default value for the type to the extern method.
-                if (externParamModifier.Kind() is SyntaxKind.OutKeyword || externParamModifier.Kind() is SyntaxKind.RefKeyword)
+                if (externParamModifier.Kind() is SyntaxKind.OutKeyword or SyntaxKind.RefKeyword)
                 {
                     if (externParam.Type is PointerTypeSyntax || externParam.Type is FunctionPointerTypeSyntax)
                     {
@@ -841,6 +850,130 @@ private IEnumerable<MethodDeclarationSyntax> DeclareFriendlyOverload(
                     RefExpression(localName),
                     RefExpression(nullRef)));
             }
+            else if (this.options.AllowMarshaling && isOptional && isOut && !isArray && parameterTypeInfo is PointerTypeHandleInfo pointerTypeHandleInfo && this.IsInterface(pointerTypeHandleInfo.ElementType))
+            {
+                // In source generated COM we can improve certain Optional out parameters to marshalled, e.g. IWbemServices.GetObject has some
+                // optional out parameters that need to be pointers in the ABI but in the optional overload they can be marshalled to ComWrappers.
+                TypeSyntax interfaceTypeSyntax = pointerTypeHandleInfo.ElementType.ToTypeSyntax(parameterTypeSyntaxSettings, GeneratingElement.FriendlyOverload, null).Type;
+                parameters[paramIndex] = parameters[paramIndex]
+                    .WithType(interfaceTypeSyntax.WithTrailingTrivia(TriviaList(Space)))
+                    .WithModifiers(TokenList(TokenWithSpace(isIn && isOut ? SyntaxKind.RefKeyword : (isIn ? SyntaxKind.InKeyword : SyntaxKind.OutKeyword))));
+
+                TypeSyntax nativeInterfaceTypeSyntax = ((PointerTypeSyntax)externParam.Type).ElementType;
+
+                if (!isIn)
+                {
+                    // Not a ref so need to assign first so we can use "ref" on the param. Use Unsafe.SkipInit(out origName) so that we can handle null refs.
+                    leadingOutsideTryStatements.Add(
+                        ExpressionStatement(
+                            InvocationExpression(
+                                MemberAccessExpression(
+                                    SyntaxKind.SimpleMemberAccessExpression,
+                                    IdentifierName(nameof(Unsafe)),
+                                    IdentifierName(nameof(Unsafe.SkipInit))),
+                                ArgumentList().AddArguments(Argument(origName).WithRefKindKeyword(Token(SyntaxKind.OutKeyword))))));
+                }
+
+                // For both in & out, declare a local:
+                // externParamTypeInterface* __origName_native = null;
+                IdentifierNameSyntax nativeLocal = IdentifierName($"__{origName.Identifier.ValueText.Replace("@", string.Empty)}_native");
+                leadingOutsideTryStatements.Add(
+                    LocalDeclarationStatement(VariableDeclaration(nativeInterfaceTypeSyntax)
+                        .AddVariables(VariableDeclarator(nativeLocal.Identifier)
+                            .WithInitializer(EqualsValueClause(LiteralExpression(SyntaxKind.NullLiteralExpression))))));
+
+                // bool __origName_present = !Unsafe.IsNullRef<TInterface>(origName);
+                string paramPresent = $"__{origName.Identifier.ValueText.Replace("@", string.Empty)}_present";
+                leadingOutsideTryStatements.Add(
+                    LocalDeclarationStatement(VariableDeclaration(PredefinedType(Token(SyntaxKind.BoolKeyword)))
+                        .AddVariables(VariableDeclarator(Identifier(paramPresent))
+                            .WithInitializer(EqualsValueClause(
+                                PrefixUnaryExpression(
+                                SyntaxKind.LogicalNotExpression,
+                                InvocationExpression(
+                                    MemberAccessExpression(
+                                        SyntaxKind.SimpleMemberAccessExpression,
+                                        IdentifierName(nameof(Unsafe)),
+                                        GenericName(nameof(Unsafe.IsNullRef), TypeArgumentList().AddArguments(interfaceTypeSyntax))),
+                                    ArgumentList().AddArguments(Argument(RefExpression(origName))))))))));
+
+                // If it's an in parameter, assign the native local from the managed parameter.
+                // __origName_native = (TNative)global::System.Runtime.InteropServices.Marshalling.ComInterfaceMarshaller<TInterface>.ConvertToUnmanaged(origName);
+                // Also remember the marshalled in pointer in case the callee modifies in for ref params.
+                // __origName_nativeIn = __origName_native;
+                if (isIn)
+                {
+                    ExpressionSyntax toNativeExpression = this.useSourceGenerators ?
+                        InvocationExpression(
+                            MemberAccessExpression(
+                                SyntaxKind.SimpleMemberAccessExpression,
+                                GenericName($"global::System.Runtime.InteropServices.Marshalling.ComInterfaceMarshaller", TypeArgumentList().AddArguments(interfaceTypeSyntax)),
+                                IdentifierName("ConvertToUnmanaged")),
+                            ArgumentList().AddArguments(Argument(origName))) :
+                        ParenthesizedExpression(ConditionalExpression(
+                            BinaryExpression(SyntaxKind.NotEqualsExpression, origName, LiteralExpression(SyntaxKind.NullLiteralExpression)),
+                            CastExpression(
+                                PointerType(PredefinedType(Token(SyntaxKind.VoidKeyword))),
+                                InvocationExpression(
+                                    MemberAccessExpression(
+                                        SyntaxKind.SimpleMemberAccessExpression,
+                                        ParseTypeName($"global::System.Runtime.InteropServices.Marshal"),
+                                        IdentifierName("GetIUnknownForObject")),
+                                    ArgumentList().AddArguments(Argument(origName)))),
+                            LiteralExpression(SyntaxKind.NullLiteralExpression)));
+
+                    leadingStatements.Add(
+                        IfStatement(
+                            IdentifierName(paramPresent),
+                            Block().AddStatements(
+                                ExpressionStatement(
+                                    AssignmentExpression(
+                                        SyntaxKind.SimpleAssignmentExpression,
+                                        nativeLocal,
+                                        CastExpression(
+                                            nativeInterfaceTypeSyntax,
+                                            toNativeExpression))))));
+                }
+
+                // If it's an out parameter, assign the out parameter from the native local.
+                // origName = global::System.Runtime.InteropServices.Marshalling.ComInterfaceMarshaller<TInterface>.ConvertToManaged(__origName_native);
+                if (isOut)
+                {
+                    ExpressionSyntax toManagedExpression = this.useSourceGenerators ?
+                        InvocationExpression(
+                            MemberAccessExpression(
+                                SyntaxKind.SimpleMemberAccessExpression,
+                                GenericName($"global::System.Runtime.InteropServices.Marshalling.ComInterfaceMarshaller", TypeArgumentList().AddArguments(interfaceTypeSyntax)),
+                                IdentifierName("ConvertToManaged")),
+                            ArgumentList().AddArguments(Argument(nativeLocal))) :
+                        ParenthesizedExpression(ConditionalExpression(
+                            BinaryExpression(SyntaxKind.NotEqualsExpression, nativeLocal, LiteralExpression(SyntaxKind.NullLiteralExpression)),
+                            CastExpression(interfaceTypeSyntax, InvocationExpression(
+                                MemberAccessExpression(
+                                    SyntaxKind.SimpleMemberAccessExpression,
+                                    ParseTypeName($"global::System.Runtime.InteropServices.Marshal"),
+                                    IdentifierName("GetObjectForIUnknown")),
+                                ArgumentList().AddArguments(
+                                    Argument(CastExpression(ParseName("nint"), nativeLocal))))),
+                            LiteralExpression(SyntaxKind.NullLiteralExpression)));
+
+                    trailingStatements.Add(
+                        IfStatement(
+                            IdentifierName(paramPresent),
+                            Block().AddStatements(
+                                ExpressionStatement(
+                                    AssignmentExpression(
+                                        SyntaxKind.SimpleAssignmentExpression,
+                                        origName,
+                                        toManagedExpression)))));
+                }
+
+                // Release the native pointers we have refs on.
+                finallyStatements.Add(this.COMFreeNativePointerStatement(nativeLocal, interfaceTypeSyntax));
+
+                // If it's an in parameter, pass the native local as the argument.
+                arguments[paramIndex] = arguments[paramIndex].WithExpression(ConditionalExpression(IdentifierName(paramPresent), PrefixUnaryExpression(SyntaxKind.AddressOfExpression, nativeLocal), LiteralExpression(SyntaxKind.NullLiteralExpression)));
+            }
 
             bool TryHandleCountParam(TypeSyntax elementType, bool nullableSource)
             {
@@ -1236,6 +1369,38 @@ bool TryHandleCountParam(TypeSyntax elementType, bool nullableSource)
         return helper;
     }
 
+    private StatementSyntax COMFreeNativePointerStatement(ExpressionSyntax nativePointer, TypeSyntax interfaceTypeSyntax)
+    {
+        if (this.useSourceGenerators)
+        {
+            // Release the nativeLocal via ComInterfaceMarshaller.Free.
+            return
+                ExpressionStatement(
+                    InvocationExpression(
+                        MemberAccessExpression(
+                            SyntaxKind.SimpleMemberAccessExpression,
+                            GenericName($"global::System.Runtime.InteropServices.Marshalling.ComInterfaceMarshaller", TypeArgumentList().AddArguments(interfaceTypeSyntax)),
+                            IdentifierName("Free")),
+                        ArgumentList().AddArguments(Argument(nativePointer))));
+        }
+        else
+        {
+            // Finally, release the nativeLocal via Marshal.Release.
+            return
+                IfStatement(
+                    BinaryExpression(SyntaxKind.NotEqualsExpression, nativePointer, LiteralExpression(SyntaxKind.NullLiteralExpression)),
+                    ExpressionStatement(
+                        InvocationExpression(
+                            MemberAccessExpression(
+                                SyntaxKind.SimpleMemberAccessExpression,
+                                ParseTypeName("global::System.Runtime.InteropServices.Marshal"),
+                                IdentifierName("Release")),
+                            ArgumentList().AddArguments(Argument(
+                                CastExpression(ParseName("nint"), nativePointer))))))
+                .WithCloseParenToken(TokenWithLineFeed(SyntaxKind.CloseParenToken));
+        }
+    }
+
     private class FriendlyMethodBookkeeping
     {
         public int NumSpanByteParameters { get; set; } = 0;

src/Microsoft.Windows.CsWin32/Generator.cs

@@ -1192,7 +1192,9 @@ internal void GetBaseTypeInfo(TypeDefinition typeDef, out StringHandle baseTypeN
                     throw new GenerationFailedException("Failed to parse template.");
                 }
 
-                specialDeclaration = specialDeclaration.WithAdditionalAnnotations(new SyntaxAnnotation(NamespaceContainerAnnotation, subNamespace));
+                specialDeclaration = specialDeclaration
+                    .WithAdditionalAnnotations(new SyntaxAnnotation(NamespaceContainerAnnotation, subNamespace))
+                    .AddAttributeLists(AttributeList().AddAttributes(GeneratedCodeAttribute));
 
                 this.volatileCode.AddSpecialType(specialName, specialDeclaration);
             });

src/Microsoft.Windows.CsWin32/HandleTypeHandleInfo.cs

@@ -159,7 +159,7 @@ internal override TypeSyntaxAndMarshaling ToTypeSyntax(TypeSyntaxSettings inputs
                 return new TypeSyntaxAndMarshaling(IdentifierName(specialName));
             }
         }
-        else if (useComSourceGenerators && simpleName is "VARIANT" && this.Generator.CanUseComVariant)
+        else if (useComSourceGenerators && !inputs.AllowMarshaling && (simpleName is "VARIANT" or "VARIANT_unmanaged") && this.Generator.CanUseComVariant)
         {
             return new TypeSyntaxAndMarshaling(QualifiedName(ParseName("global::System.Runtime.InteropServices.Marshalling"), IdentifierName("ComVariant")));
         }

test/CsWin32Generator.Tests/CsWin32GeneratorTests.cs

@@ -31,6 +31,9 @@ public async Task TestGenerateIDispatch()
         var methods = idispatchType.SelectMany(t => t.DescendantNodes().OfType<MethodDeclarationSyntax>());
         var method = Assert.Single(methods, m => m.Identifier.Text == "GetTypeInfoCount");
         Assert.Contains("(out uint pctinfo)", method.ParameterList.ToString());
+
+        var invokeMethods = methods.Where(m => m.Identifier.Text == "Invoke");
+        Assert.All(invokeMethods, m => Assert.DoesNotContain("VARIANT_unmanaged", m.ParameterList.ToString()));
     }
 
     [Fact]
@@ -197,10 +200,13 @@ public async Task PointerReturnValueIsPreserved()
         ["DeviceIoControl", "DeviceIoControl", "SafeHandle hDevice, uint dwIoControlCode, ReadOnlySpan<byte> lpInBuffer, Span<byte> lpOutBuffer, out uint lpBytesReturned, global::System.Threading.NativeOverlapped* lpOverlapped"],
         ["DeviceIoControl", "DeviceIoControl", "SafeHandle hDevice, uint dwIoControlCode, ReadOnlySpan<byte> lpInBuffer, Span<byte> lpOutBuffer, out uint lpBytesReturned, global::System.Threading.NativeOverlapped* lpOverlapped", true, "NativeMethods.IncludePointerOverloads.json"],
         ["NtQueryObject", "NtQueryObject", "global::Windows.Win32.Foundation.HANDLE Handle, winmdroot.Foundation.OBJECT_INFORMATION_CLASS ObjectInformationClass, Span<byte> ObjectInformation, out uint ReturnLength"],
-        // ["IWbemServices", "GetObject", "winmdroot.Foundation.BSTR, winmdroot.System.Wmi.WBEM_GENERIC_FLAG_TYPE, winmdroot.System.Wmi.IWbemContext, out winmdroot.System.Wmi.IWbemClassObject ppObject, out winmdroot.System.Wmi.IWbemCallResult ppCallResult"],
         ["ITypeInfo", "GetFuncDesc", "uint index, out winmdroot.System.Com.FUNCDESC_unmanaged* ppFuncDesc"],
         ["ITsSbResourcePluginStore", "EnumerateTargets", "winmdroot.Foundation.BSTR FarmName, winmdroot.Foundation.BSTR EnvName, winmdroot.System.RemoteDesktop.TS_SB_SORT_BY sortByFieldId, winmdroot.Foundation.BSTR sortyByPropName, ref uint pdwCount, out winmdroot.System.RemoteDesktop.ITsSbTarget_unmanaged** pVal"],
         ["MFEnumDeviceSources", "MFEnumDeviceSources", "winmdroot.Media.MediaFoundation.IMFAttributes pAttributes, out winmdroot.Media.MediaFoundation.IMFActivate_unmanaged** pppSourceActivate, out uint pcSourceActivate"],
+        // Check that GetObject optional parameters got an overload with marshalled interface types
+        ["IWbemServices", "GetObject", "this winmdroot.System.Wmi.IWbemServices @this, SafeHandle strObjectPath, winmdroot.System.Wmi.WBEM_GENERIC_FLAG_TYPE lFlags, winmdroot.System.Wmi.IWbemContext pCtx, ref winmdroot.System.Wmi.IWbemClassObject ppObject, ref winmdroot.System.Wmi.IWbemCallResult ppCallResult"],
+        // NativeOverlapped should be pointer even when not [Retained] as in CancelIoEx.
+        ["CancelIoEx", "CancelIoEx", "SafeHandle hFile, global::System.Threading.NativeOverlapped* lpOverlapped"],
     ];
 
     [Theory]