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

Attribute definitions included in assemblies using Nullable on the latest framework #85612

Closed
ericstj opened this issue May 1, 2023 · 15 comments · Fixed by #87857
Closed

Attribute definitions included in assemblies using Nullable on the latest framework #85612

ericstj opened this issue May 1, 2023 · 15 comments · Fixed by #87857
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Milestone

Comments

@ericstj
Copy link
Member

ericstj commented May 1, 2023

Description

I would expect that assemblies compiling against the latest framework would not need to define attribute types when using compiler features.

Instead I see a number of internal attributes included:

namespace System.Runtime.CompilerServices
{
    [CompilerGenerated]
    [Microsoft.CodeAnalysis.Embedded]
    internal sealed class IsUnmanagedAttribute : Attribute
    {
    }

    [CompilerGenerated]
    [Microsoft.CodeAnalysis.Embedded]
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Event | AttributeTargets.Parameter | AttributeTargets.ReturnValue | AttributeTargets.GenericParameter, AllowMultiple = false, Inherited = false)]
    internal sealed partial class NullableAttribute : Attribute
    {
        public readonly byte[] NullableFlags;
        public NullableAttribute(byte value) { }

        public NullableAttribute(byte[] value) { }
    }

    [CompilerGenerated]
    [Microsoft.CodeAnalysis.Embedded]
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Method | AttributeTargets.Interface | AttributeTargets.Delegate, AllowMultiple = false, Inherited = false)]
    internal sealed partial class NullableContextAttribute : Attribute
    {
        public readonly byte Flag;
        public NullableContextAttribute(byte value) { }
    }

    [CompilerGenerated]
    [Microsoft.CodeAnalysis.Embedded]
    [AttributeUsage(AttributeTargets.Module, AllowMultiple = false, Inherited = false)]
    internal sealed partial class NullablePublicOnlyAttribute : Attribute
    {
        public readonly bool IncludesInternals;
        public NullablePublicOnlyAttribute(bool value) { }
    }
    [CompilerGenerated]
    [Microsoft.CodeAnalysis.Embedded]
    [AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
    internal sealed class ScopedRefAttribute : Attribute
    {
    }
}

#29039 mentions that these were not included because

Any code reasoning about the attribute has to assume that attribute is emitted, so you can't optimize the path.

Correct. Language features shouldn't depend on type location, but that's true even for language feature supporting types we put in the framework. We don't put the type in the framework to make it easier for the compiler.

The size is small

I briefly measured the size and it seemed to increase an assembly by ~512 bytes by defining these types. While that's small it can add up. Across the framework assemblies in the ref packs and runtimes I found 476 assemblies defining these types accounting for >200KB size on disk. I'm also seeing another 300 assemblies in the SDK. Probably if you consider every customer assembly produced it would add up even more.

Reproduction Steps

Build a library using nullable and target net8.0.

Expected behavior

Only types defined by my project appear in my assembly.

Actual behavior

Language / framework types are emitted into my assembly.

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 1, 2023
@ghost
Copy link

ghost commented May 1, 2023

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

Issue Details

Description

I would expect that assemblies compiling against the latest framework would not need to define attribute types when using compiler features.

Instead I see a number of internal attributes included:

namespace Microsoft.CodeAnalysis
{
    [System.Runtime.CompilerServices.CompilerGenerated]
    [Embedded]
    internal sealed partial class EmbeddedAttribute : System.Attribute
    {
        public EmbeddedAttribute() { }
    }
}

namespace System.Runtime.CompilerServices
{
    [CompilerGenerated]
    [Microsoft.CodeAnalysis.Embedded]
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Event | AttributeTargets.Parameter | AttributeTargets.ReturnValue | AttributeTargets.GenericParameter, AllowMultiple = false, Inherited = false)]
    internal sealed partial class NullableAttribute : Attribute
    {
        public readonly byte[] NullableFlags;
        public NullableAttribute(byte value) { }

        public NullableAttribute(byte[] value) { }
    }

    [CompilerGenerated]
    [Microsoft.CodeAnalysis.Embedded]
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Method | AttributeTargets.Interface | AttributeTargets.Delegate, AllowMultiple = false, Inherited = false)]
    internal sealed partial class NullableContextAttribute : Attribute
    {
        public readonly byte Flag;
        public NullableContextAttribute(byte value) { }
    }

    [CompilerGenerated]
    [Microsoft.CodeAnalysis.Embedded]
    [AttributeUsage(AttributeTargets.Module, AllowMultiple = false, Inherited = false)]
    internal sealed partial class NullablePublicOnlyAttribute : Attribute
    {
        public readonly bool IncludesInternals;
        public NullablePublicOnlyAttribute(bool value) { }
    }

    [CompilerGenerated]
    [Microsoft.CodeAnalysis.Embedded]
    [AttributeUsage(AttributeTargets.Module, AllowMultiple = false, Inherited = false)]
    internal sealed partial class RefSafetyRulesAttribute : Attribute
    {
        public readonly int Version;
        public RefSafetyRulesAttribute(int value) { }
    }
}

#29039 mentions that these were not included because

