REFL013 warns about using events when it's not possible to do so

[monoclex]

reflection code:

		private MulticastDelegate GetOnModel(IModelSender sender)
		{
			var senderType = sender.GetType();

			var onModel = (MulticastDelegate) sender.GetType()
				.GetField("OnModel", BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)
				.GetValue(sender);

			return onModel;
		}

the (what i assume to be suggested) code:

		private MulticastDelegate GetOnModel(IModelSender sender)
		{
			var senderType = sender.GetType();

			var onModel = (MulticastDelegate) sender.OnModel;

			return onModel;
		}
``` but the OnModel can only appear on the left/right side of a `+=` or `-=`

[jnm2]

I think it's telling you to use .GetEvent("OnModel", ...).Add.Invoke(...) which wouldn't make sense here.

Your OnModel member is a normal public field, not an event, right?

[monoclex]

OnModel is a public OnModelHandler OnModel;
this is about what i have

using System;

public delegate void OnModelHandler(object e);

public interface IThing
{
    event OnModelHandler OnModel;
}

public class TestClass : IThing
{
    public event OnModelHandler OnModel;
}

[jnm2]

Ah, so you're using a field-like event. There is no guarantee that you'll be able to access the backing field of an event by that name in general. For example:

public class TestClass : IThing
{
    public event OnModelHandler OnModel
    {
        add => otherThing.OnModel += value;
        remove => otherThing.OnModel -= value;
    }
}

You are relying on a compiler implementation detail. All you know for sure if you have an event is that there is an add method, a remove method, and if the event is defined in VB, an optional Raise method. Event accessors don't let you know about the backing field (if there is one) any more than property accessors. Assuming you're already checking for GetField potentially returning null, and you're still happy with the design, that's fine.

@JohanLarsson It appears that REFL013 is triggering for GetField on event metadata. REFL013 should only trigger for GetMethod("add_OnModel", ...), GetMethod("remove_OnModel", ...), or GetMethod("raise_OnModel", ...), right?

Sharplab shows that in this scenario there are two IL members named OnModel: the public event definition and the private backing field.

    .field private class OnModelHandler OnModel
    .custom instance void [mscorlib]System.Runtime.CompilerServices.CompilerGeneratedAttribute::.ctor() = (
        01 00 00 00
    )

    .event OnModelHandler OnModel
    {
        .addon instance void TestClass::add_OnModel(class OnModelHandler)
        .removeon instance void TestClass::remove_OnModel(class OnModelHandler)
    }

[monoclex]

in my scenario, it's just for unit testing purposes, thankfully.
nonetheless, this suggestion shouldn't pop up imo

[jnm2]

@SirJosh3917 I just noticed that you have .GetField("OnModel", BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly), but that looks wrong because the OnModel field is private. Shouldn't it be BindingFlags.NonPublic instead of Public?

[jnm2]

Maybe a bug? The analyzer does not realize that a private field is emitted as a current Roslyn implementation detail:

class C
{
    public event EventHandler E;

    static EventHandler GetDelegateFromPrivateBackingField(C instance)
    {
        // If you run this code, it will work as intended until a Roslyn impl detail changes.

        return (EventHandler)typeof(C)
            .GetField(
                nameof(E),
                // REFL005 There is no member matching the filter. Expected: BindingFlags.Public |
                // BindingFlags.Instance | BindingFlags.DeclaredOnly.
                BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.DeclaredOnly)
            .GetValue(instance);
    }
}

On the other hand, Roslyn could decide to change the naming scheme of the private backing field any day as far as I know. I'd still say that the analyzer should not make incorrect definitive statements. There is a member matching the filter when the code compiles.

[jnm2]

Now, back to the repro originally reported. I think that some warning must actually be shown because of the missing BindingFlags.NonPublic which prevents the field from being accessed. Should it say something different, be located in a different place, or be a different warning ID?

class C
{
    public event EventHandler E;

    static EventHandler GetDelegateFromPrivateBackingField(C instance)
    {
         // If you run this code, it will NRE due to missing BindingFlags.NonPublic.

        return (EventHandler)typeof(C)
            // ↓ REFL013 The type C has a event named E.
            .GetField(
                nameof(E),
                BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)
            .GetValue(instance);
    }
}

[JohanLarsson]

REFL005 should handle that I think.

[JohanLarsson]

Problem is a bug in Roslyn: dotnet/roslyn#36259
Not sure how to work around it best.