Analyzer Proposal: Seal internal/private types

[stephentoub]

There are numerous performance benefits to sealing types:

1. Calls to overrides can be done directly rather than with virtual dispatch, which then also means they can be inlined.

public class C {
    internal void Call(SealedType o) => o.M();
    internal void Call(NonSealedType o) => o.M();
}

internal class BaseType
{
    public virtual void M() {}
}
internal class NonSealedType : BaseType
{
    public override void M() {}
}
internal sealed class SealedType : BaseType
{
    public override void M() {}
}

results in:

C.Call(SealedType)
    L0000: cmp [rdx], edx
    L0002: ret

C.Call(NonSealedType)
    L0000: mov rcx, rdx
    L0003: mov rax, [rdx]
    L0006: mov rax, [rax+0x40]
    L000a: mov rax, [rax+0x20]
    L000e: jmp rax

2. is/as type checks for the type can be done more efficiently, as it only needs to compare the type itself rather than account for a potential hierarchy.

public class C {
    public bool IsSealed(Object o) => o is SealedType;
    public bool IsNotSealed(Object o) => o is NonSealedType;
}

internal class NonSealedType { }
internal sealed class SealedType { }

results in:

C.IsSealed(System.Object)
    L0000: test rdx, rdx
    L0003: je short L0016
    L0005: mov rax, 0x7ff7ab9ad180
    L000f: cmp [rdx], rax
    L0012: je short L0016
    L0014: xor edx, edx
    L0016: test rdx, rdx
    L0019: setne al
    L001c: movzx eax, al
    L001f: ret

C.IsNotSealed(System.Object)
    L0000: sub rsp, 0x28
    L0004: mov rcx, 0x7ff7ab9ad048
    L000e: call System.Runtime.CompilerServices.CastHelpers.IsInstanceOfClass(Void*, System.Object)
    L0013: test rax, rax
    L0016: setne al
    L0019: movzx eax, al
    L001c: add rsp, 0x28
    L0020: ret

3. Arrays of that type don’t need covariance checks every time an element is stored into it.

public class C {
    internal void StoreSealed(SealedType[] arr, SealedType item) => arr[0] = item;
    internal void StoreNonSealed(NonSealedType[] arr, NonSealedType item) => arr[0] = item;
}

internal class NonSealedType { }
internal sealed class SealedType { }

results in:

C.StoreSealed(SealedType[], SealedType)
    L0000: sub rsp, 0x28
    L0004: cmp dword ptr [rdx+8], 0
    L0008: jbe short L001c
    L000a: lea rcx, [rdx+0x10]
    L000e: mov rdx, r8
    L0011: call 0x00007ff801db9f80
    L0016: nop
    L0017: add rsp, 0x28
    L001b: ret
    L001c: call 0x00007ff801f0bc70
    L0021: int3

C.StoreNonSealed(NonSealedType[], NonSealedType)
    L0000: sub rsp, 0x28
    L0004: mov rcx, rdx
    L0007: xor edx, edx
    L0009: call System.Runtime.CompilerServices.CastHelpers.StelemRef(System.Array, Int32, System.Object)
    L000e: nop
    L000f: add rsp, 0x28
    L0013: ret

4. Creating spans of that type don’t need to validate the actual type of the array matches the specified generic type.

using System;
public class C {
    internal Span<SealedType> CreateSealed(SealedType[] arr) => arr;
    internal Span<NonSealedType> CreateNonSealedType(NonSealedType[] arr) => arr;
}

internal class NonSealedType { }
internal sealed class SealedType { }

results in:

C.CreateSealed(SealedType[])
    L0000: test r8, r8
    L0003: jne short L000b
    L0005: xor eax, eax
    L0007: xor ecx, ecx
    L0009: jmp short L0013
    L000b: lea rax, [r8+0x10]
    L000f: mov ecx, [r8+8]
    L0013: mov [rdx], rax
    L0016: mov [rdx+8], ecx
    L0019: mov rax, rdx
    L001c: ret

C.CreateNonSealedType(NonSealedType[])
    L0000: sub rsp, 0x28
    L0004: test r8, r8
    L0007: jne short L000f
    L0009: xor eax, eax
    L000b: xor ecx, ecx
    L000d: jmp short L0026
    L000f: mov rax, 0x7ff7aba4d870
    L0019: cmp [r8], rax
    L001c: jne short L0034
    L001e: lea rax, [r8+0x10]
    L0022: mov ecx, [r8+8]
    L0026: mov [rdx], rax
    L0029: mov [rdx+8], ecx
    L002c: mov rax, rdx
    L002f: add rsp, 0x28
    L0033: ret
    L0034: call System.ThrowHelper.ThrowArrayTypeMismatchException()
    L0039: int3

5. Probably more both I’ve missed and that will emerge in the future.

We should add an analyzer, at either hidden or info level but that we’d look to turn on as a warning in dotnet/runtime, that flags:

• Internal and private non-static, non-abstract, non-sealed classes that
• Don’t have any derived types in its containing assembly
• Which also doesn't have InternalsVisibleTo on it

