XPathNodeIterator should implement IEnumerable<XPathNavigator>

[krwq]

Currently XPathNodeIterator implements only IEnumerable interface. XPathNodeIterator always iterates over objects of type XPathNavigator and thus every usage ends up with casting back and forth XPathNavigator --> object --> XPathNavigator in a loop. Implementing IEnumerable on this type would allow to avoid this indirection and in cases where iterating is useful improve perf.

cc: @tmat

EDIT 2017/12/28: In PR: dotnet/corefx#26083

Requested API changes:

namespace System.Xml.XPath
{
...
-    public abstract class XPathNodeIterator : ICloneable, IEnumerable
+    public abstract class XPathNodeIterator : ICloneable, IEnumerable<XPathNavigator>
...

[kassemsandarusi]

I have a few questions about this. When playing around with implementing this, I noticed that the ApiCompat tool started to fail.

My understanding is that even though the GetEnumerator method (public virtual IEnumerable<XPathNodeIterator> GetEnumerator()) has technically changed, it should not break any existing code that is expecting a non-generic IEnumerable to be returned.

1. Is this actually breaking .NET Standard API Compatibility? Am I missing something?
2. I can technically make the original non-generic GetEnumerator method the one that is not fully qualified in order to ensure API compatibility, but does that not seem to defeat the purpose?
   • example change IEnumerator<XPathNodeIterator> IEnumerable<XPathNodeIterator>.GetEnumerator() and public virtual IEnumerator GetEnumerator().

My terminology use may be a bit primitive so I apologize in advance.

[svick]

@kassemsandarusi

It is a breaking change according to corefx breaking change rules.

Consider for example this code:

void M(XPathNodeIterator iterator) => PrintType(iterator.GetEnumerator());

void PrintType<T>(T ignored) => Console.WriteLine(typeof(T));

Before the change, it would print System.Collections.IEnumerator, after it, it would print System.Collections.Generic.IEnumerator`1[System.Xml.XPath.XPathNavigator].

Even worse, the method is virtual, so it would break any inheritors that override that method.

I think what you need is to leave the existing GetEnumerator() method as is and implement the interface explicitly (which is what you mention in point 2). I think it does not defeat the purpose of this change: any APIs what work with IEnumerable<T> will start working (including LINQ). It won't enable foreach (var x in iterator) to work well, but I don't think that's possible without making a breaking change.

[kassemsandarusi]

@svick That reference doc for breaking rule changes is incredibly useful. And in general, thanks for your advice on this issue.

[karelz]

@krwq what is the API shape? Please add formal API proposal (see the example there).

[krwq]

@karelz, I thought the title was enough but adding for clarity

[karelz]

@krwq we should probably keep the IEnumerable as explicit interface -- I think there are subtle differences in explicitly / implicitly implemented interfaces.
Can you please add some justification to the top post? For people who don't have deep background in XML.

[krwq]

@karelz done

[terrajobst]

Video

Implementing IE<X> additionally would be fine, but this doesn't buy you anything for avoiding the cast in the foreach scenario as the compiler will still call through the existing non-generic GetEnumerator() method.

Am I missing anything?

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/dOpH6bUNbcw?t=1567 (8 min duration)

[kassemsandarusi]

@karelz Based on the conversation it sounds like the PR should be closed out till the api is approved. Does this sound correct?

[karelz]

Sounds like a very reasonable plan, if you don't mind. Sorry for the confusion.

[krwq]

Since the interface has to be implemented explicitly there will be no gain for existing apps from this work and little discoverability for new apps - I will close this issue for now - please reopen/comment if your app/library will gain anything from this work and we will reconsider given new data.