Skip to content

Fix failures around blittable collections with [Out] and chars #89149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 19, 2023
Merged

Fix failures around blittable collections with [Out] and chars #89149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
752d026
Fixed ReadOnlySpan/Span casting failure.
jkoritzinsky Jul 18, 2023
3b7f932
Fixed char value case
jkoritzinsky Jul 18, 2023
4dfb94f
Fixed char value case
jkoritzinsky Jul 18, 2023
3ccd758
Fix managed->unmanaged pinning path
jkoritzinsky Jul 18, 2023
82b390b
Remove outdated comment
jkoritzinsky Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions ...time.InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/CharMarshaller.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,20 @@ public Utf16CharMarshaller()

public ValueBoundaryBehavior GetValueBoundaryBehavior(TypePositionInfo info, StubCodeContext context)
{
if (!info.IsByRef)
if (IsPinningPathSupported(info, context))
{
return ValueBoundaryBehavior.NativeIdentifier;
}
else if (!UsesNativeIdentifier(info, context))
{
return ValueBoundaryBehavior.ManagedIdentifier;
}
else if (IsPinningPathSupported(info, context))
else if (info.IsByRef)
{
return ValueBoundaryBehavior.NativeIdentifier;
return ValueBoundaryBehavior.AddressOfNativeIdentifier;
}

return ValueBoundaryBehavior.AddressOfNativeIdentifier;
return ValueBoundaryBehavior.NativeIdentifier;
}

public ManagedTypeInfo AsNativeType(TypePositionInfo info)
Expand Down Expand Up @@ -124,7 +128,8 @@ public IEnumerable<StatementSyntax> Generate(TypePositionInfo info, StubCodeCont

public bool UsesNativeIdentifier(TypePositionInfo info, StubCodeContext context)
{
return context.IsInStubReturnPosition(info) || (info.IsByRef && !context.SingleFrameSpansNativeContext);
MarshalDirection elementMarshalDirection = MarshallerHelpers.GetMarshalDirection(info, context);
return !IsPinningPathSupported(info, context) && (elementMarshalDirection != MarshalDirection.ManagedToUnmanaged || info.IsByRef);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I find it easier to reason about when can we pass the managed identifier to a native parameter without changing anything, and then this method is not that, but I'm also fine with whatever makes the most sense to you.

Suggested change
return !IsPinningPathSupported(info, context) && (elementMarshalDirection != MarshalDirection.ManagedToUnmanaged || info.IsByRef);
return !(IsPinningPathSupported(info, context) || (elementMarshalDirection == MarshalDirection.UnmanagedToManaged && !info.IsByRef));

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather keep it as is as the current condition better matches the code in Generate I think.

}

private static bool IsPinningPathSupported(TypePositionInfo info, StubCodeContext context)
Expand Down
51 changes: 25 additions & 26 deletions ...InteropServices/gen/Microsoft.Interop.SourceGeneration/Marshalling/ElementsMarshalling.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,38 +107,37 @@ public BlittableElementsMarshalling(TypeSyntax managedElementType, TypeSyntax un

public override StatementSyntax GenerateUnmanagedToManagedByValueOutMarshalStatement(TypePositionInfo info, StubCodeContext context)
{
ExpressionSyntax destination = CastToManagedIfNecessary(CollectionSource.GetUnmanagedValuesSource(info, context));

// MemoryMarshal.CreateSpan(ref MemoryMarshal.GetReference(<GetManagedValuesSource>), <GetManagedValuesSource>.Length)
ExpressionSyntax source = InvocationExpression(
MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
ParseName(TypeNames.System_Runtime_InteropServices_MemoryMarshal),
IdentifierName("CreateSpan")),
ArgumentList(
SeparatedList(new[]
{
Argument(
InvocationExpression(
// MemoryMarshal.CreateSpan(ref MemoryMarshal.GetReference(<GetUnmanagedValuesSource>), <GetUnmanagedValuesSource>.Length)
ExpressionSyntax destination = CastToManagedIfNecessary(
InvocationExpression(
MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
ParseName(TypeNames.System_Runtime_InteropServices_MemoryMarshal),
IdentifierName("CreateSpan")),
ArgumentList(
SeparatedList(new[]
{
Argument(
InvocationExpression(
MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression,
ParseName(TypeNames.System_Runtime_InteropServices_MemoryMarshal),
IdentifierName("GetReference")),
ArgumentList(SingletonSeparatedList(
Argument(CollectionSource.GetUnmanagedValuesSource(info, context))))))
.WithRefKindKeyword(
Token(SyntaxKind.RefKeyword)),
Argument(
MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression,
ParseName(TypeNames.System_Runtime_InteropServices_MemoryMarshal),
IdentifierName("GetReference")),
ArgumentList(SingletonSeparatedList(
Argument(CollectionSource.GetManagedValuesDestination(info, context))))))
.WithRefKindKeyword(
Token(SyntaxKind.RefKeyword)),
Argument(
MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression,
CollectionSource.GetManagedValuesDestination(info, context),
IdentifierName("Length")))
})));
CollectionSource.GetUnmanagedValuesSource(info, context),
IdentifierName("Length")))
}))));

