fix: Updates container searches for non-generic types (#1567)

Projects/Server.Tests/Tests/Items/ContainerTests.cs

@@ -1,3 +1,4 @@
+using System;
 using System.Collections.Generic;
 using Server.Items;
 using Xunit;
@@ -70,4 +71,54 @@ public class ContainerTests : IClassFixture<ServerFixture>
 
         Assert.Null(staticItem);
     }
+
+    [Fact]
+    public void TestFindItemsByTypeShouldThrowWhenModified()
+    {
+        var container = new Container((Serial)0x1);
+        container.AddItem(new Item((Serial)0x2));
+        var staticItem = new Static((Serial)0x3);
+        container.AddItem(staticItem);
+        container.AddItem(new Item((Serial)0x4));
+
+        Assert.Throws<InvalidOperationException>(
+            () =>
+            {
+                foreach (var item in container.FindItemsByType<Static>())
+                {
+                    if (item == staticItem)
+                    {
+                        container.RemoveItem(staticItem);
+                    }
+                }
+            }
+        );
+    }
+
+    [Fact]
+    public void TestEnumerateItemsByTypeWhenModified()
+    {
+        var container = new Container((Serial)0x1);
+
+        var item1 = new Item((Serial)0x2);
+        container.AddItem(item1);
+        var item2 = new Static((Serial)0x3);
+        container.AddItem(item2);
+        var item3 = new Item((Serial)0x4);
+        container.AddItem(item3);
+
+        foreach (var item in container.EnumerateItemsByType<Static>())
+        {
+            if (item == item2)
+            {
+                container.RemoveItem(item2);
+            }
+        }
+
+        Assert.Equal(2, container.Items.Count);
+        Assert.Collection(container.Items,
+            item => Assert.Equal(item1, item),
+            item => Assert.Equal(item3, item)
+        );
+    }
 }

Projects/Server/Items/Container.Enumerable.cs

