dynamicexpresso / DynamicExpresso

C# expressions interpreter
http://dynamic-expresso.azurewebsites.net/
MIT License
1.99k stars 374 forks source link

Registering Generic Types #308

Closed Vennotius closed 5 months ago

Vennotius commented 5 months ago

When registering a generic type "incorrectly", there is some code in ReferenceType's constructor that looks like it wants to correct the registration (for example MyType<string, object> will become MyType<,>). But after doing that, an exception is thrown. I admit to not understanding this behaviour. :sweat_smile:

public ReferenceType(string name, Type type)
{
    if (string.IsNullOrWhiteSpace(name))
        throw new ArgumentNullException(nameof(name));

    if (type == null)
        throw new ArgumentNullException(nameof(type));

    if (type.IsGenericType && !type.IsGenericTypeDefinition)
    {
        var genericType = type.GetGenericTypeDefinition();
        var genericTypeName = genericType.Name.Substring(0, genericType.Name.IndexOf('`'));
        genericTypeName += $"<{new string(',', genericType.GetGenericArguments().Length - 1)}>";
        throw new ArgumentException($"Generic type must be referenced via its generic definition: {genericTypeName}");
    }

    Type = type;
    Name = name;
    ExtensionMethods = ReflectionExtensions.GetExtensionMethods(type).ToList();
}

Would it not be in line with the code's intention to not throw the exception?

public ReferenceType(string name, Type type)
{
    if (string.IsNullOrWhiteSpace(name))
        throw new ArgumentNullException(nameof(name));

    if (type == null)
        throw new ArgumentNullException(nameof(type));

    if (type.IsGenericType && !type.IsGenericTypeDefinition)
    {
        var genericType = type.GetGenericTypeDefinition();
        var genericTypeName = genericType.Name.Substring(0, genericType.Name.IndexOf('`'));
        genericTypeName += $"<{new string(',', genericType.GetGenericArguments().Length - 1)}>";
        Type = genericType;
        Name = genericTypeName;
    }
    else
    {
        Type = type;
        Name = name;
    }
    ExtensionMethods = ReflectionExtensions.GetExtensionMethods(Type).ToList();
}

I ran into this because I was trying to register my type correctly, but got an error, and had to register it as a closed type. public class Cache<TKey, TValue> yells at me when i register it like this Reference(typeof(Cache<>)):

Error CS0305 Using the generic type 'Cache<TKey, TValue>' requires 2 type arguments


I have looked at #264 and https://github.com/dynamicexpresso/DynamicExpresso/pull/268, but I am not any wiser.

metoule commented 5 months ago

The idea was to have an explicit error message, to help the user register the expected type. For example, Register(typeof(Cache<,>)). We could automatically change the registration to the generic definition instead of throwing, but that would silently hide the fact that it's not the closed type that must be registered.

Vennotius commented 5 months ago

I understand. Thank you. I guess this code is there just for reference if someone wanted to use it.

var genericType = type.GetGenericTypeDefinition();
var genericTypeName = genericType.Name.Substring(0, genericType.Name.IndexOf('`'));
genericTypeName += $"<{new string(',', genericType.GetGenericArguments().Length - 1)}>";