Skip to content

Commit d303781

Browse files
jkoritzinskyjkoritzinsky
authored
Report a diagnostic for return types with HResult-like named structures and provide a code-fix to do the correct marshalling (#90282)
1 parent 2d03dc4 commit d303781

30 files changed

+581
-14
lines changed

‎src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/Analyzers/AddMarshalAsToElementFixer.cs‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
using System.Collections.Immutable;
7+
using System.Composition;
8+
using System.Runtime.InteropServices;
9+
using System.Text;
10+
using System.Threading.Tasks;
11+
using Microsoft.CodeAnalysis;
12+
using Microsoft.CodeAnalysis.CodeActions;
13+
using Microsoft.CodeAnalysis.CodeFixes;
14+
using Microsoft.CodeAnalysis.CSharp;
15+
using Microsoft.CodeAnalysis.Editing;
16+
17+
namespace Microsoft.Interop.Analyzers
18+
{
19+
[ExportCodeFixProvider(LanguageNames.CSharp), Shared]
20+
public sealed class AddMarshalAsToElementFixer : CodeFixProvider
21+
{
22+
public override FixAllProvider? GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;
23+
24+
public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(GeneratorDiagnostics.Ids.NotRecommendedGeneratedComInterfaceUsage);
25+
26+
public override async Task RegisterCodeFixesAsync(CodeFixContext context)
27+
{
28+
// Get the syntax root and semantic model
29+
Document doc = context.Document;
30+
SyntaxNode? root = await doc.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
31+
if (root == null)
32+
return;
33+
34+
SyntaxNode node = root.FindNode(context.Span);
35+
36+
foreach (var diagnostic in context.Diagnostics)
37+
{
38+
if (!diagnostic.Properties.TryGetValue(GeneratorDiagnosticProperties.AddMarshalAsAttribute, out string? addMarshalAsAttribute))
39+
{
40+
continue;
41+
}
42+
43+
foreach (var unmanagedType in addMarshalAsAttribute.Split(','))
44+
{
45+
string unmanagedTypeName = unmanagedType.Trim();
46+
context.RegisterCodeFix(
47+
CodeAction.Create(
48+
$"Add [MarshalAs(UnmanagedType.{unmanagedTypeName})]",
49+
async ct =>
50+
{
51+
DocumentEditor editor = await DocumentEditor.CreateAsync(doc, ct).ConfigureAwait(false);
52+
53+
SyntaxGenerator gen = editor.Generator;
54+
55+
SyntaxNode marshalAsAttribute = gen.Attribute(
56+
TypeNames.System_Runtime_InteropServices_MarshalAsAttribute,
57+
gen.AttributeArgument(
58+
gen.MemberAccessExpression(
59+
gen.DottedName(TypeNames.System_Runtime_InteropServices_UnmanagedType),
60+
gen.IdentifierName(unmanagedTypeName.Trim()))));
61+
62+
if (node.IsKind(SyntaxKind.MethodDeclaration))
63+
{
64+
editor.AddReturnAttribute(node, marshalAsAttribute);
65+
}
66+
else
67+
{
68+
editor.AddAttribute(node, marshalAsAttribute);
69+
}
70+
71+
return editor.GetChangedDocument();
72+
},
73+
$"AddUnmanagedType.{unmanagedTypeName}"),
74+
diagnostic);
75+
}
76+
}
77+
}
78+
}
79+
}

‎src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/Analyzers/ConvertComImportToGeneratedComInterfaceFixer.cs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Collections.Immutable;
66
using System.Composition;
77
using System.Linq;
8+
using System.Reflection;
89
using System.Threading;
910
using System.Threading.Tasks;
1011
using Microsoft.CodeAnalysis;
@@ -111,6 +112,12 @@ private static async Task ConvertComImportToGeneratedComInterfaceAsync(DocumentE
111112
var generatedDeclaration = member;
112113

113114
generatedDeclaration = AddExplicitDefaultBoolMarshalling(gen, method, generatedDeclaration, "VariantBool");
115+
116+
if (method.MethodImplementationFlags.HasFlag(MethodImplAttributes.PreserveSig))
117+
{
118+
generatedDeclaration = AddHResultStructAsErrorMarshalling(gen, method, generatedDeclaration);
119+
}
120+
114121
editor.ReplaceNode(member, generatedDeclaration);
115122
}
116123

‎src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/ComInterfaceGenerator.cs‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,13 @@ private static MemberDeclarationSyntax GenerateIUnknownDerivedAttributeApplicati
211211
.WithTypeParameterList(context.ContainingSyntax.TypeParameters)
212212
.AddAttributeLists(AttributeList(SingletonSeparatedList(s_iUnknownDerivedAttributeTemplate))));
213213

