Support for new .net 8.0 features

[jods4]

This branch adds support for new .net 8.0 DI features, to support ipjohnson/Grace.DependencyInjection.Extensions#36

Required features:

• Adapters for attributes that don't implement IImportAttribute, such as [FromKeyedServices].
• Registering keyed exports for "any key".
• 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: 81b086e#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 fb66a33#diff-9b92085e0d8360eabbbb8d31d0c0f42f4d9865db2de0cfe17bb3e2803c25f411L635-L653.
• In InjectionScope still, right after that (see fb66a33#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 fb66a33#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 fb66a33#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 fb66a33#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 7f13aec ? 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.

[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 😢

[jods4]

@stephenlautier Yes, this error indicates that the DI provider (in this case Grace 7.0) does not support MS keyed DI interfaces from .net 8.0, so you can't use them.

(Note that Grace 7.0 can still be used as a DI provider because MS separated the keyed interface and made them "optional" for back-compat. Also Grace has had keyed exports for a long time and they also work fine, just not with MS DI attributes / interfaces.)

And yes, the version from this branch will fix that and provide full MS DI keyed interfaces support.
See also ipjohnson/Grace.DependencyInjection.Extensions#36.

[stephenlautier]

Yes, we do use keyed from Grace (and still does) though the issue Microsoft Orleans 8 uses the new Keyed internally which why it breaks and we don't have control over 😞

Looking forward for this change to get merged ❤️

[jods4]

@stephenlautier working on it, it's almost complete now.

[jods4]

Focused on fixing remaining incompatibilities with MS DI:

CanLocate is a little more accurate now (handles keyed collections and wrappers).
@ipjohnson I'm sure you're aware CanLocate is not a very accurate API, its logic is quite simpler than the actual internal Locate code?

I have also changed a behavior I previously described: when using ImportKey.Key, if the actual key cannot be converted into the target type Grace will throw InvalidOperationException.
Previously I would (arbitrarily) assign a default value, but the MS DI spec mandates this exception (I guess it makes sense: it avoids creating an unusable service as ImportKey.Key is key (pun intended) to services registered as Any).

I did not change the behavior when importing a non-keyed service: ImportKey.Key dependencies are fulfilled with null. This is a debatable edge-case and there's nothing in MS DI specs about it.

Please note that although the MS spec is now 100% passing 🎉, this PR is not yet fully complete. I have a bit of work left on the collection strategies.

[claylaut]

@jods4 would this fix the above issue that @stephenlautier mentioned?

If yes @ipjohnson would it be possible to release a dev (pre-release) package from this branch?

this will help us a lot to continue working on our migration and also we will be testing this feature.

[jods4]

And yes, the version from this branch will fix that and provide full MS DI keyed interfaces support.

👆

[stephenlautier]

@jods4 I downloaded your repo and built them and gave our app a test

During startup Im having the following exception (which is not from our code but from Orleans)

Alto seeing the code its seems "intended" as its marked as required and no default

This is their usage (from Microsoft Orleans)

return serviceProvider.GetServices<NamedService<IGrainDirectory>>() ?? Enumerable.Empty<NamedService<IGrainDirectory>>();

https://github.com/dotnet/orleans/blob/main/src/Orleans.Runtime/Hosting/DirectorySiloBuilderExtensions.cs#L71
So from their end it looks like it should be optional, however somehow its not

This is our config for grace (though tried to comment the Behaviors but still the same)

var graceConfig = new InjectionScopeConfiguration
{
	Behaviors =
	{
		AllowInstanceAndFactoryToReturnNull = true,
	}
};

Not sure if we need to configure something specifically, or its a bug

[stephenlautier]

Some more updates to isolate a bit the issue

I added a very basic test to test similar what they are doing however that succeeds

And i also replicated the issue with a basic orleans sample in order to see the issue (and also isolate from our stuff)

• Clone https://github.com/stephenlautier/dotnet.samples/blob/feature/grace-sample/orleans/Adventure/README.md
• Link Grace projs to your branch
• Simply run AdventureServer

You should get the above exception

[jods4]

@stephenlautier
Reminds me a bit of this: #309
Does it work any better with AutoRegisterUnknown = false?

[stephenlautier]

@stephenlautier Reminds me a bit of this: #309 Does it work any better with AutoRegisterUnknown = false?

Just tried it now, but didnt change anything

[jods4]

@stephenlautier You should probably open an issue separate from this PR to not mix discussions.

It might help if you posted the full LocateException failure log, it's cropped in the screenshot but it should show which precise dependency down the activation process failed.

In your unit test I'm not seeing any registration code and that's probably the key difference: AutoRegisterUnknown can have unpredictable effects at times.
For example, I'm strongly suspecting that Orleans registers NamedService<> as an open-generic type.
With auto-registration I believe the specific NamedService<IHeroesIndex> is registered and the resolution process of those two is subtly different.

[stephenlautier]

@jods4 agreed, will open a separate issue however will link to this as it might be related to this (unsure) as it used to work (pre .net8)

From their end no it looks "simple" were i gave the usage right above it theres the registration which is this one https://github.com/dotnet/orleans/blob/main/src/Orleans.Runtime/Hosting/DirectorySiloBuilderExtensions.cs#L42
snippet below

            collection.AddSingleton(sp => new NamedService<IGrainDirectory>(name, implementationFactory(sp, name)));
            // Check if the grain directory implements ILifecycleParticipant<ISiloLifecycle>
            if (typeof(ILifecycleParticipant<ISiloLifecycle>).IsAssignableFrom(typeof(T)))
            {
                collection.AddSingleton(s => (ILifecycleParticipant<ISiloLifecycle>)s.GetGrainDirectory(name));
            }

I'll try to dig a bit further and open another issue as you suggested. Sorry for hijacking this 🙏

[ipjohnson]

If yes @ipjohnson would it be possible to release a dev (pre-release) package from this branch?

I'm all for releasing a version from this branch. Sorry I've been incommunicado, between home life and work life I have very little time for anything these days.

[jods4]

@ipjohnson I have modified Grace design when it comes to request.LocateKey:

• Previously: this field was only used for internal requests (ctor parameters, properties, etc.). It was always null on Root requests and the likes (Other requests such as wrappers or lifestyles).
• Now: request.LocateKey is always set to the key being resolved.

This helped me simplify the wrappers, collections, and lifestyles code.

Warning

When compiling an export registered as AnyKey, the currently located LocateKey might not be a constant during future invocations.
For this reason, when injecting ImportKey of Root requests (and similar Other requests) I still refer to an extra activation delegate parameter holding the currently located key.
For nested requests (Method, Parameter, Property and similar Other requests), the key is baked-in as a constant.

[jods4]

@ipjohnson It turns out that this change did not fully solve the "non-core" collections case.
(Grace has built-in support for IEnumerable and arrays in its core; and these already work.)

Other collections such as List, ICollection, etc. surprised me a little.

They are implemented as "missing handlers", which means they only work if you have AutoRegisterUnknowns = true. It's the default, but as recently discussed in the MS Orleans bug above, I personally don't fancy this option much. (To be fair: injecting IEnumerable or an array does the job just as well as List<T>, so I can't say I ever noticed this in my projects.)

This led me to notice that missing handlers are never registered as keyed, so it just doesn't work yet -- something more I need to fix.

It's also worth noting that special collections are registered as concrete types: there is no List<> strategy, rather List<T1>, List<T2>, etc. I wonder if they would be better registered as Wrapper strategies, I'll try it out (unless you see a reason to avoid that).

A lofty goal could be to handle collections like C# 12 handles collection expressions:

• Any type that's IEnumerable<T>, has a default ctor, an Add(T) method, optionally AddRange(IEnumerable<T>) could be injected as a collection.
• For special collections like ImmutableList, C# uses [CollectionBuilder] attribute to identify a builder static method taking ReadonlySpan<T>. The issue with this is that it requires recent .net releases (for both the attribute and Span support), so relying on this would break compatibility with older framework versions.

[jods4]

@ipjohnson I have added a new public API to more easily export custom Wrapper strategies: ExportWrapper(ICompiledWrapperStrategy).
It seemed weird to me that users had to go through the IMissingExportStrategyProvider route for this.

Its usage is demonstrated in updated Optional<T> tests, which is simpler now.

[jods4]

In the end, fixing the collection strategies was much easier than expected.
It was a matter of ensuring those strategies declare themselves as keyed exports for ImportKey.Any strategies in addition to the default activation type.

So the change for collections is minimal: they are still added by the default missing strategy provider, as open generic types, but they export themselves as both keyless and keyed for Any, which is convenient to have in this situation.

I added keyed tests for supported collection types, and everything is green.

[jods4]

@ipjohnson At this point, I think I've covered all features of Grace that I could think of with the newly improved keyed support.
MS DI 8.0 test suite is fully passing as well (if you build it against this branch).

You can merge this and publish a new 8.0 pre-release when you're ok with this PR.
Sorry its huge, I didn't know what I was getting into when I started. The new MS specifications really pushed Grace keyed support beyond what it was previously supporting, which impacts the whole codebase. 🙁

There at least one point that I'd like to check with you before making a final release: caching.
If you look at InjectionScope.InternalTryLocateActivationDelegate() you'll see that compiled delegates are only added to the cache if they're keyless.
There are some additional caches in Grace internals, but I found some code paths that compile new delegates every time without caching them. As compilation is extremely expensive, this should be avoided for repeated Locate.

Is there a reason the main cache doesn't use (type, locateKey) as a key, and all compiled delegates are cached?

[jods4]

@ipjohnson I hope you can find a little bit of time to help me move this forward?

[stephenlautier]

@ipjohnson it would be nice if you can find some time to get this merged, so we can use stable version from the main repo. Thanks 🙏