search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.18k stars 4.72k forks source link

Custom Marshaller analyzers don't warn that bidirectional marshallers require a non-caller allocated buffer method, but ComInterfaceGenerator requires it #90112

Open jtschuster opened 1 year ago

jtschuster commented 1 year ago

These lines make it seem like there should be some kind of warning.

https://github.com/dotnet/runtime/blob/dd43febd533ff740a5941701239bac904279d9f8/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/ManualTypeMarshallingHelper.cs#L216-L225

These lines require that the non-caller-allocated-buffer method is present for bidirectional.

https://github.com/dotnet/runtime/blob/dd43febd533ff740a5941701239bac904279d9f8/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/ManualTypeMarshallingHelper.cs#L549-L551

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/interop-contrib See info in area-owners.md if you want to be subscribed.

Issue Details
These lines make it seem like there should be some kind of warning. https://github.com/dotnet/runtime/blob/dd43febd533ff740a5941701239bac904279d9f8/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/ManualTypeMarshallingHelper.cs#L216-L225 These lines require that the non-caller-allocated-buffer method is present for bidirectional. https://github.com/dotnet/runtime/blob/dd43febd533ff740a5941701239bac904279d9f8/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/ManualTypeMarshallingHelper.cs#L549-L551
Author: jtschuster
Assignees: -
Labels: `area-System.Runtime.InteropServices`
Milestone: 9.0.0
jkoritzinsky commented 1 year ago

Looks like this is another case in the analyzer that got lost in the refactor. We used to warn for this case. I think it's too late in .NET 8 to add the warning given that the snap is tomorrow and I probably won't get to it today, but we can add a notice in 8 and make it a warning in 9.

jkoritzinsky commented 1 year ago

Actually, it looks like the stateless and stateful code should handle this case as long as the marshaller isn't MarshalMode.Default, at least at usage time. We should probably add a check in the analyzer as well.

https://github.com/dotnet/runtime/blob/dd43febd533ff740a5941701239bac904279d9f8/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/ManualTypeMarshallingHelper.cs#L503-L505

https://github.com/dotnet/runtime/blob/dd43febd533ff740a5941701239bac904279d9f8/src/libraries/System.Runtime.InteropServices/gen/Microsoft.Interop.SourceGeneration/ManualTypeMarshallingHelper.cs#L583-L585

jtschuster commented 1 year ago

Actually, it looks like the stateless and stateful code should handle this case as long as the marshaller isn't MarshalMode.Default, at least at usage time. We should probably add a check in the analyzer as well.

Yes, sorry, I forgot to clarify, it will warn when someone tries to use a ref parameter, but everything else indicates that the marshaller should be okay. There's still a [CustomMarshaller(typeof(Managed), MarshalMode.XRef, typeof(Bidirectional))] with no warnings, but when used in a method the analyzer says there isn't a marshaller for MarshalMode.XRef.