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

AllowOverridingRegistrations breaks when generic type constraint "new()" is applied to abstraction #387

Closed
J-Hauser opened this Issue Mar 7, 2017 · 10 comments

Comments

2 participants
@J-Hauser

J-Hauser commented Mar 7, 2017

Perhaps i am doing something wrong.
Here is a test for the mentioned behavior. Removing the new()-constraint or using another constraint (like class) works.

Edit: Simplified breaking test.

using Microsoft.VisualStudio.TestTools.UnitTesting;

namespace SimpleInjector.Tests
{
    [TestClass]
    public class UnitTest1
    {
        [TestMethod]
        public void AllowOverridingRegistrations_breaks_with_generic_type_constraint_new()
        {
            var container = new Container();
            container.Register(typeof(IWithGenericTypeConstraint<>), typeof(WithGenericTypeConstraint<>));
            container.Options.AllowOverridingRegistrations = true;
            container.Register(typeof(IWithGenericTypeConstraint<>), typeof(WithGenericTypeConstraint<>));
        }
    }

    public interface IWithGenericTypeConstraint<T> where T : new()
    {
    }

    public class WithGenericTypeConstraint<T> : IWithGenericTypeConstraint<T> where T : new()
    {
    }
}
@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Mar 7, 2017

Can you please simplify your code to the absolute minimum amount of code that demonstrates your problem?

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Mar 7, 2017

I think your question is a duplicate of the following: #384

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Mar 7, 2017

Side note: also make sure that you're not abusing the packages feature. See the documentation for more information.

@J-Hauser

This comment has been minimized.

J-Hauser commented Mar 7, 2017

Not quite the same as #384 i think. If i remove the new()-Constraint it works.

RegisterConditonal could help, but i could be dealing with a hierachy of registrations. Then RegisterConditional would break at the second registration.

[TestMethod]
public void AllowOverridingRegistrations_breaks_with_generic_type_constraint_new()
{
    var container = new Container();

    container.RegisterConditional(typeof(IWithGenericTypeConstraint<>),
        typeof(WithGenericTypeConstraint<>),c => SomeMethod());

    container.RegisterConditional(typeof(IWithGenericTypeConstraint<>),
        typeof(WithGenericTypeConstraint<>), c => SomeOtherMethod());

    container.RegisterConditional(typeof(IWithGenericTypeConstraint<>),
        typeof(WithGenericTypeConstraint<>), c => !c.Handled);

    var instance = container.GetInstance<IWithGenericTypeConstraint<UnitTest1>>();
}

private bool SomeOtherMethod()
{
    throw new NotImplementedException();
}

private bool SomeMethod()
{
    throw new NotImplementedException();
}
@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Mar 7, 2017

I reduced your test case to the following:

public interface IFoo<T> where T : new() { }
public class Foo1<T> : IFoo<T> where T : new() { }
public class Foo2<T> : IFoo<T> where T : new() { }

public static void Main()
{
    var container = new Container();

    container.Options.AllowOverridingRegistrations = true;

    container.Register(typeof(IFoo<>), typeof(Foo1<>));

    container.Register(typeof(IFoo<>), typeof(Foo2<>));
}

The last line fails with the following exception:

The making of conditional registrations is not supported when AllowOverridingRegistrations is set, because it is impossible for the container to detect whether the registration should replace a different registration or not.

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Mar 7, 2017

The reason this fails is that Simple Injector is often unable to whether the generic type constraints on an implementation cause that type to describe a sub set of the possible types that the given abstraction allows. This makes Simple Injector think that the given implementation should be applied conditionally (based on its type constraints) and that's why it doesn't allow that type to be replaced, although in reality the replace would actually be safe.

This is a known limitation in Simple Injector's generic sub system, and is not something that is not easily solved.

As a work around, ensure that the registration doesn't need overriding. This can be done by moving the registration out of the default package, and instead add the registration to all packages that override the default.

@dotnetjunkie dotnetjunkie added this to the v4 milestone Mar 7, 2017

@dotnetjunkie dotnetjunkie self-assigned this Mar 7, 2017

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Mar 7, 2017

After some investigation, I noticed that this was the only 'simple' type constraint that would fail. Other type constraints like where T : class, where T : struct would work as expected.

This will be fixed in v4.

@J-Hauser

This comment has been minimized.

J-Hauser commented Mar 8, 2017

After writing this comment I saw your commit.
Nevertheless my question: I was wondering if it would suffice to ask the ConcreteType if their MappingConstraints are matching the Arguments MappingConstraints in TypeConstraintValidator?

private bool ParameterSatisfiesDefaultConstructorConstraint()
{
    if (!this.MappingHasConstraint(GenericParameterAttributes.DefaultConstructorConstraint))
    {
        return true;
    }
    
    if(this.ConcreteTypeOfMappingHasConstraint(
        GenericParameterAttributes.DefaultConstructorConstraint))
    {
        return true;
    }

    if (this.Mapping.ConcreteType.IsValueType())
    {
        // Value types always have a default constructor.
        return true;
    }

    return this.Mapping.ConcreteType.GetConstructor(Helpers.Array<Type>.Empty) != null;
}

private bool ConcreteTypOfMappingHasConstraint(GenericParameterAttributes constraint)
{
    if (!this.Mapping.ConcreteType.IsGenericParameter)
    {
        return false;
    }

    var constraints = this.Mapping.ConcreteType.GetGenericParameterAttributes();
    return (constraints & constraint) != GenericParameterAttributes.None;
}

Worked with this

using SimpleInjector;

namespace ConsoleApplication1
{
    public interface IBaz<T> where T : new() { }
    public class Baz<T> : IBaz<T> where T : new() { }
    public class Baz2<T> : IBaz<T> where T : new() { }
    public class SubBaz<T> : Baz<T> where T : new() { }

    public class Program
    {
        static void Main(string[] args)
        {
            var container = new Container();
            container.Options.AllowOverridingRegistrations = true;
            container.Register(typeof(IBaz<>), typeof(Baz<>));
            container.Register(typeof(IBaz<>), typeof(SubBaz<>));
            container.Register(typeof(IBaz<>), typeof(Baz2<>));
            container.Verify();
            var x = container.GetInstance(typeof(IBaz<Program>));
        }
    }
}

Also please be patient, if this is a dumb question, but why not remove the RegistrationEntry in GetOrCreateRegistrationalEntry if AllowOverridingRegistrations is true?

private IRegistrationEntry GetOrCreateRegistrationalEntry(Type serviceType)
{
    Type key = GetRegistrationKey(serviceType);

    var entry = this.explicitRegistrations.GetValueOrDefault(key);

    if (this.Options.AllowOverridingRegistrations)
    {
        this.explicitRegistrations.Remove(key);
        entry = null;
    }

    if (entry == null)
    {
        this.explicitRegistrations[key] = entry = RegistrationEntry.Create(serviceType, this);
    }

    return entry;
}

Thank you for your fast feedback and your great work!

@dotnetjunkie

This comment has been minimized.

Collaborator

dotnetjunkie commented Mar 8, 2017

Thank you for this code. Your code actually is an improvement to my last check-in, because my check-in will cause a first change exception in case the "new()" constraint is added to the implementation (making it conditional). In that case Simple Injector will try to construct an open-generic implementation, but this will fail. This will be caught higher up within Simple Injector, so the application will function correctly. The throwing of exceptions however does cost and could influence app startup time. On top of that, developers (of newer Visual Studio versions) sometimes get confused, because VS default behavior is to prompt the developer about first chance exceptions. This is why we try to minimize the number of exceptions that are thrown internally, and that's why your solution is better.

Thanks again.

dotnetjunkie added a commit that referenced this issue Mar 8, 2017

Improved TypeConstraintValidator.
Improved TypeConstraintValidator based on feedback from J-Hauser. This
improvement prevents first chance exceptions in case the implementation
applies the "new()" constraint, while its abstraction doesn't. Related
to #387
@J-Hauser

This comment has been minimized.

J-Hauser commented Mar 8, 2017

Thanks. Glad i could be of help. :-)

@dotnetjunkie dotnetjunkie changed the title from AllowOverridingRegistrations breaks with generic type constraint new() when overriding to AllowOverridingRegistrations breaks when generic type constraint "new()" is applied to abstraction Mar 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment