Fix weird item mutation bug with EffHealth (#6586)

src/main/java/ch/njol/skript/bukkitutil/ItemUtils.java

@@ -18,15 +18,15 @@
  */
 package ch.njol.skript.bukkitutil;
 
+import ch.njol.skript.Skript;
+import ch.njol.skript.aliases.ItemType;
 import org.bukkit.Material;
 import org.bukkit.TreeType;
 import org.bukkit.inventory.ItemStack;
 import org.bukkit.inventory.meta.Damageable;
 import org.bukkit.inventory.meta.ItemMeta;
 import org.eclipse.jdt.annotation.Nullable;
 
-import ch.njol.skript.Skript;
-
 import java.util.HashMap;
 
 /**
@@ -36,27 +36,53 @@ public class ItemUtils {
 
 	/**
 	 * Gets damage/durability of an item, or 0 if it does not have damage.
-	 * @param stack Item.
+	 * @param itemStack Item.
 	 * @return Damage.
 	 */
-	public static int getDamage(ItemStack stack) {
-		ItemMeta meta = stack.getItemMeta();
+	public static int getDamage(ItemStack itemStack) {
+		ItemMeta meta = itemStack.getItemMeta();
 		if (meta instanceof Damageable)
 			return ((Damageable) meta).getDamage();
-		return 0; // Not damageable item
+		return 0; // Non damageable item
 	}
-
+
+	/**
+	 * Sets damage/durability of an item if possible.
+	 * @param itemStack Item to modify.
+	 * @param damage New damage. Note that on some Minecraft versions,
+	 * this might be truncated to short.
+	 */
+	public static void setDamage(ItemStack itemStack, int damage) {
+		ItemMeta meta = itemStack.getItemMeta();
+		if (meta instanceof Damageable) {
+			((Damageable) meta).setDamage(damage);
+			itemStack.setItemMeta(meta);
+		}
+	}
+
+	/**
+	 * Gets damage/durability of an item, or 0 if it does not have damage.
+	 * @param itemType Item.
+	 * @return Damage.
+	 */
+	public static int getDamage(ItemType itemType) {
+		ItemMeta meta = itemType.getItemMeta();
+		if (meta instanceof Damageable)
+			return ((Damageable) meta).getDamage();
+		return 0; // Non damageable item
+	}
+
 	/**
 	 * Sets damage/durability of an item if possible.
-	 * @param stack Item to modify.
+	 * @param itemType Item to modify.
 	 * @param damage New damage. Note that on some Minecraft versions,
 	 * this might be truncated to short.
 	 */
-	public static void setDamage(ItemStack stack, int damage) {
-		ItemMeta meta = stack.getItemMeta();
+	public static void setDamage(ItemType itemType, int damage) {
+		ItemMeta meta = itemType.getItemMeta();
 		if (meta instanceof Damageable) {
 			((Damageable) meta).setDamage(damage);
-			stack.setItemMeta(meta);
+			itemType.setItemMeta(meta);
 		}
 	}
 

src/main/java/ch/njol/skript/effects/EffHealth.java

@@ -22,114 +22,119 @@
 import ch.njol.skript.aliases.ItemType;
 import ch.njol.skript.bukkitutil.HealthUtils;
 import ch.njol.skript.bukkitutil.ItemUtils;
-import ch.njol.skript.classes.Changer.ChangeMode;
-import ch.njol.skript.classes.Changer.ChangerUtils;
 import ch.njol.skript.doc.Description;
 import ch.njol.skript.doc.Examples;
 import ch.njol.skript.doc.Name;
 import ch.njol.skript.doc.Since;
 import ch.njol.skript.lang.Effect;
 import ch.njol.skript.lang.Expression;
 import ch.njol.skript.lang.SkriptParser.ParseResult;
+import ch.njol.skript.util.slot.Slot;
 import ch.njol.util.Kleenean;
 import ch.njol.util.Math2;
-import org.bukkit.entity.LivingEntity;
+import org.bukkit.entity.Damageable;
 import org.bukkit.event.Event;
 import org.bukkit.inventory.ItemStack;
 import org.eclipse.jdt.annotation.Nullable;
 
-/**
- * @author Peter Güttinger
- */
 @Name("Damage/Heal/Repair")
 @Description("Damage/Heal/Repair an entity, or item.")
-@Examples({"damage player by 5 hearts",
-		"heal the player",
-		"repair tool of player"})
+@Examples({
+	"damage player by 5 hearts",
+	"heal the player",
+	"repair tool of player"
+})
 @Since("1.0")
 public class EffHealth extends Effect {
 
 	static {
 		Skript.registerEffect(EffHealth.class,
-			"damage %livingentities/itemtypes% by %number% [heart[s]] [with fake cause %-damagecause%]",
+			"damage %livingentities/itemtypes/slots% by %number% [heart[s]] [with fake cause %-damagecause%]",
 			"heal %livingentities% [by %-number% [heart[s]]]",
-			"repair %itemtypes% [by %-number%]");
+			"repair %itemtypes/slots% [by %-number%]");
 	}
 
-	@SuppressWarnings("NotNullFieldNotInitialized")
 	private Expression<?> damageables;
 	@Nullable
-	private Expression<Number> damage;
-	private boolean heal = false;
+	private Expression<Number> amount;
+	private boolean isHealing, isRepairing;
 
-	@SuppressWarnings({"unchecked", "null"})
+	@SuppressWarnings("unchecked")
 	@Override
-	public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parser) {
+	public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) {
 		if (matchedPattern == 0 && exprs[2] != null)
 			Skript.warning("The fake damage cause extension of this effect has no functionality, " +
 				"and will be removed in the future");
 
-		damageables = exprs[0];
-		if (!LivingEntity.class.isAssignableFrom(damageables.getReturnType())) {
-			if (!ChangerUtils.acceptsChange(damageables, ChangeMode.SET, ItemType.class)) {
-				Skript.error(damageables + " cannot be changed, thus it cannot be damaged or repaired.");
-				return false;
-			}
-		}
-		damage = (Expression<Number>) exprs[1];
-		heal = matchedPattern >= 1;
-
+		this.damageables = exprs[0];
+		this.isHealing = matchedPattern >= 1;
+		this.isRepairing = matchedPattern == 2;
+		this.amount = (Expression<Number>) exprs[1];
 		return true;
 	}
 
 	@Override
-	public void execute(Event e) {
-		double damage = 0;
-		if (this.damage != null) {
-			Number number = this.damage.getSingle(e);
-			if (number == null)
+	protected void execute(Event event) {
+		double amount = 0;
+		if (this.amount != null) {
+			Number amountPostCheck = this.amount.getSingle(event);
+			if (amountPostCheck == null)
 				return;
-			damage = number.doubleValue();
+			amount = amountPostCheck.doubleValue();
 		}
-		Object[] array = damageables.getArray(e);
-		Object[] newArray = new Object[array.length];
 
-		boolean requiresChange = false;
-		for (int i = 0; i < array.length; i++) {
-			Object value = array[i];
-			if (value instanceof ItemType) {
-				ItemType itemType = (ItemType) value;
-				ItemStack itemStack = itemType.getRandom();
+		for (Object obj : this.damageables.getArray(event)) {
+			if (obj instanceof ItemType) {
+				ItemType itemType = (ItemType) obj;
+
+				if (this.amount == null) {
+					ItemUtils.setDamage(itemType, 0);
+				} else {
+					ItemUtils.setDamage(itemType, (int) Math2.fit(0, (ItemUtils.getDamage(itemType) + (isHealing ? -amount : amount)), itemType.getMaterial().getMaxDurability()));
+				}
+
+			} else if (obj instanceof Slot) {
+				Slot slot = (Slot) obj;
+				ItemStack itemStack = slot.getItem();
+
+				if (itemStack == null)
+					continue;
 
-				if (this.damage == null) {
+				if (this.amount == null) {
 					ItemUtils.setDamage(itemStack, 0);
 				} else {
-					ItemUtils.setDamage(itemStack, (int) Math2.fit(0, ItemUtils.getDamage(itemStack) + (heal ? -damage : damage), itemStack.getType().getMaxDurability()));
+					int damageAmt = (int) Math2.fit(0, (isHealing ? -amount : amount), itemStack.getType().getMaxDurability());
+					ItemUtils.setDamage(itemStack, damageAmt);
 				}
 
-				newArray[i] = new ItemType(itemStack);
-				requiresChange = true;
-			} else {
-				LivingEntity livingEntity = (LivingEntity) value;
-				if (!heal) {
-					HealthUtils.damage(livingEntity, damage);
-				} else if (this.damage == null) {
-					HealthUtils.setHealth(livingEntity, HealthUtils.getMaxHealth(livingEntity));
+				slot.setItem(itemStack);
+
+			} else if (obj instanceof Damageable) {
+				Damageable damageable = (Damageable) obj;
+
+				if (this.amount == null) {
+					HealthUtils.heal(damageable, HealthUtils.getMaxHealth(damageable));
+				} else if (isHealing) {
+					HealthUtils.heal(damageable, amount);
 				} else {
-					HealthUtils.heal(livingEntity, damage);
+					HealthUtils.damage(damageable, amount);
 				}
 
-				newArray[i] = livingEntity;
 			}
 		}
-
-		if (requiresChange)
-			damageables.change(e, newArray, ChangeMode.SET);
 	}
 
 	@Override
-	public String toString(@Nullable Event e, boolean debug) {
-		return (heal ? "heal" : "damage") + " " + damageables.toString(e, debug) + (damage != null ? " by " + damage.toString(e, debug) : "");
+	public String toString(@Nullable Event event, boolean debug) {
+
+		String prefix = "damage ";
+		if (isRepairing) {
+			prefix = "repair ";
+		} else if (isHealing) {
+			prefix = "heal ";
+		}
+
+		return prefix + damageables.toString(event, debug) + (amount != null ? " by " + amount.toString(event, debug) : "");
 	}
 
 }

src/test/skript/tests/regressions/pull-6586-effhealth item mutation.sk

@@ -0,0 +1,14 @@
+
+# Test not related to a made issue. Details at pull request.
+test "EffHealth item mutation fix":
+	set {_item1} and {_item1Copy} to diamond sword with damage 100
+	set {_item2} and {_item2Copy} to iron sword
+	repair {_item2} and {_item1}
+	assert {_item1} is iron sword to fail with "{_item1} is an iron sword even though it was diamond before repair"
+	assert {_item2} is {_item2Copy} with "{_item2} was no longer the same item"
+
+	set {_item1} to diamond sword with damage 100
+	set {_item2} to diamond with damage 10
+	repair {_item1}, {_item2}
+	assert {_item1} is diamond sword with damage 0 with "{_item1} was incorrectly repaired"
+	assert {_item2} is diamond with "{_item2} was no longer a diamond"