@@ -22,20 +22,17 @@ namespace Server.Items;
 
 public partial class Container
 {
-    [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public FindItemsByTypeEnumerator<Item> FindItems(bool recurse = true, Predicate<Item> predicate = null)
-        => FindItemsByType(recurse, predicate);
-
     /// <summary>
     ///     Performs a breadth-first search through all the <see cref="Item" />s and
     ///     nested <see cref="Container" />s within this <see cref="Container" />.
     /// </summary>
     /// <remarks>
-    ///     DO NOT consume, delete, or move items while iterating
+    ///     DO NOT consume, delete, or move items while iterating with any FindItemByType or FindItems overloads
     /// </remarks>
     /// <example>
     /// <code>
     ///     var total = 0;
+    ///
     ///     foreach (var gold in cont.FindItemsByType&lt;Gold&gt;())
     ///     {
     ///         total += gold.Amount;
@@ -57,12 +54,20 @@ public partial class Container
     ///     <paramref name="predicate" />.
     /// </returns>
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public FindItemsByTypeEnumerator<T> FindItemsByType<T>(bool recurse = true, Predicate<T> predicate = null)
+    public FindItemsByTypeEnumerator<T> FindItemsByType<T>(bool recurse = true, Predicate<T> predicate = null) where T : Item =>
-        where T : Item => new(this, recurse, predicate);
+        new(this, recurse, predicate);
 
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public QueuedItemsEnumerator<Item> EnumerateItems(bool recurse = true, Predicate<Item> predicate = null)
+    public FindItemsByTypeEnumerator<Item> FindItemsByType(Type type, bool recurse = true) =>
-        => EnumerateItemsByType(recurse, predicate);
+        new(this, recurse, type.IsInstanceOfType);
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public FindItemsByTypeEnumerator<Item> FindItemsByType(Type[] types, bool recurse = true) =>
+        new(this, recurse, item => item.InTypeList(types));
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public FindItemsByTypeEnumerator<Item> FindItems(bool recurse = true, Predicate<Item> predicate = null) =>
+        new(this, recurse, predicate);
 
     /// <summary>
     ///     Safely enumerates items using a breadth-first search through all the <see cref="Item" />s and
@@ -70,7 +75,8 @@ public partial class Container
     /// </summary>
     /// <remarks>
     ///    Use EnumerateItemsByType for situations where the item might be manipulated, consumed, or moved.
-    ///    Note: This method scans through the container before returning the enumerator for iteration.
+    ///    Note: This method scans through the container before returning the enumerator for iteration and therefore
+    ///    incurs a performance penalty from the overhead.
     /// </remarks>
     /// <example>
     /// <code>
@@ -98,16 +104,10 @@ public partial class Container
     ///     <paramref name="predicate" />.
     /// </returns>
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public QueuedItemsEnumerator<T> EnumerateItemsByType<T>(bool recurse = true, Predicate<T> predicate = null)
+    public PooledRefQueue<T> EnumerateItemsByType<T>(bool recurse = true, Predicate<T> predicate = null) where T : Item
-        where T : Item => new(QueueItemsByType(recurse, predicate));
-
-    [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public PooledRefQueue<Item> QueueItems(bool recurse = true, Predicate<Item> predicate = null) =>
-        QueueItemsByType(recurse, predicate);
-
-    public PooledRefQueue<T> QueueItemsByType<T>(bool recurse = true, Predicate<T> predicate = null) where T : Item
     {
-        var queue = PooledRefQueue<T>.Create();
+        var queue = PooledRefQueue<T>.Create(128);
+
         foreach (var item in FindItemsByType(recurse, predicate))
         {
             queue.Enqueue(item);
@@ -117,12 +117,45 @@ public partial class Container
     }
 
     [MethodImpl(MethodImplOptions.AggressiveInlining)]
-    public PooledRefList<Item> ListItems(bool recurse = true, Predicate<Item> predicate = null) =>
+    public PooledRefQueue<Item> EnumerateItemsByType(Type type, bool recurse = true)
-        ListItemsByType(recurse, predicate);
+    {
+        var queue = PooledRefQueue<Item>.Create(128);
+
+        foreach (var item in FindItemsByType<Item>(recurse))
+        {
+            if (type.IsInstanceOfType(item))
+            {
+                queue.Enqueue(item);
+            }
+        }
+
+        return queue;
+    }
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public PooledRefQueue<Item> EnumerateItemsByType(Type[] types, bool recurse = true)
+    {
+        var queue = PooledRefQueue<Item>.Create(128);
+
+        foreach (var item in FindItemsByType<Item>(recurse))
+        {
+            if (item.InTypeList(types))
+            {
+                queue.Enqueue(item);
+            }
+        }
+
+        return queue;
+    }
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public PooledRefQueue<Item> EnumerateItems(bool recurse = true, Predicate<Item> predicate = null) =>
+        EnumerateItemsByType(recurse, predicate);
 
     public PooledRefList<T> ListItemsByType<T>(bool recurse = true, Predicate<T> predicate = null) where T : Item
     {
-        var list = PooledRefList<T>.Create();
+        var list = PooledRefList<T>.Create(128);
+
         foreach (var item in FindItemsByType(recurse, predicate))
         {
             list.Add(item);
@@ -131,22 +164,69 @@ public partial class Container
         return list;
     }
 
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public PooledRefList<Item> ListItemsByType(Type type, bool recurse = true)
+    {
+        var list = PooledRefList<Item>.Create(128);
+
+        foreach (var item in FindItemsByType<Item>(recurse))
+        {
+            if (type.IsInstanceOfType(item))
+            {
+                list.Add(item);
+            }
+        }
+
+        return list;
+    }
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public PooledRefList<Item> ListItemsByType(Type[] types, bool recurse = true)
+    {
+        var list = PooledRefList<Item>.Create(128);
+
+        foreach (var item in FindItemsByType<Item>(recurse))
+        {
+            if (item.InTypeList(types))
+            {
+                list.Add(item);
+            }
+        }
+
+        return list;
+    }
+
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public PooledRefList<Item> ListItems(bool recurse = true, Predicate<Item> predicate = null) =>
+        ListItemsByType(recurse, predicate);
+
     public ref struct FindItemsByTypeEnumerator<T> where T : Item
     {
+        private const string InvalidOperation_EnumFailedVersion =
+            "Container was modified after enumerator was instantiated. Use Container.EnumerateItems method instead for safe enumerations.";
+
         private PooledRefQueue<Container> _containers;
         private Span<Item> _items;
         private int _index;
         private T _current;
-        private bool _recurse;
+        private readonly bool _recurse;
-        private Predicate<T> _predicate;
+        private readonly Predicate<T> _predicate;
+        private Container _currentContainer;
+        private int _version;
 
         public FindItemsByTypeEnumerator(Container container, bool recurse, Predicate<T> predicate)
         {
-            _containers = PooledRefQueue<Container>.Create();
+            _containers = PooledRefQueue<Container>.Create(_recurse ? 64 : 0);
 
-            if (container?.m_Items != null)
+            if (container != null)
             {
-                _items = CollectionsMarshal.AsSpan(container.m_Items);
+                if (container.m_Items != null)
+                {
+                    _items = CollectionsMarshal.AsSpan(container.m_Items);
+                }
+
+                _currentContainer = container;
+                _version = container._version;
             }
 
             _current = default;
@@ -163,8 +243,11 @@ public partial class Container
         {
             while (_containers.TryDequeue(out var c))
             {
+                _currentContainer = c;
                 _items = CollectionsMarshal.AsSpan(c.m_Items);
                 _index = 0;
+                _version = c._version;
+
                 if (SetNextItem())
                 {
                     return true;
@@ -177,6 +260,11 @@ public partial class Container
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         private bool SetNextItem()
         {
+            if (_version != _currentContainer._version)
+            {
+                throw new InvalidOperationException(InvalidOperation_EnumFailedVersion);
+            }
+
             while (_index < _items.Length)
             {
                 Item item = _items[_index++];
@@ -187,6 +275,11 @@ public partial class Container
 
                 if (item is T t && _predicate?.Invoke(t) != false)
                 {
+                    if (_version != _currentContainer._version)
+                    {
+                        throw new InvalidOperationException(InvalidOperation_EnumFailedVersion);
+                    }
+
                     _current = t;
                     return true;
                 }
@@ -207,40 +300,4 @@ public partial class Container
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         public FindItemsByTypeEnumerator<T> GetEnumerator() => this;
     }
-
-    public ref struct QueuedItemsEnumerator<T> where T : Item
-    {
-        private PooledRefQueue<T> _queue;
-        private T _current;
-
-        public QueuedItemsEnumerator(PooledRefQueue<T> queue)
-        {
-            _queue = queue;
-            _current = default;
-        }
-
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public bool MoveNext()
-        {
-            if (_queue.TryDequeue(out var item))
-            {
-                _current = item;
-                return true;
-            }
-
-            return false;
-        }
-
-        public T Current
-        {
-            [MethodImpl(MethodImplOptions.AggressiveInlining)]
-            get => _current;
-        }
-
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public void Dispose() => _queue.Dispose();
-
-        [MethodImpl(MethodImplOptions.AggressiveInlining)]
-        public QueuedItemsEnumerator<T> GetEnumerator() => this;
-    }
 }

Projects/Server/Items/Container.cs

@@ -1,7 +1,6 @@
 using System;
 using System.Collections.Generic;
 using System.IO;
-using System.Runtime.InteropServices;
 using Server.Collections;
 using Server.Logging;
 using Server.Network;
@@ -27,6 +26,7 @@ public partial class Container : Item
 
     private int m_TotalItems;
     private int m_TotalWeight;
+    private int _version;
 
     public Container(int itemID) : base(itemID)
     {
@@ -464,6 +464,18 @@ public partial class Container : Item
         }
     }
 
+    public override void OnItemAdded(Item item)
+    {
+        base.OnItemAdded(item);
+        _version++;
+    }
+
+    public override void OnItemRemoved(Item item)
+    {
+        base.OnItemRemoved(item);
+        _version++;
+    }
+
     public virtual bool OnStackAttempt(Mobile from, Item stack, Item dropped) =>
         CheckHold(from, dropped, true, false) && stack.StackWith(from, dropped);
 
@@ -800,14 +812,7 @@ public partial class Container : Item
             throw new ArgumentNullException(nameof(grouper));
         }
 
-        using var typedItems = PooledRefList<Item>.Create();
+        using var typedItems = ListItemsByType(type, recurse);
-        foreach (var item in FindItems(recurse))
-        {
-            if (type.IsInstanceOfType(item))
-            {
-                typedItems.Add(item);
-            }
-        }
 
         var groups = new List<List<Item>>();
         var idx = 0;
@@ -815,9 +820,10 @@ public partial class Container : Item
         while (idx < typedItems.Count)
         {
             var a = typedItems[idx++];
-            var group = new List<Item>();
+            var group = new List<Item>
-
+            {
-            group.Add(a);
+                a
+            };
 
             while (idx < typedItems.Count)
             {
@@ -920,14 +926,8 @@ public partial class Container : Item
         for (var i = 0; i < types.Length; ++i)
         {
             var type = types[i];
-            using var typedItems = PooledRefList<Item>.Create();
+
-            foreach (var item in FindItems(recurse))
+            using var typedItems = ListItemsByType(type, recurse);
-            {
-                if (type.IsInstanceOfType(item))
-                {
-                    typedItems.Add(item);
-                }
-            }
 
             var groups = new List<List<Item>>();
             var idx = 0;
@@ -935,9 +935,10 @@ public partial class Container : Item
             while (idx < typedItems.Count)
             {
                 var a = typedItems[idx++];
-                var group = new List<Item>();
+                var group = new List<Item>
-
+                {
-                group.Add(a);
+                    a
+                };
 
                 while (idx < typedItems.Count)
                 {
@@ -1043,19 +1044,20 @@ public partial class Container : Item
 
         for (var i = 0; i < types.Length; ++i)
         {
-            var typedItems = CollectionsMarshal.AsSpan(FindItemsByType(types[i], recurse));
+            using var typedItems = ListItemsByType(types[i], recurse);
 
             var groups = new List<List<Item>>();
             var idx = 0;
 
-            while (idx < typedItems.Length)
+            while (idx < typedItems.Count)
             {
                 var a = typedItems[idx++];
-                var group = new List<Item>();
+                var group = new List<Item>
+                {
+                    a
+                };
 
-                group.Add(a);
+                while (idx < typedItems.Count)
-
-                while (idx < typedItems.Length)
                 {
                     var b = typedItems[idx];
                     var v = grouper(a, b);
@@ -1146,16 +1148,19 @@ public partial class Container : Item
             throw new ArgumentException("length of types and amounts must match");
         }
 
-        var items = new List<Item>[types.Length];
+        var items = new Item[types.Length][];
         var totals = new int[types.Length];
 
         for (var i = 0; i < types.Length; ++i)
         {
-            items[i] = FindItemsByType(types[i], recurse);
+            using var typedItems = ListItemsByType(types[i], recurse);
 
-            for (var j = 0; j < items[i].Count; ++j)
+            items[i] = new Item[typedItems.Count];
+
+            for (var j = 0; j < typedItems.Count; ++j)
             {
-                totals[i] += items[i][j].Amount;
+                items[i][j] = typedItems[j];
+                totals[i] += typedItems[j].Amount;
             }
 
             if (totals[i] < amounts[i])
@@ -1168,7 +1173,7 @@ public partial class Container : Item
         {
             var need = amounts[i];
 
-            for (var j = 0; j < items[i].Count; ++j)
+            for (var j = 0; j < items[i].Length; ++j)
             {
                 var item = items[i][j];
 
@@ -1201,19 +1206,19 @@ public partial class Container : Item
             throw new ArgumentException("length of types and amounts must match");
         }
 
-        var items = new List<Item>[types.Length];
+        var items = new Item[types.Length][];
         var totals = new int[types.Length];
 
         for (var i = 0; i < types.Length; ++i)
         {
-            var itemList = items[i] = new List<Item>();
+            using var typedItems = ListItemsByType(types[i], recurse);
-            foreach (var item in FindItems())
+
+            items[i] = new Item[typedItems.Count];
+
+            for (var j = 0; j < typedItems.Count; ++j)
             {
-                if (types[i].IsInstanceOfType(item))
+                items[i][j] = typedItems[j];
-                {
+                totals[i] += typedItems[j].Amount;
-                    totals[i] += item.Amount;
-                    itemList.Add(item);
-                }
             }
 
             if (totals[i] < amounts[i])
@@ -1226,7 +1231,7 @@ public partial class Container : Item
         {
             var need = amounts[i];
 
-            for (var j = 0; j < items[i].Count; ++j)
+            for (var j = 0; j < items[i].Length; ++j)
             {
                 var item = items[i][j];
 
@@ -1255,20 +1260,17 @@ public partial class Container : Item
     public bool ConsumeTotal(Type type, int amount = 1, bool recurse = true, OnItemConsumed callback = null)
     {
         var total = 0;
-        using var items = PooledRefQueue<Item>.Create();
+
+        using var typedItems = ListItemsByType(type, recurse);
 
         // First pass, compute total
-        foreach (var item in FindItems(recurse))
+        foreach (var item in typedItems)
         {
-            if (type.IsInstanceOfType(item))
+            total += item.Amount;
-            {
-                items.Enqueue(item);
 
-                total += item.Amount;
+            if (total >= amount)
-                if (total >= amount)
+            {
-                {
+                break;
-                    break;
-                }
             }
         }
 
@@ -1277,10 +1279,8 @@ public partial class Container : Item
         {
             var need = amount;
 
-            while (items.Count > 0)
+            foreach (var item in typedItems)
             {
-                var item = items.Dequeue();
-
                 var theirAmount = item.Amount;
 
                 if (theirAmount < need)
@@ -1308,6 +1308,7 @@ public partial class Container : Item
         var consumed = 0;
 
         using var toDelete = PooledRefQueue<Item>.Create();
+
         RecurseConsumeUpTo(this, type, amount, recurse, ref consumed, toDelete);
 
         while (toDelete.Count > 0)
@@ -1368,14 +1369,7 @@ public partial class Container : Item
 
         var best = 0;
 
-        using var typedItems = PooledRefList<Item>.Create();
+        using var typedItems = ListItemsByType(type, recurse);
-        foreach (var item in FindItems(recurse))
-        {
-            if (type.IsInstanceOfType(item))
-            {
-                typedItems.Add(item);
-            }
-        }
 
         var groups = new List<List<Item>>();
         var idx = 0;
@@ -1437,12 +1431,12 @@ public partial class Container : Item
 
         var best = 0;
 
-        var typedItems = CollectionsMarshal.AsSpan(FindItemsByType(types, recurse));
+        var typedItems = ListItemsByType(types, recurse);
 
         var groups = new List<List<Item>>();
         var idx = 0;
 
-        while (idx < typedItems.Length)
+        while (idx < typedItems.Count)
         {
             var a = typedItems[idx++];
             var group = new List<Item>
@@ -1450,7 +1444,7 @@ public partial class Container : Item
                 a
             };
 
-            while (idx < typedItems.Length)
+            while (idx < typedItems.Count)
             {
                 var b = typedItems[idx];
                 var v = grouper(a, b);
@@ -1474,6 +1468,7 @@ public partial class Container : Item
         {
             var items = groups[j].ToArray();
             var total = 0;
+
             foreach (var item in items)
             {
                 total += item.Amount;
@@ -1499,19 +1494,20 @@ public partial class Container : Item
 
         for (var i = 0; i < types.Length; ++i)
         {
-            var typedItems = CollectionsMarshal.AsSpan(FindItemsByType(types[i], recurse));
+            using var typedItems = ListItemsByType(types[i], recurse);
 
             var groups = new List<List<Item>>();
             var idx = 0;
 
-            while (idx < typedItems.Length)
+            while (idx < typedItems.Count)
             {
                 var a = typedItems[idx++];
-                var group = new List<Item>();
+                var group = new List<Item>
+                {
+                    a
+                };
 
-                group.Add(a);
+                while (idx < typedItems.Count)
-
-                while (idx < typedItems.Length)
                 {
                     var b = typedItems[idx];
                     var v = grouper(a, b);
@@ -1554,6 +1550,7 @@ public partial class Container : Item
     public int GetAmount(Type type, bool recurse = true)
     {
         var total = 0;
+
         foreach (var item in FindItems(recurse))
         {
             if (type.IsInstanceOfType(item))
@@ -1568,6 +1565,7 @@ public partial class Container : Item
     public int GetAmount(Type[] types, bool recurse = true)
     {
         var total = 0;
+
         foreach (var item in FindItems(recurse))
         {
             if (item.InTypeList(types))
@@ -1578,35 +1576,6 @@ public partial class Container : Item
 
         return total;
     }
-
-    public List<Item> FindItemsByType(Type type, bool recurse = true)
-    {
-        var items = new List<Item>();
-        foreach (var item in FindItems(recurse))
-        {
-            if (type.IsInstanceOfType(item))
-            {
-                items.Add(item);
-            }
-        }
-
-        return items;
-    }
-
-    public List<Item> FindItemsByType(Type[] types, bool recurse = true)
-    {
-        var items = new List<Item>();
-        foreach (var item in FindItems(recurse))
-        {
-            if (item.InTypeList(types))
-            {
-                items.Add(item);
-            }
-        }
-
-        return items;
-    }
-
     public Item FindItemByType(Type type, bool recurse = true)
     {
         foreach (var item in FindItems(recurse))

Projects/Server/Items/Item.cs

@@ -209,7 +209,6 @@ public class Item : IHued, IComparable<Item>, ISpawnable, IObjectPropertyListEnt
         m_ItemID = itemID;
         Serial = World.NewItem;
 
-        // m_Items = new ArrayList( 1 );
         Visible = true;
         Movable = true;
         Amount = 1;

Projects/UOContent/Commands/Generic/Implementors/ContainedCommandImplementor.cs

@@ -67,12 +67,9 @@ namespace Server.Commands.Generic
 
                 var list = new List<object>();
 
-                foreach (var item in cont.FindItems())
+                foreach (var item in cont.EnumerateItems(true, ext.IsValid))
                 {
-                    if (ext.IsValid(item))
+                    list.Add(item);
-                    {
-                        list.Add(item);
-                    }
                 }
 
                 ext.Filter(list);

Projects/UOContent/Engines/ML Quests/Objectives/DeliverObjective.cs

@@ -193,7 +193,7 @@ namespace Server.Engines.MLQuests.Objectives
 
             var left = Objective.Amount;
 
-            foreach (var item in pack.EnumerateItemsByType<Item>(false, ClaimTypePredicate))
+            foreach (var item in pack.EnumerateItems(false, ClaimTypePredicate))
             {
                 if (left == 0)
                 {

Projects/UOContent/Items/Containers/SalvageBag.cs

@@ -251,7 +251,7 @@ public partial class SalvageBag : Bag
         var salvaged = 0;
         var notSalvaged = 0;
 
-        foreach (var item in EnumerateItemsByType<Item>())
+        foreach (var item in EnumerateItems())
         {
             if (item is not IScissorable scissorable)
             {

Projects/UOContent/Items/Misc/Key.cs

@@ -97,7 +97,7 @@ public partial class Key : Item
             return false;
         }
 
-        foreach (var item in cont.EnumerateItems())
+        foreach (var item in cont.FindItems())
         {
             if (item is Key key)
             {

Projects/UOContent/Mobiles/PlayerMobile.cs

@@ -2394,7 +2394,7 @@ namespace Server.Mobiles
             // This fixes a "bug" where players put blessed items in nested bags and they were dropped on death
             if (Core.AOS && Backpack?.Deleted == false)
             {
-                foreach (var item in Backpack.EnumerateItemsByType<Item>(predicate: FindItems_Callback))
+                foreach (var item in Backpack.EnumerateItems(true, FindItems_Callback))
                 {
                     Backpack.AddItem(item);
                 }