and flag that they should be sealed. A fixer would seal the type. We could also make it configurable on the visibility, in case someone wanted to e.g. also opt-in public types (we wouldn't/couldn't in dotnet/runtime). We could also optionally factor in whether the type declares any new virtual methods, a protected ctor, or anything else that suggests the type is intended for derivation... upon detecting such things, we could either choose not to warn, or we could warn with a different diagnostic id.

If a developer working in the library ever wants to derive from such a type, they can remove the sealed when they add the derivation.

[danmoseley]

Can the JIT not perform a similar thought process?

[stephentoub]

Can the JIT not perform a similar thought process?

It's the JIT that's doing these aforementioned optimizations based on the type being sealed, but the JIT today can't assume that non-sealed types won't ever have derived types, e.g. if a derived type were to be reflection emitted.

[jkotas]

Can the JIT not perform a similar thought process?

Full AOT that can do global analysis of all code that will ever run in the process can do this optimization.

JIT can do this optimization speculatively by assuming that the type is sealed, and undoing/rejiting all code that depends on this assumption when it is proven false. We have stayed away from speculative optimizitions like this so far. We have actually tried to build a similar speculative optimization for dictionaries of shared generic types, but we ended up rolling it back because it was hard to stabilize and because it had questionable performance characteristics.

[KalleOlaviNiemitalo]

Sealing a type might cause warning CS0628 or CA1047 if the type has protected members.

If the type is being sealed to help the JIT compiler, not for design reasons, then the developer might want to keep protected members protected so that the type can be easily unsealed again if a derived class is added.

If there is going to be a code fix that seals types, then perhaps it could also suppress those warnings.

[huoyaoyuan]

Which also doesn't have InternalsVisibleTo on it

It's quite common for a lib project to have InternalsVisibleTo for its corresponding test project.

[stephentoub]

It's quite common for a lib project to have InternalsVisibleTo for its corresponding test project.

Various optimizations are disabled by InternalsVisibleTo, e.g. the linker can't remove unused internals when building in a library marked InternalsVisibleTo. I'm fine with that being a limitation for this analyzer, as it is for other analyzers, like CA1812 (avoid uninstantiated internal classes).

[stephentoub]

Sealing a type might cause warning CS0628 or CA1047 if the type has protected members.

Yes, that's why I wrote "We could also optionally factor in whether the type declares any new virtual methods, a protected ctor, or anything else that suggests the type is intended for derivation... upon detecting such things, we could either choose not to warn, or we could warn with a different diagnostic id."

[stephentoub]

JIT can do this optimization speculatively by assuming that the type is sealed, and undoing/rejiting all code that depends on this assumption when it is proven false.

Yes, though my understanding from previous discussions with @AndyAyersMS on the topic is we have no intention of doing that sort of work any time soon, even though his OSR work starts to open the door to that.

[GrabYourPitchforks]

Might be able to expand the analyzer scope even further:

• Public types which have only private ctors and which don't have a nested subclassed type
• Public types which have only internal ctors, no subclassed types within the same assembly, and no friend assemblies defined

That last point allowed us to seal some public System.Globalization types in a previous release. See #31761.

[JamesNK]

It's quite common for a lib project to have InternalsVisibleTo for its corresponding test project.

Various optimizations are disabled by InternalsVisibleTo, e.g. the linker can't remove unused internals when building in a library marked InternalsVisibleTo. I'm fine with that being a limitation for this analyzer, as it is for other analyzers, like CA1812 (avoid uninstantiated internal classes).

Have we ever considered a flag on InternalsVisibleToAttribute to indicate that its usage is related to testing? Optimizations can then check whether the attribute exists without that flag set when making decisions.

I always use IVT for testing and never for sharing runtime code between assemblies.

[stephentoub]

Optimizations can then check whether the attribute exists without that flag set when making decisions.

What would the analyzer do differently if that was set? Ignore the attribute and then it's up to you to either change your tests or suppress the warning if the tests were dependent on the type being unsealed?

[jnm2]

A fixer would seal the type.

From personal experience actually having implemented an analyzer to warn if classes with no new virtual members and no inheritors aren't sealed and to offer a fix, Roslyn has a design issue that means that you can't offer fixes for any diagnostics reported by your analyzer during the compilation end action. And the compilation end action is the most (the only?) reasonable place to report everything, since you need to wait until all symbols have been analyzed in order to know whether any classes inherit from the ones you might report the diagnostic for.

Reported issue: dotnet/roslyn#51653

[stephentoub]

I've been sealing stuff across dotnet/runtime. Another case has come up that we might want to factor in to whether the analyzer raises a notification (or maybe a separate diagnostic ID for these less clear cases), or potentially it would just impact a fixer.

Today, you can write code like this:

using System;

class C { }

class Consumer
{
    public static IDisposable Bar(C obj) => obj as IDisposable;
}

and that will compile fine. However, if you seal C:

using System;

sealed class C { }

