From 7f6b8a69c3adbc48a1021619f2cb80a3ec51e0b6 Mon Sep 17 00:00:00 2001 From: F0x06 Date: Wed, 10 Jun 2026 21:37:24 +0200 Subject: [PATCH 1/3] fix(alchemylib): guard IngredientStack against empty Ingredient.values (#12) The main constructor read pIngredient.values[0] directly, assuming every Ingredient carries at least one value. NeoForge custom/compound ingredients (and the empty ingredient) leave the vanilla values[] array empty and expose their contents through getItems(), so decoding such a recipe at world creation threw ArrayIndexOutOfBoundsException and crashed the client. Resolve registryName via a new resolveRegistryName helper: keep the existing values[0] path when present, otherwise fall back to the first item the ingredient resolves to, and finally to the item registry default key (minecraft:air). Add regression tests for the empty-ingredient path. --- .../alchemylib/api/item/IngredientStack.java | 40 +++++++++++++++---- .../api/item/IngredientStackTest.java | 24 +++++++++-- 2 files changed, 52 insertions(+), 12 deletions(-) diff --git a/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java b/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java index b8c97fa..326da41 100644 --- a/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java +++ b/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java @@ -33,8 +33,9 @@ public class IngredientStack { /** * All other constructors reference this main constructor for creating a new IngredientStack. * - *

{@link IngredientStack#registryName} is taken from the 0th entry of the Ingredient's values array. If that - * entry is an item value the item's registry name is used; if it is a tag value the tag's location is used.

+ *

{@link IngredientStack#registryName} is derived by {@link #resolveRegistryName(Ingredient)}: normally from + * the 0th entry of the Ingredient's values array, with a fallback for ingredients whose values array is empty + * (NeoForge custom ingredients and the empty ingredient).

* * @param pIngredient {@link Ingredient} * @param pCount The count for how items are in this stack. Only a max of 64 is valid, similar to ItemStack. @@ -42,14 +43,37 @@ public class IngredientStack { public IngredientStack(Ingredient pIngredient, int pCount) { this.ingredient = pIngredient; this.count = Math.min(pCount, 64); - Ingredient.Value value = pIngredient.values[0]; - if (value instanceof Ingredient.TagValue tagValue) { - this.registryName = tagValue.tag().location(); - } else if (value instanceof Ingredient.ItemValue itemValue) { - this.registryName = BuiltInRegistries.ITEM.getKey(itemValue.item().getItem()); - } else { + this.registryName = resolveRegistryName(pIngredient); + } + + /** + * Derives a non-null registry name for an Ingredient. + * + *

The 0th entry of the Ingredient's {@code values} array is preferred: an item value yields the item's + * registry name, a tag value yields the tag's location. NeoForge custom ingredients (and the empty ingredient + * produced by {@code Ingredient.of()}) carry an empty {@code values} array, so reading {@code values[0]} + * would throw {@link ArrayIndexOutOfBoundsException} while a recipe is being decoded. For that case we fall back + * to the first item the ingredient actually resolves to, and finally to the item registry's default key + * ({@code minecraft:air}) when it resolves to nothing.

+ * + * @param pIngredient {@link Ingredient} + * @return the resolved {@link ResourceLocation}, never {@code null} + */ + private static ResourceLocation resolveRegistryName(Ingredient pIngredient) { + if (pIngredient.values.length > 0) { + Ingredient.Value value = pIngredient.values[0]; + if (value instanceof Ingredient.TagValue tagValue) { + return tagValue.tag().location(); + } else if (value instanceof Ingredient.ItemValue itemValue) { + return BuiltInRegistries.ITEM.getKey(itemValue.item().getItem()); + } throw new IllegalArgumentException("Ingredient value is neither an item nor a tag value."); } + ItemStack[] items = pIngredient.getItems(); + if (items.length > 0) { + return BuiltInRegistries.ITEM.getKey(items[0].getItem()); + } + return BuiltInRegistries.ITEM.getDefaultKey(); } public IngredientStack(Ingredient pIngredient) { diff --git a/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java b/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java index df8ef00..df5c4d7 100644 --- a/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java +++ b/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java @@ -11,6 +11,7 @@ import java.util.List; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -23,10 +24,11 @@ * They also pin the {@code toNetwork}/{@code fromNetwork} round trip -- the persistence seam the recipe packets * depend on -- and that {@code toStacks} does not mutate the Ingredient's shared cached stacks. * - *

The constructor's {@code else throw IllegalArgumentException} branch (value neither item nor tag) is - * unreachable for real ingredients -- an empty {@code Ingredient.of()} has an empty {@code values} array, so - * {@code values[0]} is an {@code ArrayIndexOutOfBoundsException} rather than that exception -- so it is left - * untested on purpose.

+ *

An empty {@code Ingredient.of()} has an empty {@code values} array (the same shape NeoForge custom + * ingredients take), so the constructor must not read {@code values[0]} blindly -- doing so threw an + * {@code ArrayIndexOutOfBoundsException} while recipes were being decoded. That empty-ingredient path is + * pinned below. The {@code throw IllegalArgumentException} branch (a non-empty value that is neither item + * nor tag) stays unreachable for real ingredients, so it is left untested on purpose.

*/ class IngredientStackTest extends BootstrappedTest { @@ -65,6 +67,20 @@ void getRegistryName_tagValue_isTagLocation() { assertEquals(ResourceLocation.fromNamespaceAndPath("minecraft", "planks"), stack.getRegistryName()); } + @Test + void constructor_emptyIngredient_doesNotThrow() { + // Regression for the ArrayIndexOutOfBoundsException at IngredientStack.: an empty Ingredient has an + // empty values[] array (the shape NeoForge custom ingredients also take), which crashed recipe decode. + assertDoesNotThrow(() -> new IngredientStack(Ingredient.of())); + } + + @Test + void getRegistryName_emptyIngredient_isItemRegistryDefaultKey() { + IngredientStack stack = new IngredientStack(Ingredient.of()); + + assertEquals(ResourceLocation.fromNamespaceAndPath("minecraft", "air"), stack.getRegistryName()); + } + @Test void equalsAndHashCode_sameCountAndRegistryName_areEqual() { IngredientStack first = new IngredientStack(Ingredient.of(Items.STONE), 4); From 1275a86430aa684922c497215a2ec12fcf3f19a6 Mon Sep 17 00:00:00 2001 From: DarkArcana Date: Wed, 10 Jun 2026 21:00:04 -0500 Subject: [PATCH 2/3] fix(alchemylib): derive IngredientStack identity from declared values, never resolving items at decode time The previous fix for #12 stopped the ArrayIndexOutOfBoundsException but fell back to Ingredient.getItems()[0] when the values array is empty. Recipes are decoded during RecipeManager.apply, before registry tags are bound to that reload, so getItems() resolves tag-backed children to NeoForge's barrier "Empty Tag" placeholder stacks and memoizes that snapshot for the Ingredient's lifetime (barrier ghosts in JEI and the recipe selector). Worse, every unresolvable custom ingredient collapsed to the same fallback name (minecraft:barrier or minecraft:air), and with equality keyed on count plus that single name, recipes collecting inputs into a LinkedHashSet silently dropped any second custom-ingredient input. Never resolve items in the constructor. Instead key both registryName and the new identity field on what the Ingredient declares: - custom ingredient (Ingredient.isCustom()): stand-in name alchemylib:custom, identity = the ICustomIngredient itself. NeoForge mandates equals/hashCode on ICustomIngredient implementations (its own, like CompoundIngredient, are records); third-party customs that skip the contract degrade to instance identity, which still keeps separately decoded ingredients distinct. - no declared values (Ingredient.of()): stand-in name alchemylib:empty, empty identity. - declared values: identity = the whole sorted value set (tag locations and item registry names), registryName = the first value's location as before. equals/hashCode now compare count plus the full identity, which also brings the 1.21.4 whole-ingredient-identity fix (b06042f) to 1.21.1: two multi-item ingredients sharing only a first item are no longer equal. Keeps the existing empty-ingredient regression tests (the stand-in expectation replaces minecraft:air) and adds coverage for the custom-ingredient constructor path, custom and multi-item equality, and the LinkedHashSet input-drop regression. --- .../alchemylib/api/item/IngredientStack.java | 94 +++++++++------ .../api/item/IngredientStackTest.java | 108 ++++++++++++++++-- 2 files changed, 162 insertions(+), 40 deletions(-) diff --git a/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java b/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java index 326da41..314e5d8 100644 --- a/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java +++ b/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java @@ -10,6 +10,7 @@ import net.minecraft.world.item.ItemStack; import net.minecraft.world.item.crafting.Ingredient; import net.minecraft.world.level.ItemLike; +import net.neoforged.neoforge.common.crafting.ICustomIngredient; import java.util.Arrays; import java.util.List; @@ -26,16 +27,45 @@ @SuppressWarnings("unused") public class IngredientStack { + /** + * Stand-in registry name for an Ingredient that declares no values at all (e.g. {@code Ingredient.of()}). It is + * namespaced to the mod so it can never collide with a real item or tag id, keeping {@link #getRegistryName()} + * non-null for that degenerate case instead of indexing into an empty values array. + */ + private static final ResourceLocation EMPTY = ResourceLocation.fromNamespaceAndPath("alchemylib", "empty"); + + /** + * Stand-in registry name for a NeoForge {@linkplain Ingredient#isCustom() custom ingredient} + * ({@link ICustomIngredient}). A custom ingredient declares no item or tag values -- its contents are only + * knowable by resolving items, which the constructor must never do (see + * {@link #IngredientStack(Ingredient, int)}) -- so this mod-namespaced stand-in keeps + * {@link #getRegistryName()} non-null without colliding with a real item or tag id. + */ + private static final ResourceLocation CUSTOM = ResourceLocation.fromNamespaceAndPath("alchemylib", "custom"); + private final Ingredient ingredient; private final int count; private final ResourceLocation registryName; + /** + * The equality key for this stack: the sorted, unmodifiable list of declared value locations (tag locations and + * item registry names) for a vanilla ingredient, or the {@link ICustomIngredient} itself for a custom one. + * NeoForge requires custom ingredients to implement {@code equals}/{@code hashCode} (its own, like + * {@code CompoundIngredient}, are records with structural equality); a third-party custom that skips that + * contract degrades to instance identity, which still keeps separately decoded ingredients distinct. + */ + private final Object identity; /** * All other constructors reference this main constructor for creating a new IngredientStack. * - *

{@link IngredientStack#registryName} is derived by {@link #resolveRegistryName(Ingredient)}: normally from - * the 0th entry of the Ingredient's values array, with a fallback for ingredients whose values array is empty - * (NeoForge custom ingredients and the empty ingredient).

+ *

Both {@link #registryName} and {@link #identity} are derived from what the Ingredient declares, + * never from the items it resolves to: recipes are decoded during {@code RecipeManager.apply}, before registry + * tags are bound to that reload, so {@link Ingredient#getItems()} would substitute -- and permanently memoize -- + * NeoForge's barrier "Empty Tag" placeholder stacks. A {@linkplain Ingredient#isCustom() custom ingredient} + * gets the {@link #CUSTOM} stand-in name and is identified by its {@link ICustomIngredient}; a vanilla + * ingredient is identified by its full declared value set (each tag's location, each item's registry name, + * sorted) with the first value's location as the representative {@link #registryName}; an ingredient with no + * values at all (e.g. {@code Ingredient.of()}) gets the {@link #EMPTY} stand-in and an empty identity.

* * @param pIngredient {@link Ingredient} * @param pCount The count for how items are in this stack. Only a max of 64 is valid, similar to ItemStack. @@ -43,37 +73,33 @@ public class IngredientStack { public IngredientStack(Ingredient pIngredient, int pCount) { this.ingredient = pIngredient; this.count = Math.min(pCount, 64); - this.registryName = resolveRegistryName(pIngredient); + if (pIngredient.isCustom()) { + this.identity = pIngredient.getCustomIngredient(); + this.registryName = CUSTOM; + } else { + this.identity = Arrays.stream(pIngredient.values) + .map(IngredientStack::valueLocation) + .sorted() + .collect(Collectors.toUnmodifiableList()); + this.registryName = pIngredient.values.length > 0 ? valueLocation(pIngredient.values[0]) : EMPTY; + } } /** - * Derives a non-null registry name for an Ingredient. + * The location an Ingredient value declares: a tag value yields the tag's location, an item value yields the + * item's registry name. Neither requires resolving the ingredient's items, so this is safe at recipe-decode + * time. * - *

The 0th entry of the Ingredient's {@code values} array is preferred: an item value yields the item's - * registry name, a tag value yields the tag's location. NeoForge custom ingredients (and the empty ingredient - * produced by {@code Ingredient.of()}) carry an empty {@code values} array, so reading {@code values[0]} - * would throw {@link ArrayIndexOutOfBoundsException} while a recipe is being decoded. For that case we fall back - * to the first item the ingredient actually resolves to, and finally to the item registry's default key - * ({@code minecraft:air}) when it resolves to nothing.

- * - * @param pIngredient {@link Ingredient} - * @return the resolved {@link ResourceLocation}, never {@code null} + * @param pValue {@link Ingredient.Value} + * @return the declared {@link ResourceLocation}, never {@code null} */ - private static ResourceLocation resolveRegistryName(Ingredient pIngredient) { - if (pIngredient.values.length > 0) { - Ingredient.Value value = pIngredient.values[0]; - if (value instanceof Ingredient.TagValue tagValue) { - return tagValue.tag().location(); - } else if (value instanceof Ingredient.ItemValue itemValue) { - return BuiltInRegistries.ITEM.getKey(itemValue.item().getItem()); - } - throw new IllegalArgumentException("Ingredient value is neither an item nor a tag value."); - } - ItemStack[] items = pIngredient.getItems(); - if (items.length > 0) { - return BuiltInRegistries.ITEM.getKey(items[0].getItem()); + private static ResourceLocation valueLocation(Ingredient.Value pValue) { + if (pValue instanceof Ingredient.TagValue tagValue) { + return tagValue.tag().location(); + } else if (pValue instanceof Ingredient.ItemValue itemValue) { + return BuiltInRegistries.ITEM.getKey(itemValue.item().getItem()); } - return BuiltInRegistries.ITEM.getDefaultKey(); + throw new IllegalArgumentException("Ingredient value is neither an item nor a tag value."); } public IngredientStack(Ingredient pIngredient) { @@ -187,7 +213,10 @@ public boolean isEmpty() { } /** - * Determines object equality of this IngredientStack against another object based on {@link ResourceLocation#equals(Object)}. + * Determines object equality of this IngredientStack against another object. Two IngredientStacks are equal + * when they share the same {@link #getCount() count} and the same {@link #identity full ingredient identity}, + * so two multi-item ingredients that merely share a first item are not equal, and two custom ingredients are + * only equal when their {@link ICustomIngredient}s are. * * @param pObject Object * @return boolean @@ -198,18 +227,19 @@ public boolean equals(Object pObject) { if (!(pObject instanceof IngredientStack that)) return false; if (getCount() != that.getCount()) return false; - return getRegistryName().equals(that.getRegistryName()); + return identity.equals(that.identity); } /** - * Calculates the hash code for this IngredientStack based on its {@link ResourceLocation registryName} hash code. + * Calculates the hash code for this IngredientStack based on its {@link #getCount() count} and its + * {@link #identity full ingredient identity}. * * @return int */ @Override public int hashCode() { int result = getCount(); - result = 31 * result + getRegistryName().hashCode(); + result = 31 * result + identity.hashCode(); return result; } diff --git a/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java b/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java index df5c4d7..a6fe8e9 100644 --- a/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java +++ b/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java @@ -7,9 +7,13 @@ import net.minecraft.world.item.ItemStack; import net.minecraft.world.item.Items; import net.minecraft.world.item.crafting.Ingredient; +import net.minecraft.world.level.ItemLike; +import net.neoforged.neoforge.common.crafting.CompoundIngredient; import org.junit.jupiter.api.Test; +import java.util.LinkedHashSet; import java.util.List; +import java.util.Set; import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -20,15 +24,24 @@ /** * Tests for {@link IngredientStack}: the constructor reads the access-transformer-opened * {@code Ingredient.values} field, so these need the Minecraft classpath and a populated item registry - * (hence {@link BootstrappedTest}). All ingredients are built from vanilla {@link Items} and {@link ItemTags}. + * (hence {@link BootstrappedTest}). All ingredients are built from vanilla {@link Items} and {@link ItemTags} + * (plus NeoForge's {@link CompoundIngredient} for the custom-ingredient paths). * They also pin the {@code toNetwork}/{@code fromNetwork} round trip -- the persistence seam the recipe packets * depend on -- and that {@code toStacks} does not mutate the Ingredient's shared cached stacks. * - *

An empty {@code Ingredient.of()} has an empty {@code values} array (the same shape NeoForge custom - * ingredients take), so the constructor must not read {@code values[0]} blindly -- doing so threw an - * {@code ArrayIndexOutOfBoundsException} while recipes were being decoded. That empty-ingredient path is - * pinned below. The {@code throw IllegalArgumentException} branch (a non-empty value that is neither item - * nor tag) stays unreachable for real ingredients, so it is left untested on purpose.

+ *

NeoForge custom ingredients and the empty {@code Ingredient.of()} both have an empty {@code values} array, + * so the constructor must not read {@code values[0]} blindly -- doing so threw an + * {@code ArrayIndexOutOfBoundsException} while recipes were being decoded. Nor may it fall back to resolving + * {@code Ingredient.getItems()}: at decode time registry tags are not yet bound, so that would memoize NeoForge's + * barrier "Empty Tag" placeholders. Both shapes instead map to mod-namespaced stand-in registry names, pinned + * below. The {@code throw IllegalArgumentException} branch (a declared value that is neither item nor tag) + * stays unreachable for real ingredients, so it is left untested on purpose.

+ * + *

Equality is keyed on the count plus the Ingredient's full identity (the declared tag locations and sorted + * item registry names, or the {@code ICustomIngredient} itself), so the multi-item cases below pin that two + * ingredients sharing only a first item are not equal while the same item set in any order is, and the custom + * cases pin that two different custom ingredients stay distinct -- with first-value-only equality they collapsed + * to one stand-in name and hash-based recipe-input sets silently dropped inputs.

*/ class IngredientStackTest extends BootstrappedTest { @@ -75,10 +88,27 @@ void constructor_emptyIngredient_doesNotThrow() { } @Test - void getRegistryName_emptyIngredient_isItemRegistryDefaultKey() { + void getRegistryName_emptyIngredient_isEmptyStandIn() { + // Pins the same empty-values shape as the no-throw test above: the registry name must be the + // mod-namespaced stand-in, not anything resolved through getItems() at decode time. IngredientStack stack = new IngredientStack(Ingredient.of()); - assertEquals(ResourceLocation.fromNamespaceAndPath("minecraft", "air"), stack.getRegistryName()); + assertEquals(ResourceLocation.fromNamespaceAndPath("alchemylib", "empty"), stack.getRegistryName()); + } + + @Test + void constructor_customIngredient_doesNotThrow() { + // Regression for the ArrayIndexOutOfBoundsException at IngredientStack. (#12): a NeoForge custom + // ingredient constructs with values = new Value[0], the exact shape that crashed world creation in packs + // whose recipes use compound/custom ingredients. + assertDoesNotThrow(() -> new IngredientStack(compoundOf(Items.STONE, Items.DIRT))); + } + + @Test + void getRegistryName_customIngredient_isCustomStandIn() { + IngredientStack stack = new IngredientStack(compoundOf(Items.STONE, Items.DIRT)); + + assertEquals(ResourceLocation.fromNamespaceAndPath("alchemylib", "custom"), stack.getRegistryName()); } @Test @@ -106,6 +136,57 @@ void equals_differentRegistryName_areNotEqual() { assertNotEquals(first, second); } + @Test + void equals_multiItemSharingFirstItem_areNotEqual() { + // Identity must reflect the whole declared value set, not just the first item: these two share IRON_INGOT + // as their first item but differ on the second, so they must not collide in hash-based dedup/lookup. + IngredientStack first = new IngredientStack(Ingredient.of(Items.IRON_INGOT, Items.GOLD_INGOT), 4); + IngredientStack second = new IngredientStack(Ingredient.of(Items.IRON_INGOT, Items.COPPER_INGOT), 4); + + assertNotEquals(first, second); + } + + @Test + void equalsAndHashCode_sameItemSetAndCount_areEqual() { + // Same full item set (regardless of declared order) and same count: equal, with matching hash codes. + IngredientStack first = new IngredientStack(Ingredient.of(Items.IRON_INGOT, Items.GOLD_INGOT), 4); + IngredientStack second = new IngredientStack(Ingredient.of(Items.GOLD_INGOT, Items.IRON_INGOT), 4); + + assertEquals(first, second); + assertEquals(first.hashCode(), second.hashCode()); + } + + @Test + void equalsAndHashCode_structurallySameCustomIngredients_areEqual() { + // Custom ingredients are identified by their ICustomIngredient. NeoForge's CompoundIngredient is a record, + // so two separately built but structurally identical compounds compare equal. + IngredientStack first = new IngredientStack(compoundOf(Items.STONE, Items.DIRT), 4); + IngredientStack second = new IngredientStack(compoundOf(Items.STONE, Items.DIRT), 4); + + assertEquals(first, second); + assertEquals(first.hashCode(), second.hashCode()); + } + + @Test + void equals_differentCustomIngredients_areNotEqual() { + IngredientStack first = new IngredientStack(compoundOf(Items.IRON_INGOT, Items.GOLD_INGOT), 4); + IngredientStack second = new IngredientStack(compoundOf(Items.STONE, Items.DIRT), 4); + + assertNotEquals(first, second); + } + + @Test + void linkedHashSet_differentCustomIngredients_keepsBoth() { + // Recipe-input-drop regression: recipes collect their inputs into a LinkedHashSet. With + // equality keyed on a single resolved name, every unresolvable custom ingredient collapsed to the same + // stand-in and the set silently dropped all but the first input of such a recipe. + Set inputs = new LinkedHashSet<>(); + inputs.add(new IngredientStack(compoundOf(Items.IRON_INGOT, Items.GOLD_INGOT), 4)); + inputs.add(new IngredientStack(compoundOf(Items.STONE, Items.DIRT), 4)); + + assertEquals(2, inputs.size()); + } + @Test void toStacks_appliesCountWithoutMutatingIngredientCache() { Ingredient ingredient = Ingredient.of(Items.STONE); @@ -144,4 +225,15 @@ private static IngredientStack encodeDecodeNetwork(IngredientStack stack) { stack.toNetwork(buffer); return IngredientStack.fromNetwork(buffer); } + + /** + * A real NeoForge custom ingredient: {@link CompoundIngredient#of} with two children wraps a + * {@code CompoundIngredient} in a vanilla {@code Ingredient} whose {@code values} array is empty and whose + * {@code isCustom()} is true -- the exact shape recipe decode hands to the IngredientStack constructor. + * Only construction is exercised here; serializing it would need the neoforge:ingredient_type registry, + * which the bare bootstrap does not populate. + */ + private static Ingredient compoundOf(ItemLike pFirst, ItemLike pSecond) { + return CompoundIngredient.of(Ingredient.of(pFirst), Ingredient.of(pSecond)); + } } From c399f395e91bc35c85d33a42201298d0eb487967 Mon Sep 17 00:00:00 2001 From: DarkArcana Date: Wed, 10 Jun 2026 22:19:03 -0500 Subject: [PATCH 3/3] fix(alchemylib): discriminate tag ids from item ids in IngredientStack identity The identity introduced in 1275a86 is a sorted list of bare declared locations, mixing tag locations and item registry names. A tag and an item may legally share a ResourceLocation, so Ingredient.of(#mymod:x) and Ingredient.of(mymod:x item) compared equal at equal counts -- reproducing, for that pair, the exact silent LinkedHashSet recipe-input merge the identity exists to prevent. Prefix each identity entry with its value kind: tag values become "tag:", item values "item:". The custom branch (identity = the ICustomIngredient itself) and registryName semantics are unchanged. Adds a regression test pinning that a tag-backed and an item-backed ingredient sharing minecraft:stone at equal counts are not equal. --- .../alchemylib/api/item/IngredientStack.java | 35 ++++++++++++++----- .../api/item/IngredientStackTest.java | 26 +++++++++++--- 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java b/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java index 314e5d8..84d0a27 100644 --- a/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java +++ b/src/main/java/com/smashingmods/alchemylib/api/item/IngredientStack.java @@ -47,11 +47,14 @@ public class IngredientStack { private final int count; private final ResourceLocation registryName; /** - * The equality key for this stack: the sorted, unmodifiable list of declared value locations (tag locations and - * item registry names) for a vanilla ingredient, or the {@link ICustomIngredient} itself for a custom one. - * NeoForge requires custom ingredients to implement {@code equals}/{@code hashCode} (its own, like - * {@code CompoundIngredient}, are records with structural equality); a third-party custom that skips that - * contract degrades to instance identity, which still keeps separately decoded ingredients distinct. + * The equality key for this stack: the sorted, unmodifiable list of kind-prefixed declared value locations + * ({@code "tag:"} for tag values, {@code "item:"} for item values) for a vanilla + * ingredient, or the {@link ICustomIngredient} itself for a custom one. The kind prefix keeps a tag and an + * item that share a location distinct -- with bare locations they compared equal and hash-based + * recipe-input sets silently merged them. NeoForge requires custom ingredients to implement + * {@code equals}/{@code hashCode} (its own, like {@code CompoundIngredient}, are records with structural + * equality); a third-party custom that skips that contract degrades to instance identity, which still keeps + * separately decoded ingredients distinct. */ private final Object identity; @@ -63,9 +66,10 @@ public class IngredientStack { * tags are bound to that reload, so {@link Ingredient#getItems()} would substitute -- and permanently memoize -- * NeoForge's barrier "Empty Tag" placeholder stacks. A {@linkplain Ingredient#isCustom() custom ingredient} * gets the {@link #CUSTOM} stand-in name and is identified by its {@link ICustomIngredient}; a vanilla - * ingredient is identified by its full declared value set (each tag's location, each item's registry name, - * sorted) with the first value's location as the representative {@link #registryName}; an ingredient with no - * values at all (e.g. {@code Ingredient.of()}) gets the {@link #EMPTY} stand-in and an empty identity.

+ * ingredient is identified by its full declared value set (each value's location prefixed with its kind, + * {@code tag:}/{@code item:}, sorted) with the first value's location as the representative + * {@link #registryName}; an ingredient with no values at all (e.g. {@code Ingredient.of()}) gets the + * {@link #EMPTY} stand-in and an empty identity.

* * @param pIngredient {@link Ingredient} * @param pCount The count for how items are in this stack. Only a max of 64 is valid, similar to ItemStack. @@ -78,13 +82,26 @@ public IngredientStack(Ingredient pIngredient, int pCount) { this.registryName = CUSTOM; } else { this.identity = Arrays.stream(pIngredient.values) - .map(IngredientStack::valueLocation) + .map(IngredientStack::valueIdentity) .sorted() .collect(Collectors.toUnmodifiableList()); this.registryName = pIngredient.values.length > 0 ? valueLocation(pIngredient.values[0]) : EMPTY; } } + /** + * The identity entry an Ingredient value contributes: its {@linkplain #valueLocation(Ingredient.Value) + * declared location} prefixed with the value's kind ({@code "tag:"} or {@code "item:"}). A tag and an item + * may legally share a location, so the bare location is not enough to keep them distinct. + * + * @param pValue {@link Ingredient.Value} + * @return the kind-prefixed declared location, never {@code null} + */ + private static String valueIdentity(Ingredient.Value pValue) { + String kind = pValue instanceof Ingredient.TagValue ? "tag" : "item"; + return kind + ":" + valueLocation(pValue); + } + /** * The location an Ingredient value declares: a tag value yields the tag's location, an item value yields the * item's registry name. Neither requires resolving the ingredient's items, so this is safe at recipe-decode diff --git a/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java b/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java index a6fe8e9..3409feb 100644 --- a/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java +++ b/src/test/java/com/smashingmods/alchemylib/api/item/IngredientStackTest.java @@ -1,9 +1,12 @@ package com.smashingmods.alchemylib.api.item; import com.smashingmods.alchemylib.testsupport.BootstrappedTest; +import net.minecraft.core.registries.Registries; import net.minecraft.network.RegistryFriendlyByteBuf; import net.minecraft.resources.ResourceLocation; import net.minecraft.tags.ItemTags; +import net.minecraft.tags.TagKey; +import net.minecraft.world.item.Item; import net.minecraft.world.item.ItemStack; import net.minecraft.world.item.Items; import net.minecraft.world.item.crafting.Ingredient; @@ -37,11 +40,12 @@ * below. The {@code throw IllegalArgumentException} branch (a declared value that is neither item nor tag) * stays unreachable for real ingredients, so it is left untested on purpose.

* - *

Equality is keyed on the count plus the Ingredient's full identity (the declared tag locations and sorted - * item registry names, or the {@code ICustomIngredient} itself), so the multi-item cases below pin that two - * ingredients sharing only a first item are not equal while the same item set in any order is, and the custom - * cases pin that two different custom ingredients stay distinct -- with first-value-only equality they collapsed - * to one stand-in name and hash-based recipe-input sets silently dropped inputs.

+ *

Equality is keyed on the count plus the Ingredient's full identity (the declared value locations, each + * prefixed with its kind -- {@code tag:}/{@code item:} -- and sorted, or the {@code ICustomIngredient} itself), + * so the multi-item cases below pin that two ingredients sharing only a first item are not equal while the same + * item set in any order is, the tag-vs-item case pins that a tag and an item sharing a location stay distinct, + * and the custom cases pin that two different custom ingredients stay distinct -- with first-value-only equality + * they collapsed to one stand-in name and hash-based recipe-input sets silently dropped inputs.

*/ class IngredientStackTest extends BootstrappedTest { @@ -136,6 +140,18 @@ void equals_differentRegistryName_areNotEqual() { assertNotEquals(first, second); } + @Test + void equals_tagAndItemSharingLocation_areNotEqual() { + // Kind-discriminator regression: a tag and an item may legally share a ResourceLocation. Identity entries + // are kind-prefixed ("tag:"/"item:") so these two must not compare equal -- with bare locations they + // collided and hash-based recipe-input sets silently merged distinct inputs. + TagKey stoneTag = TagKey.create(Registries.ITEM, ResourceLocation.fromNamespaceAndPath("minecraft", "stone")); + IngredientStack tagBacked = new IngredientStack(Ingredient.of(stoneTag), 4); + IngredientStack itemBacked = new IngredientStack(Ingredient.of(Items.STONE), 4); + + assertNotEquals(tagBacked, itemBacked); + } + @Test void equals_multiItemSharingFirstItem_areNotEqual() { // Identity must reflect the whole declared value set, not just the first item: these two share IRON_INGOT