Allow RequiresUnreferencedCode on classes and structs

[eerhardt]

Some types are inherently not trim compatible. Take, for example, BinaryFormatter. Virtually anything you do with the type is not going to be trim compatible.

Today, we only allow RequiresUnreferencedCodeAttribute on methods and constructors.

https://github.com/dotnet/runtime/blob/d2e1a86434d1c0b74d75604b4cf9fdca100108c3/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresUnreferencedCodeAttribute.cs#L14-L15

When trying to annotate the implementation of BinaryFormatter, it is causing almost every method in the library to be marked as RequiresUnreferencedCode. This is a lot of changes when just putting the attribute on a whole class would be a lot easier.

We should change the ILLinker to support RequiresUnreferencedCodeAttribute on classes and structs, and possibly interfaces.

@agocke @vitek-karas @MichalStrehovsky

[MichalStrehovsky]

Is that just syntactic sugar to save us from annotating multiple methods or do we expect more semantics from this?

E.g. do derived types inherit this setting? Does it propagate to nested types?

Putting RequiresUnreferencedCode on some methods is problematic. E.g. annotating a GetHashCode override as RequiresUnreferenced is going to warn because Object.GetHashCode also needs to be annotated like that, otherwise it's unenforceable.

[marek-safar]

Could you link the code you are referring to? I'm not sure this will be useful when used in non-trivial code. I'd expect that constructors and properties rarely need RequiresUnreferencedCode.

[eerhardt]

Could you link the code you are referring to?

See the BinaryObjectWriter.cs, BinaryObjectReader.cs, and BinaryParser.cs files in dotnet/runtime#46964.

[marek-safar]

I don't see that as good evidence that we need it right now.

BinaryParser (only 4 internal methods need it out of about 35)
BinaryObjectReader (1 internal method out of 4 methods)
BinaryObjectReader (3 internal methods out of 7 methods)

[MichalStrehovsky]

@agocke Brought this up on our 1:1 yesterday. He suggested a possible semantic for this that might work for this: treat it the same as the C# compiler treats the ObsoleteAttribute. Basically, on classes, it would behave the same as if we annotated all the constructors and static methods as RUC. We would also suppress all trimming warnings within instance methods, but we would not treat them like RUC at their callsites (this avoids the problem of ToString) - this assumes that the unsafe operation is creating the type - after that, all bets are off.

It's not easily extensible to structs, but I posit that nobody is going to miss that.

One possible complication is static virtual methods that are being added to C# and runtime as we speak. I don't know what the solution for that would be (they allow calling static methods indirectly). Maybe we can say that you can't put the attribute on a type that implements static interface methods and this will be a warning. Or maybe the C# team will come up with something clever.

[eerhardt]

it would behave the same as if we annotated all the constructors and static methods as RUC

I would also assume any usages of the Type, say for example: typeof(MyGeneric<>).MakeGenericType(typeof(MyRUCType))

[MichalStrehovsky]

I would also assume any usages of the Type, say for example: typeof(MyGeneric<>).MakeGenericType(typeof(MyRUCType))

That by itself doesn't look problematic. But if MyGeneric has a new() constraint or a method/constructor annotation on T, then yes, it probably should warn using the rules we have for RUC-annotated reflected methods.

We need to be careful about when we generate these warnings because it's easy to get into a situation where "if the type wasn't trimmed away, we will warn" because those are hard to suppress and generate warnings for (e.g. should the fact that a type implements IComparer<MyRUCType> trigger warnings?).

[eerhardt]

Here's another case where it would be beneficial to have RequiresUnreferencedCode on a whole type:

krwq/runtime-1@6ad21f3

We are up to almost 800 RequiresUnreferencedCode attributes trying to annotate each method separately.

[eerhardt]

Was this ever implemented in the linker? I see usages of this trying to pop up. ex. dotnet/runtime#55108.

[vitek-karas]

@tlakollo is working on it - it's tentatively planned for 6 right now (but no guarantees as we haven't got it working end to end yet).

[eerhardt]

Thanks. For now I'll recommend people don't use this feature until it is implemented. If we don't get it in for 6, we need to revert dotnet/runtime#50064 then.

[agocke]

@tlakollo is working on this right now has his highest priority. Hope to have it available shortly

[agocke]

@tlakollo Still need analyzer support, right?

[tlakollo]

Yes, the Merged PR would only work for Linker

[tlakollo]

This should be already working for linker and analyzer see #2330, closing