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

New ILLinker attributes are not available outside of net5.0 #36656

Closed
eerhardt opened this issue May 18, 2020 · 16 comments
Closed

New ILLinker attributes are not available outside of net5.0 #36656

eerhardt opened this issue May 18, 2020 · 16 comments
Labels
area-System.Runtime.CompilerServices linkable-framework Issues associated with delivering a linker friendly framework
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented May 18, 2020

We've added a few attributes to net5.0 to help with linker analysis. See

If a netstandard library needs to add these annotations, it will be forced to cross-compile to net5.0 just so it can add these attributes. And then of course add #if to all the attribute declarations.

We should make these attributes available down-level. They are simple attributes, and don't have dependencies on newer APIs, so they should be able to be supported on netstandard1.0.

However, this would mean shipping a new package.

An alternative to this would be developers would need to declare the internal attributes themselves (but not in net5.0, since that would conflict).

cc @vitek-karas @marek-safar @stephentoub @terrajobst @ericstj

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label May 18, 2020
@eerhardt eerhardt added the linkable-framework Issues associated with delivering a linker friendly framework label May 18, 2020
@marek-safar
Copy link
Contributor

It'd be breaking change to add them to existing netstandard version and I think going forward the communication is that net5 is the new netstandard.

@eerhardt
Copy link
Member Author

eerhardt commented May 18, 2020

It'd be breaking change to add them to existing netstandard version

I don't think this is true. We have made new types available down-level with:

@stephentoub
Copy link
Member

It's a breaking change to add them to netstandard itself. It's not a breaking change to ship an OOB NuGet package that references an existing netstandard... but we only do so in limited circumstances.

@eerhardt
Copy link
Member Author

Note that this affects our own libraries that build for TFMs other than net5.0 - for example System.Text.Json, Microsoft.Extensions.*, etc. In order to add these new attributes to these libraries we will need to do one of the following:

  1. Make the attributes available down-level
  2. Compile internal versions of these attributes in each library when building for non-net5.0 TFMs
  3. Add #if around the attribute usage
    • This may not be possible if the library doesn't build for net5.0 already. Which means it would need to start building for net5.0, if it needs one of these attributes.

@stephentoub
Copy link
Member

Compile internal versions of these attributes in each library when building for non-net5.0 TFMs

FYI, we already do this for the nullable attributes.

@eerhardt
Copy link
Member Author

we already do this for the nullable attributes.

My understanding is the difference with the nullable attributes is that we don't support nullablity down-level. Do we plan on having the same restrictions for "linker-friendliness"?

@stephentoub
Copy link
Member

Whether or not it's officially supported, developers use C# 8 features like nullable reference types with netstandard2.0.

@stephentoub
Copy link
Member

stephentoub commented May 18, 2020

In any event, I don't think it's worthwhile doing (1) currently. All of the work necessary to make the libraries linker-friendly is in .NET 5; the experience of working in alternate runtimes is going to be far less than ideal, both in terms of resulting size and in terms of warnings. I suggest we do (2) above, as it's a) straightforward, b) doesn't require a ton of ifdefing, and c) will still include the attributes in any netstandard libs we do ship and that use them. And we can always graduate from that to (1) if the situation proves it valuable.

@eerhardt
Copy link
Member Author

And we recommend our customers do the same thing?

For example, Azure.Storage.Blobs only builds for netstandard2.0, and when I try linking an app using it, I'm getting a linker warning here:

https://github.com/Azure/azure-sdk-for-net/blob/d783d90cdf1e426a9404c029e2dca3f29a55cc4f/sdk/core/Azure.Core/src/Shared/ClientDiagnostics.cs#L177-L186

        internal static string? GetResourceProviderNamespace(Assembly assembly)
        {
            foreach (var customAttribute in assembly.GetCustomAttributes(true))
            {
                // Weak bind internal shared type
                var attributeType = customAttribute.GetType();
                if (attributeType.Name == "AzureResourceProviderNamespaceAttribute")
                {
                    return attributeType.GetProperty("ResourceProviderNamespace")?.GetValue(customAttribute) as string;
                }

If they wanted to annotate this with the above attributes, we would recommend to them to copy our attributes, mark them internal, and compile them into their assembly?

@stephentoub
Copy link
Member

I'd hope more likely would be recommending the library cross-compile, building a .NET 5 asset, and taking advantage not only of the new attributes for linking but all of the other functionality .NET has to offer that's not in netstandard2.0.

@MichalStrehovsky
Copy link
Member

If they wanted to annotate this with the above attributes, we would recommend to them to copy our attributes, mark them internal, and compile them into their assembly?

In that specific case, the advice would be to do foreach (CustomAttributeData caData in CustomAttributeData.GetCustomAttributes(assembly)) that doesn't require any annotations whatsoever :).

@vitek-karas
Copy link
Member

@MichalStrehovsky That's not entirely correct - the property access would still break - we do preserve attributes but we don't preserve property getters on them.

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky That's not entirely correct - the property access would still break - we do preserve attributes but we don't preserve property getters on them.

CustomAttributeData doesn't invoke the property getters or setters (there's no instance of the attribute created so no need to set or get anything from it). CustomAttributeData is designed to work with reflection only load.

Linker would have kept the property because it's referenced from the attribute and that's all what is needed.

@vitek-karas
Copy link
Member

You're right - I missed the difference. The code as posted by @eerhardt is very problematic, but the one using CustomAttributeData is not.
I wonder if we could somehow detect the bad case and issue a warning suggesting the possible workaround. I guess we could "extend" the object.GetType to remember the declared type (even if it was not sealed) and return something like "SystemTypeDerivedFrom" node. And then when we see that it's an Attribute we could suggest using CustomAttributeData instead.

@Anipik Anipik added this to the 5.0.0 milestone Jun 24, 2020
@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Jul 9, 2020
@eerhardt
Copy link
Member Author

Closing since we've decided not to make these attributes available outside of net5.0. If a library needs to apply them in a down-level assembly, they can compile internal versions of these attributes into the library.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.CompilerServices linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

No branches or pull requests

8 participants