Constructor injection and inheritance

[jnm2]

The ideal DI strategy seems to be constructor injection of each dependency. Each dependency becomes a readonly field and the dependencies are clearly and idiomatically visible in the constructor signature. This works well both with manual instantiation and with IoC containers.

Its single downside is that it does not play well with inheritance. The constructor is pretty much only meant to be used by IoC containers, but it's also used by necessity by the derived constructor. If the base constructor needs a new dependency, it's a breaking change to add a constructor parameter.

ReSharper is very helpful when applying signature changes to automatically modify derived constructors and inject instances found at call sites, but besides the downsides of that code churn, the primary scenario is where the base class is in a different library.

I've run into this issue more and more lately and it came up in dotnet/efcore#7465.

Possible solutions

• Service location is where (Infrastructure: Add parameter objects and service rewriting code to avoid breaking base class D.I. constructors in patch/point releases efcore#7465 (comment)) is likely going to land. For my own architectures, I've learned the hard way that service location results in no small amount of pain. Much could be said but let's put that aside for now and see if we can still achieve a painless dependency injection solution.

• Property injection solves the breaking changes problem when using an IoC container. With inheritance, the container sets the inherited properties and everything works. With composition, the 'base' instance is constructor injected and everything works. Without an IoC container though, you've exchanged a breaking change at compile time (new ctor parameter) for a breaking change at runtime (unset property) which is bad. There's no way around the fact that a break should happen with manual instantiation, but it should break at compile time. Property injection also introduces lots of management to make sure the properties aren't abused and it is a poor API.

• Private field injection solves the breaking changes problem when using an IoC container. With inheritance, the container sets the inherited properties and everything works. With composition, the 'base' instance is constructor injected and everything works. But without an IoC container, there is no API to initialize this at all and nothing works.

• A DependencyArgs parameter is the only thing I can think of that works in today's C#. I've been thinking about the fact that the BCL best practice for adding parameters to event handlers in a back-compatible way is to define an EventArgs from the start. Compared to straight up constructor injection it feels like quite some overhead to write a DependencyArgs for each service, yet we've done it for every event for years so maybe it's not as bad is it first seems. Dependencies (properties on the struct) could be added without breaking changes.

  The base service could contain an immutable nested public struct DependencyArgs that contains the constructor with a parameter for each dependency and sets each dependency to a public getter-only property or public readonly field. Then the service takes this struct as a parameter instead of the list it had before. The derived class would also take a parameter of type BaseService.DependencyArgs and pass it to the base constructor, and could either borrow dependencies off that object or have them injected beside the args parameter. If the derived class itself is expected to be subclassed, it could take its own DerivedService.DependencyArgs type as well and the BaseService.DependencyArgs would be injected into the DerivedService.DependencyArgs.
  With an IoC container, you'd register all nested types named DependencyArgs so that the container automatically creates and injects the args and everything works. Adding a dependency to the base DependencyArgs does not require any change in derived classes.
  Without an IoC container, you obviously still need a breaking change to happen because your manually written composition root does not dynamically locate services. Instead of breaking because there is a missing service constructor parameter, it now breaks because there is a missing DependencyArgs constructor parameter. This is cleaner than the runtime break that property injection would cause.

  In practice this would be simpler than it sounds than when I list all the edge cases it handles. You exchange listing dependencies via constructor parameter list for exposing them via nested public struct. It's a parameter refactor. The visibility and API stay pretty much the same.

• But... what if it didn't have to be this way? I can't think of any improvement on that last solution without a language change.

Is this too far out for C# or might it actually happen? Similar to rest parameters (hear me out) but typesafe and designed for this particular scenario?

class BaseService
{
    public BaseService(Foo foo, Bar bar)
    {
        this.foo = foo;
        this.bar = bar;
    }
}

sealed class DerivedService : BaseService
{
    public DerivedService(baseparams, Baz baz) : base(baseparams)
    {
        this.foo = baseparams.foo;
        this.baz = baz;
    }
}

If baseparams is like a rest parameter:

In this scenario, baseparams is a keyword which means "insert the parameter list from the base class" as in rest parameters. This would solve the source breaking problem which is the bulk of the work and the bulk of the downside to constructor injection, but would be binary breaking. It's likely that you'll be recompiling if the base class changes, so this may be totally fine.

If baseparams is sugar for a generated struct:

In this scenario, my DependencyArgs struct solution above is compiler-generated. This solves both the source breaking problem and the binary breaking problem. The struct would have to live in the base assembly, so:

class BaseService
{
    public BaseService(encapsulateparams(Foo foo, Bar bar))
    {
        if (foo == null) throw ArgumentNullException(nameof(foo));
        this.foo = foo;
        this.bar = bar;
    }
}

sealed class DerivedService : BaseService
{
    public DerivedService(baseparams, Baz baz) : base(baseparams)
    {
        if (baseparams.foo == null) throw ArgumentNullException(nameof(baseparams.foo));
        this.foo = baseparams.foo;
        this.baz = baz;
    }
}

Desugared:

class BaseService
{
    public struct DependencyArgs
    {
        public readonly Foo foo;
        public readonly Bar bar;

        public DependencyArgs(Foo foo, Bar bar)
        {
            this.foo = foo;
            this.bar = bar;
        }
    }
    public BaseService(DependencyArgs dependencyArgs)
    {
        if (dependencyArgs.foo == null) throw ArgumentNullException(nameof(dependencyArgs.foo));
        this.foo = dependencyArgs.foo;
        this.bar = dependencyArgs.bar;
    }
}

sealed class DerivedService : BaseService
{
    public DerivedService(BaseService.DependencyArgs baseDependencyArgs, Baz baz) : base(baseDependencyArgs)
    {
        this.foo = baseDependencyArgs.foo;
        this.baz = baz;
    }
}

I'm just brainstorming here and hoping for improvements or new solutions on this entire topic. What do y'all think?

[HaloFour]

So leaving DI aside the real question here would be promoting the arguments of the base constructor to the derived constructor without having to type them all out, right? I could see something being useful in general, but I do have a few concerns.

If the base class has more than one constructor how would the compiler resolve which parameter list to include? Or would that be a compiler error?

If the base class constructor is changed to have a new parameter would that parameter appear in the middle of the derived class constructor parameter list?

Would all of the attributes of the base class parameters be copied to the derived constructor parameters? This would be particularly poignant for DI scenarios.

[jnm2]

If the base class has more than one constructor how would the compiler resolve which parameter list to include? Or would that be a compiler error?

I can't think of a good reason to have multiple constructors with DI. Perhaps if there are multiple constructors, the constructor intended for DI can be marked as such. Containers would love this.

If the base class constructor is changed to have a new parameter would that parameter appear in the middle of the derived class constructor parameter list?

In the "rest parameter" style, yes. You might want to put the base parameters at the end but I'm not sure that this matters at all. It won't affect IoC containers and if you are composing manually, you have a breaking change either way.
In the "struct sugar" style of course there is only a single parameter.

Would all of the attributes of the base class parameters be copied to the derived constructor parameters? This would be particularly poignant for DI scenarios.

In the "rest parameter" style, yes. In the "struct sugar" style, they'd move to the struct constructor parameters.

[HaloFour]

I can't think of a good reason to have multiple constructors with DI.

Language features shouldn't be designed around how specific libraries and frameworks happen to work with them. Multiple constructors are possible, and useful, even in DI scenarios.

For example, maybe I am going to take on another dependency, but optionally and with fallback logic when that dependency isn't provided.

In the "rest parameter" style, yes. You might want to put the base parameters at the end but I'm not sure that this matters at all. It won't affect IoC containers and if you are composing manually, you have a breaking change either way.

Same concern as above.

Sure, the change to the base class is a breaking change. But assuming that I'm inheriting that class from some other assembly I would much prefer that I have to manually deal with that change and opt-into how that behavior affects my own class. The idea that the language would implicitly and silently break the constructor contract of my class bothers me.

In the "rest parameter" style, yes. In the "struct sugar" style, they'd move to the struct constructor parameters.

Obviously that couldn't happen with attributes that can target parameters but not fields/properties. That would likely apply more to non-DI attributes, but it still might apply.

[jnm2]

Sure, the change to the base class is a breaking change. But assuming that I'm inheriting that class from some other assembly I would much prefer that I have to manually deal with that change and opt-into how that behavior affects my own class. The idea that the language would implicitly and silently break the constructor contract of my class bothers me.

So you'd be against the concept of rest-parameter-like behavior in general because breaking changes in referenced assemblies could cause breaking changes in your assembly when you start referencing the new one without changing code.

In the "rest parameter" style, yes. In the "struct sugar" style, they'd move to the struct constructor parameters.

Obviously that couldn't happen with attributes that can target parameters but not fields/properties. That would likely apply more to non-DI attributes, but it still might apply.

You may have misunderstood. The struct has a constructor with the same parameters and the same parameter attributes that you declared in the class constructor. Fields and properties aren't annotated with anything.

[HaloFour]

@jnm2

So you'd be against the concept of rest-parameter-like behavior in general because breaking changes in referenced assemblies could cause breaking changes in your assembly when you start referencing the new one without changing code.

Well, let's just say that I'd raise an eyebrow at the notion of parameter lists shifting in the manner described. If I were to be manually adding a new parameter as a breaking change I would still most likely want to add it to the end of the parameter list and not stick it into the middle. The "rest" always seems to belong at the end.

You may have misunderstood. The struct has a constructor with the same parameters and the same parameter attributes as the class.

I see. That seems to make some assumptions as to how the IoC would work and would be configured, as the DependencyArgs struct itself becomes a dependency.

[jnm2]

Supporting multiple base constructors, each of which may have an entirely different implementation:

When there are multiple base constructors, there is no way to disambiguate without specifying the signature. Specifying the signature removes any benefit to the syntax sugar. You may as well just list the base parameters as usual. With the "rest parameter" style you'd only be able to use this if there was a single base constructor.

The same probably follows for the "struct sugar" style.
However, this possibility could be considered:

class BaseService
{
    public BaseService(encapsulateparams(Foo foo))
    {
        if (foo == null) throw ArgumentNullException(nameof(foo));
        this.foo = foo;
    }
    public BaseService(encapsulateparams(Foo foo, Bar bar))
    {
        if (foo == null) throw ArgumentNullException(nameof(foo));
        this.foo = foo;
        this.bar = bar;
    }
}

sealed class DerivedService : BaseService
{
    public DerivedService(baseparams, Baz baz) : base(baseparams)
    {
        if (baseparams.foo == null) throw ArgumentNullException(nameof(baseparams.foo));
        this.foo = baseparams.foo;
        this.baz = baz;
    }
}

Desugars to:

class BaseService
{
    public struct DependencyArgs
    {
        public readonly Foo foo;
        public readonly Bar bar;
        internal readonly int <ctorId>;

        public DependencyArgs(Foo foo)
        {
            this.foo = foo;
            this.<ctorId> = 1;
        }

        public DependencyArgs(Foo foo, Bar bar)
        {
            this.foo = foo;
            this.bar = bar;
            this.<ctorId> = 2;
        }
    }

    public BaseService(DependencyArgs dependencyArgs)
    {
        switch (dependencyArgs.<ctorId>)
        {
            case 1:
                if (dependencyArgs.foo == null) throw ArgumentNullException(nameof(dependencyArgs.foo));
                this.foo = dependencyArgs.foo;
                break;
            case 2:
                if (dependencyArgs.foo == null) throw ArgumentNullException(nameof(dependencyArgs.foo));
                this.foo = dependencyArgs.foo;
                this.bar = dependencyArgs.bar;
                break;
            default:
                throw new InvalidOperationException("Invalid or uninitialized dependencyArgs");
        }
    }
}

sealed class DerivedService : BaseService
{
    public DerivedService(BaseService.DependencyArgs baseDependencyArgs, Baz baz) : base(baseDependencyArgs)
    {
        this.foo = baseDependencyArgs.foo;
        this.baz = baz;
    }
}

[jnm2]

@HaloFour

Well, let's just say that I'd raise an eyebrow at the notion of parameter lists shifting in the manner described. If I were to be manually adding a new parameter as a breaking change I would still most likely want to add it to the end of the parameter list and not stick it into the middle. The "rest" always seems to belong at the end.

On the other hand, I'd prefer not to have [baseparam1][baseparam2][myparam][baseparam3] and I might like [baseparam1][baseparam2][baseparam3][myparam]. Let's leave that up to the derived class author who can easily put it at the end if that's what they want.

I see. That seems to make some assumptions as to how the IoC would work and would be configured, as the DependencyArgs struct itself becomes a dependency.

As I said in the OP:

With an IoC container, you'd register all nested types named DependencyArgs so that the container automatically creates and injects the args and everything works.

[jnm2]

Putting a <ctorId> in the DependencyArgs is weird but interesting because it means you don't have constructor explosions when there are four base constructors you need to expose and then you need two versions of each in the derived class for some reason.

[HaloFour]

@jnm2

With an IoC container, you'd register all nested types named DependencyArgs so that the container automatically creates and injects the args and everything works.

Which I don't think conflicts with my statement that this makes assumptions as to how IoC containers work. For a simpler framework that would mean going through and registering each of these nested types. For someone not using a container it means having to manage all of these intermediate dependencies manually. Maybe that's acceptable. At least then you don't have to worry about shifting parameters.

In the past I've managed some of this dependency explosion through factory dependency support in Ninject, which is very similar to the proposal above. I wonder how much of that could be driven through source generators rather than relying on language enhancements.

[jnm2]

@HaloFour

Which I don't think conflicts with my statement that this makes assumptions as to how IoC containers work. For a simpler framework that would mean going through and registering each of these nested types.

Okay. I would expect that if this became a language feature, IoC containers would eventually support this out of the box. In the meantime if a container has very weak registration features, maybe program against them, send them a PR, or... find a better container. :')

For someone not using a container it means having to manage all of these intermediate dependencies manually. Maybe that's acceptable. At least then you don't have to worry about shifting parameters.

I didn't specify this, but I expected call sites to be sugared as well. Thanks for bringing it up.
(For those who are skimming this issue, this in the context of "struct sugar" style.)

new BaseService(foo);
new DerivedService(foo, baz);

class BaseService
{
    public BaseService(encapsulateparams(Foo foo))
    {
        if (foo == null) throw ArgumentNullException(nameof(foo));
        this.foo = foo;
    }
}

sealed class DerivedService : BaseService
{
    public DerivedService(baseparams, Baz baz) : base(baseparams)
    {
        if (baseparams.foo == null) throw ArgumentNullException(nameof(baseparams.foo));
        this.foo = baseparams.foo;
        this.baz = baz;
    }
}

Desugars to:

new BaseService(new BaseService.DependencyArgs(foo));
new DerivedService(new BaseService.DependencyArgs(foo), baz);

If you're composing manually, you'd be able to use the desugared version if you prefer the clarity of grouping, plus back-compat when you upgrade the language version while referencing a library that uses this feature. Old versions of C# would only be able to use the desugared versions.

[iam3yal]

@jnm2

When there are multiple base constructors, there is no way to disambiguate without specifying the signature. Specifying the signature removes any benefit to the syntax sugar. You may as well just list the base parameters as usual. With the "rest parameter" style you'd only be able to use this if there was a single base constructor.

Well, it might be possible to do baseparams([type, type, type...,]) without repeating the names of the parameters so I don't know whether it removes any benefit.

Maybe we can aim for a general feature, something like this, selectparams(method[, type, type, type...); just thinking out loud.

[RandyBuchholz]

I like the idea. I think it is worth having even if you have to go manual with multiple constructors. The MVC DI container only handles a single constructor, so there is a whole set of the ecosystem where that really isn't a(n additional) problem.

What about declaring the use of base where you declare the inheritance? You could even select the one you want to auto if you have multiple by type list.

class DerivedService : BaseService()

class DerivedService : BaseService(type, type) 

This saves having to add a constructor all of the time. Since you are pulling these (the DI) from below you wouldn't want to touch them. This keeps them out of the way. You would need to be able to have a additive constructor approach.

class DerivedService : BaseService(DiType, DiType) {
  DerivedService() : base(NonDiType)
}

Would call BaseService(DiType, DiType, NonDiType).

[jnm2]

@gafter Should I move this to csharplang?

[iam3yal]

@jnm2 Why not? but because it's your own proposal I think that you should rewrite it based on this template and maybe change things based on the feedback you got here as opposed to just copy/paste it.

[jnm2]

@eyalsk @gafter I've also got a really tempting ZenHub Move Issue button right here that would take the conversation over. Opinions?

[iam3yal]

@jnm2 Well, I guess you can do that as long as you feel that there's nothing to add and it's fine as is even though I'd say that you have an "opportunity" here to rewrite it based on the feedback. 😉

[jnm2]

Closing due to inactivity.