Fix ItemStack hashing and classified ingredient lookups

loader-common/src/main/java/org/cyclops/cyclopscore/helper/ItemStackHelpersCommon.java

@@ -109,12 +109,24 @@ public int getItemStackHashCode(ItemStack stack) {
         int result = 1;
         result = 37 * result + stack.getCount();
         result = 37 * result + stack.getItem().hashCode();
-        // Tags can be very large, and expensive to calculate, which is not needed for hashCodes.
-        // CompoundTag tagCompound = stack.getTag();
-        // result = 37 * result + (tagCompound != null ? tagCompound.hashCode() : 0);
+        if (hasComponentPatch(stack)) {
+            result = 37 * result + stack.getComponents().hashCode();
+        }
         // Not factoring in capability compatibility. Doing so would require either reflection (slow)
         // or an access transformer, it's highly unlikely that it'd be the only difference between
         // many ItemStacks in practice, and occasional hash code collisions are okay.
         return result;
     }
+
+    /**
+     * If the given stack carries data components that differ from its item's defaults.
+     *
+     * Only those have to take part in {@link #getItemStackHashCode(ItemStack)}.
+     *
+     * @param stack A non-empty stack.
+     * @return If the stack has a non-empty component patch.
+     */
+    protected boolean hasComponentPatch(ItemStack stack) {
+        return !stack.getComponentsPatch().isEmpty();
+    }
 }

loader-neoforge/src/main/java/org/cyclops/cyclopscore/helper/ItemStackHelpersNeoForge.java

@@ -10,4 +10,9 @@ public class ItemStackHelpersNeoForge extends ItemStackHelpersCommon {
     public ItemStack getCraftingRemainingItem(ItemStack itemStack) {
         return itemStack.getCraftingRemainder().create();
     }
+
+    @Override
+    protected boolean hasComponentPatch(ItemStack stack) {
+        return !stack.isComponentsPatchEmpty();
+    }
 }

loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientCollectionSingleClassified.java

@@ -176,13 +176,17 @@ public int count(T instance, M matchCondition) {
         }
         if (appliesToClassifier(matchCondition)) {
             IIngredientCollectionMutable<T, M> collection = this.classifiedCollections.get(getClassifier(instance));
-            if (collection != null) {
-                if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) {
-                    return collection.size();
-                } else {
-                    M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition());
-                    return collection.count(instance, subMatchCondition);
-                }
+            if (collection == null) {
+                // The match condition requires the classifier to be equal, so an absent classifier
+                // means nothing can match. Falling through to the unclassified path here would scan
+                // every instance for a result that is known to be zero.
+                return 0;
+            }
+            if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) {
+                return collection.size();
+            } else {
+                M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition());
+                return collection.count(instance, subMatchCondition);
             }
         }
         return super.count(instance, matchCondition);

loader-neoforge/src/main/java/org/cyclops/cyclopscore/ingredient/collection/IngredientMapSingleClassified.java

@@ -8,6 +8,7 @@
 
 import javax.annotation.Nullable;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -121,13 +122,17 @@ public int size() {
     public boolean containsKey(T instance, M matchCondition) {
         if (appliesToClassifier(matchCondition)) {
             IIngredientMapMutable<T, M, V> map = this.classifiedMaps.get(getClassifier(instance));
-            if (map != null) {
-                if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) {
-                    return true;
-                } else {
-                    M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition());
-                    return map.containsKey(instance, subMatchCondition);
-                }
+            if (map == null) {
+                // The match condition requires the classifier to be equal, so an absent classifier
+                // means nothing can match. Falling through to the unclassified path here would scan
+                // every key for a result that is known to be empty.
+                return false;
+            }
+            if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) {
+                return true;
+            } else {
+                M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition());
+                return map.containsKey(instance, subMatchCondition);
             }
         }
         return super.containsKey(instance, matchCondition);