Any code reasoning about the attribute has to assume that attribute is emitted, so you can't optimize the path.
The size is small

I briefly measured the size and it seemed to increase an assembly by ~512 bytes by defining these types. While that's small it can add up. Across the framework assemblies in the ref pack and runtime I found 185 assemblies defining these types accounting for ~100KB size on disk. Probably if you consider every customer assembly produced it would add up even more.

Reproduction Steps

Build a library using nullable and target net8.0.

Expected behavior

Only types defined by my project appear in my assembly.

Actual behavior

Language / framework types are emitted into my assembly.

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: ericstj
Assignees: -
Labels:

area-System.Runtime.CompilerServices

Milestone: -

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner labels May 26, 2023
@tannergooding tannergooding added this to the 8.0.0 milestone May 26, 2023
@ericstj
Copy link
Member Author

ericstj commented May 30, 2023

Defining EmbeddedAttribute results in an error CS8336, though we might not care about this one if we get all the others.

@stephentoub fixed RefSafetyRulesAttribute in 06e8c92.

I've removed these two and added two more that I found.

@ericstj
Copy link
Member Author

ericstj commented May 30, 2023

Here's a prototype of this: https://github.com/dotnet/runtime/compare/main...ericstj:runtime:addAttributes?expand=1

With that change we no longer have any assemblies in runtime that emit an EmbeddedAttribute when targeting net8.0

@terrajobst
Copy link
Member

@jaredpar

That is by design. We decided a while back that we don't care about including attributes that the user never has to apply and instead let the compiler spit them into the user's assembly.

@MichalStrehovsky
Copy link
Member

@jaredpar

That is by design. We decided a while back that we don't care about including attributes that the user never has to apply and instead let the compiler spit them into the user's assembly.

Would it be possible for the compiler to start encoding this into a single attribute type? Something like class PrivateImplementationDetailsAttribute : Attribute { public PrivateImplementationDetailsAttribute(byte[] blob) { } }.

If these are not a public contract and just an implementation detail of the compiler it would help size if we had a more compact encoding for these. Defining a new type into each compiled assembly is not very space efficient and we've been struggling to come up with ways to do something with the size hit. We accumulate new types on each release. Having a mechanism to say "this is a private implementation detail of the compiler, reflecting on these at runtime is not a scenario" would help us trimming this.

Not to mention the noise these attributes add to various tools, e.g.: https://sharplab.io/#v2:C4LghgzgtgPgAgJgIwFgBQcDMACOSBsuC2AwtgN7rbW456FwAs2AsgBQBOApgGbYCWAO2DYAHgEoKVGgF90MoA== (notice 3 methods in the disassembly even though one would reasonably expect to see one).

@jaredpar
Copy link
Member

jaredpar commented Jun 8, 2023

Would it be possible for the compiler to start encoding this into a single attribute type?

From the view point of strictly the compiler this would be a possibility to due. The problem though is that libraries and tools are already keyed into reading our attributes as they exist today. Changing to a new attribute would be a significant breaking change for all those entities.

@terrajobst
Copy link
Member

terrajobst commented Jun 8, 2023

What's the size concern? The attribute applications or having assemblies define their own copy? The latter we could probably easily change by putting these attributes in the runtime and let the compiler transparently use them over spitting its own copy.

@ericstj
Copy link
Member Author

ericstj commented Jun 8, 2023

The assemblies defining their own copy is what I was noticing. In the framework I observed a couple 100KB savings by defining the attributes in System.Runtime.

Actually changing the compiler's contract for storing this information was not in scope of this issue. I don't doubt that there could be more compact storage but that seems significantly breaking. Reflection would be one of the things broken by changing that: https://github.com/dotnet/runtime/blob/3fded0bb77c852b10d712bd629ced740499a3aa6/src/libraries/System.Private.CoreLib/src/System/Reflection/NullabilityInfoContext.cs cc @buyaa-n
IMO that ship has sailed.

Let's keep this issue solely about defining these attributes in the framework.

@MichalStrehovsky
Copy link
Member

IMO that ship has sailed.

Yes, the nullable ones are clearly a public API even if we don't want to treat it like that.

I meant more like the RefSafetyRulesAttribute and co. Those are for compiler's use as far as I know. The type definition is a paper cut, so is the ReadyToRun native code to support the attribute. But the paper cuts are multiplied per assembly and if we don't have a policy, we might soon realize that 10 years from now we have 20 such attributes encoding a couple bytes of information in many magnitudes more space than really needed.

@terrajobst
Copy link
Member

terrajobst commented Jun 20, 2023

Video

  • Looks good as proposed
    • Made the attributes public, added constructors, and removed marker attributes that specify these are generated/embedded.
  • Moving forward, we'll generally strive to add them to the framework, but there might be cases where for expedience and ability for the compiler to tweak the encoding, we start with embedding and later make the final shape a public API
    • Generally speaking, these attributes would go into corlib
namespace System.Runtime.CompilerServices;

public sealed class IsReadOnlyAttribute : Attribute
{
    public IsReadOnlyAttribute();
}

public sealed class IsByRefLikeAttribute : Attribute
{
    public IsByRefLikeAttribute();
}

