Is null a valid value for a dependency (from a factory method)?

[kenwarner]

Per the docs

No, it's not. null means no value and is ignored by Windsor. Be explicit - if the value is optional provide overloaded constructor that does not include it. Alternatively specify explicitly null as the default value in the signature of the constructor. This is the only scenario where Windsor will allow passing null.

In my case I've used the alternate approach and specified null as the default value in the constructor. And yet, my factory method throws an exception

{"Factory method creating instances of component 'Late bound ConsoleApplication1.IFoo' returned null. This is not allowed and most likely a bug in the factory method."}

Here's a gist to demonstrate

[tjrobinson]

This StackOverflow question probably answers your question: http://stackoverflow.com/questions/3573201/allow-optional-null-property-injection-in-castle-windsor-via-factory-method

The short version - use the Null Object pattern, i.e. an "empty" implementation of your interface, and return that instead of null in your factory method.

[DSanchen]

What if the Null Object pattern does not match my needs ? Say I have a class with an optional dependency, passed as constructor parameter. Inside that class I check wether or not my dependency is null. When it is null, I don't use it, when it's not null I use it. See this example.

By passing in a Null Object, I have no chance to check whether or not my dependency is valid. After all, I "only" have the objects interface...

Btw.: Even calling a factory function with no parameter (which should, by doc call the appropriate constructor without parameters) does not work.

This is, at least to me, unexpected. Is this intended ?

[ghost]

@DSanchen - This is very much intended. Think about it this way, you have an IoC container who's bread and butter so to speak is about dealing with types and how they are resolved. Null does not have a type. This affect resolution determinism in the container. For example:

If you have a constructor that resolves a dependent class A, when you submit null how would the container know that typeof(null) == typeof(A)?

Optional dependencies with default parameters has not been built into the container yet. Unless this leads to a pull request I am going to close this issue out for now.

Please feel free to re-open and submit a PR if you feel this was a mistake.

[jeme]

when you submit null how would the container know that typeof(null) == typeof(A)?

Why would it need to know that? o.O... That actually doesn't make any sense... typeof(WhatEverFooTheMethodActuallyReturns) != typeof(A) anyways... So it has to rely on the registration and return type anyways right?... So why does it actually care what the factory actually returns? I can't see any reason that that couldn't be ignored?

[ghost]

Why would it need to know that? o.O... That actually doesn't make any sense... typeof(WhatEverFooTheMethodActuallyReturns) != typeof(A) anyways... So it has to rely on the registration and return type anyways right?... So why does it actually care what the factory actually returns? I can't see any reason that that couldn't be ignored?

There is registration side which attempts to satisfy dependencies based on what was “known” for components during startup. During resolve time type parameters can be passed to the resolve mechanism and based on that it has to pick a constructor based the type of parameters passed to it. If you pass null, what type is it? Also there is a bug in how the weighting algo works for selecting constructors I just don’t know what the fix is yet. @jeme your example does not tie to the code samples in the OP or the SO. So I literally have no idea what you mean when you say typeof blah equals blah I thought this was about typeof null = blah. Can you clarify? Maybe I got this wrong, but the OP was about resolution determinism in Windsor from a typed factory during resolve time if the return value was null.

[jeme]

There is registration side which attempts to satisfy dependencies based on what was “known” for components during startup. During resolve time type parameters can be passed to the resolve mechanism and based on that it has to pick a constructor based the type of parameters passed to it. If you pass null, what type is it?

That is fair enough, but how does that tie to the provided GIST?

Starting from: https://gist.github.com/kenwarner/4d7429f13a958881081d#file-program-cs-L43

1. Resolve IBar
2. By Registration we know that IBar is implemented by Bar, so create a Bar
3. Bar requires (optionally) a IFoo.
4. Resolve IFoo (note, we are not resolving null, we are resolving the type IFoo)
5. By Registration we know that we have a factory method for IFoo, call that
6. Give the IFoo returned by the factory to Bar (null)

I cannot see where you logically would be missing any information for that flow in the given GIST... I can simply not see where your example of typeof(null) == typeof(A) would need to take place?...

