.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 https://github.com/dotnet/runtime/issues/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 https://github.com/dotnet/runtime/issues/67351#issuecomment-1083470077

[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: https://github.com/ninject/Ninject/blob/3ff6bdaad8cde418724b8b18edb3a457b9785f8e/src/Ninject/Injection/ExpressionInjectorFactory.cs#L116-L118

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: https://github.com/ericstj/Ninject/commit/155383ff99021dfc7f1cbf94dfc8c40a98fe9e5c

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?

[nschuessler]

You are asking about the (object) argument really being a pointer? My understanding that casting anything to an object when it is a pointer and used as parameter could be a problem.

Let's see of @EgorBo would be able to clarify if there are exceptions or cases that do work maybe like the above.

btw. Good catch. I didn't fix that one if it is needed.

[nschuessler]

@scott-xu it appears we are not getting an answer on this. In .Net 6 they have added more checks on the generated IL than previous versions. The way to answer if changes are needed may be to build a .Net 6.0 specific version as a test and then see if there are any exceptions. To ensure the IL is jitted, I believe run it under Visual Studio (press F5) with the profiler running (which causes all the check to happen). This is how I verified my fix.

[scott-xu]

@nschuessler Can you please try this build please? https://ci.appveyor.com/project/Ninject/ninject/builds/43176891/artifacts

[scott-xu]

Just noticed another problem at .NET 6. No idea what's going on. It is just to unbox a value type.

Common Language Runtime detected an invalid program.
   at System.Runtime.CompilerServices.RuntimeHelpers.CompileMethod(RuntimeMethodHandleInternal method)
   at System.Reflection.Emit.DynamicMethod.CreateDelegate(Type delegateType)
   at Ninject.Injection.DynamicMethodInjectorFactory.Create(ConstructorInfo constructor) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\Injection\DynamicMethodInjectorFactory.cs:line 59
   at Ninject.Planning.Strategies.ConstructorReflectionStrategy.Execute(IPlan plan) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\Planning\Strategies\ConstructorReflectionStrategy.cs:line 82
   at Ninject.Planning.Planner.<>c__DisplayClass9_0.<CreateNewPlan>b__0(IPlanningStrategy s) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\Planning\Planner.cs:line 110
   at Ninject.Infrastructure.Language.ExtensionsForIEnumerableOfT.Map[T](IEnumerable`1 series, Action`1 action) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\Infrastructure\Language\ExtensionsForIEnumerableOfT.cs:line 43
   at Ninject.Planning.Planner.CreateNewPlan(Type type) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\Planning\Planner.cs:line 110
   at Ninject.Planning.Planner.GetPlan(Type type) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\Planning\Planner.cs:line 71
   at Ninject.Activation.Providers.StandardProvider.Create(IContext context) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\Activation\Providers\StandardProvider.cs:line 113
   at Ninject.Activation.Context.ResolveInternal(Object scope) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\Activation\Context.cs:line 190
   at Ninject.Activation.Context.Resolve() in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\Activation\Context.cs:line 170
   at Ninject.KernelBase.Resolve(IRequest request, Boolean handleMissingBindings) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\KernelBase.cs:line 567
   at Ninject.KernelBase.Resolve(IRequest request) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\KernelBase.cs:line 351
   at Ninject.ResolutionExtensions.GetResolutionIterator(IResolutionRoot root, Type service, Func`2 constraint, IEnumerable`1 parameters, Boolean isOptional, Boolean isUnique) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\Syntax\ResolutionExtensions.cs:line 397
   at Ninject.ResolutionExtensions.Get[T](IResolutionRoot root, IParameter[] parameters) in C:\Users\scott\source\repos\ninject\Ninject\src\Ninject\Syntax\ResolutionExtensions.cs:line 47
   at Ninject.Tests.Integration.ConstructorSelectionTests.<WhenClassHasTwoConstructorsWithInjectAttributeThenAnActivationExceptionIsThrown>b__17_0()
   at FluentAssertions.Specialized.ActionAssertions.InvokeSubjectWithInterception()

IL_0000: ldarg.0    
IL_0001: ldc.i4.0   
IL_0002: ldelem.ref 
IL_0003: unbox      System.Int32
IL_0008: newobj     Void .ctor(Int32)/Ninject.Tests.Integration.ConstructorSelectionTests+ClassWithTwoInjectAttributes
IL_000d: ret

[nschuessler]

@nschuessler Can you please try this build please? https://ci.appveyor.com/project/Ninject/ninject/build/artifacts

Will try this today.

[nschuessler]

@nschuessler Can you please try this build please? https://ci.appveyor.com/project/Ninject/ninject/build/artifacts

Will try this today.

Sadly it doesn't seem to work. Same issue, unless I goofed something. Can you point me to the code change for this release? Maybe I can spot something

[nschuessler]

(For the other issue) If we can find a small repro we can post it on dotnet/runtime project for help. May help to show the signature of the method being generated.

[scott-xu]

@nschuessler Can you please try this build please? https://ci.appveyor.com/project/Ninject/ninject/build/artifacts

Will try this today.

Sadly it doesn't seem to work. Same issue, unless I goofed something. Can you point me to the code change for this release?

Check this commit please https://github.com/ninject/Ninject/commit/8624475fd8394657450adc4e8f4e5573d19c6f00

[nschuessler]

That test passes on your system? I don't know enough about IL to spot an issue. I'll open an issue on dotnet/runtime and point it at this thread.

[scott-xu]

That test passes on your system? I don't know enough about IL to spot an issue. I'll open an issue on dotnet/runtime and point it at this thread.

Try CI 269 please: https://ci.appveyor.com/project/Ninject/ninject/builds/43200302/artifacts

[nschuessler]

That test passes on your system? I don't know enough about IL to spot an issue. I'll open an issue on dotnet/runtime and point it at this thread.

Try CI 269 please: https://ci.appveyor.com/project/Ninject/ninject/builds/43200302/artifacts

Yes, this one works (just testing the string constructor though).

[nschuessler]

@scott-xu When I made a custom build, I noticed that I had to rebuild Ninject.Extensions.ChildKernel and Ninject.Extensions.NamedScope because they reference earlier versions of the Ninject library w/o the fix and seem to pull that in. Do I need to ask for updates on those projects as well once the new version is released?

[nschuessler]

@scott-xu Are you involved with the extensions projects or do I need to request separately new builds when 3.3.5 comes out?

[scott-xu]

3.3.5 should be compatible with the extensions

[scott-xu]

@nschuessler Ninject 3.3.5 rc1 is released. Please give a try https://www.nuget.org/packages/Ninject/3.3.5-rc1

[nschuessler]

I ran a bunch of tests on the RC version outside of production. It seems to be fine.