[API Proposal]: Add BindingFlags.IgnoreAccessModifiers

[terrajobst]

Background and motivation

When an existing private API is made public reflection code usually breaks because it uses BindingFlags.NonPublic rather than BindingFlags.Public | BindingFlags.NonPublic, hence excluding any public members. When calling private members it's generally desirable to look for both public and non-public members.

To make this more obvious I suggest we add an explicit flag that combines the two and also add an analyzer that warns people when they only use NonPublic but not Public. Given that we already have IgnoreXxxx flag, IgnoreAccessModifiers feels natural because it also encapsulates the intent: users want to call the API, even when it's non-public.

It is true that there are several ways private reflection can fail. Sometimes, there are cases where a library has a useful non-public API that people use; it's desirable in those cases in having existing reflection users not break when those APIs are just made public, without any other changes to its name or signature.

This is different from other changes to the signature as those change how the API is being invoked (e.g. when a static method is changed into an instance method, when its renamed, or when the signature is changed). In those cases the reflection author will need to change their code because the invocation pattern has changed.

Changing an API (public or not) can be a breaking change when an API is invoked via reflection, and this proposal doesn't try to address that. However, access modifiers are a subset of the API shape that reflection code could be invariant too, if authored in a certain way. The intent here is to promote this way to reduce the cases where private reflection can be broken over time, which is especially sad in this case because making the member public expresses the intent to make it part of the promised contract. It feels counter intuitive that doing so breaks existing reflection code.
 

API Proposal

namespace System.Reflection;

[Flags]
public enum BindingFlags
{
    // Existing:
    // Public = 16,
    // NonPublic = 32,
    // IgnoreCase = 1,
    // IgnoreReturn = 16777216,
    IgnoreAccessModifiers = Public | NonPublic
}

API Usage

BindingFlags flags = BindingFlags.Static | BindingFlags.IgnoreAccessModifiers;
PropertyInfo info = type.GetProperty("SomeProperty", flags);
object value = info.GetValue(null);

This code will continue to work when SomeProperty is made public:

public static class SomeClass
{
    internal static string SomeProperty { get; }
}

Alternative Designs

We could decide to no add the enum member and instead just recommend combining the existing flags.

Risks

None

[ghost]

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

A common failure for people doing private reflection is when an existing private API is made public their reflection code breaks because it uses BindingFlags.NonPublic but not BindingFlags.Public. When calling private members it's generally desirable to look for both public and non-public APIs.

To make this more obvious I suggest we add an explicit flag that combines the two and also add an analyzer that warns people when they only use NonPublic but not Public.

API Proposal

namespace System.Reflection
{
    [Flags]
    public enum BindingFlags
    {
        // Existing:
        // Public = 16,
        // NonPublic = 32,
        PublicOrNonPublic =  Public | NonPublic
    }
}

API Usage

BindingFlags flags = BindingFlags.Static | BindingFlags.PublicOrNonPublic;
PropertyInfo info = type.GetProperty("SomeProperty", flags);
object value = info.GetValue(null);

Alternative Designs

We could decide to no add the enum member and instead just recommend comining the existing flags.

Risks

None

Author:	terrajobst
Assignees:	-
Labels:	area-System.Reflection, code-analyzer, code-fixer, api-ready-for-review
Milestone:	9.0.0

[mgravell]

I do a lot of reflection; I don't think I've ever hit a problem with this, and if you do: it is pretty self evident. Since | is literally "or", I'm not sure that a XOrY element is needed, honestly. I also have multiple places where I explicitly test for non-public only; warning me about that feels weird. A relevant analyzer might be:

• not including any accessibility modifier
• not including any instance/static modifier

It won't hurt me if you add an XOrY element - I just don't see a hard need for it.

[Szer]

Who is the target audience of that change?
It seems that the idea behind this is to make unsafe API more accessible to those who don't know how .NET works.

[terrajobst]

@mgravell

I do a lot of reflection; I don't think I've ever hit a problem with this, and if you do: it is pretty self evident.

