dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.02k stars 4.03k forks source link

Add memberof operator #1653

Closed tumtumtum closed 7 years ago

tumtumtum commented 9 years ago

The idea of the memberof operator would be the equivalent to the typeof operator but would operate on fields, methods and properties. This is useful when working with Expression trees and is distinct from the proposed nameof operator because it returns System.Reflection.MemberInfo objects rather than strings.

Example:

var fieldInfo = memberof(obj.field1);
var methodInfo = memberof(obj.Method1);
var propertyInfo = memberof(obj.Property1);
var getMethodInfo = memberof(obj.Property1.get);
var setMethodInfo = memberof(obj.Property1.set);
var addMethodInfo = memberof(obj.Event1.add);
var removeMethodInfo = memberof(obj.Event1.remove);
svick commented 9 years ago

Eric Lippert has a blog post that explains why this feature is quite problematic. Probably the biggest problem is choosing which overload of a method to return.

HaloFour commented 9 years ago

Probably the biggest problem is choosing which overload of a method to return.

Granted it would be entirely new syntax, but I don't understand why that can't be solved through explicitly specifying the type names as arguments:

public void Foo(int value) { }
public void Bar(int value) { }
public void Bar(object value) { }

MethodBase fooMethod = methodof(Foo); // legal, only one Foo
MethodBase barMethod1 = methodof(Bar); // compiler error, ambiguous
MethodBase barMethod2 = methodof(Bar(int)); // resolves to Bar(int)

Supporting variance would be nice but I don't think it would be necessary:

MethodBase barMethod3 = methodof(Bar(string)); // compiler error, or resolve to Bar(object)?

Also, in order to support events or properties using this kind of syntax would require changes to the CLR, assuming that it uses a similar mechanism to typeof(T). That uses ldtoken to push a runtime handle onto the stack which can only be a fieldref/fielddef, methodref/methoddef or typeref/typedef. The compiler could emit calls to Type.GetProperty or Type.GetEvent by name but it would be less efficient.

I do like the addition here of a syntax to explicitly reference property/event accessor methods.

tumtumtum commented 9 years ago

Appreciate the support for this :-)

Java doesn't have a ldtoken and so the way Java "efficiently" supports their typeof equivalent (Type.class) is to use reflection and then cache the Class object as a static field of the calling class. This approach could be used for memberof on methods, fields and properties (caching the result of Type.GetProperty on the calling class).

sharwell commented 9 years ago

@svick The post by Eric is substantially outdated. During the discussion for the nameof operator, at least one clear and unambiguous way to define this feature was identified. Barriers to this feature are:

  1. It's more complicated than the final form of the nameof operator, so there would still be some implementation work
  2. The nameof operator solves many of the problems which originally caused people to request a memberof operator, and is simpler to understand and use
  3. The nameof operator resolves many situations that memberof couldn't solve, such as throw new ArgumentNullException(nameof(someArgument))

The last two items don't directly prevent memberof from appearing, but they do make it less likely to appear "early."

HaloFour commented 9 years ago

@sharwell

Are you referring to that syntax that involved either providing actual parameter values or default(T) as arguments? I thought that syntax was pretty horrid. It seemed like a cheap way to use the existing syntax to define the expression without actually using the expression.

I do think that nameof() and memberof() solve different kinds of problems. The latter is more for reflection-based resolution and of course doesn't apply to the parameter name problem. For example, creating expression trees with method calls will involve MethodInfo or MethodBase instances. Manually resolving the method by name is less performant and more error prone, especially when dealing with generic methods as the BCL provides virtually no help resolving them.

tumtumtum commented 9 years ago

Manually resolving MethodInfo using reflection is also prone to runtime errors if the APIs change as well. For example, a naive call to Type.GetMethods().First(c => c.Name == "Foo") will break if overloads to Foo are added later on. Using memberof(Type.Foo()) is much safer.

Trying to exactly resolve a method with the signature void Foo<T>(string s, out T x) where T : struct using reflection is a real pain.

svick commented 9 years ago

@tumtumtum

Trying to exactly resolve a method with the signature void Foo<T>(string s, out T x) where T : struct using reflection is a real pain.

Hmm, that sounds like something that could be solved by a library that takes a delegate type. Something like:

delegate void FooDelegate<T>(string s, out T x) where T : struct;

…

var mi = type.GetMethod("Foo", typeof(FooDelegate<>));

It feels weird to create a delegate type just to describe the signature of a method, but otherwise I think it should work pretty well.

Maybe something like this could be used for memberof too?

tumtumtum commented 9 years ago

That's actually not an unreasonable suggestion. Could get away with Func<...> and Action<...> most of the time too (just not with ref/out arguments).

HaloFour commented 9 years ago

@svick Such a method would be very helpful as an addition to the BCL. The state of reflecting types/methods is fairly poor as no real improvements have been made to the API since 1.x. That method would still be slower than a real methodof since it would have to parse out the signature of the delegate and then resolve the methods by name, and you'd need to ensure that your binding flags are correct for the accessibility of the method.