public sealed class IsUnmanagedAttribute : Attribute
{
    public IsUnmanagedAttribute();
}

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Event | AttributeTargets.Parameter | AttributeTargets.ReturnValue | AttributeTargets.GenericParameter, AllowMultiple = false, Inherited = false)]
public sealed partial class NullableAttribute : Attribute
{
    public readonly byte[] NullableFlags;
    public NullableAttribute(byte value);
    public NullableAttribute(byte[] value);
}

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Method | AttributeTargets.Interface | AttributeTargets.Delegate, AllowMultiple = false, Inherited = false)]
public sealed partial class NullableContextAttribute : Attribute
{
    public readonly byte Flag;
    public NullableContextAttribute(byte value);
}

[AttributeUsage(AttributeTargets.Module, AllowMultiple = false, Inherited = false)]
public sealed partial class NullablePublicOnlyAttribute : Attribute
{
    public readonly bool IncludesInternals;
    public NullablePublicOnlyAttribute(bool value);
}

[AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
public sealed class ScopedRefAttribute : Attribute
{
    public ScopedRefAttribute();
}

[AttributeUsage(AttributeTargets.Module, AllowMultiple = false, Inherited = false)]
public sealed class RefSafetyRulesAttribute : Attribute
{
    public readonly int Version;
    public RefSafetyRulesAttribute(int value);
}

The compiler also emits NativeIntegerAttribute, but we decided to not add it to the BCL as this was only used before nint/nuint and IntPtr/UIntPtr became synonymous.

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Event | AttributeTargets.Field | AttributeTargets.GenericParameter | AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, AllowMultiple = false, Inherited = false)]
public sealed class NativeIntegerAttribute : Attribute
{
    public readonly bool[] TransformFlags;
    public NativeIntegerAttribute();
    public NativeIntegerAttribute(bool[] value);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 20, 2023
@333fred
Copy link
Member

333fred commented Jun 20, 2023

The full list of attributes generated by the compiler:

namespace System.Runtime.CompilerServices;

public sealed class IsReadOnlyAttribute : Attribute
{
    public IsReadOnlyAttribute();
}

public sealed class IsByRefLikeAttribute : Attribute
{
    public IsByRefLikeAttribute();
}

public sealed class IsUnmanagedAttribute : Attribute
{
    public IsUnmanagedAttribute();
}

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Property | AttributeTargets.Field | AttributeTargets.Event | AttributeTargets.Parameter | AttributeTargets.ReturnValue | AttributeTargets.GenericParameter, AllowMultiple = false, Inherited = false)]
public sealed partial class NullableAttribute : Attribute
{
    public readonly byte[] NullableFlags;
    public NullableAttribute(byte value);
    public NullableAttribute(byte[] value);
}

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Struct | AttributeTargets.Method | AttributeTargets.Interface | AttributeTargets.Delegate, AllowMultiple = false, Inherited = false)]
public sealed partial class NullableContextAttribute : Attribute
{
    public readonly byte Flag;
    public NullableContextAttribute(byte value);
}

[AttributeUsage(AttributeTargets.Module, AllowMultiple = false, Inherited = false)]
public sealed partial class NullablePublicOnlyAttribute : Attribute
{
    public readonly bool IncludesInternals;
    public NullablePublicOnlyAttribute(bool value);
}

[AttributeUsage(AttributeTargets.Class | AttributeTargets.Event | AttributeTargets.Field | AttributeTargets.GenericParameter | AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, AllowMultiple = false, Inherited = false)]
public sealed class NativeIntegerAttribute : Attribute
{
    public readonly bool[] TransformFlags;
    public NativeIntegerAttribute();
    public NativeIntegerAttribute(bool[] value);
}

[AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = false)]
public sealed class ScopedRefAttribute : Attribute
{
    public ScopedRefAttribute();
}

[AttributeUsage(AttributeTargets.Module, AllowMultiple = false, Inherited = false)]
public sealed class RefSafetyRulesAttribute : Attribute
{
    public readonly int Version;
    public RefSafetyRulesAttribute(int value);
}

Of these, we should probably put all except NativeIntegerAttribute in the BCL, as the compiler will never emit NativeIntegerAttribute with a .NET 7+ BCL since the IntPtr + nint merger occurred.

@stephentoub stephentoub self-assigned this Jun 20, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jun 21, 2023
@terrajobst
Copy link
Member

@333fred I have updated my comment based on what you said. Thanks for the follow-up!

@ericstj
Copy link
Member Author

ericstj commented Jun 21, 2023

IsReadOnlyAttribute can be excluded since that's been part of the framework for some time. RefSafetyRulesAttribute was recently added by @stephentoub. Thank you for accepting this proposal 🙇‍♂️.

@huoyaoyuan
Copy link
Member

In the opposite situation, IsExternalInit is required by language feature, but not generated by the compiler. What's the reason for this? Is there any chance for the compiler to generate it on lower platform?

@333fred
Copy link
Member

333fred commented Jun 22, 2023

We have no intention of changing the compiler behavior around generating new attributes at this time.

@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.CompilerServices
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants