AnyOfConstraint enumerates multiple times

[jnm2]

@oznetmaster brought to our attention that we are enumerating AnyOfConstraint's expected values multiple times because of this guard statement:

nunit/src/NUnitFramework/framework/Constraints/AnyOfConstraint.cs

Lines 43 to 47 in facc4c8

	public AnyOfConstraint(IEnumerable expected) : base(expected)
	{
	Guard.ArgumentNotNull(expected, nameof(expected));
	Guard.ArgumentValid(expected.Cast<object>().Any(),
	$"{nameof(AnyOfConstraint)} requires non-empty expected collection!", nameof(expected));

The IEnumerable contract prevents us from being about to conclude anything from one enumeration about the next enumeration. If we take IEnumerable, we should convert it to an array as its only enumeration and use the array from there on.

I think we should be transparent about the mechanics at play and take an ICollection here instead.

[oznetmaster]

How does an ICollection solve the problem? It still can have multiple enumeration issues, I believe.

Maybe it should be an IList if you are opposed to it simply being an object[].

[jnm2]

@oznetmaster No, ICollection, ICollection<>, and IReadOnlyCollection<> all have the semantic that they are finite collections of non-transient items and not merely arbitrary enumerations.

IEnumerable and IEnumerable<> are the only types that I have ever seen warnings for.

IList, IList<> and IReadOnlyList<> mean "the order is significant." Choosing ICollection signals that this is an unordered collection of persistent items.

[ChrisMaddock]

ICollecyion sounds good to me. My understanding on semantics is inline with Joseph’s, but I’ll stand to be corrected here.

I’ve added this to 3.10 - if we’re going to change this, I assume we should do so before this feature is released?

[ChrisMaddock]

@Dreamescaper Would you be interested in making this change?

[CharliePoole]

Perhaps it would be simpler to issue the error when we iterate in order to apply the constraint, rather than on construction of the expectation. FWIW, this is the kind of thing uise of separate args can avoid. 😸

[oznetmaster]

I find nothing that documents the semantics suggested by @jnm2 .

ICollection actually inherits its GetEnumerator from the IEnumerable interface, and adds no new semantics to that.

The only real addition is the Count property, which does add finite and countable semantics, but does not apparently imply or restrict the issue of multiple enumerations.

IList does not imply any specific order, just that the collection is indexable. It has a different enumerator then ICollection/IEnumerable.

[oznetmaster]

Again, since the two AnyOf methods only take a params object[], why is there any value in having the AnyOfConstraint take anything besides an object[]?

[jnm2]

@CharliePoole

Perhaps it would be simpler to issue the error when we iterate in order to apply the constraint, rather than on construction of the expectation. FWIW, this is the kind of thing uise of separate args can avoid.

I asked @oznetmaster about this and he said he expected it to only be enumerated once for the following code:

var someConstraint = Is.AnyOf(weirdEnumerable);
Assert.That(x, someConstraint);
Assert.That(y, someConstraint);

Which would rule out that option.

@oznetmaster

ICollection actually inherits its GetEnumerator from the IEnumerable interface, and adds no new semantics to that.

As does IList. The semantics come from the name, the presence of the guaranteed count, and from convention. The word "collection" only makes sense if items are being collected together over time.

Again, since the two AnyOf methods only take a params object[], why is there any value in having the AnyOfConstraint take anything besides an object[]?

I don't object. We can always loosen it.

[jnm2]

Of course ICollection can mutate over time, but so can IList and object[] and we don't confuse them with IEnumerable's considerably freer semantics.

I'm keeping in mind that ICollection also has Add, Remove, Clear, CopyTo, and Contains. If that says anything, it's practically asking to be enumerated multiple times. Otherwise it's a pretty useless API.

IReadOnlyCollection<> does merely put Count on top of GetEnumerator, but it's clearly a narrowing of the persistence semantics of ICollection, not a freeing.

[CharliePoole]

@jnm2 I don't think it's a reasonable demand to put on an IEnumerator that it always return the same values or that it have a fixed set of values. Granted, in most cases both things are true, but there are plenty of examples of using an enumerator for an unbounded series or to return random values. It's generally accepted that an enumerator has much more abstract behavior than a collection. And although the documentation for ICollection may not say so, it's generally supposed to represent a... collection!

In this case, I would say that use of IEnumerable for the list of matching values was probably an over-generalization on our part. There's no way we want to verify that an actual value is equal to one of an infinite series of numbers, for example. The difference from other uses of IEnumerable` in NUnit comes from the fact that we're talking about expected values here.

I'd say change it to ICollection as the simplest solution. Then we can just look at the Count.

[jnm2]

@CharliePoole I was countering @oznetmaster's expectation that ICollection as well as IEnumerable should not be multiply enumerated. I would strongly agree with every point you've made there. A collection certainly is a collection!

I was hoping we'd land on ICollection as well, but I guess I don't really care if it's that or object[] since I'm not intending to use the constructor directly.

[oznetmaster]

ICollection does not have Add, Remove, or Clear.

https://msdn.microsoft.com/en-us/library/system.collections.icollection(v=vs.110).aspx?f=255&MSPPError=-2147217396

[oznetmaster]

And there is no defined expectation that multiple enumerations over an ICollection will work, since it uses the IEnumerable enumerator semantics.

I agree that in most cases that should be the case. But again, since this is only being called with an object[] argument, why the continued effort to generalize it?

[CharliePoole]

Generally, I try to accept the most general argument that's reasonable. I think IEnumerable was unreasonable here but ICollection is not. Using ICollection permits constructing the constraint from a List for example, which is very handy if you generate the accepted values programmatically.

As I'm sure you know, there is no formal semantic definition of any interface - unfortunately. Our industry describes syntax in a very structured way but semantics only vaguely. Nevertheless, the distinction between enumerators and collections we have been discussing is something I have heard and read about for years.

[jnm2]

@oznetmaster You're right, but ICollection<> does. The generic and non-generic versions of IEnumerable, ICollection and IList are treated everywhere in the BCL as having the same semantics.

[Dreamescaper]

That was one of the reasons why I put not-empty guard to Is.AnyOf initially.

Personally I know examples of proxied IList, that is reenumerated on each access, however that is very rare situation, and usually very non-intuitive.

But yeah, we can make it object[]. Don't think someone will use the constant directly anyway.
Btw, that would make custom .ToString unneeded.

[Dreamescaper]

Another option, that I think I like better - to have both object[] and IEnumerable constructors, and latter one simply converts it to array and calls the first one.
This way no unneeded conversions are done in most cases (if we use Is.AnyOf syntax), and other collection types are supported as well.

[jnm2]

@Dreamescaper If we have both constructors, we will still have to solve this issue for when the IEnumerable constructor is directly used unless we make it internal or private.

[Dreamescaper]

Seems like we have a misunderstanding. My suggestion is something like this:

        private readonly object[] _expected;

        public AnyOfConstraint(IEnumerable expected)
            : this(expected.Cast<object>().ToArray())
        {

        }

        public AnyOfConstraint(object[] expected) : base(expected)
        {
            // Guards etc
            _expected = expected;
        }

[jnm2]

I'm sorry, I did misread that! That would be fine if we wanted to be more flexible than ICollection.
(Would need expected?.Cast<object>().ToArray().)

[CharliePoole]

I already convinced myself that IEnumerable was over-generalized.

[jnm2]

Too bad we can't use IReadOnlyCollection<>, but I agree. Let's go with less generalization until we have a real use case to evaluate.

[Dreamescaper]

So what's the decision? Simply use ICollection instead of IEnumerable?

Or use object[]? I like using array a little bit better, because
a) it guarantees that collection won't be re-iterated (which is possible even for IList, technically)
b) we can remove custom .ToString, and use inherited one

