Support for new .net 8.0 features

[jods4]

This branch adds support for new .net 8.0 DI features, to support https://github.com/ipjohnson/Grace.DependencyInjection.Extensions/pull/36

Required features:

• [x] Adapters for attributes that don't implement IImportAttribute, such as [FromKeyedServices].
• [x] Registering keyed exports for "any key".
• [x] Importing service key.

I'll write implementation notes in comment below as I go.

Things worth following-up:

• [ ] Check caching of compiled delegates. Locate only caches non-keyed ones. Digging the call graph, some end up being cached lower down the stack but I don't think all do: LocateEnumerableStrategy, Scoped<>, CompiledWrapperStrategy.

[jods4]

I've done the adapters, with some noteworthy implementation details:

[!IMPORTANT] I have opted for a static adapter dictionary. It's meant to be initialized once at application startup (for DependencyInjection.Extensions I'm gonna do it from a static ctor). This keeps things much simpler and lightweight than adding state into IInjectionScope and a lot of indirections. I don't really see the case for configuring multiple containers with different attributes/adapters. In fact, MS DI extension is the main consumer of this, and might well remain the only one: I don't think many users will make use of this and I wouldn't advertise it much. Because it's not thread safe, it also means nobody should configure this while already locating services on other threads.

[!WARNING] I have turned MethodParameterInfo.LocateKey into an object (was string). This was just inconsistent with everything else, I don't know why it was this way. It is a breaking change if someone was using an object as a key for an [Import] parameter of a method, and rely on the ToString() conversion to a string key. This seems highly unlikely. On the plus side, it's an improvement because this oddity made it impossible to inject a non-string key into an [Import] method.

[!WARNING] In ActivationStrategyAttributeProcessor.ProcessMethods, I'm passing null as the activation type to ProvideImportInfos. This is for consistency with all other places where [Import] is considered on methods. This was the only place were the activated type was passed instead.

[jods4]

@ipjohnson can I enable the latest C# 12 compiler in csproj? would your appveyor CI build support it?

I'm sticking to the current csproj for now but the code would benefit from target typing, better pattern matching, or collection literals.

[ipjohnson]

I'm all for enabling the new features. Looks like .net 8 sdk is included so I'd imagine it works.

[jods4]

ok, great!

I have a massive (unfortunately) patch coming up for Locate Key injection. I'll stick to the current SDK for this commit and I'll make a separate commit after that to upgrade the SDK.

[jods4]

I've done an implementation of key injection. Sadly, this turned out to be a massive change. The problem is that currently Grace does not propagate the key into compiled strategies. The strategy is cached based on export key, but then the key is nowhere found inside the activation delegate.

One option could be to "bake in" the key inside the compiled delegate, but it would not work with the next feature "AnyKey" exports. The key has to be dynamic, so it must be passed into ActivationStrategyDelegate. As this delegate is core to all strategies compilation, adding key had a huge domino effect on the whole codebase.

[!NOTE] It helps to keep in mind that top-level requests and nested requests work slightly differently: The top level request object always has a null LocateKey, which is fitting because with upcoming AnyKey exports the located key is actually unknown at compilation time. Dependent requests are compiled with a constant key, which is indicated in LocateKey member. For this reason, compiling the activation delegate must handle two different cases: If the key of the Root request is injected, then a constant KeyParameter that is the newly added ActivationStrategyDelegate parameter is used. If the key of a dependent requests is injected, then a ConstantExpression is built with the LocateKey.

To keep internal structure changes minimal, I've decided to encode the concept of "injecting the import key" by introducing a well-known singleton key: ImportKey.Key.

Any type can be imported as keyed by ImportKey.Key and it is always fulfilled:

• If the parent request is keyed and the key has a compatible type, then that key value is the result of activation;
• If there is no parent request or the parent request is not keyed: default value of target type is the result of activation;
• Likewise if the parent key type cannot be assigned to activated type: default value is injected.

