Add support for required properties

[alistairjevans]

This change implements support for resolving required properties "by default", i.e. without requiring PropertiesAutowired to be called.

Here's an example from the tests:

public class Component
{
    required public ServiceA ServiceA { get; set; }

    required public ServiceB ServiceB { get; set; }
}

var builder = new ContainerBuilder();
builder.RegisterType<ServiceA>();
builder.RegisterType<ServiceB>();
builder.RegisterType<Component>();

var container = builder.Build();

var component = container.Resolve<Component>();

Assert.NotNull(component.ServiceA);
Assert.NotNull(component.ServiceB);

Any required property must be resolvable, otherwise you get a DependencyResolutionException; for example if you miss out the registration for ServiceB in the above example:

Autofac.Core.DependencyResolutionException : An exception was thrown while activating Autofac.Specification.Test.Features.RequiredPropertyTests+Component.
    ---- Autofac.Core.DependencyResolutionException : One or more of the required properties on type 'Autofac.Specification.Test.Features.RequiredPropertyTests+Component' could not be resolved:
    ServiceB (Autofac.Specification.Test.Features.RequiredPropertyTests+ServiceB)

The exception message details all required properties that could not be set.

SetsRequiredMembers attribute

If the selected constructor used for activation based on available parameters and registrations has the SetsRequiredMembers attribute applied to it, we will make no attempt to set those required properties; we assume the constructor does it (making much the same assumption as the C# compiler).

Parameters

Configured property parameters provided at registration time override any automatic population of required properties.

var builder = new ContainerBuilder();
builder.RegisterType<ServiceA>();
builder.RegisterType<ServiceB>();
builder.RegisterType<Component>().WithProperty(nameof(Component.ServiceB), new ServiceB { Tag = "custom" });

var container = builder.Build();

var component = container.Resolve<Component>();

Assert.Equal("custom", component.ServiceB.Tag);

Other non-property parameters are also considered; (almost) any available parameter can provide a property value.

 var builder = new ContainerBuilder();
builder.RegisterType<ServiceA>();
builder.RegisterType<Component>();

var container = builder.Build();

var component = container.Resolve<Component>(new TypedParameter(typeof(ServiceB), new ServiceB()));

Assert.NotNull(component.ServiceB);

The almost above refers to the existing NamedParameter and PositionalParameter, which cannot provide required property values. The reason for this is to do with clashing with constructors. NamedParameter could have a legitimate constructor argument value, which is also the name of the parameter in every property setter. Likewise with PositionalParameter, every property's parameter index is 0. These parameter types are ignored when checking required properties.

Mixing Constructors and Properties

Constructors and required properties can be happily mixed together:

private class MixedConstructorAndPropertyComponent
{
    public MixedConstructorAndPropertyComponent(ServiceA serviceA)
    {
        ServiceA = serviceA;
    }

    public ServiceA ServiceA { get; set; }

    required public ServiceB ServiceB { get; set; }
}

Only ServiceB above will be automatically injected as a required property.

However, we do not check before setting a required property that it hasn't already been set, or is not null. So if a constructor populates a required property from a provided service, it will double-resolve. This is extremely hard to avoid in my mind, because it's nigh-on impossible to know whether the value already assigned to a property is some form of default, or a value set by a constructor. So, for safety, we always set required properties.

Inheritance Hierarchy

We look for the RequiredMembersAttribute all the way up the inheritance hierarchy, and all required properties in that hierarchy will be set.

That is, unless the derived class' constructor has SetsRequiredMembers applied, in which case we don't set any properties in any of the parent types.

Performance Impact

Required properties are slower than regular constructors, but not by a huge amount. Given these two components:

public class ConstructorComponent
{
    public ConstructorComponent(ServiceA serviceA, ServiceB serviceB)
    {
        ServiceA = serviceA;
        ServiceB = serviceB;
    }

    public ServiceA ServiceA { get; }

    public ServiceB ServiceB { get; }
}

public class RequiredPropertyComponent
{
    required public ServiceA ServiceA { get; set; }

    required public ServiceA ServiceB { get; set; }
}

Here is the benchmark added (RequiredPropertyBenchmark):

|             Method |     Mean |     Error |    StdDev | Ratio |   Gen0 | Allocated | Alloc Ratio |
|------------------- |---------:|----------:|----------:|------:|-------:|----------:|------------:|
|  NormalConstructor | 1.049 us | 0.0036 us | 0.0030 us |  1.00 | 0.4234 |   1.73 KB |        1.00 |
| RequiredProperties | 1.181 us | 0.0044 us | 0.0037 us |  1.13 | 0.4044 |   1.66 KB |        0.95 |

It actually uses less memory for required properties (slightly), because of the short-circuited simple constructor activation.

Breaking Changes

While technically there are no breaking signature changes, any user code that had required properties before will start to throw exceptions with this version if those properties aren't resolvable.

For that reason, I'd suggest this is a major incrementing change.

Closes #1355

[tillig]

I'll try to review this soon. Something that occurs to me - if it's a major semver increment (which I agree with), are there other breaks or things to mark [Obsolete] in the major version? Things currently marked obsolete that are fine to remove? (I'm on my phone so not browsing code...)

[alistairjevans]

Only one I know about off the top of my head is the RegisterGenericDecorator one, which is #1297

[codecov]

Codecov Report

Base: 78.02% // Head: 78.22% // Increases project coverage by +0.20% 🎉

Coverage data is based on head (a75652f) compared to base (0faf5c8).
Patch coverage: 94.11% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1364      +/-   ##
===========================================
+ Coverage    78.02%   78.22%   +0.20%     
===========================================
  Files          195      197       +2     
  Lines         5596     5653      +57     
  Branches      1120     1147      +27     
===========================================
+ Hits          4366     4422      +56     
  Misses         716      716              
- Partials       514      515       +1     

Impacted Files	Coverage Δ
...ivators/Reflection/NoConstructorsFoundException.cs	36.84% <37.50%> (-6.02%)	⬇️
.../Core/Activators/Reflection/ReflectionActivator.cs	93.65% <98.36%> (+3.71%)	⬆️
...fac/Core/Activators/Reflection/BoundConstructor.cs	81.57% <100.00%> (+0.49%)	⬆️
...ac/Core/Activators/Reflection/ConstructorBinder.cs	92.18% <100.00%> (+0.25%)	⬆️
.../Activators/Reflection/DefaultConstructorFinder.cs	87.50% <100.00%> (-3.41%)	⬇️
...c/Core/Activators/Reflection/InjectableProperty.cs	100.00% <100.00%> (ø)
...e/Activators/Reflection/InjectablePropertyState.cs	100.00% <100.00%> (ø)
...Generics/OpenGenericDecoratorRegistrationSource.cs	63.04% <100.00%> (+0.82%)	⬆️
...nGenerics/OpenGenericDelegateRegistrationSource.cs	59.25% <100.00%> (+3.25%)	⬆️
...ures/OpenGenerics/OpenGenericRegistrationSource.cs	60.60% <100.00%> (+2.54%)	⬆️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[TonyValenti]

If a required property is a nullable reference type, will this allow the value to be resolved to null?

[TonyValenti]

(If it does not currently, I believe that it should)

[alistairjevans]

I think (am away from keyboard) that if a parameter is explicitly provided to set a property to null, it will accept it and initialise that property to null.

But otherwise, Autofac doesn't have the concept of activating "null".

It also feels weird to have the idea of "optional required" in the sense that having a nullable required property means the resolution is optional?

It's either required or it isn't, surely?

[TonyValenti]

Hi @alistairjevans -
When Required Properties were being invented, there was actually a lot of discussion on this very topic. It spanned multiple discussions, but here is one of them for reference:
dotnet/csharplang#4209

The decision the LDMs made was that required and NRT are orthogonal features and that required does not imply non-nullable.

It was hotly debated at first, but eventually many folks came to appreciate that a required NRT could essentially represent a "suggested" member or a member that, if null, should be clearly stated as such.

The following is valid code:

public class foo {
   public required object? Parent {get; set;}
}

And it can be used like this:

var A = new foo(){
    Parent = new object(),
};

var B = new foo(){
    Parent = null,
};

but not like this:

var C = new foo(){
    // ERROR!
};

I could be mistaken, but I am pretty certain that when AutoFac does constructor injection, it will provide null if a constructor argument is an unresolvable NRT. This is similar to what should happen if a NRT required property cannot be resolved.

[alistairjevans]

We will only provide allow a null value in the constructor (regardless of NRT), if an explicit default value is provided, i.e. IService? service1 = null.

[alistairjevans]

Thinking on this some more, I'm not opposed to NRTs being considered optional dependencies, but the same rules would need to be applied to both constructor parameters and property types to be consistent I think.

[tillig]

I think changing Autofac to allow "I explicitly initialized this to null" would break some things in insidious ways. I can't point to code right now (I'm on my phone) but I know we've always considered it a problem if a component registration returns null (eg, delegate activation returns null, provided parameter returns null).

[alistairjevans]

We cannot/will not initialise something to null; the real question here is what a nullable reference type can mean.

I think we can safely separate the issue of required properties from that of nullable reference types. Allowing IService? service as a constructor argument or a property to mean ResolveOptional() is sort of a distinct debate.

I'd suggest we raise a separate issue for discussion of "should Autofac take a nullable service to mean an optional one?", and discuss over there/seek opinions.

[tillig]

I'll start the discussion issue because I've been thinking a lot about this. I need to run a couple of tests, but my gut says no: Autofac has more restrictions than the compiler because it isn't the compiler, it's an IoC container, and if a property is optional then it shouldn't be marked required. However, let me write this up in more detail and I'll link to it here. (If I see someone respond that they agree or disagree with my statement above, I'm totally calling you out on "you didn't read that responses should be to the new discussion issue.")

[tillig]

I added a discussion issue for talking about NRT vs. optional/required values.

[tillig]

Just a couple of questions for clarification on some stuff. I don't think there's really anything to change, it looks really good. I am a little concerned over the new allocations since folks are very concerned about allocations these days. It's probably pretty trivial, all things considered.

[alistairjevans]

RE: allocations, there should not be any additional allocations for users that aren't using property injection or required members.

For people using property injection, we allocate a bit more upfront by capturing the injectable properties at the beginning and holding them in the activator, but we avoid re-allocating/fetching the GetRuntimeProperties().ToList() output every time now.

[tillig]

🍢