RegisterTypes will attempt to register abstract types (undocumented breaking change in v6.5.0)

rosslovas commented 11 months ago

Describe the Bug

In Autofac v6.4.0, a scanning registration using RegisterTypes that includes types that cannot be constructed (e.g. interfaces, abstract classes) will work fine.

In Autofac v6.5.0 onwards, it will attempt to register those non-constructible types resulting in an Autofac.Core.Activators.Reflection.NoConstructorsFoundException upon building the container.

This is a breaking change that I couldn't find documented anywhere.

Steps to Reproduce

public class ReproTest
{
    interface IInterface { }

    class Class : IInterface
    {
    }

    [Fact]
    public void Repro()
    {
        var containerBuilder = new ContainerBuilder();

        containerBuilder
            .RegisterTypes(typeof(IInterface), typeof(Class))
            .AssignableTo<IInterface>();

        var exception = Record.Exception(() => containerBuilder.Build());

        Assert.Null(exception);
    }
}

Expected Behavior

No exception, test passes (v6.4.0 behaviour)

Exception with Stack Trace

v6.5.0 onwards throws:

Autofac.Core.Activators.Reflection.NoConstructorsFoundException: No accessible constructors were found for the type 'ReproTestProject.ReproTest+IInterface'.
   at Autofac.Core.Activators.Reflection.DefaultConstructorFinder.GetDefaultPublicConstructors(Type type)
   at Autofac.Core.Activators.Reflection.DefaultConstructorFinder.FindConstructors(Type targetType)
   at Autofac.Core.Activators.Reflection.ReflectionActivator.ConfigurePipeline(IComponentRegistryServices componentRegistryServices, IResolvePipelineBuilder pipelineBuilder)
   at Autofac.Core.Registration.ComponentRegistration.BuildResolvePipeline(IComponentRegistryServices registryServices, IResolvePipelineBuilder pipelineBuilder)
   at Autofac.Core.Registration.ComponentRegistration.BuildResolvePipeline(IComponentRegistryServices registryServices)
   at Autofac.Core.Registration.ComponentRegistryBuilder.Build()
   at Autofac.ContainerBuilder.Build(ContainerBuildOptions options)
   at ReproTestProject.ReproTest.<>c__DisplayClass2_0.<Repro>b__0()

Dependency Versions

Autofac: v6.5.0

Additional Info

A workaround is for the user to filter out abstract types from the types passed to RegisterTypes themselves.

In trying to figure out why I was getting this exception after upgrading to v6.5.0 from v6.4.0 I took a look at the diff between the two versions, and the removal of !t.IsAbstract in CanBeRegistered in this commit was something that stood out to me that might be the cause.

tillig commented 11 months ago

Based on the repro, it seems like the problem has to do with interface types, not abstract types. (abstract is a keyword with special meaning.) It's that correct?

tillig commented 11 months ago

Also, reading this through, it seems like - while the behavior is, admittedly, breaking - it's potentially a desired behavior. That is, the repro is saying, "I want to try to register this interface in the container!" and the container, on build, is saying, "Sorry, you can't do that."

I know we've seen some questions before with folks wondering why we'd "allow" different behaviors that aren't "legal" or consistent. For example, if you tried to do builder.RegisterType<IInterface> you'd get an exception, but, previously, you wouldn't get the exception with the plural RegisterTypes. Now it's consistent - in neither case are you allowed to register an interface.

Does this sound like the issue you're describing?

rosslovas commented 11 months ago

Based on the repro, it seems like the problem has to do with interface types, not abstract types. (abstract is a keyword with special meaning.) It's that correct?

The repro behaviour is the same for abstract classes too. The behaviour affects all abstract types which includes interfaces, even though the abstract keyword isn't necessary for them (typeof(IInterface).IsAbstract is true).

Also, reading this through, it seems like - while the behavior is, admittedly, breaking - it's potentially a desired behavior. That is, the repro is saying, "I want to try to register this interface in the container!" and the container, on build, is saying, "Sorry, you can't do that."

I know we've seen some questions before with folks wondering why we'd "allow" different behaviors that aren't "legal" or consistent. For example, if you tried to do builder.RegisterType<IInterface> you'd get an exception, but, previously, you wouldn't get the exception with the plural RegisterTypes. Now it's consistent - in neither case are you allowed to register an interface.

Does this sound like the issue you're describing?

Yeah, the undocumented breaking change aspect is definitely the main point here. It caused a lot of unexpected difficulties in a codebase that heavily used scanning registrations over all of the types in certain assemblies, all of which had to be updated to include a !t.IsAbstract filter. I also found this comment which looks like it may be someone else hitting the same thing.

tillig commented 11 months ago

Let me see if I can figure out whether this was intentional or not so we can determine what to do.

tillig commented 11 months ago

Looking at the changes, we can see what happened: A lot was shifting around in the effort to centralize reflection type caches (to support AssemblyLoadContext unloading) and that, combined with the very few (one?) test we have for RegisterTypes meant that when the filter was refactored it looked like it was only used in open generic scanning. I guess that's a lesson in why naming and code organization is important - it's in OpenGenericScanningRegistrationExtensions but it has nothing to do with open generics in this case.

So: it was an inadvertent breaking change.

Darn it.

The question is now to figure out what to do about it.

I can absolutely see the desire to be able to pass in an array of whatever types and have the filtering done for you. It makes it easy to throw stuff in and go, and it has reasonable parity with some of the other methods like RegisterAssemblyTypes. However, it has this "silently ignore things" problem that causes some people to wonder why they think they registered a thing but it's just not registered. "I know I passed that type in, but it's not registered. What gives?"

I can also see the desire to have it work like RegisterType where it won't actually let you do builder.RegisterType<IInerface>() - it is, after all, the plural of RegisterType so that would make sense. However, the error message is bad - blowing up with a "no constructors found" is not awesome, it should be far more descriptive.

If we put the filter back, it should have no effect on users who have updated to the more "trimmed" lists of only valid types, so it wouldn't be another breaking change. It may also fix some folks who relied on the old behavior. Hence, while I'm not really interested in encouraging folks to send a raft of bad types in there, I sort of lean a tad toward fixing this up and putting the filter back on.

In either case, we should:

Capture this somehow in documentation.
Add a bunch more tests around RegisterTypes so we don't regress.
Fix up the naming of the classes where these filtering extensions live so it's more clear that they're used in scenarios other than just open generic scanning. They all appear to be internal/private so it should be zero impact on the public API.

Let me tag @alistairjevans to see if he has any additional thoughts.

tillig commented 11 months ago

Chatted with @alistairjevans and I think we're in agreement this is a bug. I was thinking it'd be nice to keep RegisterTypes(params Type[] t) behaving identical to RegisterType<T> and RegisterType(Type t) but there is some convenience to be had by not having to pre-filter the array of types oneself, making it closer to RegisterAssemblyTypes. It turns out if you do RegisterType<IInterface>() it also throws at container build time with the same "no constructors found" error, so it would probably be good to have a better/more descriptive error in that case.

So the plan of action:

Put the type filtering back on RegisterTypes to fix the inadvertent breaking change.
Add more tests to RegisterTypes.
Fix naming/refactor internals to make scanning extension/helper usage more clear.
Improve the error message that comes from incorrect RegisterType<T> and RegisterType(Type t) - possibly using the same filtering that the scanning uses to detect "registerable types."

I'll see about getting a PR in for this soon.

tillig commented 11 months ago

This is going out in 7.1.0 which should be published on NuGet within the next hour.

autofac / Autofac