ItemDictionary<T>.Enumerator has an unnecessary finalizer

[elachlan]

Fixes #7208

[KalleOlaviNiemitalo]

This looks like the nested enumerator type could be replaced with an iterator method: nested foreach loops and yield return. I don't know how the performance would be, though.

Iterators do not support IEnumerator.Reset() but the current implementation of Reset is bogus anyway: it resets _itemEnumerator too, so if you had the hierarchy ((A B C) (D E F)) and the current item is E, then after the reset, it will enumerate (D E F A B C D E F) even though the right answer would be just (A B C D E F). Which perhaps indicates that nothing actually calls Reset.

[ladipro]

@KalleOlaviNiemitalo that's indeed a bug, thank you. It looks like we should do

_itemEnumerator = GetNextItemEnumerator();

instead of resetting whatever _itemEnumerator is currently pointing to. Out of scope of this PR but would be nice to fix for sure. Also +1 on trying to replace the class with an iterator.

[KalleOlaviNiemitalo]

In principle, Reset and MoveNext should also call _itemEnumerator?.Dispose() before assigning _itemEnumerator = GetNextItemEnumerator(). In practice, _itemEnumerator is always a LinkedList<T>.Enumerator whose Dispose() method would do nothing anyway. Although LinkedList<T> is not sealed, ItemDictionary<T> does not let its callers add instances of any derived types to _itemLists.

[elachlan]

Could one of you please write up an issue to spec out this suggestion? It sounds like a good idea.

[KalleOlaviNiemitalo]

Filed #7286.