Deprecating Resolve(Type, IDictionary) in favor of IReadOnlyDictionary<string, object>

[jnm2]

Right now it's a bit of a pain to use the Resolve method because what I'll have in hand is an IReadOnlyDictionary<string, object>, usually from a method parameter.

Please consider adding this method and removing the nongeneric overload.

The nongeneric one would be a problem. So long as it's there and you pass it a type like Dictionary<string, object> which implements both interfaces, which all of the BCL dictionaries do, you'll have to disambiguate the overload by explicitly casting to one or the other interface type.

This would be a breaking change for people still using non-generic dictionaries like Hashtable, and there may be a few, so what if you removed it in v5?

[jonorossi]

What about types that don't implement IReadOnlyDictionary<K,V> but do implement IDictionary<K,V>, mostly those not in the BCL? Not sure if this is common but just the thought.

A ReadOnlyDictionary<K,V> implements IDictionary<K,V>, but obviously IReadOnlyDictionary<K,V> does not inherit from IDictionary<K,V>.

We can't use two overloads because Dictionary<K,V> implements both IDictionary<K,V> and IReadOnlyDictionary<K,V>.

[jnm2]

Right, we can't overload. I was figuring that IReadOnlyDictionary<,> is the lowest common denominator of all modern dictionaries since 2012. I'm pretty certain IDictionary<,> implementations will also choose to implement IReadOnlyDictionary<,>, but not all IReadOnlyDictionary<,> implementations will implement IDictionary<,>. Seems logical to me but I could be wrong about the stats.

[jonorossi]

Something like C5 collections still doesn't look like it implements IReadOnlyDictionary<,> since the interface only came with .NET 4.5. I'm sure my brain is still wired to care too much about back compat, we need to move things forward and this sounds like a reasonable change. People are really only using C5 collections (like a red black tree) in performance critical code where they are at micro optimisation level and that specific internal structure performances better. This API isn't one of those.

I'll assign it to v5.

[jnm2]

I guess the real driver for this is that I had to write an IDictionary adapter because I pass everything around as IReadOnlyDictionary<,> and didn't want the IEnumerable<KeyValuePair<string, object>>.ToDictionary extension method.

Another question though is net40 support and lower, so maybe this will never be possible.

[jonorossi]

Another question though is net40 support and lower, so maybe this will never be possible.

Windsor dropped support for versions before .NET 4.5 in Windsor 4, only Castle Core supports versions before .NET 4.5.

[ghost]

@jnm2 - I have added this to the v5 discussion and placed a bullet point in the description.

I have a question though, how deep would we like this to go?

I am assuming we are talking about the resolve method in the kernel here.

The problem I can see right now, is this eventually get's passed down to a resolve all method, ultimately leading to the newing up of a CreationContext. It then becomes part of the publicly accessible property AdditionalArguments on the CreationContext. You can see there is a link to Arguments. This is interesting because after investigating Arguments I can see it is interchangeable with IDictionary via the interface implementation.

All good so far but ... if this is immutable however ... you might end up with a blood bath.

My quick test here, was to remove the indexer setter on Arguments in favour of throwing an exception to see how bad this really would be and to re-run the tests.

I ended up with:

Would just like to know thoughts around scoping the problem. Happy to provide more info to clarify.

[ghost]

I think the CreationContext should be immutable period. The fact that it is not presents a big problem with resolution determinism in the container as it stands today.

[jnm2]

One thing you could do to scope the problem is copy on first write so that you aren't allocating a new hashtable unnecessarily but you also aren't modifying the dictionary someone passed into a Resolve method.

[ghost]

Exactly! We want immutability.

