Expose SeparatedSyntaxList<T>.GetWithSeparators() through interface

[Rekkonnect]

Background and Motivation

SeparatedSyntaxList<T> is a generic type, so when a value of this type is retrieved via reflection, we don't have a useful non-generic type to refer to. GetWithSeparators() returns SyntaxNodeOrTokenList which is unbound to the generic type, thus requiring unnecessary workarounds, like this:

// given:
IReadOnlyList<SyntaxNode> list;
var listType = list.GetType();

var genericDefinition = listType.GetGenericTypeDefinition();
if (genericDefinition == typeof(SeparatedSyntaxList<>))
{
    // we use the underlying node or token list to display children
    SyntaxNodeOrTokenList nodeOrTokenList = (list as dynamic).GetWithSeparators();
    return Use(nodeOrTokenList);
}

Proposed API

 namespace Microsoft.CodeAnalysis
 {
+    public interface ISeparatedSyntaxList
+    {
+        public SyntaxNodeOrTokenList GetWithSeparators();
+    }
 }

Usage Examples

The above evaluator can be rewritten as (assuming we only care about SeparatedSyntaxList<T>):

// given:
ISeparatedSyntaxList list;
SyntaxNodeOrTokenList nodeOrTokenList = list.GetWithSeparators();
return Use(nodeOrTokenList);

Alternative Designs

Retaining the design as-is.

Risks

Bloating the API with further interface abstractions.

[CyrusNajmabadi]

I don't understand why an interface is necessary here. If you're using reflection, you can get to the GetWithSeparators() method easily and can call that.

[Rekkonnect]

Yes but the point is to avoid discovering the method, or calling it dynamically like in the example. I find it more convenient to use that way, and avoiding concerns for future breaks.

[333fred]

I don't understand the use case here. Can you please elaborate on what the actual problem you're trying to solve is? Also, who would be implementing this interface? I just don't have enough information from this request to understand what you're trying to do or how this would actually help.

[Rekkonnect]

I'm traversing the syntax tree and retrieving the syntax-related properties (SyntaxNode, SyntaxToken, etc.), which recursively traverses further down the tree. This may include properties like SeparatedSyntaxList<T>, though without having the static type of the property, I can't know the type argument. Therefore, to invoke GetWithSeparators() I have to either invoke a delegate by retrieving its invoker via reflection, or by relying on dynamic.

It would help having some way to expose the GetWithSeparators() method without knowing the type of syntax nodes held inside the SeparatedSyntaxList. What I thought is having an one-off interface called ISeparatedSyntaxList exposing this method, removing the need for using reflection/dynamic altogether.

[CyrusNajmabadi]

Therefore, to invoke GetWithSeparators() I have to either invoke a delegate by retrieving its invoker via reflection

But you said you were already using reflection here. So why not continue to do that? I don't get the desire to have a hybrid mode here.

[Rekkonnect]

Writing good reflection is just tedious, that's all. If the API provided this interface, it would be trivial to invoke the method and retrieve its children nodes, assuming we have already evaluated that our object is of that type.

Right now I evaluate whether the object's type is a construction of the generic type SeparatedSyntaxList<T>, and then invoke the cached method delegate on it. That's much more tedious to write, and also worse in performance than a simple type check and virtual call.

[CyrusNajmabadi]

What's the performance scenario here where reflection is used at all?

Do you have a trace in your scenario where this is showing up as a substantive cost?

[Rekkonnect]

Removing even a few reflection calls can be helpful. Even though it's being used extensively, it's better to avoid some small parts using reflection, if they could be designed otherwise.

I think that this is more of a type system problem in C# itself than it is for Roslyn. When you can't concretely type a type argument and want to invoke a method returning a fully known result, you can't use it statically unless you have a concretely statically typed construction of the generic type of your object. If this proposal is too hard to justify, we may as well ignore it.

[CyrusNajmabadi]

Removing even a few reflection calls can be helpful.

I'd like to see data from this scenario demonstrating that though. How much actual wallclock time is being spent inside reflection here.

We don't ever do perf changes for hypothetical improvements. We need a real scenario demonstrating the problem. otherwise we can't validate taht the change actually solves the issue in the first place.