214+
private static bool IsHResultLikeType(ManagedTypeInfo type)
215+
{
216+
string typeName = type.FullTypeName.Split('.', ':')[^1];
217+
return typeName.Equals("hr", StringComparison.OrdinalIgnoreCase)
218+
|| typeName.Equals("hresult", StringComparison.OrdinalIgnoreCase);
219+
}
220+
214221
private static IncrementalMethodStubGenerationContext CalculateStubInformation(MethodDeclarationSyntax syntax, IMethodSymbol symbol, int index, StubEnvironment environment, ManagedTypeInfo owningInterface, CancellationToken ct)
215222
{
216223
ct.ThrowIfCancellationRequested();
@@ -280,7 +287,7 @@ private static IncrementalMethodStubGenerationContext CalculateStubInformation(M
280287
{
281288
if ((returnSwappedSignatureElements[i].ManagedType is SpecialTypeInfo { SpecialType: SpecialType.System_Int32 or SpecialType.System_Enum } or EnumTypeInfo
282289
&& returnSwappedSignatureElements[i].MarshallingAttributeInfo.Equals(NoMarshallingInfo.Instance))
283-
|| (returnSwappedSignatureElements[i].ManagedType.FullTypeName.Split('.', ':').LastOrDefault()?.ToLowerInvariant() is "hr" or "hresult"))
290+
|| (IsHResultLikeType(returnSwappedSignatureElements[i].ManagedType)))
284291
{
285292
generatorDiagnostics.ReportDiagnostic(DiagnosticInfo.Create(GeneratorDiagnostics.ComMethodManagedReturnWillBeOutVariable, symbol.Locations[0]));
286293
}
@@ -310,6 +317,23 @@ private static IncrementalMethodStubGenerationContext CalculateStubInformation(M
310317
})
311318
};
312319
}
320+
else
321+
{
322+
// If our method is PreserveSig, we will notify the user if they are returning a type that may be an HRESULT type
323+
// that is defined as a structure. These types used to work with built-in COM interop, but they do not work with
324+
// source-generated interop as we now use the MemberFunction calling convention, which is more correct.
325+
TypePositionInfo? managedReturnInfo = signatureContext.ElementTypeInformation.FirstOrDefault(e => e.IsManagedReturnPosition);
326+
if (managedReturnInfo is { MarshallingAttributeInfo: UnmanagedBlittableMarshallingInfo, ManagedType: ValueTypeInfo valueType }
327+
&& IsHResultLikeType(valueType))
328+
{
329+
generatorDiagnostics.ReportDiagnostic(DiagnosticInfo.Create(
330+
GeneratorDiagnostics.HResultTypeWillBeTreatedAsStruct,
331+
symbol.Locations[0],
332+
ImmutableDictionary<string, string>.Empty.Add(GeneratorDiagnosticProperties.AddMarshalAsAttribute, "Error"),
333+
valueType.DiagnosticFormattedName));
334+
}
335+
}
336+
313337
var direction = GetDirectionFromOptions(generatedComInterfaceAttributeData.Options);
314338

315339
// Ensure the size of collections are known at marshal / unmarshal in time.