@@ -137,13 +142,14 @@ public boolean containsKey(T instance, M matchCondition) {
     public int countKey(T instance, M matchCondition) {
         if (appliesToClassifier(matchCondition)) {
             IIngredientMapMutable<T, M, V> map = this.classifiedMaps.get(getClassifier(instance));
-            if (map != null) {
-                if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) {
-                    return map.size();
-                } else {
-                    M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition());
-                    return map.countKey(instance, subMatchCondition);
-                }
+            if (map == null) {
+                return 0;
+            }
+            if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) {
+                return map.size();
+            } else {
+                M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition());
+                return map.countKey(instance, subMatchCondition);
             }
         }
         return super.countKey(instance, matchCondition);
@@ -192,13 +198,14 @@ public Collection<V> values() {
     public Collection<V> getAll(T key, M matchCondition) {
         if (appliesToClassifier(matchCondition)) {
             IIngredientMapMutable<T, M, V> map = this.classifiedMaps.get(getClassifier(key));
-            if (map != null) {
-                if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) {
-                    return map.values();
-                } else {
-                    M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition());
-                    return map.getAll(key, subMatchCondition);
-                }
+            if (map == null) {
+                return Collections.emptyList();
+            }
+            if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) {
+                return map.values();
+            } else {
+                M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition());
+                return map.getAll(key, subMatchCondition);
             }
         }
         return super.getAll(key, matchCondition);
