ItemType Random Safety Improvements

src/main/java/ch/njol/skript/aliases/ItemType.java

@@ -612,15 +612,27 @@ public ItemType clone() {
 				.collect(Collectors.toList());
 		if (datas.isEmpty())
 			return null;
-		int numItems = datas.size();
-		int index = random.nextInt(numItems);
-		ItemStack is = datas.get(index).getStack();
+		ItemStack is = datas.get(random.nextInt(datas.size())).getStack();
 		assert is != null; // verified above
 		is = is.clone();
 		is.setAmount(getAmount());
 		return is;
 	}
 
+	/**
+	 * @return One random ItemStack or Material that this ItemType represents.
+	 * A Material may only be returned for ItemStacks containing a Material where {@link Material#isItem()} is false.
+	 */
+	public Object getRandomStackOrMaterial() {
+		ItemData randomData = types.get(random.nextInt(types.size()));
+		ItemStack stack = randomData.getStack();
+		if (stack == null)
+			return randomData.getType();
+		stack = stack.clone();
+		stack.setAmount(getAmount());
+		return stack;
+	}
+
 	/**
 	 * Test whether this ItemType can be put into the given inventory completely.
 	 * <p>
@@ -1396,8 +1408,11 @@ public void clearItemMeta() {
 		globalMeta = null;
 	}
 
+	/**
+	 * @return A random Material this ItemType represents.
+	 */
 	public Material getMaterial() {
-		ItemData data = types.get(0);
+		ItemData data = types.get(random.nextInt(types.size()));
 		if (data == null)
 			throw new IllegalStateException("material not found");
 		return data.getType();

src/main/java/ch/njol/skript/classes/data/BukkitClasses.java

@@ -949,7 +949,10 @@ public ItemStack parse(final String s, final ParseContext context) {
 						}
 
 						final ItemStack i = t.getRandom();
-						assert i != null;
+						if (i == null) {
+							Skript.error("'" + s + "' cannot represent an item");
+							return null;
+						}
 						return i;
 					}
 

src/main/java/ch/njol/skript/conditions/CondIsPreferredTool.java