[ghost]

Aaah I see.

I was focusing on the factory method here which was returning null.

[jeme]

I was focusing on the factory method here which was returning null.

That was my initial thought, your response gave me doubt...

That factory method could return Foo or FooBar or null, neither of that is useful information. What is useful information is that it declares that it returns an IFoo. So whatever it returns should just be treated as that.

[ghost]

The service type from the component registration?

[ghost]

The exception is trying to tell you this is not possible.

{"Factory method creating instances of component 'Late bound ConsoleApplication1.IFoo' returned null. This is not allowed and most likely a bug in the factory method."}

This is because typed factories are late bound components.

[ghost]

Also don't you think it is wasteful having a factory that returns null?

[jeme]

The service type from the component registration?

Also useful yes, still nothing to do with looking at null's type...

The exception is trying to tell you this is not possible. ... This is because typed factories are late bound components.

Seems like you misunderstood where I was getting at, one thing is what Castle disallows you to do, another is what would be possible to do in an envisioned implementation, your statement indicated that it wasn't at all possible to implement because you could not possibly know what type "null" had... What I am saying is that you don't need to know that because you know what it was meant to be (The information is there twice, A the registration, B the factory method signature)... So the argument about Null not having a type made no sense to me... (Still doesn't)...

Also don't you think it is wasteful having a factory that returns null?

That is not for me to say

[ghost]

What I am saying is that you don't need to know that because you know what it was meant to be (The information is there twice, A the registration, B the factory method signature)... So the argument about Null not having a type made no sense to me... (Still doesn't)...

Sure in code I agree, typed factories are the thing which needs consideration here.

The problem with late bound components at the moment is they use an interceptor before moving on to the factory method during the activation sequence. Component models are not available during this cycle when you need them, null makes it less explicit and harder to solve. Hence the "helpful" exception.

Still think passing null is a bad idea. If still feel strongly that this should be re-opened, I am happy to do it. Not going to cross link issues but I have proposed creating a typed factory "god" issue with a branch that has failing tests, so we can work through the bits and bobs we know about. Adding this as a test case might take some explaining though.

[ghost]

Now this is where is becomes philosophical.

https://blog.valbonne-consulting.com/2014/11/26/tony-hoare-invention-of-the-null-reference-a-billion-dollar-mistake/

@jnm2 - Might have some views on this.

[jnm2]

Heh heh I sure do, but they may not be the views you were expecting... 😈

The problem with nulls in most languages is not inherent; the problem with nulls is that the type system makes it look like the parameter type is ICanFoo when in reality the parameter type which the type system effectively gives you is ICanFoo | null, so to speak. The problem with nulls is that these type systems do not allow you to specify simply ICanFoo without the implicit | null.

null is not a thing that fulfills the ICanFoo contract any more than ICanBar is. Yet ICanFoo | ICanBar could be a useful type to specify, indicating that your code expects one of the two. Likewise, ICanFoo | null could be a useful type to specify, indicating the same thing. Type systems should not blur the line and allow you to assign ICanBar as an ICanFoo; neither should they allow you to pass null as an ICanFoo. That null-blindness in every major type system, not null itself, is the billion-dollar mistake.

As always, where the type system fails you, lean heavily on documentation, runtime checks, and testing. null is still redeemable. Use it where it makes sense to your audience. My personal sense is that you should not make your audience suffer through Maybe or Optional types or gratuitous null-object patterns. The bar is higher with frameworks than with libraries though. My expectation of a framework (or a framework-style library like Castle Windsor) is that it ought to be as agnostic as possible, interfacing with every convention with equal dexterity.

I particularly admire the way TypeScript handles null, and for them, undefined also.

[jeme]

@fir3pho3nixx First of, null being a mistake or not is opinionated, not a fact we can really test out... We can only prove that null has it's problems with the way it's used... But we can't really get further than that... But it doesn't really matter weather or not it was a mistake, null is there now for better or worse and I am not really here to defend null...

