Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type.CanMakeGenericType proposal #28033

Open
jbogard opened this issue Nov 30, 2018 · 17 comments
Open

Type.CanMakeGenericType proposal #28033

jbogard opened this issue Nov 30, 2018 · 17 comments
Labels
api-suggestion area-System.Reflection enhancement
Milestone

Comments

@jbogard
Copy link
Contributor

jbogard commented Nov 30, 2018

Background and Motivation

Today, the only way at runtime to determine if a generic type definition can successfully close based on a set of generic parameters is to call Type.MakeGenericType and catch any exceptions:

var genericTypeDefinition = typeof(List<>);
Type closedType = null;
try {
    closedType = genericTypeDefinition.MakeGenericType(typeof(int));
} catch (ArgumentException) { }
bool canClose = closedType == null;

In libraries that use open generic types heavily (such as DI containers), either the logic for determining whether or not an open generic type can be closed is duplicated, or this exception is caught. Many times, the libraries will do both - cover the basic cases but still rely on exceptions.

This is especially useful in generic constraints, where I have a collection of open generic types, and I only want to resolve the closed types that satisfy the generic constraint at runtime.

Proposed API

Implement the Tester-Doer pattern:

namespace System
{
    public abstract class Type {
        public virtual Type MakeGenericType(params Type[] typeArguments);
+       public virtual bool TryMakeGenericType(out Type type, params Type[] typeArguments);
     }

Usage Examples

Simple example:

var genericTypeDefinition = typeof(List<>);
if (genericTypeDefinition.TryMakeGenericType(out var closedType, typeof(int)) {
   // use the closed type
}

More complex example:

public interface IValidator<T> {
    bool IsValid(T);
}

public class FormValidator : IValidator<Form> {
   // impl
}

public class UserValidator<T> : IValidator<T>
    where T : IUserForm {
     // impl
}

public class ApproveUserForm : IUserForm {
}

// now when we query all closed types of IValidator<T>, we only want those that can satisfy the type constraints

services.GetServices<IValidator<Form>>(); // count is 1
services.GetServices<IValidator<ApproveUserForm>>(); // count is 2

The DI container in these examples would use the TryMakeGenericType method instead of catching the exception.

Alternative Designs

Instead of the Tester-Doer pattern, a check to see if a type can be closed:

namespace System
{
    public abstract class Type {
        public virtual Type MakeGenericType(params Type[] typeArguments);
+        public virtual bool CanMakeGenericType(params Type[] typeArguments);
     }

Risks

This API is not exposed directly in the CLR.

@jkotas jkotas transferred this issue from dotnet/coreclr Nov 30, 2018
@jkotas
Copy link
Member

jkotas commented Nov 30, 2018

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged label Feb 26, 2020
@steveharter
Copy link
Member

steveharter commented Mar 4, 2020

Can you provide details on the scenario where you don't know if MakeGenericType will succeed? Normally when calling that API you know the generic arguments.

@GrabYourPitchforks GrabYourPitchforks added needs more info and removed untriaged labels Mar 4, 2020
@jbogard
Copy link
Contributor Author

jbogard commented Mar 5, 2020

@steveharter literally every single dependency injection container you look at won't know those generic arguments until resolution time, so all of them call MakeGenericType in a try-catch block. The only one that doesn't is the MS Ext DI container, and it will allow an unhandled runtime exception, blowing up the application.

I have a PR to fix this behavior: dotnet/extensions#536 but obviously that is not ideal, I would rather use the TryXyz behavior.

Some DI containers try to reverse engineer the constraint check, but as you can imagine, it's horribly ugly and error-prone. So the @jskeet approved method is to call MakeGenericType and catch the exception.

@jkotas
Copy link
Member

jkotas commented Mar 5, 2020

From the runtime point of view, trying to create random generic instantiations without knowing that they are valid is a bad pattern.

Note that the fact MakeGenericType happens to succeed does not guarantee that the instantiation is valid. For example, Vector<T> instantiations are valid for certain Ts only that is not reflected in the type constrains; or the C# unmanaged constrain is not checked by the runtime at all.

I think that this issue can be closed. The DI systems that would like to use this pattern can keep using try / catch.

@dotnetjunkie
Copy link
Contributor

dotnetjunkie commented Jun 17, 2020

@jkotas

From the runtime point of view, trying to create random generic instantiations without knowing that they are valid is a bad pattern.

As @jbogard already mentioned, for DI Containers it is very common to build generic types. Simple Injector for instance applies decorators conditionally and filters collections based on generic type constraints. And generic type constrains can become very, very complex, but they can do so because of valid reasons.

The DI systems that would like to use this pattern can keep using try / catch.

I would argue against this. The lack of a Type.TryMakeGenericType forces DI libraries to swallow exceptions. Swallowing exceptions still causes developers to be prompted with an exception window in the Visual Studio debugger, because of the default VS settings. This leads to confusing situations, because its not always clear that the exception can be ignored, and it is a productivity killer for the debugger to break in the middle of a debugging session.

To prevent getting these first-chance exceptions, Simple Injector currently tries to do the complete analysis of generic type constraints in order to prevent developers from getting a popup they can ignore. This, however, causes a lot of complexity, because, as I already stated, generic type constraints can become extremely complex.

But Simple Injector is by far the only library that does this. All DI Containers have this problem, and MS.DI will likely get this problem in the future when it becomes more mature. And there are likely other types of libraries that apply this kind of filtering based on generic type constraints. In other words, the addition of a Type.TryMakeGenericType is highly appreciated.

@jkotas
Copy link
Member

jkotas commented Jun 17, 2020

Swallowing exceptions still causes developers to be prompted with an exception window in the Visual Studio debugger

I do not believe that it is the case. The default setting for Visual Studio debugger is to continue on exceptions (the exception gets printed into debug output window).

For reference: #21785 has a long discussion on merits of adding Try variants for existing APIs.

@dotnetjunkie
Copy link
Contributor

dotnetjunkie commented Jun 17, 2020

I certainly do understand the consequences of having Try* APIs, which has its downsides. But let's take a step back and look at what's actually requested here, where a TryMakeGenericType is just one of the solutions.

The actual problem is that we want to be able to verify whether the supplied types match a generic type's generic type constraints. Whether this is done using Type.TryMakeGenericType, Type.CanMakeGenericType, GenericTypeConstraintVerifier.VerifyTypeConstraintsFor, or anything else, is IMO not that relevant. We want to be able to reliably knowing up front whether we can safely call Type.MakeGenericType, without it throwing because of type constraint mismatches. You've got carte blanche in designing that API ;-)

@seesharper
Copy link

seesharper commented Jul 3, 2020

+1

@steveharter
Copy link
Member

steveharter commented Jul 7, 2020

We need an actual API proposal in order for this to get approved. Can @jbogard or other propose the APIs?

However, the window for making API reviews is essentially closed for 5.0, so moving to Future.

@steveharter steveharter modified the milestones: 5.0.0, Future Jul 7, 2020
@jbogard
Copy link
Contributor Author

jbogard commented Jul 9, 2020

@steveharter updated the original comment.

@jbogard jbogard changed the title Type.TryMakeGenericType proposal Type.TryMakeGenericType or CanMakeGenericType proposal Jul 20, 2020
@davidfowl
Copy link
Member

davidfowl commented Aug 14, 2020

I think we want the alternative proposal:

namespace System
{
    public abstract class Type {
        public virtual Type MakeGenericType(params Type[] typeArguments);
+        public virtual bool CanMakeGenericType(params Type[] typeArguments);
     }

I can take this to API review.

@davidfowl davidfowl changed the title Type.TryMakeGenericType or CanMakeGenericType proposal Type.CanMakeGenericType proposal Aug 14, 2020
@jkotas
Copy link
Member

jkotas commented Aug 15, 2020

I assume that this API is meant to just check the generic constraints. I would call it SatisfiesConstraints to make it clear what it does.

There is a parallel MethodInfo.MakeGenericMethod that should get symmetric treatment.

The implementation of this method is going to do foreach (var parameter in GetGenericArguments()) { if (!parameter.SatisfiesConstraints(...) return false; }. Would it make sense to have SatisfiesConstraints on the generic argument instead? It would allow you to figure which generic argument is not satisfying the constraints.

@huysentruitw
Copy link

huysentruitw commented Aug 15, 2020

IMO SatisfiesConstraints does not make it more clear, unless you'd call it SatisfiesGenericTypeConstraints. I think from the API consumer side, the CanMakeXyz that takes the same parameters as the MakeXyz would be more clear.

@svick
Copy link
Contributor

svick commented Aug 15, 2020

@jkotas If you're suggesting the usage to be e.g. typeof(List<>).SatisfiesConstraints(typeof(int)), then I think that's more confusing. I would initially read it as the nonsensical "Does List<> satisfy the int constraint?" On the other hand, I think CanMakeGenericType is more understandable. Specifically, I would read typeof(List<>).CanMakeGenericType(typeof(int)) as "Can List<> make generic type using int?"

Also, CanMakeGenericType clearly indicates it's a companion API to MakeGenericType, SatisfiesConstraints does not make that association obvious.

@jkotas
Copy link
Member

jkotas commented Aug 15, 2020

MakeGenericType can fail for number of reasons. Do you expect CanMakeGenericType to cover all reasons why it can fail, or just the constraints?

@huysentruitw
Copy link

huysentruitw commented Aug 15, 2020

From the CanMakeXyz perspective, it should return false for any failure reason. If that is what we want in the way the DI container will use it, is another question indeed.

@davidfowl
Copy link
Member

davidfowl commented Aug 15, 2020

I agree with @jkotas that we really only care about constraints. For example, another reason it can fail today is for generic arity mismatches (we don't care as much about that).

namespace System
{
    public abstract class Type 
    {
+       public virtual bool SatisfiesGenericConstraints(params Type[] typeArguments);
    }

I do think we want something that's simple to use so something like this would be preferred. The other API we could consider adding is the one that works on generic parameters:

The implementation of this method is going to do foreach (var parameter in GetGenericArguments()) { if (!parameter.SatisfiesConstraints(...) return false; }. Would it make sense to have SatisfiesConstraints on the generic argument instead? It would allow you to figure which generic argument is not satisfying the constraints.

namespace System
{
    public abstract class Type 
    {
+       public virtual bool SatisfiesConstraints(Type parameter);
    }

Example:

class MyType<T> where T : class
{
}
typeof(MyType<>).GetGenericArguments()[0].SatisfiesConstraints(typeof(int)) // This should be false
typeof(MyType<>).SatisfiesGenericConstraints(typeof(int)) // This should be false.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion area-System.Reflection enhancement
Projects
No open projects
Development

No branches or pull requests