Analysis hole: attributes on instance methods in RUC types

[sbomer]

RUC on type silences warnings from instance methods, but those instance methods may have attributes that should produce warnings. We incorrectly treat RUC on type as suppressing those warnings, even though reflection access to those methods can call the attribute ctor at runtime. Example:

class RequiresOnAttributeCtorAttribute : Attribute
{
    [RUC("--ATTRIBUTE CTOR--")]
    public RequiresOnAttributeCtorAttribute()
    {
        Console.WriteLine("ATTRIBUTE CTOR CALLED!");
    }
}

[RequiresUnreferencedCode("--TypeWithRequires--")]
class TypeWithRequires
{
    [RequiresOnAttributeCtor]
    public void InstanceMethod() { }
}

public static void Test()
{
    foreach (var method in typeof(TypeWithRequires).GetMethods(BindingFlags.Public | BindingFlags.Instance))
    {
        foreach (var a in method.GetCustomAttributes()) ;
    }
}

[MichalStrehovsky]

In similar spirit, this one won't warn either. Maybe the problem is that we don't warn that the method is reflection-accessed?

using System;
using System.Diagnostics.CodeAnalysis;

var del = typeof(Foo).GetMethod(nameof(Foo.Get)).CreateDelegate<Func<Foo, string, Type>>();
var t = del(null, Console.ReadLine());
Console.WriteLine(t);

[RequiresUnreferencedCode("Don't use")]
class Foo
{
    public Type Get(string s) => Type.GetType(s);
}

[sbomer]

Thanks for pointing that out, I didn't realize that reflection-invoke could call an instance method on null without throwing a NRE.

[MichalStrehovsky]

One can also do the same with an IL call instruction. We just rely on the fact that C# mandates calling instance method with a null this throw NRE and Roslyn therefore uses callvirt for all instance method calls where it cannot prove they're not null.

(Note that calling instance method with a null this has issues and people should only do it as a party trick, if that's the kind of parties they go to.)

[sbomer]

I am working on a change that will warn on reflection access to instance methods of a RUC type to address the above.

However, instance fields still have the same problem with attributes that methods have. @MichalStrehovsky do you think it would make sense to warn on reflection access to instance fields of a RUC type as well, to cover this? I couldn't think of any independent justification for doing so like we have for methods.

[sbomer]

Maybe it would be best to limit the warnings to fields which have RUC attribute instances on them. Otherwise compiler-generated code for RUC user methods will have lots of generated fields that are logically in a RUC scope but will never have any problematic attribute instances on them.

[MichalStrehovsky]

However, instance fields still have the same problem with attributes that methods have. @MichalStrehovsky do you think it would make sense to warn on reflection access to instance fields of a RUC type as well, to cover this?

Should RUC on a type apply to instance fields in the first place? RUC is about behaviors, not storage locations. IIRC the only reason why RUC does something with fields at all is that accessing static fields triggers the cctor and that's a behavior (I didn't fully agree on that one and would have preferred RUC on type to not apply to cctor - to basically have no way to mark a cctor as RUC). But accessing instance fields can't trigger a cctor. I can't come up with a situation where it wouldn't be safe.

[sbomer]

Maybe not - that's kind of what I'm trying to decide. I agree that we originally decided RUC on type would only apply to static fields because they could trigger the cctor.

The only situation I can come up with where it would be unsafe to reflection-access an instance field is where the field has attributes with a RUC constructor, so I'm wondering what the warning location should be for that scenario - whether it's the attribute instance, or reflection-access to the field. Since we are saying that attributes on methods in RUC types don't warn (being "covered" by the reflection-access warnings), it just feels inconsistent for them to warn on instance fields. And it feels especially inconsistent if they don't warn on static fields.

I'm fixing the hole for methods in https://github.com/dotnet/linker/pull/3147, but there I'm leaving instance fields alone until we decide on the behavior.

[sbomer]

Another thing to consider is that the reflection-access-to-RUC-member warnings will all have the same warning code, so suppressing one of them will suppress all of them. This is good and bad - it means that somebody who is sure they're not doing anything "unsafe" doesn't need to worry about suppressing for each potentially accessed member, but also that suppressed warnings could hide problems if new RUC members are introduced. For the instance field scenario, I think it means that the extra warnings aren't too troublesome - anyone reflecting over a RUC type probably won't need extra suppressions for this.