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

Add TFM and version support query to marshaller interface. #61064

Merged
merged 4 commits into from
Nov 3, 2021

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Nov 1, 2021

Fixes #60662

If any marshaller fails to support the current tfm version, fallback to using the built-in DllImport attribute.

This logic permits using the source generator for any .NET TFM and will gracefully "fallback" to only support platforms that can consume the generated source.

/cc @jkoritzinsky @elinor-fung @stephentoub @eerhardt

If any marshaller fails to support the current tfm version, fallback
to using the built-in DllImport attribute.
@AaronRobinsonMSFT AaronRobinsonMSFT added area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature labels Nov 1, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT added this to the 7.0.0 milestone Nov 1, 2021
Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

The approach for checking supportability based on TFM works great for any out-of-band projects and any external customers, but still might cause issues with solving the following item on our list of features to productize:

Enable DllImportGenerator by default for all source projects instead of just NETCoreApp with specific references (closer to an in-box generator experience)

One option to solve this problem would be to just say "we'll enable the generator for all source projects and if someone adds a stub that needs a missing type, we'll the dotnet/runtime dev will see an error message about a type that's not available and they'll have to add a reference themselves". Given how rare this will likely be (maybe once in a release cycle max), I think this could be a reasonable solution if we add something in the docs about it.

@AaronRobinsonMSFT
Copy link
Member Author

AaronRobinsonMSFT commented Nov 1, 2021

but still might cause issues with solving the following item on our list of features to productize

Can you elaborate on why this would be an issue? The source generator is restricted to using public APIs only — similar to CrossGen2. Is there an example when this would fail?

@jkoritzinsky
Copy link
Member

The problem would be when we're in the product since assemblies in the product reference a subset of the ref assemblies. For example, we added a direct reference to System.Runtime.CompilerServices.Unsafe to System.Console in #59579 to ensure that the Unsafe class was available for use.

As of today, every assembly with P/Invokes in dotnet/runtime references the assemblies needed, but that isn't guaranteed to be the case if someone adds a new assembly in the repo.

Copy link
Member

@jkoritzinsky jkoritzinsky left a comment

Choose a reason for hiding this comment

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

As we discussed offline, my concerns are for a small enough group (a small subset of dotnet/runtime contributors) that we can add some in-repo documentation to handle this case when we update our dotnet/runtime interop best practices/guidance.

@elinor-fung
Copy link
Member

I think we should add:

  • doc about the target framework support/fallback behaviour - new section in the compatibility doc? That doc currently says we require .NET 6+.
  • unit tests for down-level targets
    • signature that should be supported to validate we generate a stub
    • signature that should not be supported to validate we generate a forwarder

…allers

  are supported on the target framework.

Update compatibility document with fallback support.
TestUtils.AssertPreSourceGeneratorCompilation(comp);

var newComp = TestUtils.RunGenerators(
comp,
new DllImportGeneratorOptionsProvider(useMarshalType: false, generateForwarders: true),
Copy link
Member

Choose a reason for hiding this comment

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

You gon rid of the test for the GenerateForwarders option?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. Is this something we want to validate?

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 6527f54 into dotnet:main Nov 3, 2021
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the check_framework branch November 3, 2021 04:27
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DllImportGenerator] Support down-level targets
3 participants