class Consumer
{
    public static IDisposable Bar(C obj) => obj as IDisposable;
}

that cast will now fail to compile:

error CS0039: Cannot convert type 'C' to 'System.IDisposable' via a reference conversion, boxing conversion, unboxing conversion, wrapping conversion, or null type conversion

because the compiler can now see that C definitely doesn't implement the interface (whereas before a type derived from C may have).

This can also happen even without derived types in the picture, where casts are used to go from a concrete generic instantiation to an open generic, e.g. this compiles fine:

interface IMyInterface<T> {}

class C<T> : IMyInterface<T> {}

class Int32C : C<int>
{
    public static Int32C Instance { get; } = new Int32C();
}

class Consumer
{
    static IMyInterface<T> Bar<T>()
    {
        if (typeof(T) == typeof(int))
        {
            return (IMyInterface<T>)Int32C.Instance;
        }
        
        return null;
    }
}

Int32C implements IMyInterface<T>; the compiler doesn't know that it will in fact satisfy the cast at run time (it only knows the type implements IMyInterface<int> and it doesn't know that T == int), but because Int32C isn't sealed, it allows the cast to compile because a type derived from Int32C could implement IMyInterface<T>. But, if you seal Int32C:

interface IMyInterface<T> {}

class C<T> : IMyInterface<T> {}

sealed class Int32C : C<int>
{
    public static Int32C Instance { get; } = new Int32C();
}

class Consumer
{
    static IMyInterface<T> Bar<T>()
    {
        if (typeof(T) == typeof(int))
        {
            return (IMyInterface<T>)Int32C.Instance;
        }
        
        return null;
    }
}

now you get:

error CS0030: Cannot convert type 'Int32C' to 'IMyInterface<T>'

Both of these cases can be solved by inserting an (object) cast in the middle.

[GrabYourPitchforks]

@stephentoub I made a small modification to your sample code and wanted to get your thoughts.

using System;

public class C {
    public static IDisposable M(MyClass c) {
        return c as IDisposable;  // CS0039
    }
}

public sealed class MyClass : System.Collections.Generic.List<int>
{
}

This feels wrong to me. The C# compiler in theory has no way of knowing whether that cast will succeed or fail at runtime, since it has no way of knowing the shape of the actual runtime superclass type. It only knows about what's exposed via the reference assembly.

Obviously we're not going to add IDisposable to List<T>, but consider this as a generalized example. Since we also now have the ability to introduce default interface members, this means (in theory) if sealed class Foo : IBar, and we later make IBar : IBaz (at runtime, via default interface implementations so as not to break API contracts), then Foo also implicitly implements IBaz (at runtime) even though it's never explicitly stated in the code or compiled into the assembly.

[stephentoub]

@GrabYourPitchforks, to make sure I understand, you're questioning the C# compiler's warnings/errors in these cases and suggesting it shouldn't validate interface casts at all?

[GrabYourPitchforks]

you're questioning the C# compiler's warnings/errors in these cases and suggesting it shouldn't validate interface casts at all?

Correct. Or at most, a warning ("are you sure you intended to write this? it probably won't work at runtime."), not an error ("it definitely won't work at runtime."). That would hopefully reduce any complexity in your proposed analyzer and fixer.

[Youssef1313]

@stephentoub Does it make sense to extend this to public types where the base type doesn't have any public constructors?

[stephentoub]

Does it make sense to extend this to public types where the base type doesn't have any public constructors?

If it doesn't have any public, protected, or protected internal constructors, then we could also factor it in.

[askazakov]

@stephentoub I'm sorry I've not catch is it https://rules.sonarsource.com/csharp/RSPEC-3260 about the same issue? Did I miss something important?

[stephentoub]

@askazakov, that's in an unrelated static analysis library rather than the analyzers that ship as part of the .NET SDK, and at least from the description it's only about private rather than non-public, e.g. it doesn't seem to cover internal types (at least not from the short description you linked to).

[meziantou]

I think the analyzer should exclude classes declared as [ComImport]/[CoClass] from the analysis as the fix may break the compilation:

using System.Runtime.InteropServices;

_ = (IFileSaveDialog)new FileSaveDialogRCW(); // If FileSaveDialogRCW is sealed: error CS0030: Cannot convert type 'FileSaveDialogRCW' to 'IFileSaveDialog'

[ComImport]
[Guid("C0B4E2F3-BA21-4773-8DBA-335EC946EB8B")]
[InterfaceType(ComInterfaceType.InterfaceIsIUnknown)]
[CoClass(typeof(FileSaveDialogRCW))]
internal interface IFileSaveDialog
{
}

[ComImport]
[ClassInterface(ClassInterfaceType.None)]
[Guid("C0B4E2F3-BA21-4773-8DBA-335EC946EB8B")]
class FileSaveDialogRCW // 👈 sealed is not valid here because of `(IFileSaveDialog)new NativeFileSaveDialog()`
{
}

[ericstj]

@meziantou -- it looks like this is currently under PR here dotnet/roslyn-analyzers#5594 Would you mind adding your feedback there?