This problem tends to manifest itself heavily within the CreationContext where that dictionary is mutable throughout the resolve process(in fact the CreationContext get's recreated sometimes, god!). I was just pondering how far we would like to take this. You are suggesting an "outside-in" safe guard via the public API. This is fairly easy to do and it does not stop anyone from rewriting this dictionary with a single linq statement in your code base.

I guess what I am asking you to consider is that this problem endures all the way into the bowls of the MicroKernel. What are your thoughts on trying to fix that?

I think you raised a big problem in the first instance to do with resolution determinism here. In some cases(esp with open generic implementations) this is not always possible but what about fixing the stuff we can fix?

[jnm2]

That's something you'll have to decide as maintainers as you go along, but an iterative approach might be better than a boil-the-ocean approach. For the moment I'm only concerned with whether I can pass an instance known statically as only IReadOnlyDictionary<string, object> which won't be mutated if does happen to implement IDictionary or IDictionary<,> at runtime.

[ghost]

That's something you'll have to decide as maintainers as you go along, but an iterative approach might be better than a boil-the-ocean approach.

Fair enough, I do believe the ocean is already boiling. We are just hacking below the steam.

For the moment I'm only concerned with whether I can pass an instance known statically as only IReadOnlyDictionary<string, object> which won't be mutated if does happen to implement IDictionary or IDictionary<,> at runtime.

I guess what I am trying to tell you here, is it will get mutated anyway. Windsor expects this.

[jnm2]

@Fir3pho3nixx I mean I don't care if a copy is mutated by Windsor, but it shouldn't be mutating the dictionary instance I pass to Resolve because it may collide with my logic between calls.

[jonorossi]

@Fir3pho3nixx thanks for looking into this more, I had just thought we'd do as @jnm2 suggested and implement it at the API surface level but this is something we'll definitely need to look at more.

If Windsor does mutate the user's dictionary then we could probably implement copy-on-write using the cheshire cat pattern or something similar so the Windsor internals are none the wiser that they were even passed a readonly dictionary.

However, we've already got copy-on-write with the overload that takes an anonymous type which Windsor obviously can't modify, Windsor uses the ReflectionBasedDictionaryAdapter to read the contents and methods like Add throw a NotImplementedException, maybe some consolidation of code paths:

object Resolve(Type service, object argumentsAsAnonymousType);

[ghost]

@jnm2 - Totally got you on that, don't worry I really did not take offense at all. I am just looking at this from an 'internals' point of view. No harm done at all. 👍

@jonorossi - Loving that name "Cheshire the cat" pattern! I did come across the method you mentioned but I think my brain was in immutability mode so I did not pay anymore attention.

I have left this in the V5 deprecation issue and wont be taking it out. We are still good to go.

[jnm2]

I also wanted to add this for reference: https://github.com/dotnet/corefx/issues/17917#issuecomment-298706121

[jnm2]

@Fir3pho3nixx We're good, I see no sign of offense either direction. =)

[jonorossi]

I also wanted to add this for reference: dotnet/corefx#17917 (comment)

Good to know they are looking at making IDictionary<,> inherit IReadOnlyDictionary<,>.

[ghost]

I know we have implemented this, but stuff like this spooks the crap out of me. Happy to let it go for now. I have a stinking feeling we are not done here!

[ghost]

Re-opening this while we discuss this further on #420.

[jonorossi]

With 8 Resolve and 4 ResolveAll methods on IWindsorContainer accepting either an IDictionary or an object I'm trying to get my head around what is being proposed:

In #420 it sounds like you want to do the following (overloads omitted for brevity):

• Remove T Resolve<T>(IDictionary arguments)
• Add T Resolve<T>(IReadOnlyDictionary<string, object> arguments)
• Remove T Resolve<T>(object argumentsAsAnonymousType)

If you remove the anonymous type one that you wanted to catch IDictionarys (including Hashtable and wherever people have explicitly used IDictionary) then what supports them?

Looking at Autofac they've got a Parameter type which Resolve takes an IEnumerable<Parameter> and they've got at least named, type, and Func<T>-based parameters (https://github.com/autofac/Autofac/blob/master/src/Autofac/ResolutionExtensions.cs). This feels a bit more like our fluent registration API, and actually with Autofac those same Parameter types can be used with its registration API or resolution API.

[ghost]

What we are saying is we should deprecate support for data structures with mutable API’s until we can address the underlying mutation problem.

This means not supporting hashtable and dictionary on Resolve at the WindsorContainer level and obsolescence for the doppelgänger API’s in the MicroKernel. For now of course.

We need a new issue to track why the mutations are happening in the first place.

Your idea of introducing a separate type is a great one. I think you proposed using Arguments for this purpose and then doing implicit conversions. The problem I have with this is that although we have reduced the surface level API bloat here, Arguments might end up becoming this weird esoteric thing that does too much type conversion.

Happy to clarify anything I have said as I typed this out on my mobile while commuting.

[jonorossi]

What we are saying is we should deprecate support for data structures with mutable API’s until we can address the underlying mutation problem.

If this is the problem you want to solve the proposed solution does way more than this. You are already proposing to clone the map of whatever is passed to it irrespective of the proposed public API change, cloning would solve this.

The proposed API change itself without the cloning does not solve this problem because there is no semantic difference between IDictionary and IReadOnlyDictionary with respect to immutability, since IReadOnlyDictionary does not mean it is an immutable dictionary just one that you (the receiver of the reference) can't change (i.e. a read-only view), this is why corefx also has a IImmutableDictionary<K,V> interface. I've mentioned the IsReadOnly property on both these interfaces but I don't think anyone actually makes their maps immutable.

I understand that @jnm2 wants to pass in IReadOnlyDictionarys, but I'm still missing the argument for this change other than just that someone is requesting this feature. I don't mind the request, but I fail to see how these 2 problems are related.

[ghost]

If this is the problem you want to solve the proposed solution does way more than this. You are already proposing to clone the map of whatever is passed to it irrespective of the proposed public API change, cloning would solve this.

Correct. However we are on the eve of releasing V5. Worth discussing right?

Let's not forget, the PR descended into chaos because I tried to introduce a Resolve(IReadOnlyDictionary) overload and removed the Resolve(IDictionary) overload. This played up hell with the Resolve(object) API. Everything that was IDictionary fell back onto Resolve(object) for all the tests and caused an absolute bloodbath when running them. I decided then that having this general chasm for resolve parameters via the Resolve(object) overload is just a bad idea. My PR attempts to deprecate Resolve(IDictionary) in favour of Resolve(IReadonlyDictionary<string, object>) as this issue title suggests. I committed an unrefined PR to try and demonstrate the type acrobatics(or type conversion) we have signed ourselves up for. The only reason I am flagging this now is because I thought we agreed we could try and do this at the surface API level.

I understand that @jnm2 wants to pass in IReadOnlyDictionarys, but I'm still missing the argument for this change other than just that someone is requesting this feature. I don't mind the request, but I fail to see how these 2 problems are related.

I can happily revert to the copy up front mechanism in the Resolve(IDictionary) overload. I think I am going to raise a separate issue later that describes the pitfalls of Resolve(object) and suggest an API that might be better to work with using stronger typing.

If we are all happy with this, I can amend my PR fairly soon.

[jonorossi]

Correct. However we are on the eve of releasing V5. Worth discussing right?

Definitely, it was just that the justification (mutation) didn't apply to breaking change (public API change).

Let's not forget, the PR descended into chaos because I tried to introduce a Resolve(IReadOnlyDictionary) overload and removed the Resolve(IDictionary) overload

Yep 😢

I decided then that having this general chasm for resolve parameters via the Resolve(object) overload is just a bad idea

Agreed, see my comments: #420 (comment)

The only reason I am flagging this now is because I thought we agreed we could try and do this at the surface API level.

Are we talking about IReadOnlyDictionary here or dictionary mutation?

I can happily revert to the copy up front mechanism in the Resolve(IDictionary) overload. I think I am going to raise a separate issue later that describes the pitfalls of Resolve(object) and suggest an API that might be better to work with using stronger typing.

If we are all happy with this, I can amend my PR fairly soon.

This issue is titled in relation to adding IReadOnlyDictionary, I'd love a new issue about the dictionary mutation problem with some sample code illustrating the problem.

I think the time is probably now to raise that issue about Resolve overloads accepting object for arguments, see my comments (#420 (comment)) on what I'm proposing. I'd love to hear what you are thinking to have in its place.

[jonorossi]

[..] I'd love a new issue about the dictionary mutation problem with some sample code illustrating the problem.

Would love to see the use case for why CreationContext.AdditionalArguments is a mutable dictionary (as I can't see the reason), and what CreationContext.EnsureAdditionalArgumentsWriteable achieves, feels like there is something hacky here.

[ghost]

Would love to see the use case for why CreationContext.AdditionalArguments is a mutable dictionary (as I can't see the reason), and what CreationContext.EnsureAdditionalArgumentsWriteable achieves, feels like there is something hacky here.

Deleted. Please see https://github.com/castleproject/Windsor/pull/420/files#diff-16ccc2c284d39fff9d45a051973cd1c8L354

[ghost]

@jnm2

The extension Arguments.FromReadOnlyDictionary now calls a shallow copy constructor here.

I think this coupled with making Arguments readonly for any side effects when it comes to calling Add, Clear or Remove by setting isReadOnly here gets you what you need.

It is also a tidy way of getting rid of bad methods as it turns out :)

[ghost]

Are we talking about IReadOnlyDictionary here or dictionary mutation?

As it turns out we are talking about methods that apparently enforce mutation semantics privately. You already had a sense of this when you mentioned CreationContext.EnsureAdditionalArgumentsWriteable. If I used the words "mutated dictionaries" I do apologise. I will try to limit my brevity in the future and explain myself better.

Either way, was a great win. Thanks for helping out.