Inconsistent behavior of different RegistrationExtensions.WithProperty overloads regarding nullable properties

[maxmitti]

Describe the Bug

The WithProperty(Expression<Func<TLimit, TProperty>> propertyExpression, TProperty propertyValue) throws when propertyValue is null, regardless whether TProperty is nullable. Nullable analysis does not flag an issue when passing null.
The relevant check is here:

Autofac/src/Autofac/RegistrationExtensions.cs

Lines 687 to 690 in b222d0f

	if (propertyValue == null)
	{
	throw new ArgumentNullException(nameof(propertyValue));
	}

All other WithProperty overloads accept null or null!.

Steps to Reproduce

using Autofac;
using System;
using Xunit;

namespace Tests;

public class WithPropertyTests
{
    private class Test
    {
        public required int? Property { get; init; }
    }

    [Fact]
    public void WithProperty_NullPropertyValue()
    {
        var builder = new ContainerBuilder();
        builder.RegisterType<Test>()
            .WithProperty(nameof(Test.Property), null!)
            .AsSelf();

        builder.RegisterType<Test>()
            .WithProperty(new TypedParameter(typeof(int?), null))
            .AsSelf();

        Exception? exception = Record.Exception(() =>
            builder.RegisterType<Test>()
                .WithProperty(t => t.Property, null)
                .AsSelf());
        Assert.Null(exception);
    }
}

Expected Behavior

I would expect WithProperty(Expression<Func<TLimit, TProperty>> propertyExpression, TProperty propertyValue) to accept a null propertyValue when TProperty is nullable, similar to the other overloads.

Exception with Stack Trace

Xunit.Sdk.NullException
Assert.Null() Failure: Value is not null
Expected: null
Actual:   System.ArgumentNullException: Value cannot be null. (Parameter 'propertyValue')
   at Autofac.RegistrationExtensions.WithProperty[TLimit,TReflectionActivatorData,TStyle,TProperty](IRegistrationBuilder`3 registration, Expression`1 propertyExpression, TProperty propertyValue)
   at Tests.WithPropertyTests.<>c__DisplayClass1_0.<WithProperty_NullPropertyValue>b__0() in […]\Tests\WithPropertyTests.cs:line 27
   at Xunit.Record.Exception(Func`1 testCode) in /_/src/xunit.core/Record.cs:line 47
   at Tests.WithPropertyTests.WithProperty_NullPropertyValue() in […]\Tests\WithPropertyTests.cs:line 30
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Dependency Versions

Autofac: 7.1.0

[tillig]

This is sort of an interesting thing where I'm not entirely sure what the right answer is.

The reason I say that is that, generally speaking, we don't allow Autofac to "resolve null values." For example, you can't do this:

builder.RegisterInstance<IService>(null);

And if you have property injection enabled on something and say container.InjectProperties(obj); then Autofac will never intentionally inject a null value - you'd get an exception instead.

On the other hand, as you point out, it doesn't look like things are consistent from a parameter perspective - that we sometimes do allow null as a provided parameter and sometimes don't. And I agree, it should be consistent.

My question is - in which direction should parameters be made consistent? Should we stop null everywhere (why are folks intentionally injecting a null?) or should we allow null everywhere ("I want to intentionally set this one parameter to null to force a particular constructor/property behavior")?

On first glance it looks like most of the parameter mechanisms allow for null, which means making it consistently allow null for explicitly provided parameters would be the least breaking way to go, though it could be argued this is inconsistent with the nature of resolved values since you can't resolve a null.

@alistairjevans what do you think?

[maxmitti]

I fully understand your concerns.

In our case, we have a class with various required properties.
We want some of these properties to be injected implicitly from other registrations and provide the missing properties using WithProperty.
The missing properties don’t make sense to be registered generally.
We have this issue with a nullable enum property.

[alistairjevans]

I think if the majority of the WithProperty overloads let you pass null, they probably all should.

I consider WithProperty to be relatively advanced, so if users have a usecase where they have a property they explicitly want nulled, I'm ok with it. That differs from a scenario where you say "Autofac, resolve and provide that value for me", in which case that can never be null.

[tillig]

Sounds good. I'll see if I can get a PR in to consistently allow null for this, unless someone beats me to it.