‎src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/ComInterfaceGenerator.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
<Compile Include="..\Common\OperationExtensions.cs" Link="Common\OperationExtensions.cs" />
2828
<Compile Include="..\Common\ConvertToSourceGeneratedInteropFixer.cs" Link="Common\ConvertToSourceGeneratedInteropFixer.cs" />
2929
<Compile Include="..\Common\FixAllContextExtensions.cs" Link="Common\FixAllContextExtensions.cs" />
30+
<Compile Include="$(CoreLibSharedDir)System\Index.cs" Link="Common\System\Index.cs" />
3031
</ItemGroup>
3132

3233
<ItemGroup>

‎src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/ComInterfaceGeneratorHelpers.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ internal static class ComInterfaceGeneratorHelpers
3232
InteropGenerationOptions interopGenerationOptions = new(UseMarshalType: true);
3333
generatorFactory = new MarshalAsMarshallingGeneratorFactory(interopGenerationOptions, generatorFactory);
3434

35+
generatorFactory = new StructAsHResultMarshallerFactory(generatorFactory);
36+
3537
IMarshallingGeneratorFactory elementFactory = new AttributedMarshallingModelGeneratorFactory(
3638
// Since the char type in an array will not be part of the P/Invoke signature, we can
3739
// use the regular blittable marshaller in all cases.

‎src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/GeneratorDiagnostics.cs‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -474,6 +474,16 @@ public class Ids
474474
DiagnosticSeverity.Info,
475475
isEnabledByDefault: true);
476476

477+
/// <inheritdoc cref="SR.HResultTypeWillBeTreatedAsStructMessage"/>
478+
public static readonly DiagnosticDescriptor HResultTypeWillBeTreatedAsStruct =
479+
new DiagnosticDescriptor(
480+
Ids.NotRecommendedGeneratedComInterfaceUsage,
481+
GetResourceString(nameof(SR.HResultTypeWillBeTreatedAsStructTitle)),
482+
GetResourceString(nameof(SR.HResultTypeWillBeTreatedAsStructMessage)),
483+
Category,
484+
DiagnosticSeverity.Info,
485+
isEnabledByDefault: true);
486+
477487
/// <summary>
478488
/// Report diagnostic for invalid configuration for string marshalling.
479489
/// </summary>

