Skip to content
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.

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

Use bidirectional marshallers for elements. #90056

Merged
merged 5 commits into from
Aug 8, 2023

Conversation

jkoritzinsky
Copy link
Member

Fixes #89893

Also updates the analyzer to require the element shapes to be bidirectional in all cases.

Technically this is a source-breaking change in the analyzer, but since it can be suppressed like all analyzers, I'm not sure if we need to fill out a breaking-change form.

@ghost
Copy link

ghost commented Aug 4, 2023

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

Issue Details

Fixes #89893

Also updates the analyzer to require the element shapes to be bidirectional in all cases.

Technically this is a source-breaking change in the analyzer, but since it can be suppressed like all analyzers, I'm not sure if we need to fill out a breaking-change form.

Author: jkoritzinsky
Assignees: -
Labels:

area-System.Runtime.InteropServices

Milestone: 8.0.0

…ectional in the generator.

Only require it if a marshaller is explicitly Element*.

Aka, only enforce the requirement in the analyzer, not the generator.
@jkoritzinsky
Copy link
Member Author

I've figured out a way to limit the breaking change to cases where the user explicitly specifies the ElementIn, ElementRef, or ElementOut marshal modes. I think those cases will be rare enough that merging this in this late in the release may be okay.

I found one user outside of dotnet/runtime and dotnet/samples that uses the Element marshaller modes specifically: https://grep.app/search?q=MarshalMode.Element.

If we're not sure, we can also merge this into .NET 9 as soon as we branch.

@AaronRobinsonMSFT @elinor-fung what do you think?

@AaronRobinsonMSFT
Copy link
Member

I found one user outside of dotnet/runtime and dotnet/samples that uses the Element marshaller modes specifically: https://grep.app/search?q=MarshalMode.Element.

That looks like a rather popular library. How impactful will this be to them?

@jkoritzinsky
Copy link
Member Author

They should be able to change the marshaller that they're using to the stateless marshaller that they already have for the ElementRef case.

I can also try to make the errors into warnings for the Element modes (like we do with the Default mode) to reduce the impact further.

…the repo.

Update analyzer unit and integration tests.
@jkoritzinsky jkoritzinsky added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Aug 8, 2023
@ghost ghost added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Aug 8, 2023
@ghost
Copy link

ghost commented Aug 8, 2023

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

  1. Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
  2. Ask a committer to mail the .NET Breaking Change Notification DL.

Tagging @dotnet/compat for awareness of the breaking change.

@jkoritzinsky jkoritzinsky merged commit 9666832 into dotnet:main Aug 8, 2023
113 checks passed
@jkoritzinsky jkoritzinsky deleted the fix-element-marshaller-mode branch August 8, 2023 16:28
@lambdageek
Copy link
Member

@jkoritzinsky I'm seeing

##[error]src/libraries/System.Runtime.InteropServices/tests/TestAssets/SharedTypes/ComInterfaces/IStatelessPinnableCollectionBlittableElements.cs(53,92): error SYSLIB1057: (NETCORE_ENGINEERING_TELEMETRY=Build) The type 'SharedTypes.ComInterfaces.StatelessPinnableCollectionMarshaller<T, TUnmanagedElement>.ManagedToUnmanaged' specifies that it supports the 'ElementIn' marshal mode for 'SharedTypes.ComInterfaces.StatelessPinnableCollection<T>' but does not provide a two-parameter 'AllocateContainerForManagedElements' method that takes the unmanaged type as the first parameter and an 'int' as the second parameter

from Linux and Windows libraries builds on my PR (#90166)

@jtschuster
Copy link
Member

jtschuster commented Aug 8, 2023

@lambdageek I think I merged a PR with a new test type that this PR warns on between when this passed CI and when it was merged. I'll get a fix out, thanks for the head up!

edit: PR is here: #90176

@ghost ghost locked as resolved and limited conversation to collaborators Sep 7, 2023
@jkoritzinsky jkoritzinsky removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices breaking-change Issue or PR that represents a breaking API or functional change over a prerelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ComInterfaceGenerator] PreserveSig with collection return type will not properly marshal back to caller
4 participants