Add nullablePublicOnly Roslyn Feature

[eerhardt]

This tells Roslyn to not add [AllowNull] attributes in IL to non-public APIs. The same feature is set for dotnet/runtime.

See https://github.com/dotnet/runtime/blob/8033217b3c9d59da81436e120b32dd5ec74a856c/Directory.Build.props#L249.

This is causing the AllowNullAttribute to be in a default Blazor WASM app, and should allow for it to be trimmed.

cc @stephentoub

[stephentoub]

This tells Roslyn to not add [AllowNull] attributes in IL to non-public APIs

I was confused about this when we were talking... we should still make this change, but it's not about AllowNull but rather about the Nullable/NullableContext attributes. Those are the attributes the compiler itself emits to track nullable annotations in metadata.

That said, if we don't have a linker feature that enables removing an allow list of attributes when on non-public surface area, we should consider adding one, and then add all of the System.Diagnostic.CodeAnalysis nullable attributes.

[eerhardt]

if we don't have a linker feature that enables removing an allow list of attributes when on non-public surface area, we should consider adding one, and then add all of the System.Diagnostic.CodeAnalysis nullable attributes.

We already have the setting to strip these attributes for WASM:

https://github.com/dotnet/runtime/blob/8033217b3c9d59da81436e120b32dd5ec74a856c/src/mono/netcore/System.Private.CoreLib/src/ILLink/ILLink.LinkAttributes.xml#L33-L53

But since the ASP.NET assemblies aren't marked for trimming, the linker needs to leave the attributes applied in those assemblies.

Looks like this is another case where making ASP.NET libraries "aggressively trimmed" (as opposed to "type granular" trimmed) will help. FYI - @pranavkm

[pranavkm]

@eerhardt could you add this snippet to

• src/Mvc/Mvc.DataAnnotations/test/Microsoft.AspNetCore.Mvc.DataAnnotations.Test.csproj
• src/Mvc/test/Mvc.IntegrationTests/Microsoft.AspNetCore.Mvc.IntegrationTests.csproj

<!-- This project has tests that rely on nullability on non-public types. Undo nullablePublicOnly configured by default -->
<Features>$(Features.Replace('nullablePublicOnly', ''))</Features>

Also FYI @roji in case you wanted to set this feature flag in EF

[roji]

@pranavkm thanks. This would be problematic in EF Core since pubternal types are in extensive use - we have very little real internals, instead we place public types under Internal namespaces.

• Does the compiler emit attributes in the IL for private members? If so, is there a similar feature to turn only that off?
• Do I understand correctly that if EF Core is correctly marked for trimming, this becomes unnecessary? Because we have plans to work on that for 6.0.

[stephentoub]

This would be problematic in EF Core since pubternal types are in extensive use

"pubternal" is just a convention: from the compiler's perspective, it's all still public, so nothing would be removed from them. So I'm not following why this setting would be problematic for that case...?

Do I understand correctly that if EF Core is correctly marked for trimming, this becomes unnecessary? Because we have plans to work on that for 6.0.

For final trimmed wasm app size, I believe it should be fine. For the binaries in the nuget package, the attributes on non-public surface area would all still be there, which is unnecessary unless you actually rely on code being able to inspect Nullable{Context} via reflection on true internals.

[eerhardt]

@eerhardt could you add this snippet to

Done. Thanks for the help, @pranavkm!

[roji]

This would be problematic in EF Core since pubternal types are in extensive use

"pubternal" is just a convention: from the compiler's perspective, it's all still public, so nothing would be removed from them. So I'm not following why this setting would be problematic for that case...?

What I meant was that since there are practically no (true) internal types/members in EF Core, publicOnly simply wouldn't do much - everything is public anyway Unless the point here is to avoid generating the attributes for private members; are those emitted without publicOnly being set?

Thanks for the wasm/linker info, I still have no idea how feasible it is to get EF Core working with aggressive linking, given the extensive reliance on reflection. But it's something I'm planning to look into.

[stephentoub]

Unless the point here is to avoid generating the attributes for private members; are those emitted without publicOnly being set?

Yes, the compiler emits all Nullable/NullableContext attributes on all members regardless of visibility, unless you specify nullablePublicOnly, in which case it only does so for publics.

[roji]

Thanks, all clear now. Will do this for EF Core as well.