As a result the new public API surface is pretty small:

• ImportKey.Key is a singleton used to identify "imported key" injection;
• ImportKeyAttribute can be used for attribute-based configuration: [ImportKey] behaves like [Import(Key = ImportKey.Key)]. Note that the latter actually doesn't compile, as CLR wants all attributes parameters to be constant, which object instances are not;
• Arg.ImportKey<T>() is a convenience that works like Arg.Import<T>(ImportKey.Key);
• LocateWithImportKey() is a convenience fluent API that works like LocateWithKey(ImportKey.Key).

There's one part of this PR that I'm not really satisfied with, but I couldn't find any better way. It's for dynamic strategies, please take a careful look at this: https://github.com/ipjohnson/Grace/pull/314/commits/81b086ef9191e81ca46f7974b58725f66d7a735e#diff-53219a75f4a18ed35629d49fa3559693bb7f0ae284d0915f15febf2e6f2fd214R114-R117

I was writing tests to cover more wrapped strategies, like lifestyles, Meta, Lazy, Func, IEnumerable and more but it seems like they don't support keyed imports? @ipjohnson did I miss something or is Locate<IEnumerable<T>>(withKey: "key") is not supposed to work? If so I think that it would be nice to make these work... I have 3 failing tests at the moment (for Meta, Lazy and IEnumerable). If we make them work, we'd need to add more (e.g. Owned, Scoped) and do a mega-test (e.g. IEnumerable<Func<Meta<Owned<T>>>>).

[jods4]

I have opted into the latest C# compiler version with <LangVersion>latest</LangVersion>. If AppVeyor has .net 8.0 SDK, that would be C# 12.

I took this opportunity to introduce a global Directory.build.props file to share common csproj properties accross all projects, so that things like compiler version and target frameworks can be defined in a single place.

