Why are private nested classes included in ref assemblies?

[drewnoakes]

I've been working with a customer (@Mike-E-angelo) on a build performance issue he reported.

Specifically, this change triggers a cascade of builds that takes a minute to complete:

This changes the capture class, which in turn changes the ref assembly, which in turn causes all referencing projects (transitively) to build, which takes a long time.

Looking at the reference assemblies shows it's only an unused private nested class that changes:

 .class public auto ansi sealed beforefieldinit Starbeam.Features.Production.Issuance.Production.LicenseDisplayName
         extends class [DragonSpark.Application]DragonSpark.Application.Entities.Queries.Composition.Projection`2<class [Starbeam.Entities]Starbeam.Entities.Marketplace.Publishing.MarketplaceLicense, string>
 {
         .custom instance void [System.Runtime]System.Runtime.CompilerServices.NullableContextAttribute::.ctor(uint8) = (
                 01 00 01 00 00
         )
         .custom instance void [System.Runtime]System.Runtime.CompilerServices.NullableAttribute::.ctor(uint8[]) = (
                 01 00 03 00 00 00 00 01 01 00 00
         )
         // Nested Types
-       .class nested private auto ansi sealed serializable beforefieldinit '<>c'
+       .class nested private auto ansi sealed beforefieldinit '<>c__DisplayClass4_0'
                 extends [System.Runtime]System.Object
         {
                 .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = (
                         01 00 00 00
                 )
                 // Fields
-               .field public static initonly class Starbeam.Features.Production.Issuance.Production.LicenseDisplayName/'<>c' '<>9'
-               .custom instance void [System.Runtime]System.Runtime.CompilerServices.NullableAttribute::.ctor(uint8) = (
-                       01 00 00 00 00
+               .field public class [System.Linq.Expressions]System.Linq.Expressions.Expression`1<class [System.Runtime]System.Func`2<class [Starbeam.Entities]Starbeam.Entities.Marketplace.Publishing.Issuance, string>> name
+               .custom instance void [System.Runtime]System.Runtime.CompilerServices.NullableAttribute::.ctor(uint8[]) = (
+                       01 00 04 00 00 00 00 01 01 01 00 00
 
                 // Methods
                 .method public hidebysig specialname rtspecialname 
                         instance void .ctor () cil managed 
                 {
                         // Method begins at RVA 0x4050
                         // Header size: 1
                         // Code size: 2 (0x2)
                         .maxstack 8
 
                         // throw null;
                         IL_0000: ldnull
                         IL_0001: throw
-               } // end of method '<>c'::.ctor
+               } // end of method '<>c__DisplayClass4_0'::.ctor

-       } // end of class <>c
+       } // end of class <>c__DisplayClass4_0
 
 
         // Methods
         .method public hidebysig specialname static 
                 class Starbeam.Features.Production.Issuance.Production.LicenseDisplayName get_Default () cil managed 
         {
                 .custom instance void [System.Runtime]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = (
                         01 00 00 00
                 )
                 // Method begins at RVA 0x4050
                 // Header size: 1
                 // Code size: 2 (0x2)
                 .maxstack 8
 
                 // throw null;
                 IL_0000: ldnull
                 IL_0001: throw
         } // end of method LicenseDisplayName::get_Default
 
         .method public hidebysig specialname rtspecialname 
                 instance void .ctor (
                         class [System.Linq.Expressions]System.Linq.Expressions.Expression`1<class [System.Runtime]System.Func`2<class [Starbeam.Entities]Starbeam.Entities.Marketplace.Publishing.Issuance, string>> name
                 ) cil managed 
         {
                 // Method begins at RVA 0x4050
                 // Header size: 1
                 // Code size: 2 (0x2)
                 .maxstack 8
 
                 // throw null;
                 IL_0000: ldnull
                 IL_0001: throw
         } // end of method LicenseDisplayName::.ctor
 
         // Properties
         .property class Starbeam.Features.Production.Issuance.Production.LicenseDisplayName Default()
         {
                 .get class Starbeam.Features.Production.Issuance.Production.LicenseDisplayName Starbeam.Features.Production.Issuance.Production.LicenseDisplayName::get_Default()
         }
 
 } // end of class Starbeam.Features.Production.Issuance.Production.LicenseDisplayName

I was surprised that a private nested class would be included in the ref assembly.

Reading refout.md:

Ref assemblies further remove metadata (private members) from metadata-only assemblies:
...

• But all types (including private or nested types) are kept in ref assemblies. All attributes are kept (even internal ones), as well as their (internal) constructors.

I can't see why this private nested class would be needed in the reference assembly, and having it there impacts incremental build performance. Without it, the above change would build in just a few seconds.

Could private tested classes be removed from reference assemblies? The future section of the doc suggests there are potential improvements that could be made. Is this one?

[drewnoakes]

Pinging @jaredpar @jcouv for insight here. Addressing this would improve incremental build in some scenarios, and potentially make ref assemblies smaller. Is there a reason we couldn't exclude private nested classes?

[CyrusNajmabadi]

Isn't one of the reasons so that we can tell if a struct is unmanaged or not? We need to know its internal layout (which may be private, and which may reference private types) in order to make that determination. Not sure if there are other reasons beyond that.

[drewnoakes]

In this case it's a class, and it's unreferenced by anything else in the ref assembly.

[CyrusNajmabadi]

In this case it's a class,

It's not that the type is a class or not, it's that it might be necessary to know what that type is in case it is referenced inside a struct.

and it's unreferenced by anything else in the ref assembly.

If this is hte only reason we're tracking this info, then it's possible we could try to track that, and then not emit this.

I think the main problem is that we've discovered over the years people and tools depending on this info. So removing ends up breaking things. That doesn't mean it can't be removed, just that it's potentially dangerous for unknown unknown reasons.

[jaredpar]

The goal of reference assemblies is that they match the behavior of implementation assemblies exactly from the perspective of compilation. In order to safely exclude a type or member from reference assembly we must be certain that it does not impact evaluation in any way. Unfortunately C# is a very complex language and knowing whether a member does or does not impact semantic analysis can be tricky.

When we first started implementing reference assemblies we approached it in the manner described in this bug: picking specific combinations of accessibility, nesting, etc ... and excluding it. Every time we did this though, another member of the compiler team was able to come up with an example where that particular exclusion lead to observable differences in compilation. After a few rounds it became clear that we needed a more principled approach.

Eventually we settled on a very simple principal for whether or not we should include a type or member in a reference assembly. Essentially

If the compiler imports a type / member when reading metadata then we need to include that type / member in the reference assembly

This is a conservative strategy for reference assemblies but it's also one that is very easy to prove to be correct.

This does lead to lots of curious entries in reference assemblies. For example you'll see that lambda closure types appear in reference assemblies and it's pretty clear that they don't impact compilation. The general mentality we've had on the compiler team is that we're open to making reference assemblies smaller but there are two criteria:

1. Need to demonstrate that it's having a meaningful impact on the ecosystem. Essentially justify doing the work.
2. Need to be able to prove, quite rigorously, that it's safe to exclude. Likely start by no longer importing the type / member to demonstrate that it doesn't have an impact on compilation.

We are aware that other reference assembly technologies take a different approach here and are more aggressive with trimming types / members. In most cases we've demonstrated that the resulting reference assemblies produces meaningful different results in compilation. A particular sore point is how struct private members are handled. I've written before about the problems those have caused us. Even when the most impactful bugs are fixed there often remain a number of corner cases that behave differently, and we can't accept those.

[jasonmalinowski]

Likely by marking the assembly with an attribute or some such.

If we're having to mark attributes in some form, would it make sense just to have an attribute on the type that says "I the compiler emitted this, and we swear it doesn't need to be imported for purposes of other compilers?"

[jaredpar]

If we're having to mark attributes in some form, would it make sense just to have an attribute on the type that says "I the compiler emitted this, and we swear it doesn't need to be imported for purposes of other compilers?"

Possibly. It's definitely a design area to explore. There are other conversations that are converging on us needing to add an attribute to the assembly that more / less asserts an assembly was compiled with C# compiler. That is why my initial thought went in that direction.

[CyrusNajmabadi]

both seem fine to me. HOwever, one path seems to only require one attribute, while the other would require N. My preference leads toward the former. But i do imagine we might box ourselves in with that single attribute, and regret it later.

[drewnoakes]

Other languages could generate types that meet this exact criteria. Those languages could also use those types in other parts of the assembly (say as a private field, or as a parameter on a private member). Thus excluding that type entirely from import could lead to errors elsewhere because we're unable to resolve those types.

I don't follow why other languages factor in. Aren't we only talking about ref assemblies created by Roslyn for C# projects? If so, Roslyn can omit its own private nested display classes, and other languages can leave them in, and they're still imported. What am I missing?

[jaredpar]

Aren't we only talking about ref assemblies created by Roslyn for C# projects?

Consider that the compiler needs to treat ref and impl assemblies equally. While we generally only see ref assemblies during build, there are plenty of cases where we have to process impl assemblies. Easiest example is when customers disable reference assembly generation entirely. Another example is the assemblies that come from NuPkg (these are almost always impl assemblies).

If we move forward with partial exclusion then the impl assembly will have more importable types than the ref assembly. To maintain the invariant we have today, which is "if we import it then we must emit it", then we have to change the compiler to not import those types when reading an impl assembly. That will ensure that ref and impl assemblies have the same set of types / members during import.

For this example we'd need to ensure the compiler doesn't import display classes from a C# impl assembly when reading metadata. More specifically we'd need to only do this for assemblies generated from a C# compiler version X or later.

This is why other languages come into play. There is nothing stopping another language, or even ilasm, from creating types that match the exact structure that C# display classes have but are important to import for type correctness. That assembly can then flow to the compiler through a NuPkg, msbuild magic, etc ... So making import decisions on name + structure only isn't enough. Have to find some way to give us confidence the type meets a certain structure and was generated from the C# compiler.