// <source>.CopyTo(<destination>);
// <GetManagedValuesDestination>.CopyTo(<source>);
return ExpressionStatement(
InvocationExpression(
MemberAccessExpression(
SyntaxKind.SimpleMemberAccessExpression,
source,
CollectionSource.GetManagedValuesDestination(info, context),
IdentifierName("CopyTo")))
.AddArgumentListArguments(
Argument(destination)));
Expand Down
33 changes: 12 additions & 21 deletions ...ies/System.Runtime.InteropServices/tests/ComInterfaceGenerator.Unit.Tests/CompileFails.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Diagnostics;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Runtime.InteropServices.Marshalling;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis;
Expand Down Expand Up @@ -742,10 +743,6 @@ public static IEnumerable<object[]> ByValueMarshalAttributeOnPinnedMarshalledTyp
.WithLocation(0)
.WithLocation(1)
.WithArguments(SR.InOutAttributes, paramName, SR.InAttributeOnlyIsDefault);
// Pinned arrays cannot be [In]
var inAttributeNotSupportedOnBlittableArray = new DiagnosticResult(GeneratorDiagnostics.ParameterTypeNotSupportedWithDetails)
.WithLocation(0)
.WithArguments(SR.InAttributeNotSupportedWithoutOutBlittableArray, paramName);
var inAttributeNotSupportedOnPinnedParameter = new DiagnosticResult(GeneratorDiagnostics.ParameterTypeNotSupportedWithDetails)
.WithLocation(0)
.WithArguments(SR.InAttributeOnlyNotSupportedOnPinnedParameters, paramName);
Expand Down Expand Up @@ -791,24 +788,20 @@ public static IEnumerable<object[]> ByValueMarshalAttributeOnPinnedMarshalledTyp
yield return new object[] {
ID(),
codeSnippets.ByValueMarshallingOfType(inAttribute + outAttribute + constElementCount, "char[]", paramNameWithLocation, (StringMarshalling.Utf16, null)),
//https://github.com/dotnet/runtime/issues/88708
new DiagnosticResult[] { inOutAttributeIsDefaultDiagnostic }
};

// [Out] Should not warn
// https://github.com/dotnet/runtime/issues/88708
//yield return new object[] {
// ID(),
// codeSnippets.ByValueMarshallingOfType(outAttribute + constElementCount, "int[]", paramNameWithLocation),
// new DiagnosticResult[] { }
//};

// https://github.com/dotnet/runtime/issues/88708
//yield return new object[] {
// ID(),
// codeSnippets.ByValueMarshallingOfType(outAttribute + constElementCount, "char[]", paramNameWithLocation, (StringMarshalling.Utf16, null)),
// new DiagnosticResult[] { }
//};
yield return new object[] {
ID(),
codeSnippets.ByValueMarshallingOfType(outAttribute + constElementCount, "int[]", paramNameWithLocation),
new DiagnosticResult[] { }
};

yield return new object[] {
ID(),
codeSnippets.ByValueMarshallingOfType(outAttribute + constElementCount, "char[]", paramNameWithLocation, (StringMarshalling.Utf16, null)),
new DiagnosticResult[] { }
};
}

[Theory]
Expand All @@ -822,8 +815,6 @@ public async Task VerifyByValueMarshallingAttributeUsage(string id, string sourc
{
TestCode = source,
TestBehaviors = TestBehaviors.SkipGeneratedSourcesCheck,
// https://github.com/dotnet/runtime/issues/88708
CompilerDiagnostics = diagnostics.Length != 0 ? CompilerDiagnostics.None : CompilerDiagnostics.Errors,
};
test.ExpectedDiagnostics.AddRange(diagnostics);
await test.RunAsync();
Expand Down