P.S. Don't really matters to me, since I believe that 98% of usages would be via Is syntax :)

[oznetmaster]

Obviously, I prefer the object[] option.

[jnm2]

I could still go either way. object[] is the least amount of generalization so in that sense it's even better than ICollection until we have a real demand for it. Since we can always go the other direction after shipping it, let's go with object[].

[Dreamescaper]

Great, will send PR soon

[Dreamescaper]

Keep in mind, however, that without additional constructor we won't be able to call .ToArray for arbitrary collections easily. We'll need to do something like myList.Cast<object>().ToArray().
Same applies for Is syntax as well.

[jnm2]

In my experience, non-generic collections are few and far between. Let's cross that bridge if we ever come to it.

[Dreamescaper]

I wasn't talking about non-generic collections.
That is not possible:

var myStringList = new List<string> {"A", "B", "C"};
Is.AnyOf(myStringList.ToArray());

//or that 
new AnyOfConstraint(myStringList.ToArray());

You can't cast string[] to object[], unlike IEnumerable

[jnm2]

Actually, you can.

I know how broken array variance is, but so long as it's only read and not written to...
https://blogs.msdn.microsoft.com/ericlippert/2007/10/17/covariance-and-contravariance-in-c-part-two-array-covariance/

[Dreamescaper]

Ha. Okay, you're right.
Can't do same with int's though.

[jnm2]

Let's see if it's actually a problem. For example if it's coming from a static field or test parameter, you could just create a new object[] in the first place.

[CharliePoole]

I lean toward ICollection but object[] is probably going to satisfy most usage. For everything we do, there's always the risk that somebody will want to use it in ways we didn't intend, of course.

[oznetmaster]

I must have missed something.

Why not make AnyOf generic? Sort of:

		public static AnyOfConstraint AnyOf<T> (params T[] expected)

and other mods accordingly

[jnm2]

I almost suggested it, but we don't want to make the constraint class generic.

[CharliePoole]

@oznetmaster I suggested that at one point but people felt there would be too many cases where the parameter type couldn't be deduced.

[oznetmaster]

One can always explicitly add the type if necessary.

However, thinking about my usage for this, I would think that it would always be deducible for the cases where I might use it.