‎src/libraries/System.Runtime.InteropServices/gen/ComInterfaceGenerator/Marshallers/StructAsHResultMarshallerFactory.cs‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
using System.Runtime.InteropServices;
7+
using System.Text;
8+
using Microsoft.CodeAnalysis.CSharp;
9+
using Microsoft.CodeAnalysis.CSharp.Syntax;
10+
using static Microsoft.CodeAnalysis.CSharp.SyntaxFactory;
11+
12+
namespace Microsoft.Interop
13+
{
14+
internal sealed class StructAsHResultMarshallerFactory : IMarshallingGeneratorFactory
15+
{
16+
private static readonly Marshaller s_marshaller = new();
17+
18+
private readonly IMarshallingGeneratorFactory _inner;
19+
20+
public StructAsHResultMarshallerFactory(IMarshallingGeneratorFactory inner)
21+
{
22+
_inner = inner;
23+
}
24+
25+
public ResolvedGenerator Create(TypePositionInfo info, StubCodeContext context)
26+
{
27+
// Value type with MarshalAs(UnmanagedType.Error), to be marshalled as an unmanaged HRESULT.
28+
if (info is { ManagedType: ValueTypeInfo, MarshallingAttributeInfo: MarshalAsInfo(UnmanagedType.Error, _) })
29+
{
30+
return ResolvedGenerator.Resolved(s_marshaller);
31+
}
32+
33+
return _inner.Create(info, context);
34+
}
35+
36+
private sealed class Marshaller : IMarshallingGenerator
37+
{
38+
public ManagedTypeInfo AsNativeType(TypePositionInfo info) => SpecialTypeInfo.Int32;
39+
40+
public IEnumerable<StatementSyntax> Generate(TypePositionInfo info, StubCodeContext context)
41+
{
42+
var (managed, unmanaged) = context.GetIdentifiers(info);
43+
44+
switch (context.CurrentStage)
45+
{
46+
case StubCodeContext.Stage.Marshal:
47+
if (MarshallerHelpers.GetMarshalDirection(info, context) is MarshalDirection.ManagedToUnmanaged or MarshalDirection.Bidirectional)
48+
{
49+
// unmanaged = Unsafe.BitCast<managedType, int>(managed);
50+
yield return ExpressionStatement(
51+
AssignmentExpression(SyntaxKind.SimpleAssignmentExpression,
52+
IdentifierName(unmanaged),
53+
InvocationExpression(
54+
MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression,
55+
ParseTypeName(TypeNames.System_Runtime_CompilerServices_Unsafe),
56+
GenericName(Identifier("BitCast"),
57+
TypeArgumentList(
58+
SeparatedList(
59+
new[]
60+
{
61+
info.ManagedType.Syntax,
62+
AsNativeType(info).Syntax
63+
})))),
64+
ArgumentList(SingletonSeparatedList(Argument(IdentifierName(managed)))))));
65+
}
66+
break;
67+
case StubCodeContext.Stage.Unmarshal:
68+
if (MarshallerHelpers.GetMarshalDirection(info, context) is MarshalDirection.UnmanagedToManaged or MarshalDirection.Bidirectional)
69+
{
70+
// managed = Unsafe.BitCast<int, managedType>(unmanaged);
71+
yield return ExpressionStatement(
72+
AssignmentExpression(SyntaxKind.SimpleAssignmentExpression,
73+
IdentifierName(managed),
74+
InvocationExpression(
75+
MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression,
76+
ParseTypeName(TypeNames.System_Runtime_CompilerServices_Unsafe),
77+
GenericName(Identifier("BitCast"),
78+
TypeArgumentList(
79+
SeparatedList(
80+
new[]
81+
{
82+
AsNativeType(info).Syntax,
83+
info.ManagedType.Syntax
84+
})))),
85+
ArgumentList(SingletonSeparatedList(Argument(IdentifierName(unmanaged)))))));
86+
}
87+
break;
88+
default:
89+
break;
90+
}
91+
}
92+
93+
public SignatureBehavior GetNativeSignatureBehavior(TypePositionInfo info)
94+
{
95+
return info.IsByRef ? SignatureBehavior.PointerToNativeType : SignatureBehavior.NativeType;
96+
}
97+
98+
public ValueBoundaryBehavior GetValueBoundaryBehavior(TypePositionInfo info, StubCodeContext context)
99+
{
100+
if (info.IsByRef)
101+
{
102+
return ValueBoundaryBehavior.AddressOfNativeIdentifier;
103+
}
104+
105+
return ValueBoundaryBehavior.NativeIdentifier;
106+
}
107+
108+
public bool IsSupported(TargetFramework target, Version version) => target == TargetFramework.Net && version.Major >= 8;
109+
110+
public ByValueMarshalKindSupport SupportsByValueMarshalKind(ByValueContentsMarshalKind marshalKind, TypePositionInfo info, StubCodeContext context, out GeneratorDiagnostic? diagnostic)
111+
=> ByValueMarshalKindSupportDescriptor.Default.GetSupport(marshalKind, info, context, out diagnostic);
112+
113+
public bool UsesNativeIdentifier(TypePositionInfo info, StubCodeContext context) => true;
114+
}
115+
}
116+
}

‎src/libraries/System.Runtime.InteropServices/gen/Common/ConvertToSourceGeneratedInteropFixer.cs‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
using System.Text;
99
using System.Threading;
1010
using System.Threading.Tasks;
11+
using System.Runtime.InteropServices;
1112