@@ -208,13 +215,14 @@ public Collection<V> getAll(T key, M matchCondition) {
     public IngredientSet<T, M> keySet(T key, M matchCondition) {
         if (appliesToClassifier(matchCondition)) {
             IIngredientMapMutable<T, M, V> map = this.classifiedMaps.get(getClassifier(key));
-            if (map != null) {
-                if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) {
-                    return map.keySet();
-                } else {
-                    M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition());
-                    return map.keySet(key, subMatchCondition);
-                }
+            if (map == null) {
+                return new IngredientHashSet<>(getComponent());
+            }
+            if (Objects.equals(getCategoryType().getMatchCondition(), matchCondition)) {
+                return map.keySet();
+            } else {
+                M subMatchCondition = getComponent().getMatcher().withoutCondition(matchCondition, getCategoryType().getMatchCondition());
+                return map.keySet(key, subMatchCondition);
             }
         }
         return super.keySet(key, matchCondition);

loader-neoforge/src/test/java/org/cyclops/cyclopscore/helper/TestItemStackHelpersHashCode.java

@@ -0,0 +1,137 @@
+package org.cyclops.cyclopscore.helper;
+
+import net.minecraft.core.Holder;
+import net.minecraft.core.MappedRegistry;
+import net.minecraft.core.component.DataComponentMap;
+import net.minecraft.core.component.DataComponents;
+import net.minecraft.core.registries.BuiltInRegistries;
+import net.minecraft.network.chat.Component;
+import net.minecraft.world.item.Item;
+import net.minecraft.world.item.ItemStack;
+import org.cyclops.cyclopscore.inventory.ItemDummy;
+import org.junit.jupiter.api.Test;
+
+import java.util.HashSet;
+import java.util.Set;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * Unit tests for the ItemStack hash code of {@link ItemStackHelpersCommon}.
+ *
+ * The hash has to take data components into account. Ingredient collections key on it, and a
+ * component-blind hash puts every stack of the same item in one bucket, which turns their
+ * lookups into scans over full component comparisons.
+ *
+ * @author rubensworks
+ */
+public class TestItemStackHelpersHashCode {
+
+    static {
+        ((MappedRegistry) BuiltInRegistries.ITEM).unfreeze(true);
+    }
+
+    private static final Item ITEM1 = new ItemDummy();
+    private static final Item ITEM2 = new ItemDummy();
+
+    static {
+        ((Holder.Reference<Item>) ITEM1.builtInRegistryHolder()).bindComponents(DataComponentMap.EMPTY);
+        ((Holder.Reference<Item>) ITEM2.builtInRegistryHolder()).bindComponents(DataComponentMap.EMPTY);
+    }
+
+    private static final IItemStackHelpers HELPERS = new ItemStackHelpersNeoForge();
+
+    private static int hash(ItemStack stack) {
+        return HELPERS.getItemStackHashCode(stack);
+    }
+
+    private static ItemStack named(Item item, int count, String name) {
+        ItemStack stack = new ItemStack(item, count);
+        stack.set(DataComponents.CUSTOM_NAME, Component.literal(name));
+        return stack;
+    }
+
+    @Test
+    public void testEqualStacksHashEqual() {
+        assertThat(hash(new ItemStack(ITEM1)), is(hash(new ItemStack(ITEM1))));
+        assertThat(hash(new ItemStack(ITEM1, 7)), is(hash(new ItemStack(ITEM1, 7))));
+        assertThat(hash(named(ITEM1, 3, "a")), is(hash(named(ITEM1, 3, "a"))));
+        assertThat(hash(ItemStack.EMPTY), is(hash(ItemStack.EMPTY)));
+    }
+
+    @Test
+    public void testDifferentItemsHashDifferently() {
+        assertThat(hash(new ItemStack(ITEM1)), is(not(hash(new ItemStack(ITEM2)))));
+    }
+
+    @Test
+    public void testDifferentCountsHashDifferently() {
+        assertThat(hash(new ItemStack(ITEM1, 1)), is(not(hash(new ItemStack(ITEM1, 2)))));
+    }
+
+    /**
+     * The regression this guards: stacks of one item differing only by components used to share a hash.
+     */
+    @Test
+    public void testComponentsAffectHash() {
+        assertThat(hash(named(ITEM1, 1, "a")), is(not(hash(named(ITEM1, 1, "b")))));
+        assertThat(hash(new ItemStack(ITEM1)), is(not(hash(named(ITEM1, 1, "a")))));
+    }
+
+    /**
+     * A single differing hash could be luck. Over a sample of same-item stacks the hash has to
+     * spread, or hash-based collections degrade to linear scans.
+     */
+    @Test
+    public void testComponentVariantsSpreadOverManyBuckets() {
+        int samples = 1000;
+        Set<Integer> hashes = new HashSet<>();
+        for (int i = 0; i < samples; i++) {
+            hashes.add(hash(named(ITEM1, 1, "variant " + i)));
+        }
+        assertThat("Component variants of one item have to produce distinct hashes",
+                hashes.size() > samples * 0.99, is(true));
+    }
+
+    /**
+     * Stacks carrying no component patch skip the component hash, so this pins that they still
+     * spread over items and counts, and still agree with equality.
+     */
+    @Test
+    public void testPlainStacksSpreadOverItemsAndCounts() {
+        assertThat(hash(new ItemStack(ITEM1, 1)), is(not(hash(new ItemStack(ITEM2, 1)))));
+        assertThat(hash(new ItemStack(ITEM1, 1)), is(not(hash(new ItemStack(ITEM1, 2)))));
+        assertThat(hash(new ItemStack(ITEM1, 5)), is(hash(new ItemStack(ITEM1, 5))));
+    }
+
+    /**
+     * A stack whose only component is set back to its default carries no patch any more, so it is
+     * equal to the plain stack and has to hash like one.
+     */
+    @Test
+    public void testComponentSetBackToDefaultHashesAsPlain() {
+        ItemStack plain = new ItemStack(ITEM1);
+        ItemStack restored = new ItemStack(ITEM1);
+        restored.set(DataComponents.CUSTOM_NAME, Component.literal("a"));
+        assertThat(hash(restored), is(not(hash(plain))));
+        restored.set(DataComponents.CUSTOM_NAME, null);
+        assertThat(ItemStack.isSameItemSameComponents(plain, restored), is(true));
+        assertThat(hash(restored), is(hash(plain)));
+    }
+
+    /**
+     * The hash may never distinguish two stacks that count as equal, or lookups miss.
+     */
+    @Test
+    public void testHashIsConsistentWithComponentEquality() {
+        for (int i = 0; i < 100; i++) {
+            ItemStack a = named(ITEM1, 1 + (i % 5), "variant " + i);
+            ItemStack b = named(ITEM1, 1 + (i % 5), "variant " + i);
+            assertThat(ItemStack.isSameItemSameComponents(a, b), is(true));
+            assertThat(a.getCount(), is(b.getCount()));
+            assertThat(hash(a), is(hash(b)));
+        }
+    }
+}