.Net 6 + Ninject 3.3.4: InvalidProgramException when using delegates for initializing constructor parameters

[nschuessler]

Given the simplified repro below, the following code throws InvalidProgramException on the delegate used to create the eventId parameter (marked). This works with .Net 5.0.

Any tips what to do here?

The code:

namespace Example
{
    using Ninject;
    using Ninject.Modules;
    using System;
    using System.Collections.Generic;

    public class Program
    {
        public static void Main()
        {
            MessageModule messageModule = new MessageModule();
            StandardKernel standardKernel = new StandardKernel(messageModule);
            IMessageProcessor processor = standardKernel.Get<IMessageProcessor>();
        }
    }
    public class MessageModule : NinjectModule
    {
        public override void Load()
        {
            this.Bind<string>().ToConstant("event-id-constant").Named("EventId");
            this.Bind<string>().ToConstant("event-type-constant").Named("EventType");
            this.Bind<IMessage>().ToConstant(new DoNothingMessage());

            Action<IMessageEvent> e = (IMessageEvent evt) => { };
            this.Bind<Action<IMessageEvent>>().ToConstant(e);

            this.Bind<IDictionary<string, string>>().ToConstant(new Dictionary<string, string>()).Named("LoggingProperties");

            this.Bind<IMessageProcessor>()
                .To<MessageProcessor>()
                .WithConstructorArgument(
                    "eventId",
                    // *** InvalidProgramException here:
                    context => { return context.Kernel.Get<string>("EventId"); })
                .WithConstructorArgument(
                    "eventType",
                    context => { return context.Kernel.Get<string>("EventType"); })
                .WithConstructorArgument(
                    "loggingProperties",
                    context => { return context.Kernel.Get<IDictionary<string, string>>("LoggingProperties"); });
        }
    }

    public interface IMessageProcessor
    {
    }

    public interface IMessage
    {
    }

    public interface IMessageEvent
    {
    }

    public class MessageProcessor : IMessageProcessor
    {
        public MessageProcessor(
            string eventId,
            string eventType,
            IMessage message,
            Action<IMessageEvent> onMessageEventCreated,
            IDictionary<string, string> loggingProperties)
        {

        }
    }

    public class DoNothingMessage : IMessage { }
    public class DoNothingMessageEvent : IMessageEvent { }
}

[EgorBo]

As was diagnosed in dotnet/runtime#67351 it's some invalid IL Ninject produces that wasn't properly validated in .NET 5.0 so it somehow worked (most likely could crash at a random point)
see dotnet/runtime#67351 (comment)

[nschuessler]

It's treating all parameters as object it appears, which probably close to where the problem is. ExpressionInjectorFactory.cs

       public ConstructorInjector Create(ConstructorInfo constructor)
        {
            var parameterArrayExpression = Expression.Parameter(typeof(object[]));
            var parameters = constructor.GetParameters();
            var typedParameterExpressions = CreateTypedParameterExpressions(parameters, parameterArrayExpression);

            var lambda = Expression.Lambda<ConstructorInjector>(
                Expression.New(constructor, typedParameterExpressions),
                parameterArrayExpression);

            return lambda.Compile();
        }

This code uses System.Linq.Expressions namespace to generate the code.

[ericstj]

This looks relevant:

Ninject/src/Ninject/Injection/ExpressionInjectorFactory.cs

Lines 116 to 118 in 3ff6bda

	typedParameterExpressions[i] = parameterType.IsValueType ?
	Expression.Unbox(parameterExpression, parameterType) :
	Expression.TypeAs(parameterExpression, parameterType);

So that's not handling boxing or unboxing pointer types, which don't behave like value types. snippet

I'm not even sure why this repro uses a pointer constructor overload to string, so that might be something to look into. It could be something in metadata enumeration order caused it to start using that constructor when it didn't before. That's a common problem folks run into with loosely constrained uses of reflection.

[nschuessler]

The problem seems to come because it's trying to stuff everything into object[].

Wonder if we can avoid the whole problem by generating a dynamic lambda signature to use for the generic argument to Lambda<>. Because System.Linq.Expression cannot define types (it is only for expressions right)? you'd have to use System.Refelection.Emit directly to define that type, then use Expression.Call to call Expression.Lambda with the list of ParameterExpression[]. I.e. you use IL generate to call a method to generate IL to call the constructor :| All the UnaryExpression (casting) would go away

[ericstj]

IMO you need to really understand why you're using the pointer constructor and if that's intentional before going down the path of supporting calling methods that use pointers. For example, if calling a method that takes pointer will never work (and never worked previously) maybe those methods should be excluded.

[ericstj]

@bartdesmet mentioned on another thread that ninject looks at all constructors and adds them to the plan just in case they might be needed. This includes things that might never work.

I was able to throw together a hack fix for your issue here: ericstj@155383f

Probably a better solution would be to make ninject consider this in its https://github.com/ninject/Ninject/blob/3ff6bdaad8cde418724b8b18edb3a457b9785f8e/src/Ninject/Selection/Selector.cs to exclude constructors, methods, and perhaps other things which it cannot generate code for. There are probably other new language things like ref struct parameters that it cannot support as well.

[nschuessler]

To try and avoid the constructor enumeration I was hoping that making the string both constants ToConstant and singletons InSingletonScope that it would figure out it wouldn't need any constructors and skip that plan construction, but of course that didn't work.

Yup the hack does work.

[ericstj]

I thought of another workaround. If you can avoid adding strings but instead create a wrapping type that might avoid this problem.

Something like:

public class MessageProcessorEvent
{
  public string Id { get; set; } 
  public string Type { get; set; }
}

Then add a constructor parameter that accepts that. That might avoid Ninject trying to generate the expressions that handle string constructors that it never uses.

[nschuessler]

Thanks for the suggestion. The same problem exists with methods and I presume properties. i.e. it enumerates and generates code replacements for them. I fixed it for constructors and methods using the hack, but now there is another case of DynamicMethodInjectorFactory for which I cannot find the source code.

To try and find where this is defined, I had to rebuild two extension libraries with my local build of Ninject (Ninject.Extensions.ChildKernel, Ninject.Extensions.NamedScope). They are out of date wrt the master branch of Ninject (i.e. they rely on modifying collections that were changed to read only).

At this point I think this making a fix to Ninject may not be a possible solution.

[scott-xu]

Just want to mention that Ninject 3.3.4 uses IL Emit whereas the main branch uses ExpressionTree

[nschuessler]

@scott-xu Which branch is considered current / which did 3.3.4 get built from?

[scott-xu]

• current https://github.com/ninject/Ninject/tree/master/src
• 3.3.4: https://github.com/ninject/Ninject/tree/3.3.4/src

[nschuessler]

@scott-xu I have a patch based on the 3.3.4 tag. What is the best branch to submit a PR to with the fix? Any possibly of releasing an update?

The fix is here: https://github.com/nschuessler/Ninject/tree/nschuessler/net6
The change is basically adding the following code to MethodReflectionStrategy.cs and ConstructionReflectionStrategy.cs to avoid generating IL for ctors and methods with pointers in the args:

                bool isSupported = true;
                foreach (ParameterInfo parameter in method.GetParameters())
                {
                    if (parameter.ParameterType.IsPointer)
                    {
                        // IL generation does not support pointers.
                        isSupported = false;
                        break;
                    }
                }

                if (!isSupported)
                {
                    continue;
                }

[scott-xu]

What about emit call void* [System.Private.CoreLib]System.Reflection.Pointer::Unbox(object) if type is pointer?