From 4db52ea50cabd68ce5b3e26169dca74c3514071b Mon Sep 17 00:00:00 2001 From: Alistair Evans Date: Wed, 25 Jan 2023 09:34:12 +0000 Subject: [PATCH 1/9] In-progress, initial required property support in place. --- .../Activators/Reflection/BoundConstructor.cs | 6 + .../Reflection/ConstructorBinder.cs | 10 ++ .../Reflection/ReflectionActivator.cs | 112 ++++++++++++++++-- .../ReflectionActivatorResources.resx | 59 ++++----- src/Autofac/Core/RequiredPropertyParameter.cs | 39 ++++++ .../Features/RequiredPropertyTests.cs | 58 +++++++++ 6 files changed, 248 insertions(+), 36 deletions(-) create mode 100644 src/Autofac/Core/RequiredPropertyParameter.cs create mode 100644 test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs diff --git a/src/Autofac/Core/Activators/Reflection/BoundConstructor.cs b/src/Autofac/Core/Activators/Reflection/BoundConstructor.cs index 3488b2e12..ad323b592 100644 --- a/src/Autofac/Core/Activators/Reflection/BoundConstructor.cs +++ b/src/Autofac/Core/Activators/Reflection/BoundConstructor.cs @@ -71,6 +71,12 @@ internal BoundConstructor(ConstructorBinder binder, ParameterInfo firstNonBindab /// public ConstructorBinder Binder { get; } + /// + /// Gets a value indicating whether the constructor has the SetsRequiredMembers attribute, + /// indicating we can skip population of required properties. + /// + public bool SetsRequiredMembers => Binder.SetsRequiredMembers; + /// /// Gets the constructor on the target type. The actual constructor used /// might differ, e.g. if using a dynamic proxy. diff --git a/src/Autofac/Core/Activators/Reflection/ConstructorBinder.cs b/src/Autofac/Core/Activators/Reflection/ConstructorBinder.cs index 08d07c4d9..bb70bb5b3 100644 --- a/src/Autofac/Core/Activators/Reflection/ConstructorBinder.cs +++ b/src/Autofac/Core/Activators/Reflection/ConstructorBinder.cs @@ -26,6 +26,10 @@ public ConstructorBinder(ConstructorInfo constructorInfo) Constructor = constructorInfo ?? throw new ArgumentNullException(nameof(constructorInfo)); _constructorArgs = constructorInfo.GetParameters(); +#if NET7_0_OR_GREATER + SetsRequiredMembers = constructorInfo.GetCustomAttribute() is not null; +#endif + // If any of the parameters are unsafe, do not create an invoker, and store the parameter // that broke the rule. _illegalParameter = DetectIllegalParameter(_constructorArgs); @@ -44,6 +48,12 @@ public ConstructorBinder(ConstructorInfo constructorInfo) /// public ConstructorInfo Constructor { get; } + /// + /// Gets a value indicating whether the constructor has the SetsRequiredMembers attribute, + /// indicating we can skip population of required properties. + /// + public bool SetsRequiredMembers { get; } + /// /// Gets the set of parameters to bind against. /// diff --git a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs index 0561d060b..526fc9f45 100644 --- a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs +++ b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs @@ -4,6 +4,7 @@ using System.Globalization; using System.Reflection; using System.Runtime.CompilerServices; +using System.Runtime.Serialization; using System.Text; using Autofac.Core.Resolving; using Autofac.Core.Resolving.Pipeline; @@ -21,6 +22,7 @@ public class ReflectionActivator : InstanceActivator, IInstanceActivator private readonly Parameter[] _defaultParameters; private ConstructorBinder[]? _constructorBinders; + private IReadOnlyList? _requiredProperties; /// /// Initializes a new instance of the class. @@ -93,10 +95,17 @@ public void ConfigurePipeline(IComponentRegistryServices componentRegistryServic binders[idx] = new ConstructorBinder(availableConstructors[idx]); } + var skipRequiredPropertyPopulation = false; + var singleConstructorSelected = false; + if (binders.Length == 1) { - UseSingleConstructorActivation(pipelineBuilder, binders[0]); - return; + var singleConstructor = binders[0]; + + UseSingleConstructorActivation(pipelineBuilder, singleConstructor); + + skipRequiredPropertyPopulation = singleConstructor.SetsRequiredMembers; + singleConstructorSelected = true; } else if (ConstructorSelector is IConstructorSelectorWithEarlyBinding earlyBindingSelector) { @@ -106,9 +115,40 @@ public void ConfigurePipeline(IComponentRegistryServices componentRegistryServic { UseSingleConstructorActivation(pipelineBuilder, matchedConstructor); - return; + skipRequiredPropertyPopulation = matchedConstructor.SetsRequiredMembers; + singleConstructorSelected = true; + } + } + +#if NET7_0_OR_GREATER + if (!skipRequiredPropertyPopulation) + { + // Note that the RequiredMemberAttribute is only applied to types that directly + // define required members, so we must check parent types as well to determine if we need to do any of + // the 'required' behaviour. + if (_implementationType.GetCustomAttribute(inherit: true) is not null) + { + var runtimeProps = _implementationType.GetRuntimeProperties(); + var requiredPropParameterSet = new List(); + + foreach (var prop in runtimeProps) + { + // By virtue of the rules for required members, if a property has the RequiredMember + // attribute, it must be writeable and non static. + if (prop.GetCustomAttribute() is not null) + { + requiredPropParameterSet.Add(new RequiredPropertyParameter(prop)); + } + } + + _requiredProperties = requiredPropParameterSet; } } +#endif + if (singleConstructorSelected) + { + return; + } _constructorBinders = binders; @@ -147,7 +187,7 @@ private void UseSingleConstructorActivation(IResolvePipelineBuilder pipelineBuil var instance = boundConstructor.Instantiate(); - InjectProperties(instance, ctxt); + InjectProperties(instance, ctxt, skipRequiredMembers: boundConstructor.SetsRequiredMembers); ctxt.Instance = instance; @@ -171,7 +211,7 @@ private void UseSingleConstructorActivation(IResolvePipelineBuilder pipelineBuil var instance = bound.Instantiate(); - InjectProperties(instance, ctxt); + InjectProperties(instance, ctxt, skipRequiredMembers: bound.SetsRequiredMembers); ctxt.Instance = instance; @@ -218,7 +258,7 @@ private object ActivateInstance(IComponentContext context, IEnumerable? failingRequiredProperties = null; + + foreach (var requiredProperty in _requiredProperties) + { + // We use the remaining properties in 'actualProperties' here, + // so any explicitly configured property parameters override the 'automatic' ones. + foreach (var actualProperty in actualProperties) + { + if (actualProperty != requiredProperty.Property) + { + continue; + } + + if (requiredProperty.CanSupplyValue(requiredProperty.Parameter, context, out var vp)) + { + actualProperties.Remove(actualProperty); + actualProperty.SetValue(instance, vp(), null); + } + else + { + failingRequiredProperties ??= new(); + failingRequiredProperties.Add(requiredProperty); + } + + break; + } + } + + if (failingRequiredProperties is not null) + { + throw new DependencyResolutionException(BuildRequiredPropertyResolutionMessage(failingRequiredProperties)); + } + } + + private string BuildRequiredPropertyResolutionMessage(IReadOnlyList failingRequiredProperties) + { + var propertyDescriptions = new StringBuilder(); + + foreach (var failed in failingRequiredProperties) + { + propertyDescriptions.AppendLine(); + propertyDescriptions.Append($"{failed.Property.Name} ({failed.Property.PropertyType.Name})"); + } + + return string.Format( + CultureInfo.CurrentCulture, + ReflectionActivatorResources.RequiredPropertiesCouldNotBeBound, + _implementationType, + propertyDescriptions); } } diff --git a/src/Autofac/Core/Activators/Reflection/ReflectionActivatorResources.resx b/src/Autofac/Core/Activators/Reflection/ReflectionActivatorResources.resx index 947acb4d6..81ffa790c 100644 --- a/src/Autofac/Core/Activators/Reflection/ReflectionActivatorResources.resx +++ b/src/Autofac/Core/Activators/Reflection/ReflectionActivatorResources.resx @@ -1,17 +1,17 @@  - @@ -128,4 +128,7 @@ See https://autofac.rtfd.io/help/no-constructors-bindable for more info. - + + One or more of the required properties on type '{0}' could not be resolved:{1} + + \ No newline at end of file diff --git a/src/Autofac/Core/RequiredPropertyParameter.cs b/src/Autofac/Core/RequiredPropertyParameter.cs new file mode 100644 index 000000000..d31099429 --- /dev/null +++ b/src/Autofac/Core/RequiredPropertyParameter.cs @@ -0,0 +1,39 @@ +// Copyright (c) Autofac Project. All rights reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. + +using System.Reflection; +using Autofac.Core.Activators.Reflection; +using Autofac.Util; + +namespace Autofac.Core; + +/// +/// A required property by its property info. +/// +/// Internal because we also use this property to cache setter methods and parameter info. +internal class RequiredPropertyParameter : AutowiringParameter +{ + /// + /// Initializes a new instance of the class. + /// + /// The property info of the property being set. + public RequiredPropertyParameter(PropertyInfo property) + { + Property = property; + + if (property.SetMethod is null) + { + throw new ArgumentException("Property does not have a setter", nameof(property)); + } + + Setter = property.SetMethod; + + Parameter = Setter.GetParameters()[0]; + } + + public PropertyInfo Property { get; } + + public MethodInfo Setter { get; } + + public ParameterInfo Parameter { get; } +} diff --git a/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs new file mode 100644 index 000000000..66f620b46 --- /dev/null +++ b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs @@ -0,0 +1,58 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace Autofac.Specification.Test.Features; + +#if NET7_0_OR_GREATER + +public class RequiredPropertyTests +{ + [Fact] + public void ResolveRequiredProperties() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType(); + builder.RegisterType(); + + var container = builder.Build(); + + var component = container.Resolve(); + + Assert.NotNull(component.ServiceA); + Assert.NotNull(component.ServiceB); + } + + private class Component + { + required public ServiceA ServiceA { get; set; } + + required public ServiceB ServiceB { get; set; } + } + + private class ServiceA + { + public ServiceA() + { + Tag = "Default"; + } + + public string Tag { get; } + } + + private class ServiceB + { + public ServiceB() + { + Tag = "Default"; + } + + public string Tag { get; } + } + +} + +#endif From 7c4d778d804babacc3a74b6125401663d4ae9135 Mon Sep 17 00:00:00 2001 From: Alistair Evans Date: Thu, 26 Jan 2023 09:23:12 +0000 Subject: [PATCH 2/9] Take a new approach to resolving required properties that allows the full set of parameters provided for the constructor, so required properties behave a lot more like any other constructor argument. Also, cache the set of properties to write to, so we don't have to call GetRuntimeProperties() every time. --- .../Reflection/ReflectionActivator.cs | 219 +++++++++++------- src/Autofac/Core/RequiredPropertyParameter.cs | 39 ---- .../Features/RequiredPropertyTests.cs | 62 ++++- 3 files changed, 197 insertions(+), 123 deletions(-) delete mode 100644 src/Autofac/Core/RequiredPropertyParameter.cs diff --git a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs index 526fc9f45..a46303efd 100644 --- a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs +++ b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs @@ -8,6 +8,7 @@ using System.Text; using Autofac.Core.Resolving; using Autofac.Core.Resolving.Pipeline; +using Autofac.Util.Cache; namespace Autofac.Core.Activators.Reflection; @@ -22,7 +23,8 @@ public class ReflectionActivator : InstanceActivator, IInstanceActivator private readonly Parameter[] _defaultParameters; private ConstructorBinder[]? _constructorBinders; - private IReadOnlyList? _requiredProperties; + private bool _anyRequiredMembers; + private ResolvedPropertyInfoState[]? _defaultFoundPropertySet; /// /// Initializes a new instance of the class. @@ -80,6 +82,26 @@ public void ConfigurePipeline(IComponentRegistryServices componentRegistryServic throw new ArgumentNullException(nameof(pipelineBuilder)); } +#if NET7_0_OR_GREATER + _anyRequiredMembers = _implementationType.GetCustomAttribute(inherit: true) is not null; +#endif + + if (_anyRequiredMembers || _configuredProperties.Length > 0) + { + // Get the full set of properties. + var actualProperties = _implementationType + .GetRuntimeProperties() + .Where(pi => pi.CanWrite) + .ToList(); + + _defaultFoundPropertySet = new ResolvedPropertyInfoState[actualProperties.Count]; + + for (int idx = 0; idx < actualProperties.Count; idx++) + { + _defaultFoundPropertySet[idx] = new ResolvedPropertyInfoState(new FoundProperty(actualProperties[idx])); + } + } + // Locate the possible constructors at container build time. var availableConstructors = ConstructorFinder.FindConstructors(_implementationType); @@ -95,17 +117,13 @@ public void ConfigurePipeline(IComponentRegistryServices componentRegistryServic binders[idx] = new ConstructorBinder(availableConstructors[idx]); } - var skipRequiredPropertyPopulation = false; - var singleConstructorSelected = false; - if (binders.Length == 1) { var singleConstructor = binders[0]; UseSingleConstructorActivation(pipelineBuilder, singleConstructor); - skipRequiredPropertyPopulation = singleConstructor.SetsRequiredMembers; - singleConstructorSelected = true; + return; } else if (ConstructorSelector is IConstructorSelectorWithEarlyBinding earlyBindingSelector) { @@ -115,40 +133,9 @@ public void ConfigurePipeline(IComponentRegistryServices componentRegistryServic { UseSingleConstructorActivation(pipelineBuilder, matchedConstructor); - skipRequiredPropertyPopulation = matchedConstructor.SetsRequiredMembers; - singleConstructorSelected = true; - } - } - -#if NET7_0_OR_GREATER - if (!skipRequiredPropertyPopulation) - { - // Note that the RequiredMemberAttribute is only applied to types that directly - // define required members, so we must check parent types as well to determine if we need to do any of - // the 'required' behaviour. - if (_implementationType.GetCustomAttribute(inherit: true) is not null) - { - var runtimeProps = _implementationType.GetRuntimeProperties(); - var requiredPropParameterSet = new List(); - - foreach (var prop in runtimeProps) - { - // By virtue of the rules for required members, if a property has the RequiredMember - // attribute, it must be writeable and non static. - if (prop.GetCustomAttribute() is not null) - { - requiredPropParameterSet.Add(new RequiredPropertyParameter(prop)); - } - } - - _requiredProperties = requiredPropParameterSet; + return; } } -#endif - if (singleConstructorSelected) - { - return; - } _constructorBinders = binders; @@ -187,7 +174,7 @@ private void UseSingleConstructorActivation(IResolvePipelineBuilder pipelineBuil var instance = boundConstructor.Instantiate(); - InjectProperties(instance, ctxt, skipRequiredMembers: boundConstructor.SetsRequiredMembers); + InjectProperties(instance, ctxt, boundConstructor, GetAllParameters(ctxt.Parameters)); ctxt.Instance = instance; @@ -211,7 +198,7 @@ private void UseSingleConstructorActivation(IResolvePipelineBuilder pipelineBuil var instance = bound.Instantiate(); - InjectProperties(instance, ctxt, skipRequiredMembers: bound.SetsRequiredMembers); + InjectProperties(instance, ctxt, bound, prioritisedParameters); ctxt.Instance = instance; @@ -244,7 +231,9 @@ private object ActivateInstance(IComponentContext context, IEnumerable parameters) + private BoundConstructor[] GetAllBindings(ConstructorBinder[] availableConstructors, IComponentContext context, IEnumerable allParameters) { - var prioritisedParameters = GetAllParameters(parameters); - var boundConstructors = new BoundConstructor[availableConstructors.Length]; var validBindings = availableConstructors.Length; for (var idx = 0; idx < availableConstructors.Length; idx++) { - var bound = availableConstructors[idx].Bind(prioritisedParameters, context); + var bound = availableConstructors[idx].Bind(allParameters, context); boundConstructors[idx] = bound; @@ -350,76 +337,96 @@ private string GetBindingFailureMessage(BoundConstructor[] constructorBindings) } } - private void InjectProperties(object instance, IComponentContext context, bool skipRequiredMembers) + private void InjectProperties(object instance, IComponentContext context, BoundConstructor constructor, IEnumerable allParameters) { - if (_configuredProperties.Length == 0 && (_requiredProperties is null || skipRequiredMembers)) + // We only need to do any injection if we have a set of property information. + if (_defaultFoundPropertySet is null) + { + return; + } + + // If this constructor sets all required members, + // and we have no configured properties, we can just jump out. + if (_configuredProperties.Length == 0 && constructor.SetsRequiredMembers) { return; } - var actualProperties = instance - .GetType() - .GetRuntimeProperties() - .Where(pi => pi.CanWrite) - .ToList(); + var workingArray = (ResolvedPropertyInfoState[])_defaultFoundPropertySet.Clone(); foreach (var configuredProperty in _configuredProperties) { - foreach (var actualProperty in actualProperties) + for (var propIdx = 0; propIdx < workingArray.Length; propIdx++) { - var setter = actualProperty.SetMethod; + ref var prop = ref workingArray[propIdx]; - if (setter != null && - configuredProperty.CanSupplyValue(setter.GetParameters()[0], context, out var vp)) + if (prop.Set) { - actualProperties.Remove(actualProperty); - actualProperty.SetValue(instance, vp(), null); + // Skip, already seen. + continue; + } + if (prop.Property.TrySupply(instance, configuredProperty, context)) + { + prop.Set = true; break; } } } - if (_requiredProperties is null) + if (_anyRequiredMembers && !constructor.SetsRequiredMembers) { - return; - } - - List? failingRequiredProperties = null; + List? failingRequiredProperties = null; - foreach (var requiredProperty in _requiredProperties) - { - // We use the remaining properties in 'actualProperties' here, - // so any explicitly configured property parameters override the 'automatic' ones. - foreach (var actualProperty in actualProperties) + for (var propIdx = 0; propIdx < workingArray.Length; propIdx++) { - if (actualProperty != requiredProperty.Property) + ref var prop = ref workingArray[propIdx]; + + if (!prop.Property.IsRequired) { + // Only auto-populate required properties. continue; } - if (requiredProperty.CanSupplyValue(requiredProperty.Parameter, context, out var vp)) + if (prop.Set) { - actualProperties.Remove(actualProperty); - actualProperty.SetValue(instance, vp(), null); + // Only auto-populate things not already populated by a specific property + // being set. + continue; } - else + + foreach (var parameter in allParameters) { - failingRequiredProperties ??= new(); - failingRequiredProperties.Add(requiredProperty); + if (parameter is NamedParameter || parameter is PositionalParameter) + { + // Skip Named and Positional parameters, because if someone uses 'value' as a + // constructor parameter name, it would also match the property, and cause confusion. + continue; + } + + if (prop.Property.TrySupply(instance, parameter, context)) + { + prop.Set = true; + + break; + } } - break; + if (!prop.Set) + { + failingRequiredProperties ??= new(); + failingRequiredProperties.Add(prop.Property); + } } - } - if (failingRequiredProperties is not null) - { - throw new DependencyResolutionException(BuildRequiredPropertyResolutionMessage(failingRequiredProperties)); + if (failingRequiredProperties is not null) + { + throw new DependencyResolutionException(BuildRequiredPropertyResolutionMessage(failingRequiredProperties)); + } } } - private string BuildRequiredPropertyResolutionMessage(IReadOnlyList failingRequiredProperties) + private string BuildRequiredPropertyResolutionMessage(IReadOnlyList failingRequiredProperties) { var propertyDescriptions = new StringBuilder(); @@ -435,4 +442,52 @@ private string BuildRequiredPropertyResolutionMessage(IReadOnlyList() is not null; +#endif + } + + public PropertyInfo Property { get; } + + public bool IsRequired { get; } + + public bool TrySupply(object instance, Parameter p, IComponentContext ctxt) + { + if (p.CanSupplyValue(_setterParameter, ctxt, out var vp)) + { + Property.SetValue(instance, vp(), null); + + return true; + } + + return false; + } + } + + private struct ResolvedPropertyInfoState + { + public ResolvedPropertyInfoState(FoundProperty property) + { + Property = property; + Set = false; + } + + public FoundProperty Property { get; } + + public bool Set { get; set; } + } } diff --git a/src/Autofac/Core/RequiredPropertyParameter.cs b/src/Autofac/Core/RequiredPropertyParameter.cs deleted file mode 100644 index d31099429..000000000 --- a/src/Autofac/Core/RequiredPropertyParameter.cs +++ /dev/null @@ -1,39 +0,0 @@ -// Copyright (c) Autofac Project. All rights reserved. -// Licensed under the MIT License. See LICENSE in the project root for license information. - -using System.Reflection; -using Autofac.Core.Activators.Reflection; -using Autofac.Util; - -namespace Autofac.Core; - -/// -/// A required property by its property info. -/// -/// Internal because we also use this property to cache setter methods and parameter info. -internal class RequiredPropertyParameter : AutowiringParameter -{ - /// - /// Initializes a new instance of the class. - /// - /// The property info of the property being set. - public RequiredPropertyParameter(PropertyInfo property) - { - Property = property; - - if (property.SetMethod is null) - { - throw new ArgumentException("Property does not have a setter", nameof(property)); - } - - Setter = property.SetMethod; - - Parameter = Setter.GetParameters()[0]; - } - - public PropertyInfo Property { get; } - - public MethodInfo Setter { get; } - - public ParameterInfo Parameter { get; } -} diff --git a/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs index 66f620b46..c3ecbcfc8 100644 --- a/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs +++ b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs @@ -3,6 +3,7 @@ using System.Linq; using System.Text; using System.Threading.Tasks; +using Autofac.Core; namespace Autofac.Specification.Test.Features; @@ -26,6 +27,63 @@ public void ResolveRequiredProperties() Assert.NotNull(component.ServiceB); } + [Fact] + public void MissingRequiredPropertyServiceThrowsException() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType(); + + var container = builder.Build(); + + var exception = Assert.Throws(() => container.Resolve()); + + Assert.Contains(nameof(Component.ServiceB), exception.InnerException.Message); + } + + [Fact] + public void ExplicitParameterOverridesRequiredAutowiring() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType(); + builder.RegisterType().WithProperty(nameof(Component.ServiceB), new ServiceB { Tag = "custom" }); + + var container = builder.Build(); + + var component = container.Resolve(); + + Assert.Equal("custom", component.ServiceB.Tag); + } + + [Fact] + public void ExplicitParameterCanTakePlaceOfRegistration() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType().WithProperty(nameof(Component.ServiceB), new ServiceB { Tag = "custom" }); + + var container = builder.Build(); + + var component = container.Resolve(); + + Assert.Equal("custom", component.ServiceB.Tag); + } + + [Fact] + public void GeneralTypePropertyParameterCanTakePlaceOfRegistration() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType().WithParameter(new TypedParameter(typeof(ServiceB), new ServiceB())); + + var container = builder.Build(); + + var component = container.Resolve(); + + Assert.NotNull(component.ServiceB); + } + private class Component { required public ServiceA ServiceA { get; set; } @@ -40,7 +98,7 @@ public ServiceA() Tag = "Default"; } - public string Tag { get; } + public string Tag { get; set; } } private class ServiceB @@ -50,7 +108,7 @@ public ServiceB() Tag = "Default"; } - public string Tag { get; } + public string Tag { get; set; } } } From 6f7de4988bc7f522f35c3633033f0914f6250bb6 Mon Sep 17 00:00:00 2001 From: Alistair Evans Date: Sat, 28 Jan 2023 09:48:26 +0000 Subject: [PATCH 3/9] Add benchmark and tests for required properties. --- .../Autofac.BenchmarkProfiling.csproj | 2 +- .../Autofac.Benchmarks.csproj | 2 +- bench/Autofac.Benchmarks/Benchmarks.cs | 47 +++---- ...ncyBenchmak.cs => ConcurrencyBenchmark.cs} | 0 .../RequiredPropertyBenchmark.cs | 61 +++++++++ .../Reflection/InjectableProperty.cs | 69 ++++++++++ .../Reflection/InjectablePropertyState.cs | 30 +++++ .../Reflection/ReflectionActivator.cs | 80 +++-------- .../Features/RequiredPropertyTests.cs | 127 +++++++++++++++++- 9 files changed, 326 insertions(+), 92 deletions(-) rename bench/Autofac.Benchmarks/{ConcurrencyBenchmak.cs => ConcurrencyBenchmark.cs} (100%) create mode 100644 bench/Autofac.Benchmarks/RequiredPropertyBenchmark.cs create mode 100644 src/Autofac/Core/Activators/Reflection/InjectableProperty.cs create mode 100644 src/Autofac/Core/Activators/Reflection/InjectablePropertyState.cs diff --git a/bench/Autofac.BenchmarkProfiling/Autofac.BenchmarkProfiling.csproj b/bench/Autofac.BenchmarkProfiling/Autofac.BenchmarkProfiling.csproj index 248ba87d6..165a5eac8 100644 --- a/bench/Autofac.BenchmarkProfiling/Autofac.BenchmarkProfiling.csproj +++ b/bench/Autofac.BenchmarkProfiling/Autofac.BenchmarkProfiling.csproj @@ -2,7 +2,7 @@ Exe - net6.0 + net7.0 x64 enable diff --git a/bench/Autofac.Benchmarks/Autofac.Benchmarks.csproj b/bench/Autofac.Benchmarks/Autofac.Benchmarks.csproj index eb53fe099..a3de60a98 100644 --- a/bench/Autofac.Benchmarks/Autofac.Benchmarks.csproj +++ b/bench/Autofac.Benchmarks/Autofac.Benchmarks.csproj @@ -1,7 +1,7 @@  - net6.0 + net7.0 $(NoWarn);CS1591 Exe false diff --git a/bench/Autofac.Benchmarks/Benchmarks.cs b/bench/Autofac.Benchmarks/Benchmarks.cs index c4bd6f04c..93f20013a 100644 --- a/bench/Autofac.Benchmarks/Benchmarks.cs +++ b/bench/Autofac.Benchmarks/Benchmarks.cs @@ -6,27 +6,28 @@ public static class Benchmarks { public static readonly Type[] All = { - typeof(ChildScopeResolveBenchmark), - typeof(ConcurrencyBenchmark), - typeof(ConcurrencyNestedScopeBenchmark), - typeof(KeyedGenericBenchmark), - typeof(KeyedNestedBenchmark), - typeof(KeyedSimpleBenchmark), - typeof(KeylessGenericBenchmark), - typeof(KeylessNestedBenchmark), - typeof(KeylessNestedSharedInstanceBenchmark), - typeof(KeylessNestedLambdaBenchmark), - typeof(KeylessNestedSharedInstanceLambdaBenchmark), - typeof(KeylessSimpleBenchmark), - typeof(KeylessSimpleSharedInstanceBenchmark), - typeof(KeylessSimpleLambdaBenchmark), - typeof(KeylessSimpleSharedInstanceLambdaBenchmark), - typeof(DeepGraphResolveBenchmark), - typeof(EnumerableResolveBenchmark), - typeof(PropertyInjectionBenchmark), - typeof(RootContainerResolveBenchmark), - typeof(OpenGenericBenchmark), - typeof(MultiConstructorBenchmark), - typeof(LambdaResolveBenchmark), - }; + typeof(ChildScopeResolveBenchmark), + typeof(ConcurrencyBenchmark), + typeof(ConcurrencyNestedScopeBenchmark), + typeof(KeyedGenericBenchmark), + typeof(KeyedNestedBenchmark), + typeof(KeyedSimpleBenchmark), + typeof(KeylessGenericBenchmark), + typeof(KeylessNestedBenchmark), + typeof(KeylessNestedSharedInstanceBenchmark), + typeof(KeylessNestedLambdaBenchmark), + typeof(KeylessNestedSharedInstanceLambdaBenchmark), + typeof(KeylessSimpleBenchmark), + typeof(KeylessSimpleSharedInstanceBenchmark), + typeof(KeylessSimpleLambdaBenchmark), + typeof(KeylessSimpleSharedInstanceLambdaBenchmark), + typeof(DeepGraphResolveBenchmark), + typeof(EnumerableResolveBenchmark), + typeof(PropertyInjectionBenchmark), + typeof(RootContainerResolveBenchmark), + typeof(OpenGenericBenchmark), + typeof(MultiConstructorBenchmark), + typeof(LambdaResolveBenchmark), + typeof(RequiredPropertyBenchmark), + }; } diff --git a/bench/Autofac.Benchmarks/ConcurrencyBenchmak.cs b/bench/Autofac.Benchmarks/ConcurrencyBenchmark.cs similarity index 100% rename from bench/Autofac.Benchmarks/ConcurrencyBenchmak.cs rename to bench/Autofac.Benchmarks/ConcurrencyBenchmark.cs diff --git a/bench/Autofac.Benchmarks/RequiredPropertyBenchmark.cs b/bench/Autofac.Benchmarks/RequiredPropertyBenchmark.cs new file mode 100644 index 000000000..7b343afd2 --- /dev/null +++ b/bench/Autofac.Benchmarks/RequiredPropertyBenchmark.cs @@ -0,0 +1,61 @@ +using Autofac.Core; + +namespace Autofac.Benchmarks; + +public class RequiredPropertyBenchmark +{ + private IContainer _container; + + [GlobalSetup] + public void Setup() + { + var builder = new ContainerBuilder(); + + builder.RegisterType(); + builder.RegisterType(); + builder.RegisterType(); + builder.RegisterType(); + + _container = builder.Build(); + } + + [Benchmark(Baseline = true)] + public void NormalConstructor() + { + _container.Resolve(); + } + + [Benchmark] + public void RequiredProperties() + { + _container.Resolve(); + } + + public class ServiceA + { + } + + public class ServiceB + { + } + + 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; } + } +} diff --git a/src/Autofac/Core/Activators/Reflection/InjectableProperty.cs b/src/Autofac/Core/Activators/Reflection/InjectableProperty.cs new file mode 100644 index 000000000..729f31eb6 --- /dev/null +++ b/src/Autofac/Core/Activators/Reflection/InjectableProperty.cs @@ -0,0 +1,69 @@ +// Copyright (c) Autofac Project. All rights reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. + +using System.Globalization; +using System.Reflection; +using System.Runtime.CompilerServices; +using System.Runtime.Serialization; +using System.Text; +using Autofac.Core.Resolving; +using Autofac.Core.Resolving.Pipeline; +using Autofac.Util; +using Autofac.Util.Cache; + +namespace Autofac.Core.Activators.Reflection; + +/// +/// Holds an instance of a known property on a type instantiated by the . +/// +internal class InjectableProperty +{ + private readonly MethodInfo _setter; + private readonly ParameterInfo _setterParameter; + + /// + /// Initializes a new instance of the class. + /// + /// The property info. + public InjectableProperty(PropertyInfo prop) + { + Property = prop; + + _setter = prop.SetMethod!; + + _setterParameter = _setter.GetParameters()[0]; + +#if NET7_0_OR_GREATER + IsRequired = prop.GetCustomAttribute() is not null; +#endif + } + + /// + /// Gets the underlying property. + /// + public PropertyInfo Property { get; } + + /// + /// Gets a value indicating whether this field is marked as required. + /// + public bool IsRequired { get; } + + /// + /// Try and supply a value for this property using the given parameter. + /// + /// The object instance. + /// The parameter that may provide the value. + /// The component context. + /// True if the parameter could provide a value, and the property was set. False otherwise. + public bool TrySupplyValue(object instance, Parameter p, IComponentContext ctxt) + { + if (p.CanSupplyValue(_setterParameter, ctxt, out var vp)) + { + Property.SetValue(instance, vp(), null); + + return true; + } + + return false; + } +} diff --git a/src/Autofac/Core/Activators/Reflection/InjectablePropertyState.cs b/src/Autofac/Core/Activators/Reflection/InjectablePropertyState.cs new file mode 100644 index 000000000..d5d55b860 --- /dev/null +++ b/src/Autofac/Core/Activators/Reflection/InjectablePropertyState.cs @@ -0,0 +1,30 @@ +// Copyright (c) Autofac Project. All rights reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. + +namespace Autofac.Core.Activators.Reflection; + +/// +/// Structure used to track whether or not we have set a property during activation. +/// +internal struct InjectablePropertyState +{ + /// + /// Initializes a new instance of the struct. + /// + /// The property. + public InjectablePropertyState(InjectableProperty property) + { + Property = property; + Set = false; + } + + /// + /// Gets the property. + /// + public InjectableProperty Property { get; } + + /// + /// Gets or sets a value indicating whether this property has already been set. + /// + public bool Set { get; set; } +} diff --git a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs index a46303efd..184d3dabc 100644 --- a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs +++ b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs @@ -8,6 +8,7 @@ using System.Text; using Autofac.Core.Resolving; using Autofac.Core.Resolving.Pipeline; +using Autofac.Util; using Autofac.Util.Cache; namespace Autofac.Core.Activators.Reflection; @@ -24,7 +25,7 @@ public class ReflectionActivator : InstanceActivator, IInstanceActivator private ConstructorBinder[]? _constructorBinders; private bool _anyRequiredMembers; - private ResolvedPropertyInfoState[]? _defaultFoundPropertySet; + private InjectablePropertyState[]? _defaultFoundPropertySet; /// /// Initializes a new instance of the class. @@ -84,6 +85,8 @@ public void ConfigurePipeline(IComponentRegistryServices componentRegistryServic #if NET7_0_OR_GREATER _anyRequiredMembers = _implementationType.GetCustomAttribute(inherit: true) is not null; +#else + _anyRequiredMembers = false; #endif if (_anyRequiredMembers || _configuredProperties.Length > 0) @@ -94,11 +97,11 @@ public void ConfigurePipeline(IComponentRegistryServices componentRegistryServic .Where(pi => pi.CanWrite) .ToList(); - _defaultFoundPropertySet = new ResolvedPropertyInfoState[actualProperties.Count]; + _defaultFoundPropertySet = new InjectablePropertyState[actualProperties.Count]; for (int idx = 0; idx < actualProperties.Count; idx++) { - _defaultFoundPropertySet[idx] = new ResolvedPropertyInfoState(new FoundProperty(actualProperties[idx])); + _defaultFoundPropertySet[idx] = new InjectablePropertyState(new InjectableProperty(actualProperties[idx])); } } @@ -352,13 +355,13 @@ private void InjectProperties(object instance, IComponentContext context, BoundC return; } - var workingArray = (ResolvedPropertyInfoState[])_defaultFoundPropertySet.Clone(); + var workingSetOfProperties = (InjectablePropertyState[])_defaultFoundPropertySet.Clone(); foreach (var configuredProperty in _configuredProperties) { - for (var propIdx = 0; propIdx < workingArray.Length; propIdx++) + for (var propIdx = 0; propIdx < workingSetOfProperties.Length; propIdx++) { - ref var prop = ref workingArray[propIdx]; + ref var prop = ref workingSetOfProperties[propIdx]; if (prop.Set) { @@ -366,7 +369,7 @@ private void InjectProperties(object instance, IComponentContext context, BoundC continue; } - if (prop.Property.TrySupply(instance, configuredProperty, context)) + if (prop.Property.TrySupplyValue(instance, configuredProperty, context)) { prop.Set = true; break; @@ -376,11 +379,11 @@ private void InjectProperties(object instance, IComponentContext context, BoundC if (_anyRequiredMembers && !constructor.SetsRequiredMembers) { - List? failingRequiredProperties = null; + List? failingRequiredProperties = null; - for (var propIdx = 0; propIdx < workingArray.Length; propIdx++) + for (var propIdx = 0; propIdx < workingSetOfProperties.Length; propIdx++) { - ref var prop = ref workingArray[propIdx]; + ref var prop = ref workingSetOfProperties[propIdx]; if (!prop.Property.IsRequired) { @@ -404,7 +407,7 @@ private void InjectProperties(object instance, IComponentContext context, BoundC continue; } - if (prop.Property.TrySupply(instance, parameter, context)) + if (prop.Property.TrySupplyValue(instance, parameter, context)) { prop.Set = true; @@ -426,14 +429,17 @@ private void InjectProperties(object instance, IComponentContext context, BoundC } } - private string BuildRequiredPropertyResolutionMessage(IReadOnlyList failingRequiredProperties) + private string BuildRequiredPropertyResolutionMessage(IReadOnlyList failingRequiredProperties) { var propertyDescriptions = new StringBuilder(); foreach (var failed in failingRequiredProperties) { propertyDescriptions.AppendLine(); - propertyDescriptions.Append($"{failed.Property.Name} ({failed.Property.PropertyType.Name})"); + propertyDescriptions.Append(failed.Property.Name); + propertyDescriptions.Append(" ("); + propertyDescriptions.Append(failed.Property.PropertyType.Name); + propertyDescriptions.Append(')'); } return string.Format( @@ -442,52 +448,4 @@ private string BuildRequiredPropertyResolutionMessage(IReadOnlyList() is not null; -#endif - } - - public PropertyInfo Property { get; } - - public bool IsRequired { get; } - - public bool TrySupply(object instance, Parameter p, IComponentContext ctxt) - { - if (p.CanSupplyValue(_setterParameter, ctxt, out var vp)) - { - Property.SetValue(instance, vp(), null); - - return true; - } - - return false; - } - } - - private struct ResolvedPropertyInfoState - { - public ResolvedPropertyInfoState(FoundProperty property) - { - Property = property; - Set = false; - } - - public FoundProperty Property { get; } - - public bool Set { get; set; } - } } diff --git a/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs index c3ecbcfc8..700a72dde 100644 --- a/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs +++ b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs @@ -1,8 +1,7 @@ -using System; -using System.Collections.Generic; -using System.Linq; -using System.Text; -using System.Threading.Tasks; +// Copyright (c) Autofac Project. All rights reserved. +// Licensed under the MIT License. See LICENSE in the project root for license information. + +using System.Diagnostics.CodeAnalysis; using Autofac.Core; namespace Autofac.Specification.Test.Features; @@ -71,7 +70,7 @@ public void ExplicitParameterCanTakePlaceOfRegistration() } [Fact] - public void GeneralTypePropertyParameterCanTakePlaceOfRegistration() + public void GeneralTypeParameterCanTakePlaceOfRegistration() { var builder = new ContainerBuilder(); builder.RegisterType(); @@ -84,6 +83,119 @@ public void GeneralTypePropertyParameterCanTakePlaceOfRegistration() Assert.NotNull(component.ServiceB); } + [Fact] + public void ParameterPassedAtResolveUsedForRequiredProperty() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType(); + + var container = builder.Build(); + + var component = container.Resolve(new TypedParameter(typeof(ServiceB), new ServiceB())); + + Assert.NotNull(component.ServiceB); + } + + [Fact] + public void NamedParametersIgnoredForRequiredProperties() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType().WithParameter("value", new ServiceB()); + + var container = builder.Build(); + + Assert.Throws(() => container.Resolve()); + } + + [Fact] + public void PositionalParametersIgnoredForRequiredProperties() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType().WithParameter(new PositionalParameter(0, new ServiceB())); + + var container = builder.Build(); + + Assert.Throws(() => container.Resolve()); + } + + [Fact] + public void SetsRequiredMembersConstructorSkipsRequiredProperties() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + + var container = builder.Build(); + + var component = container.Resolve(); + + Assert.Null(component.ServiceA); + Assert.Null(component.ServiceB); + } + + [Fact] + public void SetsRequiredMembersConstructorSkipsRequiredPropertiesEvenWhenRegistered() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType(); + builder.RegisterType(); + + var container = builder.Build(); + + var component = container.Resolve(); + + Assert.Null(component.ServiceA); + Assert.Null(component.ServiceB); + } + + [Fact] + public void OnlySelectedConstructorConsideredForSetsRequiredProperties() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + + var container = builder.Build(); + + var component = container.Resolve(); + + // Allowed to not be set, because empty constructor was selected. + Assert.Null(component.ServiceA); + + using var scope = container.BeginLifetimeScope(b => b.RegisterType()); + + // Fails, because constructor with SetsRequiredProperties attribute is selected. + Assert.Throws(() => scope.Resolve()); + } + + private class ConstructorComponent + { + [SetsRequiredMembers] + public ConstructorComponent() + { + } + + required public ServiceA ServiceA { get; set; } + + required public ServiceB ServiceB { get; set; } + } + + private class MultiConstructorComponent + { + [SetsRequiredMembers] + public MultiConstructorComponent() + { + } + + public MultiConstructorComponent(ServiceC serviceC) + { + } + + required public ServiceA ServiceA { get; set; } + } + private class Component { required public ServiceA ServiceA { get; set; } @@ -111,6 +223,9 @@ public ServiceB() public string Tag { get; set; } } + private class ServiceC + { + } } #endif From 8a38698d1c2557108523c5cf76f226924a415417 Mon Sep 17 00:00:00 2001 From: Alistair Evans Date: Sat, 28 Jan 2023 10:08:07 +0000 Subject: [PATCH 4/9] Additional tests for component hierarchy, and open generics (just in case). --- .../Reflection/ReflectionActivator.cs | 14 +++- .../Features/RequiredPropertyTests.cs | 70 +++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs index 184d3dabc..259a3b184 100644 --- a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs +++ b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs @@ -84,7 +84,19 @@ public void ConfigurePipeline(IComponentRegistryServices componentRegistryServic } #if NET7_0_OR_GREATER - _anyRequiredMembers = _implementationType.GetCustomAttribute(inherit: true) is not null; + // The 'inherited' flag on GetCustomAttribute does not appear to work as expected with regards to RequiredMemberAttribute, + // so we must walk the inheritance hierarchy checking each one. + var currentType = _implementationType; + while (currentType is not null && !currentType.Equals(typeof(object))) + { + if (currentType.GetCustomAttribute(inherit: true) is not null) + { + _anyRequiredMembers = true; + break; + } + + currentType = currentType.BaseType; + } #else _anyRequiredMembers = false; #endif diff --git a/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs index 700a72dde..e542557bf 100644 --- a/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs +++ b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs @@ -170,6 +170,63 @@ public void OnlySelectedConstructorConsideredForSetsRequiredProperties() Assert.Throws(() => scope.Resolve()); } + [Fact] + public void DerivedClassResolvesPropertiesInParent() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType(); + builder.RegisterType(); + + var container = builder.Build(); + + var component = container.Resolve(); + + Assert.NotNull(component.ServiceA); + Assert.NotNull(component.ServiceB); + } + + [Fact] + public void DerivedClassResolvesPropertiesInParentAndSelf() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType(); + builder.RegisterType(); + builder.RegisterType(); + + var container = builder.Build(); + + var component = container.Resolve(); + + Assert.NotNull(component.ServiceA); + Assert.NotNull(component.ServiceB); + Assert.NotNull(component.ServiceC); + } + + [Fact] + public void CanResolveOpenGenericComponentRequiredProperties() + { + var builder = new ContainerBuilder(); + builder.RegisterGeneric(typeof(OpenGenericService<>)); + builder.RegisterGeneric(typeof(OpenGenericComponent<>)); + + var container = builder.Build(); + + var component = container.Resolve>(); + + Assert.NotNull(component.Service); + } + + private class OpenGenericComponent + { + required public OpenGenericService Service { get; set; } + } + + private class OpenGenericService + { + } + private class ConstructorComponent { [SetsRequiredMembers] @@ -203,6 +260,19 @@ private class Component required public ServiceB ServiceB { get; set; } } + private class DerivedComponentWithProp : Component + { + public DerivedComponentWithProp() + { + } + + required public ServiceC ServiceC { get; set; } + } + + private class DerivedComponent : Component + { + } + private class ServiceA { public ServiceA() From 22dc230ab2ee93fe72d871d175d035e2e6547377 Mon Sep 17 00:00:00 2001 From: Alistair Evans Date: Sat, 28 Jan 2023 10:16:51 +0000 Subject: [PATCH 5/9] Use full name of type when listing required property failures --- src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs index 259a3b184..7ef0714ea 100644 --- a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs +++ b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs @@ -450,7 +450,7 @@ private string BuildRequiredPropertyResolutionMessage(IReadOnlyList Date: Sat, 28 Jan 2023 10:30:28 +0000 Subject: [PATCH 6/9] Add mixed constructor + property tests. --- .../Features/RequiredPropertyTests.cs | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs index e542557bf..7bab62bb8 100644 --- a/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs +++ b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs @@ -40,6 +40,22 @@ public void MissingRequiredPropertyServiceThrowsException() Assert.Contains(nameof(Component.ServiceB), exception.InnerException.Message); } + [Fact] + public void CanMixConstructorsAndProperties() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType(); + builder.RegisterType(); + + var container = builder.Build(); + + var component = container.Resolve(); + + Assert.NotNull(component.ServiceA); + Assert.NotNull(component.ServiceB); + } + [Fact] public void ExplicitParameterOverridesRequiredAutowiring() { @@ -239,6 +255,18 @@ public ConstructorComponent() required public ServiceB ServiceB { get; set; } } + private class MixedConstructorAndPropertyComponent + { + public MixedConstructorAndPropertyComponent(ServiceA serviceA) + { + ServiceA = serviceA; + } + + public ServiceA ServiceA { get; set; } + + required public ServiceB ServiceB { get; set; } + } + private class MultiConstructorComponent { [SetsRequiredMembers] From 6ad196effef52cdbca0b04db26302820e757012b Mon Sep 17 00:00:00 2001 From: Alistair Evans Date: Sat, 28 Jan 2023 10:35:45 +0000 Subject: [PATCH 7/9] Fix indentation. --- src/Autofac/Core/Activators/Reflection/InjectableProperty.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Autofac/Core/Activators/Reflection/InjectableProperty.cs b/src/Autofac/Core/Activators/Reflection/InjectableProperty.cs index 729f31eb6..f439f4cd5 100644 --- a/src/Autofac/Core/Activators/Reflection/InjectableProperty.cs +++ b/src/Autofac/Core/Activators/Reflection/InjectableProperty.cs @@ -34,7 +34,7 @@ public InjectableProperty(PropertyInfo prop) _setterParameter = _setter.GetParameters()[0]; #if NET7_0_OR_GREATER - IsRequired = prop.GetCustomAttribute() is not null; + IsRequired = prop.GetCustomAttribute() is not null; #endif } From c64f5ca082f65d07d1211a6156ad2207d1f63a49 Mon Sep 17 00:00:00 2001 From: Alistair Evans Date: Sun, 29 Jan 2023 08:57:08 +0000 Subject: [PATCH 8/9] Specify 7.0.101 SDK version to resolve https://github.com/microsoft/vstest/issues/4014; coverlet files not being generated in the right folder. --- global.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/global.json b/global.json index 1982217ab..ed6506965 100644 --- a/global.json +++ b/global.json @@ -1,6 +1,6 @@ { "sdk": { - "version": "7.0.100", + "version": "7.0.101", "rollForward": "latestFeature" }, From a75652f378950567776b1f4fc14b458bbdda39cc Mon Sep 17 00:00:00 2001 From: Alistair Evans Date: Fri, 3 Feb 2023 16:44:45 +0000 Subject: [PATCH 9/9] Improve comment for RequiredMember, add double-resolve test and remove extraneous local variable. --- .../Reflection/ReflectionActivator.cs | 10 +++---- .../Features/RequiredPropertyTests.cs | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs index 7ef0714ea..1c0ad351c 100644 --- a/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs +++ b/src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs @@ -84,12 +84,12 @@ public void ConfigurePipeline(IComponentRegistryServices componentRegistryServic } #if NET7_0_OR_GREATER - // The 'inherited' flag on GetCustomAttribute does not appear to work as expected with regards to RequiredMemberAttribute, - // so we must walk the inheritance hierarchy checking each one. + // The RequiredMemberAttribute has Inherit = false on its AttributeUsage options, + // so we can't use the expected GetCustomAttribute(inherit: true) option, and must walk the tree. var currentType = _implementationType; while (currentType is not null && !currentType.Equals(typeof(object))) { - if (currentType.GetCustomAttribute(inherit: true) is not null) + if (currentType.GetCustomAttribute() is not null) { _anyRequiredMembers = true; break; @@ -134,9 +134,7 @@ public void ConfigurePipeline(IComponentRegistryServices componentRegistryServic if (binders.Length == 1) { - var singleConstructor = binders[0]; - - UseSingleConstructorActivation(pipelineBuilder, singleConstructor); + UseSingleConstructorActivation(pipelineBuilder, binders[0]); return; } diff --git a/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs index 7bab62bb8..00dfe07b9 100644 --- a/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs +++ b/test/Autofac.Specification.Test/Features/RequiredPropertyTests.cs @@ -56,6 +56,32 @@ public void CanMixConstructorsAndProperties() Assert.NotNull(component.ServiceB); } + [Fact] + public void PropertiesPopulatedOnAllSubsequentResolves() + { + var builder = new ContainerBuilder(); + builder.RegisterType(); + builder.RegisterType(); + builder.RegisterType(); + + var container = builder.Build(); + + var component = container.Resolve(); + + Assert.NotNull(component.ServiceA); + Assert.NotNull(component.ServiceB); + + var component2 = container.Resolve(); + + Assert.NotNull(component2.ServiceA); + Assert.NotNull(component2.ServiceB); + + var component3 = container.Resolve(); + + Assert.NotNull(component3.ServiceA); + Assert.NotNull(component3.ServiceB); + } + [Fact] public void ExplicitParameterOverridesRequiredAutowiring() {