The build is fixed (failure was myself carelessly using a C# 9 feature, which didn't work when compiling for old fx as the default C# version depends on target fx).

There are 3 (new) tests failing, it's related to Meta, Lazy and IEnumerable as explained in my previous comment.

[ipjohnson]

I was writing tests to cover more wrapped strategies, like lifestyles, Meta, Lazy, Func, IEnumerable and more but it seems like they don't support keyed imports?

I agree this should work and agree it should be fixed.

As for the dynamic strategies that's not a horrible solution, dynamic strategies in and of themselves are a bit hacky.

[jods4]

I agree this should work and agree it should be fixed.

Do you know how, and have the time to do it? If not, I'll try to take a look.

In the meantime I intend on tackling the "any key" feature, shouldn't be too hard.

[jods4]

Progress on ImportKey.Any.

New public API surface is small:

• ImportKey.Any is a well-known object to be used at registration time. It will satisfy any key when Located.
• [ExportAnyKeyedType(T)] is a convenience for [ExportKeyedType(T, ImportKey.Any)], except of course the latter doesn't compile because const parameters in attributes.

Lifestyles are sill WIP (I have one failing test for them). The issue is that for lifestyle purposes, Any-keyed instances must be handled as per-key during Locate. I need to look how to add this idea higher in the stack, as I don't want to introduce such logic in every lifestyle (could be singleton, singleton per scope, per request, or even user-provided?!).

@ipjohnson several points worth your attention:

• In InjectionScope, I had to twist the resolution logic a bit because I want to look up the parents for the specific key first, before looking for Any starting from the children again. The refactored code skips LocateFromChildScope in this situation, which ends up skipping a bunch of interfaces/virtual calls that were only implemented once anyway. Everything should work as before, but mostly because there's only one implementation and no crazy alternative IInjectionScope implementation. I think in this context it makes sense, see https://github.com/ipjohnson/Grace/pull/314/commits/fb66a333b3aaf0f4d178448dac179f31bb176618#diff-9b92085e0d8360eabbbb8d31d0c0f42f4d9865db2de0cfe17bb3e2803c25f411L635-L653.
• In InjectionScope still, right after that (see https://github.com/ipjohnson/Grace/pull/314/commits/fb66a333b3aaf0f4d178448dac179f31bb176618#diff-9b92085e0d8360eabbbb8d31d0c0f42f4d9865db2de0cfe17bb3e2803c25f411L654-L663) I kept the fact that AllowInjectionScopeLocator and IInjectionContextProvider were looked for at the highest (root) level. I'm not sure if this is important or not. If it can be down at the child level as well, then we can get rid of the new GetRootScopeConfiguration (see https://github.com/ipjohnson/Grace/pull/314/commits/fb66a333b3aaf0f4d178448dac179f31bb176618#diff-9b92085e0d8360eabbbb8d31d0c0f42f4d9865db2de0cfe17bb3e2803c25f411R673-R683)
• I created a test for LocateAll but it's skipped for now, as Grace doesn't support keyed IEnumerable. This is definitely a requirement if we want to be compatible with MS specifications. We're supposed to return both the exact keys and Any keys.
• I left a TODO in ArrayExpressionCreator. I adapted the code to support Any as LocateKey, but clearly the key is lost in the resolution process, so I assume ImportKey.Key would not work, but I'm not sure where this comes into play. Maybe this is linked to the lack of keyed LocateAll support? See https://github.com/ipjohnson/Grace/pull/314/commits/fb66a333b3aaf0f4d178448dac179f31bb176618#diff-108e83768dc3c38bf875d8010ae790e7df1ebef150f334aa955290585fb58e9eR275
• I left a similar TODO in WrapperExpressionCreator, for similar reason. I adapted code but clearly the key is lost in the process. Likely to be linked to the lack of support for keyed wrappers? See https://github.com/ipjohnson/Grace/pull/314/commits/fb66a333b3aaf0f4d178448dac179f31bb176618#diff-2439334b679e2407162056ed5a8943bb21a9af036e7fdd2e729082ebeb440a1aR168

[jods4]

@ipjohnson Would love your opinion how you would implement the Lifestyles, it's tricky. As of now I don't see an "easy" way to fix this.

Also I added a failing test: when locating (top-level) a singleton, the key is lost. This is because at the top-level the key is not in request.LocateKey: it's supposed to be compiled into a delegate parameter instead. So SingletonLifestyle.ProvideLifestyleExpression has no way of knowing what the key actually is.

As long as the located instance doesn't inject ImportKey that has no consequence, but if it does then it would need to work like DeferredSingletonLifestyle.

I haven't fixed this, because it is closely related to the problem of Any keyed registrations, which I'm not sure how we want to address.

[ipjohnson]

I must admit I missed the nuance of the [ServiceKey] attribute combined with the any key when this was first pitched :(

It very much seems like the key is viral and needs to be carried though the lifestyle. Is it possible to treat it like DeferredSingletonLifestyle for this specific scenario (maybe a new lifestyle type) as it's pretty rare?

[jods4]

It's still gonna be a complex change.

For regular lifestyles, ProvideLifestyleExpression "compiles" an expression and it can depend on the resolve key, which -- depending on situation -- is either a ConstantExpression that contains request.LocatedKey or a ParameterExpression that I added to the activation delegate.

Based on this we could inject some "per-key" lifestyle logic but one issue is that I'm not sure where to store state (it'd need an immutable hashmap based on key, with a cloned lifestyle for each), short of adding that optional hashmap to all lifestyles. At this point I'm thinking: let's go full breaking change and introduce an abstract base CompiledLifestyle class.

This does not solve everything, though. (I'm writing as I'm thinking here:)

• Unless we want to pay for the immutable hashmap indirection for every keyed import (when 90% would NOT be based off Any), we need to know if we're locating an Any registration or not... and this info is not available. The key value discussed above is the located key, not the registered one.
• I need to look out for a way to pass that info into ProvideLifestyleExpression but it might get messy as it's called in multiple places in deep call stacks where the information is long lost 😭
• A middle ground is to have a fast path when it's always the same key, e.g. associate the lifestyle instance with the first key, then only go to hashmap for an alternate instance when encountering a different key. There's still a cost to pay for every locate, but it's much lower. The benefit is that the changes to the codebase are much simpler.

Then there's the case of Singleton. The problematic is similar: we need to know when we're dealing with Any, and when we do we should swap in a DeferredSingleton.

Things would be simpler if we could more easily relate a lifestyle with a registered key. Design-wise, the fact that a single registration could be for multiple types/keys/names is a bit at odds with the way Any should work.

There's one last point that's unclear to me: is this behavior specific to when ServiceKey is involved or for all Any registrations? I'm working under the assumption that it's for all Any registration. On one hand they would all be exactly the same without ServiceKey, but on the other: why use Any then?

[jods4]

One question: is there a specific point in code where all registrations are "frozen" and processed, before anything is located, and after which nothing can be modified anymore?

That would be a solution as we could look for Any registrations, split them off and wrap their lifestyle with a "per-key" lifestyle (unfortunate naming as SingletonPerKeyLifestyle exists and means something else).

[jods4]

I coded a solution for the lifetime-per-key behavior of ImportKey.Any.

@ipjohnson can you carefully review commit https://github.com/ipjohnson/Grace/pull/314/commits/7f13aec150011140dab48a9a54081856ae02db3b ? it's not big.

• I found a point to intercept all registrations when they're added to container. At which level exactly the if was added is debatable.
• I created a new wrapper AnyKeyLifestyle that wraps other lifestyles when the target is possibly registered as Any key.
• It contains a bit of lock-free concurrent code, nothing too fancy it should be safe. Please review.
• It's efficient for non-root requests, as there's no dynamic key there.
• For root, I'm holding onto requests to be able to compile a delegate just in time based on key. I'm not sure if this is ok for all Grace features?

Somehow I noticed that all lifestyles basically compile and cache an ActivationDelegate, that is then invoke in conjunction with the caching/re-use strategy. I feel like if we introduced a breaking change in the ICompiledLifestyle interface, we could take this to our advantage and not hold onto those requests object. Immediately compile the ActivationDelegate then pass it to an new ICompiledLifestyle method?

[jods4]

Fixed the Singleton lifecycle as well, by merging DeferredSingletonLifecycle and SingletonLifecycle into a single class, it doesn't really make SingletonLifecycle heavier (and there's less code overall).

Kept DeferredSingletonLifecycle as a subclass for compat. We could make the new ctor SingletonLifecycle(bool deferred) public, I did not.

[jods4]

Great news as this should be everything for .net 8 (phew that was a lot more than I expected 😩). What remains is the limitation that Grace doesn't support keyed wrappers, and most importantly (for MS DI test suite) keyed enumerations.

[ipjohnson]

I feel like if we introduced a breaking change in the ICompiledLifestyle interface, we could take this to our advantage and not hold onto those requests object. Immediately compile the ActivationDelegate then pass it to an new ICompiledLifestyle method?

What would the signature change look like?

[jods4]

What would the signature change look like?

Not sure! I didn't spend too much time thinking about it as I went for the fully compatible solution first.

[jods4]

MS keyed DI specifications led to me to find a bug/limitation in the "Best Match" ctor selection: it was not considering keys import. I have fixed this in be118fc. I also did a few minor shortcuts/optimizations in related code.

Note that for .net fx 4.6.2 I added a new dependency: PolySharp This is so that we can use new C# features in the code base even when compiling for old .net fx. It simply adds a few missing System types/attributes that C# wants. This dependency is absent from .net 6+ compilaton.

@ipjohnson Note that the Dynamic construtor selection suffers from the same lack of keyed imports support. For this, we would need to create a similar fix in DynamicConstructorExpressionCreator methods DynamicLocate and DynamicCanLocate.

[jods4]

Added support for injecting keyed service (and hence, service key itself) into factories, as this is required by MS DI AddExportFactory((scope, key) => ..).

This is done by supporting attributes on parameters of DelegateWrapperStrategy.

Would be a perfect opportunity of removing the hacky bits for Dynamic injection, but sadly I don't think it's possible to add attributes on a ParameterExpression :(

So now we can register things like

c.ExportFactory(
  ([Import(Key = "A")] ISimpleObject objA) => new ImportSingleSimpleObject(objA));
  
c.ExportFactory(([ImportKey] string key) => $"Key is {key}")
  .AsKeyed<string>(ImportKey.Any);

[jods4]

@ipjohnson I've got a question for you.

I'm working on key support in LocateAll. There's a part of ArrayExpressionCreator that I don't know the intent of, and which potentially can block me.

Look at GetActivationResultsFromStrategies: https://github.com/ipjohnson/Grace/blob/master/src/Grace/DependencyInjection/Impl/Expressions/ArrayExpressionCreator.cs#L139-L152

There's code that checks if LocateKey is enumerable. If not then the array will contain one activation result, for keyed LocateKey. If so, then the array will contain one activation per key in the enumerable.

Where does the latter comes into play? I'm even surprised there's keyed code in arrays as LocateAll does not take withKey parameter in v7?

I need to modify this code but depending on what uses it today it might be breaking (?).

Basically when request an array with key K I'm gonna return potentially multiple Ks and also multiple Anys activations: K in v7 becomes [..K, ..Any(K)] in v8. Is it ok to turn [A, B, C] into [..A, ..Any(A), ..B, ..Any(B), ..C, ..Any(C)] ? What is this for?

[ipjohnson]

I'll be honest I don't totally remember the use case but it's possible related to locating an enumerable through the Locate and providing an array of keys.

Is it ok to turn [A, B, C] into [..A, ..Any(A), ..B, ..Any(B), ..C, ..Any(C)] ? What is this for?

This seems reasonable given the requirements and wanting to be backwards compatible.

[jods4]

So we retro-specify that Locate<IEnumerable<T>>(withKey: [1, 2, 3]) is supposed to return all keyed exports for all provided keys? I'll double check how Locate<IEnumerable> work with keys, I thought it wasn't working but I might be wrong.

[jods4]

@ipjohnson I did a few test. Locate<IEnumerable>() does not support keys: neither a single one, nor an array of keys. It ignores the withKey parameter and returns all keyless registrations.

Then I figured I should remove the code and run all tests. 2 tests failed, apparently injecting IEnumerable<T> with one or several keys does work in ctors declared with fluent API WithCtorParam().LocateWithKey(). It's oddly specific, here's the most straightforward test case:

public void Container_IEnumerable_Locate_Key()
{
    // ...
    container.Configure(c =>
    {
        c.Export<SimpleCompositePattern1>().AsKeyed<ICompositePattern>(1);
        c.Export<SimpleCompositePattern2>().AsKeyed<ICompositePattern>(2);
        c.Export<CompositePattern>()
            .As<ICompositePattern>()
            .WithCtorParam<IEnumerable<ICompositePattern>>()
            .LocateWithKey(new[] { 1, 2 });
    });

    var value = container.Locate<ICompositePattern>();
    // [SimpleCompositePattern1, SimpleCompositePattern2] was injected
    // ...
}

If we want to keep compatibility with this, I suppose LocateAll and all variations of IEnumerable should support locating all keys in an enumerable key argument.

[ipjohnson]

That makes sense, I remembered righting the code and running into the issue where string was being enumerated incorrectly but i couldn't remember the configuration method that lead to it. Honestly so many entry points between scanning, fluent, and injection extensions.

The feature was added in 12/2016 so I don't remember the specifics of the feature :|

[jods4]

@ipjohnson Is ActivationStrategyCollection supposed to be thread safe?

I assume it was meant to, as it internally uses ImmutableArray and ImmutableHashTree. Otherwise, if the type was supposed to be frozen after creation, regular array and dictionary would simply be more efficient.

But then as I was looking at its only mutating method: AddStrategy, I immediately noticed that it wasn't thread-safe at all.

Should this be made more thread-safe, or could we use simpler collections?

[jods4]

I pushed my progress on keyed IEnumerable so far. There are already quite a few trade-offs and design choices here:

Multiple registrations for the same key are now possible.

• As before, these keyed exports are basic compared to non-keyed: there's no support for Conditions.
• To preserve as much back-compat' as possible, I give highest priority to the last registered export (as today keyed exports replace existing ones).

LocateAll works with keys.

• As a consequence of previous point, by default exports are returned in reverse export order (last one comes first);
• ImportKey.Any are also returned, by default after matching keys.

IEnumerable<T> and T[] can be imported (non-root).

• The old weird trick of enumerable keys is supported here (but I did not port it to LocateAll).
• I kept the existing ordering logic, which for keyed is different from LocateAll: by Priority first, then ExportOrder.

This is WIP:

• Strategies for other collections IList, etc. Should be simple.
• Missing support for Root requests Locate<T[]> (and IEnumerable). Root just works differently for keys.

Regarding that last point, I'd like some input from you @ipjohnson: Am I missing something or are keyed Locate not cached and compiled every time? That seems quite wasteful? It seems to me the flow is:

• InjectionScope.Locate() then, when withKey != null:
• InjectionScope.InternalLocate()
• InjectionScope.InternalTryLocateActivationDelegate()
• ActivationStrategyCompiler.FindDelegate() but as key != null the resulting compiled delegate won't be cached.
• (internally FindDelegate() will call LocateEnumerableStrategy, which will create and compile an expression on the fly.

[jods4]

Added support for keyed wrappers. They seem to work but I need to add more tests.

[!WARNING] A previous contributor really went a long way to be able to make an Option<T> custom wrapper work with keyed types. I removed the base class he introduced for that as it isn't required anymore (all wrappers work with keyed type) and wasn't well-aligned with the new design. This is a breaking change! His tests are still there and I made all of them pass, but I changed the approach for OptionalWrapperStrategy (provides Option<T>) very significantly. It's IMHO much simpler now, but also much different.

There's an improvement opportunity here, that would make this kind of registration much more simple, if we could register a factory for wrappers, not just ctor with ExportWrapper(typeof(Option<>)).

// Argument to ExportWrapperFactory() has to be loosely typed as LambdaExpression, so we need explicit typing :(
Expression<Func<T, Option<T>> factory = x => x == null ? Option.None<T> : Option.Some(x);
c.ExportWrapperFactory(typeof(Option<>), factory);

It wouldn't even be difficult to add, but I'm focused on .net 8 right now, so I'm leaving this idea aside for now.

[jods4]

Locate<IEnumerable<T>> and Locate<T[]> now work.

I decided to revert the discrepancy in order between LocateAll and IEnumerable: without an explicit sort order, results are always in export order, with "any key" after "requested key".

Last thing I need to do is to fix the alternate collection strategies (IList, etc.) but I took a big decision :( With wrappers and IEnumerable and then alternate collections, it very apparent that we should pass the located key in requests even for Root requests. I've worked around this so far by creating overloads that take an explicit key in addition to the request, but it's getting too convoluted.

So when I find courage, I'll always include request.LocateKey. The only difference in behavior is that for Root, the key must not be baked in compiled code (as ImportKey.Any delegates take a dynamic key) and use a parameter instead.

With IEnumerable in the box, there are only 3 failures remaining in MS DI test suite. I don't think we miss any feature for MS DI anymore, I need to debug those failures to check out what's wrong.

[stephenlautier]

This would fix the following error i suppose?

As we are trying to update to .net8 (and we use Orleans) and currently stuck with this error 😢