But I certainly don't like if a framework like Windsor dictates such design choices for us... The question would be if this could be made into a convention which could be swapped out, so that the users them self could make that choice... ""If you don't like that Windsor does not allow you to pass null around ass services, here go and replace the 'DontAllowNullConvention'""

That would probably be optimal solution IMO.

[ghost]

@jeme

I have given you hell on this issue. Thanks for sticking with it.

I am not really here to defend null...

In terms of performance alone you are launching N+1 stack-frame method invocations during run-time only to yield a null which you have to branch for in your client code adding even more completely unnecessary code branches or further stack-frame invocations as a result of bad refactorings. Why would you do this? It is bad when it comes to performance because of the null checks alone. Null coalescence makes this easier but it adds weight every time you do it. If we disallow this, it is because we want your code to run like "shit off a shovel" (fast).

But I certainly don't like if a framework like Windsor dictates such design choices for us...

I am listening, but already I am "negotiating" my position. Please consider my point of view.

[ghost]

This is a "feature" as far as I am concerned which needs a helpful exception and docs.

[jeme]

In terms of performance alone you are launching N+1 stack-frame method invocations during run-time only to yield a null which you have to branch for in your client code adding even more completely unnecessary code branches or further stack-frame invocations as a result of bad refactorings. Why would you do this? It is bad when it comes to performance because of the null checks alone. Null coalescence makes this easier but it adds weight every time you do it. If we disallow this, it is because we want your code to run like "shit off a shovel" (fast).

I find this argument silly, I don't believe that anyone would introduce Castle Windsor in their project and then go, "Yes, now we never have to do a null check again"... They will still add lots of "if null then throw"... Because that's just common sense to do if you can't function without that injected servicen...

So from a performance perspective, I find it extremely hard to believe that you would see a difference in "If null then throw" or "if null then default"... Please prove that...

If your referring to that Castle has to do allot of null checks internally beyond the one it clearly already does, then we are perhaps talking a bigger redesign (of what I would consider a bad design) in order to facilitate a feature like this... And if no-one wish to spend the time to implement that, that is just called prioritization - and that is fair enough...

I am listening, but already I am "negotiating" my position. Please consider my point of view.

Your point if view we be perfectly perserved if this was a configurable strategy... In fact, your point of view would be the one I would suggest was the default strategy, and the only one that would ship with the product out of the box...

I am merely arguing that allowing others to have the ability to replace this in Castle would be a solution that catered for everyone's point of view, rather that just a single point of you.

[jeme]