It's less about getting it wrong for a given API, it's about getting it right for the current state, shipping code, and then being broken when the target of the reflection calls makes the private API public.

The proposal here is to allow reflection authors to write code that uses private reflection and have that code resilient to the target being made public later.

@Szer

It seems that the idea behind this is to make unsafe API more accessible to those who don't know how .NET works.

Why do you think this makes it more accessible?

[theo-albers]

There is already IgnoreCase and IgnoreReturn, so why not IgnoreAccessModifier?

[aloraman]

I think adding BindingFlags.PublicOrNonPublic opens a whole new can of worms: if you add one explicit flag - what would stop you from adding more explicit flags? BindingFlags.PublicStatic, BindingFlags.PublicNonStatic, BindingFlags.PublicOrNonPublicStatic and so on...

[niemyjski]

Yeah, growth of combinations would get way out of hand. I'm against this change unless it's adding an option that is used 90% of the time by the community.

New Feature: Enum Extensions

[tannergooding]

I'd really rather this just be dotnet/csharplang#2926 to make it shorter

If we believe users missing this is wrong the majority of the time, then an analyzer fills the need of ensuring they specify both.

[peteraritchie]

I think adding BindingFlags.PublicOrNonPublic opens a whole new can of worms: if you add one explicit flag - what would stop you from adding more explicit flags? BindingFlags.PublicStatic, BindingFlags.PublicNonStatic, BindingFlags.PublicOrNonPublicStatic and so on...

i.e. are you attempting to make the API better or account for people who already missed that they could have done BindingFlags.Public | BindingFlags.NonPublic?

[jkotas]

A common failure for people doing private reflection is when an existing private API is made public their reflection code breaks

Any examples (links to issues) where this happened?