@@ -80,13 +80,14 @@ public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelaye
 	public boolean check(Event event) {
 		return blocks.check(event, block ->
 			items.check(event, item -> {
-				if (block instanceof Block) {
-					return ((Block) block).isPreferredTool(item.getRandom());
-				} else if (block instanceof BlockData) {
-					return ((BlockData) block).isPreferredTool(item.getRandom());
-				} else {
-					return false;
+				ItemStack stack = item.getRandom();
+				if (stack != null) {
+					if (block instanceof Block)
+						return ((Block) block).isPreferredTool(stack);
+					if (block instanceof BlockData)
+						return ((BlockData) block).isPreferredTool(stack);
 				}
+				return false;
 			}), isNegated());
 	}
 

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

@@ -68,7 +68,7 @@ protected void execute(final Event e) {
 		ItemType itemType = book.getSingle(e);
 		if (itemType != null) {
 			ItemStack itemStack = itemType.getRandom();
-			if (itemStack.getType() == Material.WRITTEN_BOOK) {
+			if (itemStack != null && itemStack.getType() == Material.WRITTEN_BOOK) {
 				for (Player player : players.getArray(e)) {
 					player.openBook(itemStack);
 				}

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

@@ -129,9 +129,10 @@ private void replace(Event event, Object[] needles, Expression<?> haystackExpr)
 
 						if (new ItemType(itemStack).isSimilar(needle)) {
 							ItemStack newItemStack = ((ItemType) replacement).getRandom();
-							newItemStack.setAmount(itemStack.getAmount());
-
-							inv.setItem(slot, newItemStack);
+							if (newItemStack != null) {
+								newItemStack.setAmount(itemStack.getAmount());
+								inv.setItem(slot, newItemStack);
+							}
 						}
 					}
 		}

src/main/java/ch/njol/skript/entity/BoatChestData.java

@@ -110,8 +110,7 @@ public boolean isSupertypeOf(EntityData<?> e) {
 	public boolean isOfItemType(ItemType itemType) {
 		int ordinal = -1;
 
-		ItemStack stack = itemType.getRandom();
-		Material type = stack.getType();
+		Material type = itemType.getMaterial();
 		if (type == Material.OAK_CHEST_BOAT)
 			ordinal = 0;
 		else if (type == Material.SPRUCE_CHEST_BOAT)

src/main/java/ch/njol/skript/entity/BoatData.java

@@ -112,9 +112,8 @@ public boolean isSupertypeOf(EntityData<?> e) {
 
 	public boolean isOfItemType(ItemType i){
 		int ordinal = -1;
-
-		ItemStack stack = i.getRandom();
-		Material type = stack.getType();
+
+		Material type = i.getMaterial();
 		if (type == Material.OAK_BOAT)
 			ordinal = 0;
 		else if (type == Material.SPRUCE_BOAT)

src/main/java/ch/njol/skript/entity/DroppedItemData.java

@@ -64,8 +64,15 @@ public DroppedItemData(ItemType @Nullable [] types) {
 
 	@Override
 	protected boolean init(Literal<?>[] expressions, int matchedPattern, ParseResult parseResult) {
-		if (expressions.length > 0 && expressions[0] != null)
+		if (expressions.length > 0 && expressions[0] != null) {
 			types = (ItemType[]) expressions[0].getAll();
+			for (ItemType type : types) {
+				if (!type.getMaterial().isItem()) {
+					Skript.error("'" + type + "' cannot represent a dropped item");
+					return false;
+				}
+			}
+		}
 		return true;
 	}
 
@@ -97,6 +104,7 @@ public void set(final Item entity) {
 		final ItemType t = CollectionUtils.getRandom(types);
 		assert t != null;
 		ItemStack stack = t.getItem().getRandom();
+		assert stack != null; // should be true by init checks
 		entity.setItemStack(stack);
 	}
 
@@ -159,6 +167,7 @@ public boolean canSpawn(@Nullable World world) {
 		final ItemType itemType = CollectionUtils.getRandom(types);
 		assert itemType != null;
 		ItemStack stack = itemType.getItem().getRandom();
+		assert stack != null; // should be true by init checks
 
 		Item item;
 		if (consumer == null) {

src/main/java/ch/njol/skript/expressions/ExprMaxStack.java

@@ -24,6 +24,8 @@
 import ch.njol.skript.doc.Name;
 import ch.njol.skript.doc.Since;
 import ch.njol.skript.expressions.base.SimplePropertyExpression;
+import org.bukkit.Material;
+import org.bukkit.inventory.ItemStack;
 
 /**
  * @author joeuguce99
@@ -40,8 +42,11 @@ public class ExprMaxStack extends SimplePropertyExpression<ItemType, Long> {
 
 	@SuppressWarnings("null")
 	@Override
-	public Long convert(final ItemType i) {
-		return (long) i.getRandom().getMaxStackSize();
+	public Long convert(ItemType itemType) {
+		Object random = itemType.getRandomStackOrMaterial();
+		if (random instanceof Material)
+			return (long) ((Material) random).getMaxStackSize();
+		return (long) ((ItemStack) random).getMaxStackSize();
 	}
 
 	@Override

src/main/java/ch/njol/skript/expressions/ExprPlain.java

@@ -32,6 +32,7 @@
 import ch.njol.util.Kleenean;
 
 import org.bukkit.event.Event;
+import org.bukkit.inventory.ItemStack;
 import org.eclipse.jdt.annotation.Nullable;
 
 @Name("Plain Item")
@@ -61,7 +62,7 @@ protected ItemType[] get(Event e) {
 		ItemType itemType = item.getSingle(e);
 		if (itemType == null)
 			return new ItemType[0];
-		ItemData data = new ItemData(itemType.getRandom().getType());
+		ItemData data = new ItemData(itemType.getMaterial());
 		data.setPlain(true);
 		ItemType plain = new ItemType(data);
 		return new ItemType[]{plain};

src/main/java/ch/njol/skript/util/visual/VisualEffects.java

@@ -151,8 +151,11 @@ private static void registerDataSupplier(String id, BiFunction<Object, Location,
 				}
 				if (raw == null)
 					return Bukkit.createBlockData(Material.AIR);
-				if (raw instanceof ItemType)
-					return Bukkit.createBlockData(((ItemType) raw).getRandom().getType());
+				if (raw instanceof ItemType) {
+					ItemType type = (ItemType) raw;
+					ItemStack random = type.getRandom();
+					return Bukkit.createBlockData(random != null ? random.getType() : type.getMaterial());
+				}
 				return raw;
 			};
 			registerDataSupplier("Particle.BLOCK", blockDataSupplier);

src/test/java/org/skriptlang/skript/test/tests/regression/ExprPlainAliasTest.java

@@ -52,7 +52,7 @@ public void test() {
 		ContextlessEvent event = ContextlessEvent.get();
 		Variables.setVariable("item", itemType, event, true);
 
-		EasyMock.expect(itemType.getRandom()).andReturn(new ItemStack(Material.STONE)).atLeastOnce();
+		EasyMock.expect(itemType.getMaterial()).andReturn(Material.STONE).atLeastOnce();
 		EasyMock.replay(itemType);
 		TriggerItem.walk(getPlainRandomItemEffect, event);
 		EasyMock.verify(itemType);