1213
using Microsoft.CodeAnalysis;
1314
using Microsoft.CodeAnalysis.CodeActions;
@@ -259,5 +260,34 @@ static SyntaxNode GenerateMarshalAsUnmanagedTypeBoolAttribute(SyntaxGenerator ge
259260
generator.DottedName(TypeNames.System_Runtime_InteropServices_UnmanagedType),
260261
generator.IdentifierName(unmanagedTypeMemberIdentifier))));
261262
}
263+
264+
protected static SyntaxNode AddHResultStructAsErrorMarshalling(SyntaxGenerator generator, IMethodSymbol methodSymbol, SyntaxNode generatedDeclaration)
265+
{
266+
if (methodSymbol.ReturnType is { TypeKind: TypeKind.Struct }
267+
&& IsHResultLikeType(methodSymbol.ReturnType)
268+
&& !methodSymbol.GetReturnTypeAttributes().Any(attr => attr.AttributeClass?.ToDisplayString() == TypeNames.System_Runtime_InteropServices_MarshalAsAttribute))
269+
{
270+
generatedDeclaration = generator.AddReturnAttributes(generatedDeclaration,
271+
GeneratedMarshalAsUnmanagedTypeErrorAttribute(generator));
272+
}
273+
274+
return generatedDeclaration;
275+
276+
277+
static bool IsHResultLikeType(ITypeSymbol type)
278+
{
279+
string typeName = type.Name;
280+
return typeName.Equals("hr", StringComparison.OrdinalIgnoreCase)
281+
|| typeName.Equals("hresult", StringComparison.OrdinalIgnoreCase);
282+
}
283+
284+
// MarshalAs(UnmanagedType.Error)
285+
static SyntaxNode GeneratedMarshalAsUnmanagedTypeErrorAttribute(SyntaxGenerator generator)
286+
=> generator.Attribute(TypeNames.System_Runtime_InteropServices_MarshalAsAttribute,
287+
generator.AttributeArgument(
288+
generator.MemberAccessExpression(
289+
generator.DottedName(TypeNames.System_Runtime_InteropServices_UnmanagedType),
290+
generator.IdentifierName(nameof(UnmanagedType.Error)))));
291+
}
262292
}
263293
}

‎src/libraries/System.Runtime.InteropServices/gen/Common/Resources/Strings.resx‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -883,4 +883,10 @@
883883
<data name="ComMethodReturningIntWillBeOutParameterTitle" xml:space="preserve">
884884
<value>The return value in the managed definition will be converted to an additional 'out' parameter at the end of the parameter list when calling the unmanaged COM method.</value>
885885
</data>
886+
<data name="HResultTypeWillBeTreatedAsStructMessage" xml:space="preserve">
887+
<value>The type '{0}' will be treated as a struct in the native signature, not as a native HRESULT. To treat this as an HRESULT, add '[return:MarshalAs(UnmanagedType.Error)]' to the method.</value>
888+
</data>
889+
<data name="HResultTypeWillBeTreatedAsStructTitle" xml:space="preserve">
890+
<value>This type will be treated as a struct in the native signature, not as a native HRESULT</value>
891+
</data>
886892
</root>

‎src/libraries/System.Runtime.InteropServices/gen/Common/Resources/xlf/Strings.cs.xlf‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,16 @@
437437
<target state="translated">Poskytnutý graf obsahuje cykly a nelze ho řadit topologicky.</target>
438438
<note />
439439
</trans-unit>
440+
<trans-unit id="HResultTypeWillBeTreatedAsStructMessage">
441+
<source>The type '{0}' will be treated as a struct in the native signature, not as a native HRESULT. To treat this as an HRESULT, add '[return:MarshalAs(UnmanagedType.Error)]' to the method.</source>
442+
<target state="new">The type '{0}' will be treated as a struct in the native signature, not as a native HRESULT. To treat this as an HRESULT, add '[return:MarshalAs(UnmanagedType.Error)]' to the method.</target>
443+
<note />
444+
</trans-unit>
445+
<trans-unit id="HResultTypeWillBeTreatedAsStructTitle">
446+
<source>This type will be treated as a struct in the native signature, not as a native HRESULT</source>
447+
<target state="new">This type will be treated as a struct in the native signature, not as a native HRESULT</target>
448+
<note />
449+
</trans-unit>
440450
<trans-unit id="InAttributeNotSupportedWithoutOutBlittableArray">
441451
<source>The '[In]' attribute is not supported unless the '[Out]' attribute is also used. Blittable arrays cannot be marshalled as '[In]' only.</source>
442452
<target state="translated">Atribut [In] není podporován, pokud není použit také atribut [Out]. Blittable arrays nelze zařadit pouze jako [In].</target>

0 commit comments

Comments
 (0)