Private reflection has number of failure modes. I do not remember seeing this failure mode. The most common failure mode of private reflection that I have seen are ambiguous matches introduced by changing private implementation details. We have been occasionally forced to change the internal implementation details for compensate for it (for example #28613).

[terrajobst]

@aloraman

I think adding BindingFlags.PublicOrNonPublic opens a whole new can of worms: if you add one explicit flag - what would stop you from adding more explicit flags? BindingFlags.PublicStatic, BindingFlags.PublicNonStatic, BindingFlags.PublicOrNonPublicStatic and so on...

The problem this proposal is trying to solve is reducing the negative impact of making a useful private member public. Changing a method from static to non-static (or vice versa) wouldn't work anyways due to signature differences.

The only reason to add well-named member for the combination is to create the incentive to use it over the simpler version and to have a target that the analyzer can point to. However, the proposal would equally work if we don't introduce a new API and just have the analyzer recommend combining Public and NonPublic, which is why I listed that under alternatives.

That said, this proposal from @theo-albers also looks good to me and wouldn't introduce combinations:

There is already IgnoreCase and IgnoreReturn, so why not IgnoreAccessModifier?

I like that quite a bit because it conveys what calling a private API usually means. The caller means to by-pass access checks, not that they only want to call a non-public API and intend the bind to fail when a public member exists.

@jkotas

Any examples (links to issues) where this happened?

I know of reports in .NET Framework where us making a useful but private API public for a third-party has broken the very people who asked for it. @GrabYourPitchforks recently brought this up in another API review, maybe he has more specifics.

[theo-albers]

I haven't run into the described case myself either. What I do see is that refactoring and code suggestions may force you to change the implementation from instance to static method. In that case the other party would still not be helped with the flag enum expansion. When a signature changes from instance to static, or is even moved to an extension method, you have to change your invoke call on the reflected info. Adding a new enum for the minor use case doesn't seem worth it to me.

[svrooij]

i.e. are you attempting to make the API better or account for people who already missed that they could have done ?

This is a very good reason not to do this.

It's called Binding Flags telling everybody it's a Flag, which means you can use the | to do or.

My honest opinion, if you're using reflection you have to make sure to account for the fact that there might be something going public. Using the syntax with the | requires a change in their code and using your solution also requires a change in their code.

It does not solve anything, and people who use reflection already have a decent way of solving this issue.

And one last thought, let's say we have PublicOrNonPublic and someone wants to also use one of the other options, will they be using the | with your new options or will they be doing it with all the individual options?

[aloraman]

@terrajobst

The problem this proposal is trying to solve is reducing the negative impact of making a useful private member public

Still, this is a situation of user's code being broken because it has a dependency on private implementation details. Generally speaking, such an issue is resolved with the following, from most likely to the least:

• It's private implementation detail. Closed.
• It's private implementation detail though, but we'll think about it. Milestone: future
  ...
• Let's do something about it.

In other words, is the proposed feature worth the opportunity cost?

The only reason to add well-named member for the combination is to create the incentive to use it over the simpler version and to have a target that the analyzer can point to. However, the proposal would equally work if we don't introduce a new API and just have the analyzer recommend combining Public and NonStatic, which is why I listed that under alternatives

So, the problem is that user can type just BindingFlags.NonPublic, and the code will be broken if requested member will become public in the future, so you'd like to suggest to the user to use BindingFlags.NonPublic | BindingFlags.Public. Adding BindingFlags.PublicOrNonPublic is not enough - because it doesn't stop the user from typing BindingFlags.NonPublic, so you'll need an analyzer to make such a suggestion as well. However, the analyzer can also suggest BindingFlags.NonPublic | BindingFlags.Public - so there is no need to introduce BindingFlags.PublicOrNonPublic.

Basically, it's an esthetical choice. In my opinion, both @theo-albers proposal or language feature referenced by @tannergooding are much more esthetically pleasing than BindingFlags.PublicOrNonPublic.

[buyaa-n]

Could we add BindingFlags.Family that help with filtering protected members specifically?

[terrajobst]

I have updated to proposal to better express the intent.

@buyaa-n

Could we add BindingFlags.Family that help with filtering protected members specifically?

AFAIK protected/protected internal/private protected are all treated as non-public because reflection invocation is always performed outside the class hierarchy, which means they are all equally inaccessible.

I'd rather no add more ways to filter based on access modifiers because it seems odd to me to filter based on them anyway. It makes sense to opt-into binding to members that would normally be inaccessible, but I'd think name and shape are the primary decision points.

[stephentoub]

I understand the goal here, but I'm still not particuarly excited about adding another flag for this. This is already possible today just with Public | NonPublic, I now need to learn the relationship between IgnoreAccessModifiers and Public / NonPublic to figure out which I want to use if I don't immediately realize they're the same thing, and I'm not convinced this will actually move the needle for folks hitting the problem alluded to; I suspect folks that were previously only specifying NonPublic will continue to specify NonPublic. Doesn't seem worthwhile to me.

[terrajobst]

@stephentoub the design includes an analyzer; are you saying we should still do the analyzer (and push people towards combining NonPublic with Public) or are you saying the entire proposal isn't worth it?

[stephentoub]

If we think NonPublic without Public is almost never correct, then an analyzer/fixer to correct it to also include "| Public" sounds reasonable.

[Sergio0694]

Would such an analyzer also track the current [DynamicallyAccessedMembers] scope? Because if not it would be a bit awkward if you were eg. inside a method that's annotated with [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NonPublicMethods)], and see the analyzer suggest changing that to instead also use | Public. To make it happy you'd have to add that, and now to also make the trimmer analyzer happy you'd have to also add PublicMethods there. And now you're just causing potentially a boatload more code and metadata to be preserved where you might not actually need that. Or would this work in some other way that I'm missing? I mean the thing I'm not sure I get here is that the "you should always also have | Public" guidance seems to be fundamentally at odds with "you should always preserve the least possible amount of reflection metadata". Or if the analyzer would instead respect the annotation and not emit a warning on correctly annotated uses of Private, does this mean that assuming your codebase is already correctly annotated for trimminng, then this analyzer would just... Basically never trigger at all, ever? 🤔

[GrabYourPitchforks]

@GrabYourPitchforks recently brought this up in another API review, maybe he has more specifics.

Basically what was already mentioned earlier. We've had a few cases where within .NET Framework we changed visibility of members: making public an internal method on an internal type (so the member still isn't part of the reference API), or making public an internal members on a public type (so the member becomes part of the reference API).

