Deprecating `Resolve(Type, IDictionary)` in favour of `Resolve(Type, IReadOnlyDictionary<string, object>)` #420

ghost · 2018-07-14T15:50:51Z

Resolves: #346

This was implemented at the upper most surface API's for IWindsorContainer and WindsorContainer.

Once the overload Resolve(Type, IDictionary) was renamed to Resolve(Type, IReadOnlyDictionary<string, object>), the compiler then redirected all old calls from Resolve(Type, IDictionary) to Resolve(Type, object). This caused quite a few tests to fail, it also required a bit of type checking and casting for the resolve methods in the WindsorContainer to make sure they got to the right overload in the MicroKernel. After that tests started passing again.

We should definitely revisit this at some point.

ghost · 2018-07-14T15:51:36Z

@jnm2 Can you have a quick look?

jnm2 · 2018-07-14T15:54:56Z

src/Castle.Windsor/Windsor/WindsorContainer.cs

+			if (argumentsAsAnonymousType is IDictionary)
+			{
+				return kernel.Resolve(key, service, (IDictionary) argumentsAsAnonymousType);
+			}


Any chance that argumentsAsAnonymousType could be IReadOnlyDictionary<string, object> here?

If you cast IReadOnlyDictionary<string, object> to object then yes this overload will get called. These overloads love fighting with each other! :) Let me add another check.

@Fir3pho3nixx Another thought is that the ReadOnlyDictionary class also implements IDictionary, so you probably need ToDictionary there too. My main concern is that no matter what kind of dictionary I pass in, Castle Windsor will never mutate it—even if it's not readonly.

Edit: typing fail lol

jnm2 · 2018-07-14T16:51:35Z

LTGM as someone who isn't familiar with the codebase. Thanks so much for doing this!

Nit, the "copy-on-write" commit looks like it does defensive copying before any writes, which is slightly different but also simpler and safer than copy-on-write. The reason is that with a defensive copy up front, there's no chance that the person who passed you the dictionary is mutating the dictionary you're using later on. (Copy-on-write works only if all participants abide by it; the dictionary is effectively immutable.)

ghost · 2018-07-14T17:06:15Z

Nit, the "copy-on-write" commit looks like it does defensive copying before any writes, which is slightly different but also simpler and safer than copy-on-write. The reason is that with a defensive copy up front, there's no chance that the person who passed you the dictionary is mutating it themselves at various points.

Yes, this is more like a "copy up front". To this properly we would need to see why state is being mutated and then assess whether that was necessary or not to implement the copy on write. I also do not know what the severity of the mutations are (I have not investigated this too deeply), however if there are many writes scattered throughout the codebase for one resolve sequence, this could end up exploding the number of copies which is not efficient when it comes to memory usage. So thought this was by far a safer way to go for me just before we release :)

LTGM as someone who isn't familiar with the codebase. Thanks so much for doing this!

No probs :)

jnm2 · 2018-07-14T17:10:10Z

Yeah, just to be clear, the defensive copy as you have it here is the only way to go. 👍 Copy-on-write is never a possibility as far as I can see, because whoever passed in the dictionary might mutate the dictionary while Castle Windsor is still using it and expecting it to not have changed.

ghost · 2018-07-14T17:27:52Z

/cc #418

ghost · 2018-07-23T23:26:17Z

My personal take on this, is we could probably try to ~~DRY it up a little(each one calls a different overload in the kernel!) and~~ update docs. I am happy to absorb this to get ASP.NET Core out. ~~I would recommend we keep the original issue open until we have sorted out the Resolve API's.~~

jonorossi · 2018-07-31T17:16:56Z

Is there a reason the issue (#346) is closed but this pull request is open?

I'm really not a fan of the overload that is meant to be for anonymous types being abused to accept all sorts of dictionaries, in the future this overload should probably change to accepting dynamic because ExpandoObject is going to perform better than reflection on properties.

@Fir3pho3nixx I haven't tried it and it is late here, but did you try having both an IDictionary and an IReadOnlyDictionary<K,V> overload, is the C# compiler smart enough to pick the one of the defined object?

Another idea: could we have an overload that accepted a Arguments object and add implicit conversion operators to that class from the types we want, it wouldn't be that discoverable, and I guess if the C# compiler can't pick the overloaded method it probably can't pick overloaded operators.

public virtual object Resolve(Type service, object argumentsAsAnonymousType)
{
	return Resolve(service, new ReflectionBasedDictionaryAdapter(argumentsAsAnonymousType));
	if (argumentsAsAnonymousType is IReadOnlyDictionary<string, object>)
	{
		return kernel.Resolve(service, CloneDictionary((IReadOnlyDictionary<string, object>) argumentsAsAnonymousType));
	}
	if (argumentsAsAnonymousType is IDictionary)
	{
		return kernel.Resolve(service, (IDictionary)argumentsAsAnonymousType);
	}
	return kernel.Resolve(service, new ReflectionBasedDictionaryAdapter(argumentsAsAnonymousType));
}

This appears to creates a poor situation where if the collection implements IReadOnlyDictionary<string, object> then the dictionary entries get copied, however if it doesn't implement this interface they do not. Since System.Collections.Hashtable does not implement this generic interface, but System.Collections.Generic.Dictionary<K,V> does things become a bit unpredictable, many developers wouldn't even know that Dictionary<K,V> implements the interface.

IReadOnlyDictionary<K,V> represents read-only access to a collection, but it does not mean the collection is read-only as something else can modify the Dictionary<K,V> instance. I've never tried using IDictionary.IsReadOnly and ICollection<T>.IsReadOnly, and don't know how the BCL collections work with this property, but could this be useful? However this aside, copying the dictionary obviously doesn't copy the object instances.

This change would probably need to make its way into DefaultKernel too.

I am happy to absorb this to get ASP.NET Core out.

By "absorb", did you mean absolve. I'm happy for you to park this pull request and reopen the issue and we can discuss this in more detail, it is a pretty important change to the API contract, Resolve is used by everyone.

ghost · 2018-08-01T23:00:03Z

Is there a reason the issue (#346) is closed but this pull request is open?

Open now.

I'm really not a fan of the overload that is meant to be for anonymous types being abused to accept all sorts of dictionaries, in the future this overload should probably change to accepting dynamic because ExpandoObject is going to perform better than reflection on properties.

Yup but that is simply what one gets with an API that carries the most general type in CSharp as a parameter. The type battle is already lost. We should bin this API. It was clearly written when anonymous types became a thing, it now has some serious implications going forward. Let me respond to your comments and summarise at the end.

I thought an ExpandoObject was basically a dictionary at its core. What scares me off the thing is that it uses expressions for all the late bound meta data during runtime via it's IDynamicMetaObjectProvider interface. If you scroll further down, you will see the use of expressions for all the late binding. I have also historically found that anything that engages the DLR sometimes to be a little slow and always opted for not using it if possible unless really needed. I have no evidence to suggest this is still the case today though.

@Fir3pho3nixx I haven't tried it and it is late here, but did you try having both an IDictionary and an IReadOnlyDictionary<K,V> overload, is the C# compiler smart enough to pick the one of the defined object?

Yes, let's break this out into overloads. However if this ends up becoming N API's for N Types where we need to be more explicit, then we need to revisit this. Also if IReadOnlyDictionary eventually extends IDictionary who is to say what will happen with situations where one has an IReadOnlyDictionary reference and casts it to an IDictionary typed local and then calls Resolve? I think it is pretty messed up we even have to ask ourselves this question considering the root cause is Windsor is mutating the dictionary in the first place.

Another idea: could we have an overload that accepted a Arguments object and add implicit conversion operators to that class from the types we want, it wouldn't be that discoverable, and I guess if the C# compiler can't pick the overloaded method it probably can't pick overloaded operators.

I looked at this little guy. My first thought was YAGNI(You ain't gonna need it). This looked like a perfect candidate for obsolescence. You will be writing type converters till it comes out of your ears ... nope! Good shout though. Not sure about this just yet.

This appears to creates a poor situation where if the collection implements IReadOnlyDictionary<string, object> then the dictionary entries get copied, however if it doesn't implement this interface they do not. Since System.Collections.Hashtable does not implement this generic interface, but System.Collections.Generic.Dictionary<K,V> does things become a bit unpredictable, many developers wouldn't even know that Dictionary<K,V> implements the interface.

Correct. Type confusion bad. Let's please get rid of Resolve(object).

IReadOnlyDictionary<K,V> represents read-only access to a collection, but it does not mean the collection is read-only as something else can modify the Dictionary<K,V> instance. I've never tried using IDictionary.IsReadOnly and ICollection.IsReadOnly, and don't know how the BCL collections work with this property, but could this be useful? However this aside, copying the dictionary obviously doesn't copy the object instances.

This change would probably need to make its way into DefaultKernel too.

Ideally, this is what I want. Let's do the damn thing properly. I only have 2 hands, and I am heavily invested in overhauling Typed Factories and supporting ASP.NET Core once we release(including the perf stuff). I think we should re-introduce the IDictionary overload for resolve first and monitor the situation.

By "absorb", did you mean absolve. I'm happy for you to park this pull request and reopen the issue and we can discuss this in more detail, it is a pretty important change to the API contract, Resolve is used by everyone.

absolve(verb): declare (someone) free from guilt, obligation, or punishment.

Nope. Just trying to be acquiescent, and trust me I am going to get my ass whipped for ASP.NET Core. Just trying to pick my battles.

ghost · 2018-08-01T23:12:51Z

@jonorossi

We agreed to implement this at the "surface level" API, I think we should honour this. I also think we should re-introduce the Resolve(IDictionary) overload to minimise the type acrobatics in Resolve(object) but what we have learn't from this is that Resolve(object) becomes the garbage collector for types that fall out these explicitly typed API's. Resolve(object) is utter bollocks. +1 for deprecating this shit(later on of course). We actually need a new issue. There is a big difference between tracking what users want, and what we want.

ghost · 2018-08-01T23:33:52Z

On a personal note, this is very close to my ❤️. I have seen so many NodeJs projects fail at scale in London because of type safety. Resolve(object) === Resolve(YouTryAndFigureOutWhatIMeantWithMutations:any).

jnm2 · 2018-08-02T12:11:49Z

I used to use the anonymous type syntax a fair amount until I realized that typed factories are the way to go. You have type safety with typed factories.

If you have both IReadOnlyDictionary and IDictionary overloads, then overload resolution will be ambiguous if you try to pass a Dictionary-typed expression. The same problem happens with any type that implements both IReadOnlyDictionary and IDictionary. The corefx team opted not to add extension methods on IDictionary for this reason.
Regardless of the shape you're adapting, the behavior should be the same: iterate over the keys and values and make an up-front copy.

Another strong reason to make an up-front copy is that the original dictionary may have an arbitrarily strange key comparer. By making an up-front copy into a dictionary which uses the default comparer, you get to validate as close as possible to the call site that there are no duplicate keys.

ghost · 2018-08-03T17:47:52Z

If you have both IReadOnlyDictionary and IDictionary overloads, then overload resolution will be ambiguous if you try to pass a Dictionary-typed expression. The same problem happens with any type that implements both IReadOnlyDictionary and IDictionary.

True. I can see it being a little clunky because you might have to cast your parameters so the compiler can pick the right overload. Maybe we should just revert to the API's we had before but "Shallow Copy Up Front" for the IDictionary overload.

jnm2 · 2018-08-03T17:56:22Z

I actually am passing around an IReadOnlyDictionary instance, so I'd have to keep my own adapter if you go with IDictionary. What if you just go the IReadOnlyDictionary route? That'll just work with anyone's concrete dictionary type and will only break if you're working with the IDictionary interface type, which is good with me since I think it's the wrong abstraction to pass to Castle Windsor :D

ghost · 2018-08-03T18:57:04Z

I actually am passing around an IReadOnlyDictionary instance, so I'd have to keep my own adapter if you go with IDictionary.

I would like to help you get rid of your adapter. Let's kill it.

What if you just go the IReadOnlyDictionary route?

I am totally happy with that. All the tests for Resolve(IDictionary) fell back on to Resolve(object), we should remove Resolve(object) from IWindsorContainer and mark all Resolve(object) methods in the MicroKernel as obsolete with a link back to this PR.

ghost · 2018-08-03T19:02:01Z

@jonorossi

What do you think?

I like the direction this is heading in and I also believe we are about to jump a major version, so we could at least formalise our learnings in the code before we release the alpha.

I will write this up extensively in a new issue and make the necessary changes when I get time if we all agree.

jonorossi · 2018-08-09T07:15:57Z

@Fir3pho3nixx I assume you don't want me looking at the code in this pull request anymore?

I'll move the discussing back to the issue.

ghost · 2018-08-09T18:00:42Z

@jonorossi - I just need you to agree on the already agreed approach between @jnm2 and myself of:

Remove WindsorContainer.Resolve(object)
Remove WindsorContainer.Resolve(IDictionary)
Obsolete MicroKernel.Resolve(object)
Obsolete MicroKernel.Resolve(IDictionary)
Add WindsorContainer(IReadOnlyDictionary)

This way we can start heading towards a stronger typed model that gives @jnm2 what he needs for now to delete an adapter in his code base.

I can do this tonight, just need your agreement.

ghost · 2018-08-09T18:02:21Z

This would be small enough to get into the V5 release. Anything else would require more work. I would recommend we raise a new issue so we can discuss internals at length.

jonorossi · 2018-08-14T04:47:49Z

I just need you to agree on the already agreed approach between @jnm2 and myself of:

Remove WindsorContainer.Resolve(object)

Remove WindsorContainer.Resolve(IDictionary)

Obsolete MicroKernel.Resolve(object)

Obsolete MicroKernel.Resolve(IDictionary)

Why the difference between IWindsorContainer and IKernel? You can't really recommend the transition path for people wanting to use an IDictionary reference or an anonymous object use IWindsorContainer.Kernel.Resolve if you mark them deprecated. [Off topic: I think the whole public API between Windsor and MicroKernel makes Windsor so bloated, the MicroKernel was merged into Windsor so long ago and I don't think there is anyone that wants to use MicroKernel without Windsor, but obviously something for another day].

Add WindsorContainer(IReadOnlyDictionary)

This way we can start heading towards a stronger typed model that gives @jnm2 what he needs for now to delete an adapter in his code base.

What do you mean by a "stronger typed model", I get that @jnm2 wants the IReadOnlyCollection overload but that breaks the 10+ years of users that have written code using an IDictionary reference or an anonymous object and doesn't even leave them with a transition option, users will have to write an IReadOnlyDictionary<K,V> adapter from IDictionary or an anonymous dictionary.

jnm2 · 2018-08-14T12:34:41Z

@jonorossi Is it more likely that users are passing an expression typed as the IDictionary interface or as an actual dictionary class? All dictionary classes implement both IReadOnlyDictionary and IDictionary. The only reason I'd remove the IDictionary overload is that overload resolution will fail if you try to pass a class that implements both interfaces (which is every BCL dictionary).

Corefx decided to target only IReadOnlyDictionary for things like the GetValueOrDefault extension method for this reason. (See https://github.com/dotnet/corefx/issues/17917#issuecomment-298706121 and preceding conversation.)

If folks are passing around an IDictionary-typed instance and you make this change in v4, they will need to rewrite to IReadOnlyDictionary which is closer to being semantically correct because they aren't giving Windsor access to Add/Remove methods. No rewrite is necessary if they're using a BCL dictionary class type, only if they're casting to IDictionary<,> before passing.
If that's not something you want to do, would you please consider using a new method name which can receive an IReadOnlyDictionary to avoid the overload resolution problem?

jonorossi · 2018-08-14T13:50:12Z

Is it more likely that users are passing an expression typed as the IDictionary interface or as an actual dictionary class?

That's the part I don't know unfortunately which is the reason I'm concerned, I also don't know how many people are using anonymous types to pass arguments. I'm hesitant to make a change that might have a greater impact than we know.

If that's not something you want to do, would you please consider using a new method name which can receive an IReadOnlyDictionary to avoid the overload resolution problem?

I was looking at the inline dependency docs to see if there is some way to add an overload (or another method) that would start working in the direction of unifying register and resolve arguments.

I then came docs on the Arguments class again and remembered how much guidance and design are around using this class which implements IDictionary, dropping the IDictionary overload kills everything using this class via the public API:

I know I've suggested Resolve having an overload for Arguments earlier, but this would allow you to do something like:

container.Resolve<IMyService>(Arguments.FromReadOnlyDictionary(myMap));
container.Resolve<IMyService>(new Arguments(myMap));

If you went with new Arguments(myMap) (constructor taking object) you'd put all the magic in just this class to handle that conversion, we've said that isn't the most discoverable, but then we might as well just pass object directly into Resolve. We could have subclasses or similar a bit like Autofac has, but it has one for each parameter not the whole dictionary, which is more like our Register Dependency and Property classes.

@jnm2 what were your thoughts on how you'd add another without dropping IDictionary?

jnm2 · 2018-08-14T14:36:37Z

v4 seems like the best time possible to prune APIs that have been driven by legacy, but the decision's yours. I can sympathize with either choice.

I then came docs on the Arguments class again and remembered how much guidance and design are around using this class which implements IDictionary, dropping the IDictionary overload kills everything using this class via the public API:

So long as Arguments also implements IReadOnlyDictionary, these scenarios would be unaffected when you drop IDictionary.

what were your thoughts on how you'd add another without dropping IDictionary?

.Resolve<Foo>(Arguments.FromReadOnlyDictionary(bar)) is the best option that I can think of. To make it more discoverable, you could drop all Resolve overloads except for one taking an Arguments parameter. This would lead ReSharper etc. to prompt factory methods such as Arguments.FromReadOnlyDictionary.

ghost · 2018-08-16T19:26:31Z

Yup, hence Arguments would become a glorified type converter. My concern with this approach. At the end of the day Resolve(object) is what started this mess. Sorry I have been out of the frame, my internet was down for a week.

jonorossi · 2018-08-17T10:15:39Z

v4 seems like the best time possible to prune APIs that have been driven by legacy, [..]

My concern with this approach. At the end of the day Resolve(object) is what started this mess

I understand v5 would be a good time to make breaking changes but nothing stops us from having a v6 in a couple months, #346 was assigned to v5 in September last year, at that time we assumed we knew what the fix would be.

Even though Resolve<T>(object) like overloads aren't the reason we can't just add another overload I think we are all in agreement that Resolve overloads that accept object are bad for allowing us to evolve the API. We don't want a catch all which leads to runtime bugs. Since most the docs actually recommend using the Arguments to pass an anonymous type, do either of you have an objection for us to get this change in right now?

https://github.com/castleproject/Windsor/blob/master/docs/arguments.md
https://github.com/castleproject/Windsor/blob/master/docs/passing-arguments.md
https://github.com/castleproject/Windsor/blob/master/docs/how-dependencies-are-resolved.md

That change being removal of Resolve overloads that accept an object argument (for anonymous types), and update any documentation using this to recommend usage of the Arguments class (which will use the IDictionary parameter).

Can we then get a unit test and fix in for the dictionary being mutable and Windsor stuffing that up on people.

And we'll continue with the IReadOnlyDictionary<K,V> discussion.

jonorossi · 2018-08-17T10:27:30Z

hence Arguments would become a glorified type converter

It sort of already is a bit, accepting any IDictionary and with another constructor taking an anonymous type which results in a ReflectionBasedDictionaryAdapter which implements IDictionary. Maybe we should have a static factory method rather than a constructor for this.

I feel that not trying to bring some consistency between the registration and resolution APIs for arguments is a missed opportunity, but obviously that would require a massive analysis and break down of the whole API which I think has many weak spots, especially compared to other containers written more recently that don't have so much baggage. This isn't going to happen any time soon.

Since IDictionary<K,V> doesn't inherit IReadOnlyDictionary<K,V> then it seems unfair to only support one of them as the next request we'll get will be for the other.

@Fir3pho3nixx I know you see Arguments being a type converter, but what else is going to do it? Sorry if I'm completely missing something, but I don't feel like I'm on the same wavelength, I feel like I'm just not getting it.

Do you dislike an API like this:

public class Arguments : IDictionary
{
    public Arguments(IDictionary arguments) { }

    public Arguments FromDictionary(IDictionary<string, object> arguments) { }
    public Arguments FromReadOnlyDictionary(IReadOnlyDictionary<string, object> arguments) { }
    public Arguments FromAnonymousType(object arguments) { }
}

I also can't really see any need for the IArgumentsComparer overloads, need to have another look at that to try and understand the use case.

With something like this we could make this class immutable in the future so it captures the passed arguments.

I think an Arguments class like that would be much less breaking and open up support of quite a few more collection types, we could even have an IEnumerable one.

jonorossi

@Fir3pho3nixx looks great, I've left 10 fairly minor comments.

jonorossi · 2018-09-11T04:39:27Z

CHANGELOG.md

@@ -21,6 +21,7 @@ Breaking Changes:
 - Removal of deprecated BasedOn methods that reset registrations when fluently chained (@fir3pho3nixx, #338)
 - Removal of deprecated member LifestyleHandlerType on CustomLifestyleAttribute (@fir3pho3nixx, #338)
 - Removed Event Wiring, Factory Support and Synchronize facilities (@jonorossi, #403)
+- Deprecating `Resolve(object/IDictionary)` in favour of `Resolve(Castle.MicroKernel.Arguments)` (@fir3pho3nixx, #346)


"Deprecated".

Probably also mention at the end of the line "along with new Arguments class factory methods" including support for IReadOnlyDictionary.

jonorossi · 2018-09-11T04:42:38Z

docs/arguments.md

@@ -25,18 +23,30 @@ new Arguments(new { logLevel = LogLevel.High });
 When you don't care about names of the dependencies you can pass them as array, in which case they will be matched by their type.

 ```csharp
-new Arguments(new[] { LogLevel.High });
+var args = Arguments.FromTypedArguments(new[] { LogLevel.High });


Now that we are using a factory method and aren't polluting the constructor, could we make Arguments.FromTypedArguments a params so the user doesn't have to care about creating the array?

jonorossi · 2018-09-11T04:45:14Z

docs/arguments.md

+
+#### Custom read-only dictionary of arguments
+
+You can also pass a dictionary to `Arguments` via the `FromReadOnlyDictionary` method which also enforces read-only operations.


I'd mention the IReadOnlyDictionary interface somewhere in the docs here so it is clear how this overload works. Or even in the example just assign the new dictionary to a variable defined as the interface, then pass the variable.

which also enforces read-only operations.

Does this imply that FromDictionary does not enforce read-only operations, i.e. might write to the dictionary that is passed?

I'd mention the IReadOnlyDictionary interface somewhere in the docs here so it is clear how this overload works. Or even in the example just assign the new dictionary to a variable defined as the interface, then pass the variable.

Done.

Does this imply that FromDictionary does not enforce read-only operations, i.e. might write to the dictionary that is passed?

All FromDictionary/FromReadOnlyDictionary are shallow copied up front.

jonorossi · 2018-09-11T05:01:24Z

src/Castle.Windsor/MicroKernel/Arguments.cs

@@ -110,13 +92,17 @@ bool IDictionary.IsReadOnly
 			get { return arguments.IsReadOnly; }
 		}

+		public static Arguments Empty { get; } = new Arguments();


The way I read the code the Arguments.Empty collection instance isn't readonly!

Also is there a reason you decided to use a property rather than a field like String.Empty, Guid.Empty, Type.EmptyTypes, EventArgs.Empty? Performance-wise it probably isn't a big deal since people aren't going to call it often, but I didn't think the CLR can optimise across assembly boundaries for properties that just return a backing field.

Interesting, I always knew that the optimisation you speak of happened for constants. Did not know that was a thing for fields. Does this only happen for fields where the readonly modifier is used?

Getting rid of the property is a good shout. Less boxing/unboxing.

jonorossi · 2018-09-11T05:02:09Z

src/Castle.Windsor/MicroKernel/Arguments.cs

 		public void Add(object key, object value)
 		{
+			EnsureReadOnly();


I think EnsureReadOnly is the opposite of what this method does which is why I suggested EnsureWritable.

jonorossi · 2018-09-11T05:05:26Z

src/Castle.Windsor/MicroKernel/Arguments.cs

+		/// </summary>
+		/// <param name="anonymous">Anonymous type where the property values are reflected for values and copied into <see cref="Arguments"/> and then used as dependencies</param>
+		/// <returns><see cref="Arguments"/></returns>
+		public static Arguments FromProperties(object anonymous)


As we discussed in the thread FromProperties works just as well with a POCO as it does an anonymous type, so we should make this factory method clear. I'd change the parameter name to "instance" or "object" or something, and update the XML docs to reflect it works on "public properties of .NET objects including anonymous types".

jonorossi · 2018-09-11T05:16:10Z

src/Castle.Windsor/MicroKernel/Arguments.cs

@@ -110,13 +92,17 @@ bool IDictionary.IsReadOnly
 			get { return arguments.IsReadOnly; }


Arguments.IsReadOnly should return isReadOnly not the internal dictionary's state.

src/Castle.Windsor/MicroKernel/Context/CreationContext.cs

jonorossi · 2018-09-11T05:44:00Z

src/Castle.Windsor/Windsor/IWindsorContainer.cs

@@ -157,17 +157,9 @@ IWindsorContainer AddFacility<TFacility>(Action<TFacility> onCreate)
 		///   Returns a component instance by the service
 		/// </summary>
 		/// <param name = "service"></param>
-		/// <param name = "arguments"></param>
+		/// <param name = "arguments">Arguments to resolve the service. Please see the following factory methods: <see cref="Arguments.FromDictionary"/>, <see cref="Arguments.FromProperties"/>, <see cref="Arguments.FromReadOnlyDictionary"/> and <see cref="Arguments.FromTypedArguments"/></param>


I'd prefer not to have to maintain a hardcoded list of all the factory methods in the XML docs on each IWIndsorContainer overload, rather just mention the static factory methods of the Arguments class.

jonorossi · 2018-09-11T05:46:57Z

src/Castle.Windsor/MicroKernel/Registration/ComponentDependencyRegistrationExtensions.cs

@@ -57,7 +57,7 @@ public static IDictionary InsertAnonymous(this IDictionary arguments, object nam
 		/// <summary>
 		///   Inserts a new typed argument with given type. If an argument for this type already exists, it will be overwritten.
 		/// </summary>
-		public static IDictionary InsertTyped<TDependencyType>(this IDictionary arguments, TDependencyType value)
+		public static T InsertTyped<T, TDependencyType>(this T arguments, TDependencyType value) where T: IDictionary


I wonder if these extension methods should be amended to come in line with the other changes. These extension methods used IDictionary but because it has methods for typed arguments it sort of makes the assumption they'll be used for an Arguments instance, so why not just mark them on Arguments rather than for all IDictionarys which would pop up in places that are unwanted.

Do we even needed these extension methods? Are they used for something else which I've overlooked? If we keep them we'll want to bring them in line with some of the naming we've changed to with the Arguments factory methods and add IReadOnlyDictionary.

You are right, we should probably move this over to Arguments, trying to get rid of them reveals that InsertTyped is used in DelegateFactory in typed factories.

Are you also OK with me fixing some of the IDictionary parameters I found on delegates that appear to be used as part of a Resolve? DynamicParametersResolveDelegate is an example.

I was thinking, possibly getting rid of the delegates and using vanilla Func's. Also instead of passing IDictionary we could replace that with Arguments. That would for example allow me to move an extension like InsertTyped over. What do you think? I will do this as a single commit which can be reviewed separately and we can rebase out if we think it touches on too many things.

I eventually opted for not removing the delegates. They ended up being nicer to work with and read.

Please this commit for review: 6329505

jonorossi · 2018-09-11T13:02:21Z

src/Castle.Windsor/MicroKernel/Arguments.cs

-			: this(new ReflectionBasedDictionaryAdapter(namedArgumentsAsAnonymousType), customComparers)
-		{
-		}
+		protected IDictionary arguments;


The arguments field used to be protected but I see no reason to leave it protected, I think we should make it private.

Can we also now mark the field readonly since the internal collection is only ever created in the constructor now that we don't maintain the dictionary passed in.

jonorossi · 2018-09-11T13:02:49Z

@Fir3pho3nixx Just looking at my proposal in #420 (comment) and this hasn't been done:

Extension methods on IWindsorContainer for Resolve and ResolveAll accepting IReadOnlyDictionary<string, object> (and obviously Dictionary<string, object>)

Were you still going to add them?

ghost · 2018-09-15T12:21:34Z

@jonorossi About adding these as extension methods:

Extension methods on IWindsorContainer for Resolve and ResolveAll accepting IReadOnlyDictionary<string, object> (and obviously Dictionary<string, object>)

Could we make them actual members of IWindsorContainer/WindsorContainer? The only thing I don't like about extension methods, is that they are typically hard to mock in unit tests.

ghost · 2018-09-15T13:31:50Z

@jonorossi && @jnm2 thanks for reviewing. We seem to have cleared up quite a bit in this PR, we are in much better shape.

ghost

Man, I think the docs have a hole in them now. I also think removal of test code where variable names are named bla is a good idea. I also would like to remove references to 666 and foobar.

ghost · 2018-09-17T23:13:06Z

src/Castle.Windsor.Tests/TransientMultiConstructorTestCase.cs

@@ -31,10 +31,10 @@ public void TransientMultiConstructorTest()
 			DefaultKernel container = new DefaultKernel();
 			((IKernel)container).Register(Component.For(typeof(FooBar)).Named("FooBar"));

-			var arguments1 = new Dictionary<object, object>();
+			var arguments1 = Arguments.FromDictionary(new Dictionary<object, object>());
 			arguments1.Add("integer", 1);


This is probably overkill. We could just new up an empty Arguments without using the FromDictionary extension.

#LetsFixWindsorTests

That is obvious now that we changed Arguments to be the go to class 😉, I suspect when these tests were written Windsor didn't have an Arguments class. Feel free to do some clean up later though.

This could be changed to:

var arguments1 = new Arguments();

jonorossi · 2018-09-18T08:34:34Z

src/Castle.Windsor.Tests/TransientMultiConstructorTestCase.cs

@@ -31,10 +31,10 @@ public void TransientMultiConstructorTest()
 			DefaultKernel container = new DefaultKernel();
 			((IKernel)container).Register(Component.For(typeof(FooBar)).Named("FooBar"));

-			var arguments1 = new Dictionary<object, object>();
+			var arguments1 = Arguments.FromDictionary(new Dictionary<object, object>());
 			arguments1.Add("integer", 1);


That is obvious now that we changed Arguments to be the go to class 😉, I suspect when these tests were written Windsor didn't have an Arguments class. Feel free to do some clean up later though.

jonorossi · 2018-09-18T09:01:32Z

src/Castle.Windsor/Windsor/IWindsorContainer.cs

 		/// <returns></returns>
-		object Resolve(Type service, object argumentsAsAnonymousType);
+		object Resolve(Type service, IReadOnlyDictionary<string, object> arguments);


Extension methods on IWindsorContainer for Resolve and ResolveAll accepting IReadOnlyDictionary<string, object> (and obviously Dictionary<string, object>)

Could we make them actual members of IWindsorContainer/WindsorContainer? The only thing I don't like about extension methods, is that they are typically hard to mock in unit tests.

Why would you need or want to mock IWindsorContainer.Resolve? That sounds like a poor unit test design, and you could just mock the Resolve(Arguments) method instead anyway if you can't change the design.

I'd prefer them to be extension methods as we discussed, because it simplifies the IWindsorContainer API quite a bit and makes it clear everything goes via the Arguments overload.

Why would you need or want to mock IWindsorContainer.Resolve? That sounds like a poor unit test design.

Enabling things to be mockable is never a bad idea. If you are looking to implement a unit test that does not actually want to test Windsor integration(typically things at the bottom of the test pyramid) then with extensions you have to write wrappers and you start losing the benefit of a mocking library like Moq.

That said I am not precious over how this goes. We just have different opinions. Will move them out to extensions. 👍

jonorossi · 2018-09-18T09:06:02Z

src/Castle.Windsor/MicroKernel/Internal/LazyEx.cs

@@ -23,7 +23,7 @@ public class LazyEx<T> : Lazy<T>, IDisposable
 		private readonly IKernel kernel;

 		public LazyEx(IKernel kernel, IDictionary arguments)
-			: base(() => kernel.Resolve<T>(arguments))
+			: base(() => kernel.Resolve<T>(Arguments.FromDictionary(arguments)))


Did you have a look at what is using LazyEx, should its constructor accept an Arguments instance instead of it doing the conversion to Arguments?

There are a few of these lying around. Let me investigate.

Alright, I tried a couple of things. I eventually opted for changing the IDictionary arguments to Arguments and leaving all the overloads.

Why are we creating magical Lazy support? What ever happened to keeping things simple? A CMC would cover this easily!

var container = new WindsorContainer(); container.Register(Component.For<Lazy<MyFoo>>()) // The intent container.Resolve<Lazy<MyFoo>>() // Should match the usage

Why are we trying to magically cater for this? This is bizarre. Also this unit test would suggest this stuff does not work well with ResolveAll.

I abhor this approach.

Why are we creating magical Lazy support? What ever happened to keeping things simple? A CMC would cover this easily!

var container = new WindsorContainer(); container.Register(Component.For<Lazy<MyFoo>>()) // The intent container.Resolve<Lazy<MyFoo>>() // Should match the usage

Why are we trying to magically cater for this? This is bizarre. Also this unit test would suggest this stuff does not work well with ResolveAll.

I don't think that is the use case. I've not used it before but looking at the docs it is meant to be used like a typed factory where the instance gets created when you first use it. See:

https://github.com/castleproject/Windsor/blob/master/docs/whats-new-3.0.md#added-support-for-lazyt-components

I was wrong, I just saw the unit tests you updated for this and they do as you wrote. Yer, no idea why you'd want to do that, and I think Func<TService> as a delegate factory is a much nicer API to depend on than Lazy<TService>.

jonorossi · 2018-09-18T09:08:47Z

CHANGELOG.md

@@ -21,6 +21,9 @@ Breaking Changes:
 - Removal of deprecated BasedOn methods that reset registrations when fluently chained (@fir3pho3nixx, #338)
 - Removal of deprecated member LifestyleHandlerType on CustomLifestyleAttribute (@fir3pho3nixx, #338)
 - Removed Event Wiring, Factory Support and Synchronize facilities (@jonorossi, #403)
+- Deprecated `Resolve(object/IDictionary)` in favour of `Resolve(Castle.MicroKernel.Arguments)` along with new Arguments class factory methods including support for IReadOnlyDictionary (@fir3pho3nixx, #346)


Would be good to mention IWindsorContainer and IKernel to make it clearer for readers.

src/Castle.Windsor/MicroKernel/Registration/DynamicParametersDelegate.cs

jonorossi · 2018-09-18T09:19:25Z

src/Castle.Windsor/MicroKernel/Arguments.cs

+				if (@object == null)
+				{
+					throw new ArgumentNullException("typedArguments",
+													"Given array has null values. Only non-null values can be used as arguments.");


jonorossi · 2018-09-18T09:25:34Z

src/Castle.Windsor/MicroKernel/Arguments.cs

+
+		bool IDictionary.IsReadOnly => isReadOnly;
+
+		public static readonly Arguments Empty = new Arguments();


Doesn't this still leave the Arguments.Empty instance writable? i.e. someone can write Arguments.Empty.Add("a", "b"). I think you just need to change it to ... = new Arguments { isReadOnly = true };.

jonorossi · 2018-09-18T09:25:56Z

src/Castle.Windsor/MicroKernel/Arguments.cs

-			get { return arguments[key]; }
-			set { arguments[key] = value; }
+			get => arguments[key];
+			set => arguments[key] = value;


EnsureWritable

👍 awesome. Good spot.

jonorossi · 2018-09-18T09:27:56Z

src/Castle.Windsor/MicroKernel/Arguments.cs

-		{
-		}
+		private bool isReadOnly;
+		private readonly IDictionary arguments;

 		public Arguments(IDictionary values, params IArgumentsComparer[] customComparers)


I'm interested to know if we even need to support custom comparers anymore since we shallow copy all collections now, is that something we could drop since we are making heaps of change anyway?

Or are people using it to make keys case-sensitive/insensitive or something? Could we handle that with something simple like a StringComparison object for string keys?

I had 3 goes at trying to make this work.

The problem is to do with the generic dictionary definition itself. It is new Dictionary<object, object>. To make it work with StringComparison that would involve instantiating it as new Dictionary<string, object>.

It turns into a blood bath when running the unit tests.

An example of where you can see how the dictionary needs to have a key of an object type is in DefaultComponentActivator. Here the instance itself is getting used as the key. You can see the implementation of SetContextualProperty here.

This would be awesome to sort out. It is unfortunately beyond the scope of this PR.

Sorry I wasn't very clear, I know the key must be an object because we support keyed and typed arguments so wasn't saying it could just be replaced one-for-one. I was asking to understand what it is currently used for to see if it is even needed, Windsor parameters have always been case insensitive so it doesn't help much allowing the arguments to be case sensitive because only one value can make its way through:

https://github.com/castleproject/Windsor/blob/master/docs/inline-dependencies.md

If there is still a use case for the IArgumentsComparers then let's move on. Wondering because it is a good time to drop them if they serve no purpose.

Yeah, almost looked like it was possible. So I appreciate you asking. Always good to try and get rid of things we don't need.

Yeah, almost looked like it was possible. So I appreciate you asking. Always good to try and get rid of things we don't need.

Did I miss something? I still don't see how they are used (other than a few pointless unit tests making all strings equal).

The only Arguments constructor usage I see useful at the moment is by providing no IArgumentsComparers, which results in the internal dictionary of Arguments being initialised with an instance of ArgumentsComparer which is a System.Collections.Generic.IEqualityComparer<object> not an IArgumentsComparer, this comparer is the one that handles case-insensitive string keys and does normal object.Equals for types.

Turns out, if you just get rid of the comparers as you say then all you are left with is 2 failing unit tests. This and this. I took the liberty of deleting them.

Thanks :)

I am also going to update the CHANGELOG.md with the breaking change.

jonorossi · 2018-09-18T10:44:21Z

src/Castle.Windsor/MicroKernel/Arguments.cs

+		/// <summary>
+		///   Inserts a new named argument with given key. If an argument for this name already exists, it will be overwritten.
+		/// </summary>
+		public Arguments Insert(string key, object value) 


What was the reason for moving these from extension methods to methods on Arguments? I feel they fit exactly to what extension methods should be used for. I do like the changes to update them to align with the other changes.

Why?

I think we need to discuss why you think implementing extension methods are valid when you have access to source code. If you own the source the co-related type you should really just implement it as a member on that type.

Extensions methods are typically viable for types where you don't have access to the source.

I am clearly missing something here.

@jnm2 Do you think extensions are viable even when you have access to the source?

I question this approach when we are not doing things at scale like ASP.NET Core which spans multiple assemblies.

@Fir3pho3nixx The .NET BCL team is big on extension methods instead of instance methods for generic types because each instance method adds to the memory and CPU required to JIT each generic instantiation of the type before it can be used at all. That doesn't apply here.

Beyond that, there's no problem per se with using extension methods as part of your API. You can be forced to do this due to assembly layering or wanting stricter generic constraints for the method than the type. This doesn't result in a problematic API, so I wouldn't think of it as being problematic to use extension methods even when you aren't forced to.

It's really up to the semantics you want to convey: is this a core use case, without which the type would not be properly usable? Instance feels right to me for that. Is it add-on behavior? Extension methods can express that, though I do default to instance members myself.

One practical difference:
Extension methods require a using statement to be in scope, but instance methods do not. This is a great tool because you can hide extension methods behind a namespace on purpose to avoid clutter for people who don't opt in.
On the flip side, I've been annoyed a few times at having to add using System.Reflection; because System.Reflection.TypeExtensions backfills APIs using extension methods, where other target frameworks have instance methods and no need for the using.

Just to jokingly add more fuel to the fire, we could get rid of the static factory methods. :)

Serious I don't mind changing it if you agree. My only recommendation is that we probably raise a new issue, to track it and set the expectation to do it as a patch release after V5.

Let's see how this weekend goes might have time.

Just to jokingly add more fuel to the fire, we could get rid of the static factory methods. :)

Serious I don't mind changing it if you agree.

Might be a good idea as it'll simplify things and mean there isn't 2 sets of APIs. What would the alternatives look like, there isn't one for the last 2:

Arguments.FromDictionary(dictionary) new Arguments(dictionary) Arguments.FromProperties(instance) new Arguments().InsertProperties(instance) Arguments.FromReadOnlyDictionary(readOnlyDictionary) ? Arguments.FromTypedArguments(typedArguments) ?

Arguments.FromDictionary(dictionary) new Arguments(dictionary) Arguments.FromProperties(instance) new Arguments().InsertProperties(instance) Arguments.FromReadOnlyDictionary(readOnlyDictionary) new Arguments().InsertReadOnly(instance) Arguments.FromTypedArguments(typedArguments) new Arguments().InsertTypedArguments(instance)

Continue with the insert convention?

Continue with the insert convention?

Yes, but a few comments:

Probably should add Insert(IDictionary) to complete the set, and make it check the key is of type string or Type.

Should we go with InsertTyped rather than InsertTypedArguments?

You've not responded to my comment (https://github.com/castleproject/Windsor/pull/420/files#r218357134) about my dislike for the FromReadOnlyDictionary factory method name since IReadOnlyDictionary<K,V> just represents a read-only view not a read-only collection. How about we add InsertNamed(IReadOnlyDictionary<string, object>) instead of InsertReadOnly since most people will pass something concrete like Dictionary<string, object> anyway, it'll match what it does and read easily with InsertTyped.

Probably should add Insert(IDictionary) to complete the set, and make it check the key is of type string or Type.

I have checked keys to be string or object for Insert(IDictionary) and to only be string for InsertNamed(IReadOnlyDictionary<string, object>).

Should we go with InsertTyped rather than InsertTypedArguments?

Like it. Done.

You've not responded to my comment.

Sorry missed that. I have renamed the method to InsertNamed. This works well as the string enforces that things are named only.

ghost · 2018-09-18T20:01:11Z

This PR is bigger than what we wanted it to be but again all good changes. Thanks @jonorossi :)

ghost · 2018-09-26T22:52:15Z

src/Castle.Windsor/MicroKernel/Arguments.cs

-			{
-				arguments = new Dictionary<object, object>(new ArgumentsComparerExtended(customComparers));
-			}
+			arguments = new Dictionary<object, object>(new ArgumentsComparer());


Where I went wrong before. I tried to get rid of the ArgumentsComparer by converting Dictionary<object, object> to Dictionary<string, object> so I could use StringComparer instead. ArgumentsComparer needs to hang around for a bit.

jonorossi · 2018-09-27T05:06:17Z

src/Castle.Windsor/MicroKernel/Arguments.cs

 				}
-				return base.Equals(x, y);
+				return x == y;


This change from object.Equals to == shouldn't break anything since our keys should always be types here, but if you weren't aware the == operator is overloaded not overridden in the class hierarchy, so since x and y are both defined as object it'll result in a reference equality which wouldn't work for a string (but we handle strings above).

See https://blogs.msdn.microsoft.com/csharpfaq/2004/03/29/when-should-i-use-and-when-should-i-use-equals/ if you want an explanation with example.

One Resharper alt-enter too many late last night :) Will put it back.

jonorossi · 2018-09-27T05:08:01Z

src/Castle.Windsor/MicroKernel/Arguments.cs

-
-				Add(@object.GetType(), @object);
-			}
+			dictionary = new Dictionary<object, object>(new ArgumentsComparer());


Probably a micro-optimisation, but we could use a single instance of ArgumentsComparer for all Arguments rather than allocating a new one.

Cool, made this a private static read-only field and passed the reference.

ghost · 2018-09-29T18:48:08Z

@jonorossi - We now have a tidy single set of instance methods on Arguments. I am much happier with this PR.

General thoughts:

Factory methods are valid but I think they create a certain amount of indirection around the new operator when it comes to discoverability. Making these instance methods flows nicely from here now. I know this does not compete with any of the arguments before, it is just a preference of mine when working with a type does not have a factory suffix in it's name.
Extension methods hide implementation code from the type you are reading source for, they are literally somewhere else. I know they are useful for many things, but in this case for me personally they are not discovered until I do something that breaks them. It is much nicer for me to have these members on the classes themselves. That way I can assess the magnitude of risk before making changes more easily because it is all there in plain sight. It also helps me avoid refactoring, discovering the extension and ultimately reverting my assumptions, then rince and repeat with bigger scope. This to me makes their liberal use a little dangerous in terms of a devs effort to understand/change the bigger picture.

That said, I thank you for taking the time to review this. We are in a great place and I could not have done it without you. Thanks.

@jnm2 are you happy with the change in direction? Hopefully it is all the same as before except we are not using factories and we are not using extension methods. Sorry this took so long.

docs/arguments.md

jnm2 · 2018-09-29T19:02:08Z

are you happy with the change in direction?

Yes, thanks for asking! I think you two came up with a really nice API and my scenario is taken care of (resolving when what I have is a IReadOnlyDictionary, shallow copying rather than casting and mutating).

docs/arguments.md

CHANGELOG.md

ghost · 2018-09-29T19:41:19Z

docs/arguments.md

-The `Arguments` class is used by Windsor to pass arguments [down the invocation pipeline](how-dependencies-are-resolved.md). The class is a simple implementation of non-generic `IDictionary`, but it has some useful capabilities.
-
-:information_source: **Custom `IDictionary` is respected:** When you call `container.Resolve` passing your own custom implementation of `IDictionary` Windsor will respect that and not replace it with `Arguments`. It is advised that you use `Arguments` though.
+The `Arguments` class is used by Windsor to pass arguments [down the invocation pipeline](how-dependencies-are-resolved.md). The class is a simple implementation of non-generic `IDictionary`, but it has some useful capabilities explained further below.

 ### Constructors

 The class has several constructors:

 #### `namedArgumentsAsAnonymousType`


@jonorossi

This documentation could be improved.

I think what we need here, is a concise clear document that preludes with a definition of what typed/named arguments are in terms of dependency resolution(including examples). Then we follow with a body of instance methods including usages/examples.

I think what we need here, is a concise clear document that preludes with a definition of what typed/named arguments are in terms of dependency resolution(including examples). Then we follow with a body of instance methods including usages/examples.

As usual documentation can always be improved. I'll let you decide when to do that, before or after merge.

I'll do another code review to make sure we are good to merge.

Nope. Not right. Let's release ASP.NET Core first. I want to fix this properly.

src/Castle.Windsor.Tests/RuntimeParametersTestCase.cs

ghost · 2018-09-29T21:13:41Z

src/Castle.Windsor/MicroKernel/Arguments.cs

+		/// </summary>
+		/// <param name="values"><see cref="IReadOnlyDictionary{S, O}"/> where the dictionary is shallow copied up front into <see cref="Arguments"/> and then used as dependencies</param>
+		/// <returns><see cref="Arguments"/></returns>
+		public Arguments InsertNamed(IReadOnlyDictionary<string, object> values)


I also think we should mark the methods as virtual, so folks out there can inherit and override our API without having to raise PR's for further named/type support.

ghost · 2018-09-29T21:29:08Z

src/Castle.Windsor/MicroKernel/Arguments.cs

+		/// <summary>
+		/// Inserts a new named argument with given key. If an argument for this name already exists, it will be overwritten.
+		/// </summary>
+		public Arguments Insert(string key, object value)


This method could probably be safely exposed as a constructor.

ghost · 2018-09-29T21:29:28Z

src/Castle.Windsor/MicroKernel/Arguments.cs

+		/// <summary>
+		/// Inserts a new typed argument with given type. If an argument for this type already exists, it will be overwritten.
+		/// </summary>
+		public Arguments Insert(Type dependencyType, object value)


This method could probably be safely exposed as a constructor.

@Fir3pho3nixx

…f `WindsorContainer.Resolve(Castle.MicroKernel.Arguments)` along with new Arguments class factory methods including support for IReadOnlyDictionary (@Fir3pho3nixx, #346) Additional breaking changes: - Changed CreationContext.AdditionalArguments to use Arguments instead of IDictionary (@Fir3pho3nixx, #346) - Removed ComponentDependencyRegistrationExtensions(Insert, InsertAnonymous, InsertTyped, InsertTypedCollection) and created Insert, InsertNamed, InsertProperties and InsertTyped Arguments instance methods (@Fir3pho3nixx, #346) - ComponentRegistration.DependsOn and ComponentRegistration.DynamicParameters changed to use Arguments via DynamicParametersDelegate (@Fir3pho3nixx, #346) - Removed IArgumentsComparer[] from Arguments (@Fir3pho3nixx, #346)

jonorossi

Sorry for more comments, hopefully we can make the turnaround on these quicker.

jonorossi · 2018-10-01T05:37:24Z

CHANGELOG.md

@@ -21,6 +21,11 @@ Breaking Changes:
 - Removal of deprecated BasedOn methods that reset registrations when fluently chained (@fir3pho3nixx, #338)
 - Removal of deprecated member LifestyleHandlerType on CustomLifestyleAttribute (@fir3pho3nixx, #338)
 - Removed Event Wiring, Factory Support and Synchronize facilities (@jonorossi, #403)
+- Deprecated `WindsorContainer.Resolve(object/IDictionary)` in favour of `WindsorContainer.Resolve(Castle.MicroKernel.Arguments)` along with new Arguments class factory methods including support for IReadOnlyDictionary (@fir3pho3nixx, #346)


Factory methods are gone, and we've also got new IReadOnlyDictionary Resolve extension methods on IWindsorContainer.

jonorossi · 2018-10-01T05:40:33Z

src/Castle.Windsor/Windsor/IWindsorContainer.cs

@@ -157,17 +157,9 @@ IWindsorContainer AddFacility<TFacility>(Action<TFacility> onCreate)
 		///   Returns a component instance by the service
 		/// </summary>
 		/// <param name = "service"></param>
-		/// <param name = "arguments"></param>
+		/// <param name = "arguments">Arguments to resolve the service. Please see the factory methods in <see cref="Arguments"/></param>


No factory methods, the other methods have this same XML doc.

jonorossi · 2018-10-01T05:43:14Z

docs/arguments.md


-### `Insert` extension method
+#### Custom read-only dictionary of arguments


I think this still sounds like you have to pass a collection that isn't writable.

jonorossi · 2018-10-01T05:44:40Z

docs/arguments.md

@@ -25,18 +23,30 @@ new Arguments(new { logLevel = LogLevel.High });
 When you don't care about names of the dependencies you can pass them as array, in which case they will be matched by their type.

 ```csharp
-new Arguments(new[] { LogLevel.High });
+var args = new Arguments().InsertTyped(LogLevel.High);
 ```

 :information_source: **Typed arguments are matched exactly:** When you don't specify type of the dependency it's exact type will be used. So if you pass for example `MemoryStream` Windsor will try to match it to dependency of type `MemoryStream`, but not of the base type `Stream`. If you want to match it to `Stream` use `Insert` extension method.



No extension method, this is referring to the old extension methods.

jonorossi · 2018-10-01T05:53:13Z

src/Castle.Windsor/MicroKernel/Arguments.cs

+		/// </summary>
+		/// <param named="named"></param>
+		/// <param named="value"></param>
+		public Arguments(string named, object value) : this()


I'm not sure I'm a fan of these 2 new Arguments constructors, why do you think they are needed?

new Arguments("key", "value"); new Arguments(typeof(Thing), myThing);

That replaces the typed array constructor that is in the released API, which you can use like this:

new Arguments(first, second, third);

The new constructors would be a bit like having the following on Dictionary:

new Dictionary<string, string>("key", "value");

The Insert* extension methods already stepped out of the IDictionary implementation and decided to go off and create a fluent interface. We brought them in, it merely reduces cruft, think about chanining Insert.

var args = new Arguments().Insert("key1", "value1").Insert("key2", "value2");

Or

var args = new Arguments("key1", "value1").Insert("key2", "value2");

The day Insert got authored as an extension, Arguments stopped becoming a pure dictionary implementation.

Ah good point, the first example does looks a little stupid. I'm now a fan of this change.

jonorossi · 2018-10-01T05:59:33Z

src/Castle.Windsor/MicroKernel/Arguments.cs

+				return new Arguments(dictionary);
+			}
+
+			return new Arguments(dictionary);


I'm confused by this Clone method, how do the 2 different branches work different and why do they need to?

I am confused also. Rubbish code.

jonorossi · 2018-10-01T06:07:05Z

src/Castle.Windsor/MicroKernel/Arguments.cs

 				}
 			}

-			return new Arguments(arguments);
+			isReadOnly = true;


I thought we were on the same page about IReadOnlyDictionary being handled just like any other dictionary, the Arguments instance shouldn't be made read-only just because you insert some items from a collection implementing a read-only view? This behaviour is non-obvious and doesn't align with the behaviour of the other Insert overloads.

It now looks like the only use for the isReadOnly flag is the Empty instance, but that might change when we look at (post v5) the next part when the Arguments instance is passed to Windsor for resolution, I suspect Windsor will probably freeze/make-read-only the instance before it starts using it.

I thought we were on the same page about IReadOnlyDictionary being handled just like any other dictionary, the Arguments instance shouldn't be made read-only just because you insert some items from a collection implementing a read-only view? This behaviour is non-obvious and doesn't align with the behaviour of the other Insert overloads.

Not too sure about that. I would like to see that read only contract honoured throughout the Windsor code base(incl. Facilities). If something is mutating it I would rather have tests fail on PR's. We simply cannot accept mutative behaviour downstream for anyone that uses this API. Ideally for performance, we would later on eliminate the "copy up front" approach.

I would like to see that read only contract honoured

I think this is our biggest disagreement in this pull request. IReadOnlyDictionary doesn't define an immutable contract, it defines a readable view of a dictionary, nothing stops the dictionary changing from when you were given a reference to the view. Mentally I think of it as IReadableDictionary.

I don't see why the following code shouldn't be allowed:

new Arguments("first", "value") .Insert("key", "value") .Insert(new Dictionary<string, object>()) .Insert("third", "value"); // <- throws NotSupportedException: Collection is read-only

jonorossi · 2018-10-01T06:13:12Z

src/Castle.Windsor/MicroKernel/Arguments.cs

+			{
+				if (entry.Key is string || entry.Key is Type)
+				{
+					dictionary[entry.Key] = entry.Value;


Missing EnsureWritable since you are using dictionary[] = x not this[] = x, same applies to the following InsertNamed method.

jonorossi · 2018-10-01T06:15:23Z

src/Castle.Windsor/MicroKernel/Arguments.cs

 			{
-				var comparerExtended = dictionary.Comparer as ArgumentsComparerExtended;
-				if (comparerExtended != null)
+				if (entry.Key is string)


This check seems redundant, how can it not be a string?

jonorossi · 2018-10-01T06:36:59Z

src/Castle.Windsor/MicroKernel/Arguments.cs

+		/// </summary>
+		/// <param named="namedOrTyped">Named parameter</param>
+		/// <param named="value">Instance dependency</param>
+		public virtual Arguments Insert(string named, object value)


I saw your comment by email notification about marking the Insert overloads virtual but can't find the comment in the GitHub UI. What was the reason for wanting to do that? I just can't see anyone wanting to override these methods, and it probably isn't a good idea overridding them because it may break assumptions other facilities have made about their behaviour.

Won't marking them virtual just bloat the v-table, prevent the CLR inlining some of the really simple overloads, and also force the C# compiler to emit callvirt instructions (very minor performance increase) for all Insert calls rather than just call instructions when it knows your Arguments variable can't be null?

I saw your comment by email notification about marking the Insert overloads virtual but can't find the comment in the GitHub UI

#420 (comment)

What was the reason for wanting to do that? I just can't see anyone wanting to override these methods, and it probably isn't a good idea overridding them because it may break assumptions other facilities have made about their behaviour.

You say people wont want to extend Arguments, I say they would. Anyone adapting this container to any framework in the wild would most definitely be interested in us applying the Open–closed principal when it comes to substituting Resolve(object) with Resolve(Arguments). I personally side stepped all this by using types in the ASP.NET Core facility. You can see examples of this here, here and here.

Won't marking them virtual just bloat the v-table, prevent the CLR inlining some of the really simple overloads, and also force the C# compiler to emit callvirt instructions (very minor performance increase) for all Insert calls rather than just call instructions when it knows your Arguments variable can't be null?

Probably the reason I closed out the PR. We have already disagreed on extension methods and factories. I don't have the energy for this right now, not on this PR. We need to release ASP.NET Core before we address this. I will re-open once that is done.

You say people wont want to extend Arguments

I didn't say people won't want to extend Arguments with new methods, just that it probably isn't a good idea to override the behaviour of these methods.

Probably the reason I closed out the PR. We have already disagreed on extension methods and factories. I don't have the energy for this right now, not on this PR. We need to release ASP.NET Core before we address this. I will re-open once that is done.

I probably added too much detail here. They can stay virtual, if someone does override them and breaks the basic assumptions then it isn't something we can control, not really much different to other extension points.

ghost · 2018-10-01T20:49:30Z

My give up

jnm2 · 2018-10-02T12:46:35Z

My give up

What happened, need any help getting this through?

ghost requested a review from jonorossi July 14, 2018 15:51

jnm2 reviewed Jul 14, 2018

View reviewed changes

ghost mentioned this pull request Jul 23, 2018

Release 5.0.0-beta001 #418

Closed

ghost mentioned this pull request Aug 1, 2018

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

Closed

ghost self-assigned this Aug 16, 2018

jonorossi requested changes Sep 11, 2018

View reviewed changes

jonorossi reviewed Sep 11, 2018

View reviewed changes

ghost commented Sep 17, 2018

View reviewed changes

jonorossi requested changes Sep 18, 2018

View reviewed changes

ghost commented Sep 26, 2018

View reviewed changes

jonorossi reviewed Sep 27, 2018

View reviewed changes

jnm2 reviewed Sep 29, 2018

View reviewed changes

docs/arguments.md Outdated Show resolved Hide resolved

ghost commented Sep 29, 2018

View reviewed changes

docs/arguments.md Outdated Show resolved Hide resolved

ghost commented Sep 29, 2018

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

ghost commented Sep 29, 2018

View reviewed changes

src/Castle.Windsor.Tests/RuntimeParametersTestCase.cs Outdated Show resolved Hide resolved

ghost commented Sep 29, 2018

View reviewed changes

jonorossi requested changes Oct 1, 2018

View reviewed changes

ghost closed this Oct 1, 2018

ghost mentioned this pull request Oct 1, 2018

Deprecate Lock in favour of ReaderWriterLockSlim #426

Closed

mario-d-s mentioned this pull request Oct 9, 2018

Ignore 13 failing tests on master and v4.1.0 #438

Closed

jonorossi mentioned this pull request Oct 14, 2018

Arguments class and Resolve methods refactor #444

Merged

This pull request was closed.


		#### Custom read-only dictionary of arguments

		You can also pass a dictionary to `Arguments` via the `FromReadOnlyDictionary` method which also enforces read-only operations.

		@@ -110,13 +92,17 @@ bool IDictionary.IsReadOnly
		get { return arguments.IsReadOnly; }


		bool IDictionary.IsReadOnly => isReadOnly;

		public static readonly Arguments Empty = new Arguments();


		### `Insert` extension method
		#### Custom read-only dictionary of arguments

Deprecating Resolve(Type, IDictionary) in favour of Resolve(Type, IReadOnlyDictionary<string, object>) #420

Deprecating Resolve(Type, IDictionary) in favour of Resolve(Type, IReadOnlyDictionary<string, object>) #420

Conversation

ghost commented Jul 14, 2018

ghost commented Jul 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnm2 Jul 14, 2018 • edited Loading

Choose a reason for hiding this comment

Edit: typing fail lol

jnm2 commented Jul 14, 2018 • edited Loading

ghost commented Jul 14, 2018

jnm2 commented Jul 14, 2018

ghost commented Jul 14, 2018

ghost commented Jul 23, 2018 • edited by ghost Loading

jonorossi commented Jul 31, 2018

ghost commented Aug 1, 2018 • edited by ghost Loading

ghost commented Aug 1, 2018 • edited by ghost Loading

ghost commented Aug 1, 2018 • edited by ghost Loading

jnm2 commented Aug 2, 2018 • edited Loading

ghost commented Aug 3, 2018

jnm2 commented Aug 3, 2018

ghost commented Aug 3, 2018

ghost commented Aug 3, 2018

jonorossi commented Aug 9, 2018

ghost commented Aug 9, 2018

ghost commented Aug 9, 2018

jonorossi commented Aug 14, 2018

jnm2 commented Aug 14, 2018 • edited Loading

jonorossi commented Aug 14, 2018

jnm2 commented Aug 14, 2018 • edited Loading

ghost commented Aug 16, 2018

jonorossi commented Aug 17, 2018

jonorossi commented Aug 17, 2018

jonorossi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonorossi commented Sep 11, 2018

ghost commented Sep 15, 2018 • edited by ghost Loading

ghost commented Sep 15, 2018

ghost left a comment • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 17, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Sep 23, 2018 • edited by ghost Loading

Deprecating `Resolve(Type, IDictionary)` in favour of `Resolve(Type, IReadOnlyDictionary<string, object>)` #420

Deprecating `Resolve(Type, IDictionary)` in favour of `Resolve(Type, IReadOnlyDictionary<string, object>)` #420

jnm2 Jul 14, 2018 •

edited

Loading

jnm2 commented Jul 14, 2018 •

edited

Loading

ghost commented Jul 23, 2018 •

edited by ghost

Loading

ghost commented Aug 1, 2018 •

edited by ghost

Loading

ghost commented Aug 1, 2018 •

edited by ghost

Loading

ghost commented Aug 1, 2018 •

edited by ghost

Loading

jnm2 commented Aug 2, 2018 •

edited

Loading

jnm2 commented Aug 14, 2018 •

edited

Loading

jnm2 commented Aug 14, 2018 •

edited

Loading

ghost commented Sep 15, 2018 •

edited by ghost

Loading

ghost left a comment •

edited by ghost

Loading

ghost Sep 17, 2018 •

edited by ghost

Loading

ghost Sep 23, 2018 •

edited by ghost

Loading

ghost Sep 18, 2018 •

edited by ghost

Loading

jnm2 Sep 18, 2018 •

edited

Loading

ghost Sep 26, 2018 •

edited by ghost

Loading