I've proposed other helper methods to be added to the BCL for generic type/method reflection based on extension methods that I've written and have been using very successfully for some years:

Proposal: Additional methods to aid in reflection of generic types

dsaf commented 9 years ago

memberof creates a cognitive dissonance because it reads as "get member of 'member'" instead of "get member of 'type'". typeof and nameof don't have this problem. Shouldn't it be member instead? In this case the keyword could be reused for matching or constraints, e.g. F# has this: https://msdn.microsoft.com/en-us/library/dd548046.aspx

Also reflection is used quite rarely in comparison to other features. Maybe fixing the existing reflection API and object model to make it more usable would make more sense?

HaloFour commented 9 years ago

The purpose is to get the member of the provided token, so memberof is appropriate. It's the same logic behind typeof, and in the case of methods and fields it is nearly functionally equivalent since IL ldtoken supports pushing metadata tokens of those members which can be converted into MethodBase or FieldInfo instances respectively using static methods.

dsaf commented 9 years ago

"member of the provided token"

What does this mean? It reads like: "here is a token x, give me a member that is a part of it". Which is not what you want. You need something that reads like: "give me a member x".

Maybe what you want is infoof? This would make much more sense, especially given the recent split of Type and TypeInfo:

typeof(int); //Get an instance of Type.
infoof(int); //Get an instance of TypeInfo.
infoof(int.parse); //Get an instance of MethodInfo.
HaloFour commented 9 years ago

The name is a lot less relevant to me than the capability. I often throw around methodof() as there is built in support for resolving a method by token in the CLR, just as you would resolve a type. In fact Reflector has always decompiled a combination of ldtoken and MethodBase::GetMethodFromHandle as a synthetic methodof() syntax, although they never bothered working out the syntax of overload resolution.

My issue with infoof() or even memberof() is that it has to do different things based on how it's used. With fields and methods it can easily resort to metadata tokens, which are resolved at compile time. For anything else the compiler would have to emit reflection calls.

Ricciolo commented 9 years ago

Any news about this request? Because many times I've this need. The compiler can work in the same way it compiles the Expression, using ldtoken MethodBase::GetMethodFromHandle. As workaround, in order to have compile time check, I generally use Expression then I look for the member:

static void Main(string[] args)
{
    Expression<Action<string>> t = s => Test(s);
    MethodBase method = ((MethodCallExpression)t.Body).Method;
}

private static void Test(string s)
{

}

But I don't like it so much

tumtumtum commented 8 years ago

For now Ricciolo's excellent suggestion is what I'm doing

var indexOfCharMethod = TypeUtils.GetMethod<string>(c => c.IndexOf(default(char)));

TypeUtils.cs

tumtumtum commented 8 years ago

With regards to naming, memberof could read as "the member of x" or "an instance of System.MemberInfo for x". The later makes sense and in fact matches typeof which reads as "an instance of System.Type for x".

Although System.Type extends MemberInfo so theoretically memberof should work on System.Type. Maybe infoof isn't such a bad idea ;-)

gafter commented 7 years ago

We are now taking language feature discussion in other repositories:

Features that are under active design or development, or which are "championed" by someone on the language design team, have already been moved either as issues or as checked-in design documents. For example, the proposal in this repo "Proposal: Partial interface implementation a.k.a. Traits" (issue 16139 and a few other issues that request the same thing) are now tracked by the language team at issue 52 in https://github.com/dotnet/csharplang/issues, and there is a draft spec at https://github.com/dotnet/csharplang/blob/master/proposals/default-interface-methods.md and further discussion at issue 288 in https://github.com/dotnet/csharplang/issues. Prototyping of the compiler portion of language features is still tracked here; see, for example, https://github.com/dotnet/roslyn/tree/features/DefaultInterfaceImplementation and issue 17952.

In order to facilitate that transition, we have started closing language design discussions from the roslyn repo with a note briefly explaining why. When we are aware of an existing discussion for the feature already in the new repo, we are adding a link to that. But we're not adding new issues to the new repos for existing discussions in this repo that the language design team does not currently envision taking on. Our intent is to eventually close the language design issues in the Roslyn repo and encourage discussion in one of the new repos instead.

Our intent is not to shut down discussion on language design - you can still continue discussion on the closed issues if you want - but rather we would like to encourage people to move discussion to where we are more likely to be paying attention (the new repo), or to abandon discussions that are no longer of interest to you.

If you happen to notice that one of the closed issues has a relevant issue in the new repo, and we have not added a link to the new issue, we would appreciate you providing a link from the old to the new discussion. That way people who are still interested in the discussion can start paying attention to the new issue.

Also, we'd welcome any ideas you might have on how we could better manage the transition. Comments and discussion about closing and/or moving issues should be directed to https://github.com/dotnet/roslyn/issues/18002. Comments and discussion about this issue can take place here or on an issue in the relevant repo.


I am not moving this particular issue because I don't have confidence that the LDM would likely consider doing this.