Actually after digging a bit deeper, we can get the desired end behavior by using a custom NULL activator instead, (see: https://gist.github.com/jeme/208041346f4a7867dcd783508f66c1b8)

That means we should be able to retrofit this in for factory methods as well adding an extension method to ComponentRegistration which registers a custom implementation of the FactoryMethodActivator...

This also means that it wouldn't be to difficult to make this a configurable convention that dictates which activator would be used by default, however it's very questionable if that is worth it...

---

That means that it's pointless to argue about, and the answer to this issue is that you CAN actually do this by using custom implementations of activators, and for convenience if you wish, you can add extension methods that are similar to UseFactory and make those use the custom activator...

[jeme]

So here is a complete example of something that actually works... (Please ignore the awful naming- and code-style, this is just a prof of concept)

However there is a minor catch, in order to mimic the built in UserFactoryMethod function, we had to access the potentialServices field by reflection because it's not publicly accessible.

So that is a required change to allow for this in a "Supported" way.

In general when looking at the implementation, many of the methods end up in AddDescriptor, so it would make perfect sense to me to start a process where UseFactoryMethod, Activator, DependsOn all was pulled out into Extension implementations as to move out all those concerns and allow for a more extensible model.

---

using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Text;
using System.Threading.Tasks;
using Castle.Core;
using Castle.Core.Internal;
using Castle.DynamicProxy;
using Castle.MicroKernel;
using Castle.MicroKernel.ComponentActivator;
using Castle.MicroKernel.Context;
using Castle.MicroKernel.Registration;
using Castle.Windsor;

namespace AllowNull
{
    public class Program
    {
        public static void Main(string[] args)
        {
            IWindsorContainer container = new WindsorContainer();
            container.Register(Component.For<IFoo>()
                .UsingFactoryMethodAllowNull(Foo.CreateFoo));
            container.Register(Component
                .For<IBar>().ImplementedBy<Bar>());

            var service = container.Resolve<IBar>();
            service.PeekABoo();
        }
    }

    public static class CRE
    {
        public static ComponentRegistration<TService> UsingFactoryMethodAllowNull<TImpl, TService>(this ComponentRegistration<TService> self, Func<TImpl> factoryMethod,
            bool managedExternally = false)
            where TImpl : TService where TService : class
        {
            return self.UsingFactoryMethodAllowNull((k, m, c) => factoryMethod(), managedExternally);
        }
        public static ComponentRegistration<TService> UsingFactoryMethodAllowNull<TImpl, TService>(this ComponentRegistration<TService> self,
            Func<IKernel, TImpl> factoryMethod,
            bool managedExternally = false)
            where TImpl : TService where TService : class
        {
            return self.UsingFactoryMethodAllowNull((k, m, c) => factoryMethod(k), managedExternally);
        }

        public static ComponentRegistration<TService> UsingFactoryMethodAllowNull<TService, TImpl>(
            this ComponentRegistration<TService> self,
            Func<IKernel, ComponentModel, CreationContext, TImpl> factoryMethod,
            bool managedExternally = false)
            where TImpl : TService where TService : class
        {
            self.Activator<FactoryMethodActivatorAllowingNull<TImpl>>()
                .ExtendedProperties(Property.ForKey("factoryMethodDelegate").Eq(factoryMethod));
            if (managedExternally)
            {
                self.ExtendedProperties(Property.ForKey("factory.managedExternally").Eq(managedExternally));
            }

            //Note: Hack, I would sugest providing a public property for this list which returned the list, as readonly unless there could be some value in changes to it.
            List<Type> potentialServices = (List<Type>)self.GetType()
                .GetField("potentialServices", BindingFlags.NonPublic | BindingFlags.Instance)
                .GetValue(self);
            var firstService = potentialServices.First().GetTypeInfo();

            //var firstService = self.Services.First().GetTypeInfo();
            if (self.Implementation == null &&
                (firstService.IsClass == false || firstService.IsSealed == false))
            {
                self.ImplementedBy(typeof(LateBoundComponent));
            }
            return self;
        }
    }

    public class FactoryMethodActivatorAllowingNull<T> : DefaultComponentActivator, IDependencyAwareActivator
    {
        protected readonly Func<IKernel, ComponentModel, CreationContext, T> creator;
        protected readonly bool managedExternally;

        public FactoryMethodActivatorAllowingNull(ComponentModel model, IKernelInternal kernel, ComponentInstanceDelegate onCreation, ComponentInstanceDelegate onDestruction)
            : base(model, kernel, onCreation, onDestruction)
        {
            creator = Model.ExtendedProperties["factoryMethodDelegate"] as Func<IKernel, ComponentModel, CreationContext, T>;
            managedExternally = (Model.ExtendedProperties["factory.managedExternally"] as bool?).GetValueOrDefault();
            if (creator == null)
            {
                throw new ComponentActivatorException(
                    $"{GetType().Name} received misconfigured component model for {Model.Name}. Are you sure you registered this component with \'UsingFactoryMethod\'?", Model);
            }
        }

        public bool CanProvideRequiredDependencies(ComponentModel component)
        {
            // the factory will take care of providing all dependencies.
            return true;
        }

        public bool IsManagedExternally(ComponentModel component)
        {
            return managedExternally;
        }

        protected override void ApplyCommissionConcerns(object instance)
        {
            if (managedExternally || instance == null)
            {
                return;
            }
            base.ApplyCommissionConcerns(instance);
        }

        protected override void ApplyDecommissionConcerns(object instance)
        {
            if (managedExternally || instance == null)
            {
                return;
            }
            base.ApplyDecommissionConcerns(instance);
        }

        public override object Create(CreationContext context, Burden burden)
        {
            var instance = InternalCreate(context);
            if (instance != null)
                burden.SetRootInstance(instance);
            OnCreation?.Invoke(Model, instance);
            return instance;
        }

        protected override object Instantiate(CreationContext context)
        {
            object instance = creator(Kernel, Model, context);
            if (ShouldCreateProxy(instance))
            {
                instance = Kernel.ProxyFactory.Create(Kernel, instance, Model, context);
            }
            return instance;
        }

        protected override void SetUpProperties(object instance, CreationContext context)
        {
            // we don't
        }

        private bool ShouldCreateProxy(object instance)
        {
            if (instance == null)
            {
                return false;
            }
            if (Kernel.ProxyFactory.ShouldCreateProxy(Model) == false)
            {
                return false;
            }
            return ProxyUtil.IsProxy(instance) == false;
        }
    }

    public interface IFoo { }
    public class Foo : IFoo
    {
        public static IFoo CreateFoo() => null;
    }

    public interface IBar
    {
        void PeekABoo();
    }

    public class Bar : IBar
    {
        private readonly IFoo foo;
        public Bar(IFoo foo = null) => this.foo = foo;
        public void PeekABoo() => Console.WriteLine("My foo was " + (foo?.ToString() ?? "NULL"));
    }
}

---

Ofc if the parameter isn't optional, castle throws another exception, namely that it can't resolve the required parameter. - As noted in the Topic...

But this is very interesting in the light of the whole null is evil and bad design discussion, because that is IMO clearly a really bad use of null... Castle is indicating that there wasn't a component registered for IFoo, but that is incorrect... This will leave the developer confused...

The solution here is ofc not to pass the instance it self around, but rather A "Resolved" wrapper instead... which could contain the actual value or null (if any activators was registered that returned null).

This way, Castle could actually provide a far more meaningful error message in the Resolve call where appropriate, again this could be conventional so it would allow people to say "Well it's ok i get null if I do container.Resolve()" and the documentation around that could be reformed to fit that. (In fact, it's already conventional because it's in the implementation of the DefaultResolver which i believe can be replaced as well - Edit: Verified, that can be replaced as well...)

This is a bigger design change I guess however. (Edit:: Looking further at how Resolve works, providing the improved error doesn't seem to be too difficult in it self as ResolveCore actually uses the CanResolve methods and they return true in this case)

[mario-d-s]

This is a "feature" as far as I am concerned which needs a helpful exception and docs.

Is this why you've closed this issue? Does that mean "won't fix" or...?

[jeme]

Sorry for returning to this issue so late, but just wanted to add a refinement of the previous example.

I have since discovered that since replacing the activator for a component is supported out of box, we can absolutely do this with Castle, no changes required. Building on the example above, we can remove the need for the extension methods, and just use the following code:

        public static void Main(string[] args)
        {
            IWindsorContainer container = new WindsorContainer();
            container
                .Register(Component.For<IFoo>()
                .Activator<FactoryMethodActivatorAllowingNull<IFoo>>()
                .UsingFactoryMethod(Foo.CreateFoo));
            container
                .Register(Component
                .For<IBar>().ImplementedBy<Bar>());

            var service = container
                .Resolve<IBar>();
            service.PeekABoo();
        }

Now we could wrap the Activator and UsingFactoryMethod in their own extension method to "ease" the use a bit, but I will leave that up to people them self.

Which means there is no action needed for this issue, I don't know if that sort of answers your question @mario-d-s, as in, we don't really need any changes here???...

---

That is not saying that there couldn't be some good refinements to Castle Windsor as pr. the Caveat I mentioned above, however that should be requested as a separate issue and not be part of this one.

[jnm2]

https://github.com/castleproject/Windsor/pull/522 was just merged which allows you to specify null for any dependency of a nullable value type (System.Nullable<T>). It could be extended to decode C# 8 nullable reference types.

[jeme]

@jnm2 Yes I actually noticed :) - and expanding to C#8 would perhaps be great for the future as that can also clearly indicate intent, however this was a solution that works now so I thought it was worth adding (back when I added it) - the initial version required a small hack though, so the addition i gave today is "hack free".