DotNetAnalyzers / ReflectionAnalyzers

Analyzers checking System.Reflection
MIT License
80 stars 8 forks source link

Missing 'REFL006 The binding flags can be more precise.' when public member is found #230

Closed jnm2 closed 4 years ago

jnm2 commented 4 years ago

Even though private members are an unknown, the fact that a public member was found means that no private member can exist with the exact same signature.

using System;
using System.Reflection;

class C
{
    void M()
    {
        typeof(InvalidOperationException).GetConstructor(
            BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance,
            null,
            Type.EmptyTypes,
            null);
    }
}

Missing:

REFL006 The binding flags can be more precise. Expected: BindingFlags.NonPublic | BindingFlags.Instance | BindingFlags.DeclaredOnly.

JohanLarsson commented 4 years ago

For some reason we currently have tests asserting that we don't warn when a type is not in source.

brian-reichle commented 4 years ago

Sometimes an the visibility of a member changes from one version to the next (though generally only becoming more visible). Sometimes you might need to include both Public and NonPublic in the binding flags so that it works against both versions.

jnm2 commented 4 years ago

This is a fair point. It equally applies to DeclaredOnly whenever there is a base class that a member could move to.

JohanLarsson commented 4 years ago

So we broke it now, question is what is the best default?

I'm leaning towards keeping the way we have it now. My thinking is a #pragma with an explanation why we want both public and nonpublic for example is good.

jnm2 commented 4 years ago

I think the default depends on whether the member is currently nonpublic (possibly going public, consider also protected members) or currently public (breaking change to make nonpublic, pretty unlikely).

jnm2 commented 4 years ago

My argument in favor of keeping this check when a currently public member is known to exist is this: The break that occurs if the member becomes nonpublic is equivalent to the break that occurs if you had been calling that member without reflection and it went nonpublic.

I could see the currently nonpublic case to be a matter of preference. Do you want unit tests to fail if the member becomes public? I have thought about this over the months and I think that I do want that assertion at runtime. I do have CI tests covering all usages of reflection against the real dependency versions that will end up shipping (or not) based on those tests.

If there was a separate ID for public/nonpublic, or a configuration file, both preferences could be catered to without pragmas.