Generalize this to cover private reflection not just into .NET Framework, but into libraries and dependencies overall. The goal isn't to make this scenario supported - you're using private reflection, after all! But ideally we'd tell devs "if you absolutely do need to use reflection for this, here's a way to do it in a somewhat less fragile manner."

[bartonjs]

Video

We're not sure that there's benefit in adding the enum member that isn't already (and better) covered by the proposed analyzer. So, until/unless there's something to tip the scales in favor of adding it we feel like the new API should not be added but the analyzer is good.

Analyzer approved
Category: Maintainability
Severity: Warning

[Sergio0694]

"but the analyzer is good"

@bartonjs I'm sorry, can you elaborate more on the "analyzer is good"? The proposed analyzer would push people towards less trimmable code and make them have to preserve additional metadata (all public members) that they might not need at all.

Just my two cents, but I don't think we should ever be adding new analyzers that push people towards less trimmable code 😅

[GrabYourPitchforks]

https://developercommunity.visualstudio.com/t/TextTransformationGenerationEnvironment/10369514 (see also https://developercommunity.visualstudio.com/t/Create-or-recreate-a-Model-does-not-work/10318478) presents a pretty good recent example of where the pattern of using only BindingFlags.NonPublic has bitten people when the target method was later made public.

[GrabYourPitchforks]

Spoke with Sergio offline, and he gave good feedback that we didn't properly convey the scope of the analyzer, which led to his concerns about interfering with the trimming system. We certainly don't want this analyzer to lead people down a path that makes their apps non-trimmable.

My interpretation of this work item is that only this pattern is in scope:

var type = Type.GetType("SomeWellKnownType, SomeAssembly"); // or typeof(SomeWellKnownType)
if (type is not null) {
    var member = type.GetMember("LiteralMemberName", BindingFlags.NonPublic /* | other flags excluding Public */);
    // or GetField / GetProperty / GetMethod / etc.
}

That is, the analyzer triggers only when you are looking for a known member (by name) on a known type (either because the type token or the type name is hardcoded). That's the scenario where we have observed breaks when the target member changes visibility unexpectedly. Another way of looking at it: "You clearly have a distinct target member you're trying to look up, so make your code more reliable by removing visibility checks as an implicit variable you care about."

The analyzer should not recommend including BindingFlags.Public for general reflection scenarios, including scenarios where the target type or target member is unknown / non-literal, or for scenarios where the caller is trying to reflect over all members of a type.

[terrajobst]

The analyzer should not recommend including BindingFlags.Public for general reflection scenarios, including scenarios where the target type or target member is unknown / non-literal, or for scenarios where the caller is trying to reflect over all members of a type.

That's fair.

My general feeling is that folks using private reflection aren't doing themselves any favors for trimmability to begin with. In general, I don't think pushing people towards keeping their private reflection code working when members become public is a significant burden towards this goal.

Said differently, we're not pushing people towards reflection (private or public). We encourage people to use static typing and hence replace reflection / reflection emit solutions with source generators as much as possible.

However, when people are using private reflection we do want their code to be less fragile with respect to visibility changes.

[RenderMichael]

If this analyzer is only intended to fire for a hard-coded member or field name, would this scenario be better served by adding a fixer to transform the code to use [UnsafeAccessor]? At least when using typeof() (which seems to me the common case).

UsafeAccessor is an advanced type with pitfalls and specific implementation requirements (eg. no generics in the attributed method). But as long as we’re doing private reflection anyway, the fixer could ensure it’s properly implemented, and the problem is solved.

Additionally, if a member disappears then the exception with this attribute is much clearer, compared with a return (and since most people suppress null warnings for something they “know” exists, eventually a null reference).