Return type accuracy and usage improvements (SkriptLang#7891)

* Return type improvements

* Silly debug statement

* Safety improvements

* Type resolution improvements

* Use possibleReturnTypes in ExprFunctionCall

* Implement return type logic on more expressions

* Implement return type logic on even more expressions

Hopefully this is it

Author: APickledWalrus

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

@@ -53,12 +53,12 @@ private enum Type {
 
 		/**
 		 * Determines the appropriate Type based on the return type of an expression.
-		 * @param returnType The class representing the return type
+		 * @param expression The expression to determine the type of.
 		 * @return The corresponding Type
 		 */
-		public static Type fromClass(Class<?> returnType) {
-			boolean isBlockType = Block.class.isAssignableFrom(returnType);
-			boolean isBooleanType = Boolean.class.isAssignableFrom(returnType);
+		public static Type fromClass(Expression<?> expression) {
+			boolean isBlockType = expression.canReturn(Block.class);
+			boolean isBooleanType = expression.canReturn(Boolean.class);
 
 			if (isBlockType && !isBooleanType) {
 				return BLOCKS;
@@ -90,7 +90,7 @@ public boolean init(Expression<?>[] expressions, int matchedPattern, Kleenean is
 		action = patterns.getInfo(matchedPattern);
 
 		// Determine expression type using the enum method
-		type = Type.fromClass(togglables.getReturnType());
+		type = Type.fromClass(togglables);
 
 		// Validate based on type
 		if (type == Type.BOOLEANS && 

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

@@ -79,7 +79,7 @@ public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelaye
 				Skript.warning("Entity effects are visible to all players");
 			if (!hasLocationEffect && !direction.isDefault())
 				Skript.warning("Entity effects are always played on an entity");
-			if (hasEntityEffect && !Entity.class.isAssignableFrom(where.getReturnType())) {
+			if (hasEntityEffect && !where.canReturn(Entity.class)) {
 				Skript.error("Entity effects can only be played on entities");
 				return false;
 			}

src/main/java/ch/njol/skript/events/EvtClick.java

@@ -80,7 +80,7 @@ public class EvtClick extends SkriptEvent {
 	public boolean init(Literal<?>[] args, int matchedPattern, ParseResult parseResult) {
 		click = parseResult.mark == 0 ? ANY : parseResult.mark;
 		type = args[matchedPattern];
-		if (type != null && !ItemType.class.isAssignableFrom(type.getReturnType()) && !BlockData.class.isAssignableFrom(type.getReturnType())) {
+		if (type != null && !type.canReturn(ItemType.class) && !type.canReturn(BlockData.class)) {
 			Literal<EntityData<?>> entitydata = (Literal<EntityData<?>>) type;
 			if (click == LEFT) {
 				if (Vehicle.class.isAssignableFrom(entitydata.getSingle().getType())) {

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

@@ -47,7 +47,7 @@ public class ExprAge extends SimplePropertyExpression<Object, Integer> {
 	public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) {
 		isMax = parseResult.hasTag("max");
 		setExpr(exprs[0]);
-		if (isMax && Entity.class.isAssignableFrom(getExpr().getReturnType())) {
+		if (isMax && !getExpr().canReturn(Block.class)) {
 			Skript.error("Cannot use 'max age' expression with entities, use just the 'age' expression instead");
 			return false;
 		}

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

@@ -61,7 +61,7 @@ protected Entity[] get(Event e) {
 	}
 
 	@Nullable
-	private static Entity getAttacker(@Nullable Event e) {
+	static Entity getAttacker(@Nullable Event e) {
 		if (e == null)
 			return null;
 		if (e instanceof EntityDamageByEntityEvent) {

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

@@ -218,20 +218,24 @@ private Consumer<Banner> getAllBlockChanger(ChangeMode mode, List<Pattern> patte
 
 	@Override
 	public void change(Event event, Object @Nullable [] delta, ChangeMode mode) {
-		Pattern[] patterns = (Pattern[]) delta;
-		int placement = patternNumber == null ? 0 : patternNumber.getSingle(event);
-		List<Pattern> patternList = patterns != null ? Arrays.stream(patterns).toList() : new ArrayList<>();
-
-		Consumer<BannerMeta> metaChanger = null;
-		Consumer<Banner> blockChanger = null;
+		int placement = 0;
+		if (patternNumber != null) {
+			Integer patternNum = patternNumber.getSingle(event);
+			if (patternNum != null) {
+				placement = patternNum;
+			}
+		}
+		List<Pattern> patterns = delta != null ? Arrays.stream(delta).map(Pattern.class::cast).toList() : new ArrayList<>();
 
+		Consumer<BannerMeta> metaChanger;
+		Consumer<Banner> blockChanger;
 		if (placement >= 1) {
-			Pattern pattern = patternList.size() == 1 ? patternList.get(0) : null;
+			Pattern pattern = patterns.size() == 1 ? patterns.get(0) : null;
 			metaChanger = getPlacementMetaChanger(mode, placement, pattern);
 			blockChanger = getPlacementBlockChanger(mode, placement, pattern);
 		} else {
-			metaChanger = getAllMetaChanger(mode, patternList);
-			blockChanger = getAllBlockChanger(mode, patternList);
+			metaChanger = getAllMetaChanger(mode, patterns);
+			blockChanger = getAllBlockChanger(mode, patterns);
 		}
 
 		for (Object object : objects.getArray(event)) {

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

@@ -189,8 +189,12 @@ public boolean isSingle() {
 	}
 
 	@Override
-	public Class<Object> getReturnType() {
-		return Object.class;
+	public Class<?> getReturnType() {
+		return switch (valueType) {
+			case PRIMARY, SECONDARY -> PotionEffectType.class;
+			case RANGE -> Double.class;
+			case TIER -> Integer.class;
+		};
 	}
 
 	@Override

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

@@ -92,27 +92,19 @@ protected Color[] get(Event event, Object[] source) {
 
 	@Override
 	public Class<?> @Nullable [] acceptChange(ChangeMode mode) {
-		Class<?> returnType = getExpr().getReturnType();
+		Expression<?> expression = getExpr();
 
-		if (returnType.isAssignableFrom(FireworkEffect.class))
+		if (expression.canReturn(FireworkEffect.class))
 			return CollectionUtils.array(Color[].class);
 
-		// double assignable checks are to allow both parent and child types, since variables return Object
-		// This does mean we have to be more stringent in checking the validity of the change mode in change() itself.
-		if ((returnType.isAssignableFrom(Display.class) || Display.class.isAssignableFrom(returnType)) && (mode == ChangeMode.RESET || mode == ChangeMode.SET))
+		if ((mode == ChangeMode.RESET || mode == ChangeMode.SET) && expression.canReturn(Display.class))
 			return CollectionUtils.array(Color.class);
 
-		// the following only support SET
-		if (mode != ChangeMode.SET)
-			return null;
-		if (returnType.isAssignableFrom(Entity.class)
-			|| Entity.class.isAssignableFrom(returnType)
-			|| returnType.isAssignableFrom(Block.class)
-			|| Block.class.isAssignableFrom(returnType)
-			|| returnType.isAssignableFrom(ItemType.class)
-		) {
+		if (mode == ChangeMode.SET &&
+			(expression.canReturn(Entity.class) || expression.canReturn(Block.class) || expression.canReturn(ItemType.class))) {
 			return CollectionUtils.array(Color.class);
 		}
+
 		return null;
 	}
 

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

@@ -9,93 +9,77 @@
 import ch.njol.skript.lang.ExpressionType;
 import ch.njol.skript.lang.SkriptParser;
 import ch.njol.skript.lang.util.SimpleExpression;
-import org.skriptlang.skript.lang.converter.Converters;
 import ch.njol.skript.util.LiteralUtils;
 import ch.njol.skript.util.Utils;
 import ch.njol.util.Kleenean;
 import org.bukkit.event.Event;
-import org.jetbrains.annotations.Nullable;
 
-import java.lang.reflect.Array;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.Set;
 
 @Name("Default Value")
 @Description("A shorthand expression for giving things a default value. If the first thing isn't set, the second thing will be returned.")
 @Examples({"broadcast {score::%player's uuid%} otherwise \"%player% has no score!\""})
 @Since("2.2-dev36")
-@SuppressWarnings("null")
-public class ExprDefaultValue<T> extends SimpleExpression<T> {
+public class ExprDefaultValue extends SimpleExpression<Object> {
 
 	static {
 		Skript.registerExpression(ExprDefaultValue.class, Object.class, ExpressionType.COMBINED,
 				"%objects% (otherwise|?) %objects%");
 	}
 
-	private final ExprDefaultValue<?> source;
-	private final Class<? extends T>[] types;
-	private final Class<T> superType;
-	@Nullable
-	private Expression<Object> first;
-	@Nullable
-	private Expression<Object> second;
+	private Class<?>[] types;
+	private Class<?> superType;
 
-	@SuppressWarnings("unchecked")
-	public ExprDefaultValue() {
-		this(null, (Class<? extends T>) Object.class);
-	}
-
-	@SuppressWarnings("unchecked")
-	private ExprDefaultValue(ExprDefaultValue<?> source, Class<? extends T>... types) {
-		this.source = source;
-		if (source != null) {
-			this.first = source.first;
-			this.second = source.second;
-		}
-		this.types = types;
-		this.superType = (Class<T>) Utils.getSuperType(types);
-	}
+	private Expression<Object> values;
+	private Expression<Object> defaultValues;
 
 	@Override
 	public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, SkriptParser.ParseResult parseResult) {
-		first = LiteralUtils.defendExpression(exprs[0]);
-		second = LiteralUtils.defendExpression(exprs[1]);
-		return LiteralUtils.canInitSafely(first, second);
-	}
-
-	@Override
-	@SuppressWarnings("unchecked")
-	protected T[] get(Event e) {
-		Object[] first = this.first.getArray(e);
-		Object values[] = first.length != 0 ? first : second.getArray(e);
-		try {
-			return Converters.convert(values, types, superType);
-		} catch (ClassCastException e1) {
-			return (T[]) Array.newInstance(superType, 0);
+		values = LiteralUtils.defendExpression(exprs[0]);
+		defaultValues = LiteralUtils.defendExpression(exprs[1]);
+		if (!LiteralUtils.canInitSafely(values, defaultValues)) {
+			return false;
 		}
+
+		Set<Class<?>> types = new HashSet<>();
+		Collections.addAll(types, values.possibleReturnTypes());
+		Collections.addAll(types, defaultValues.possibleReturnTypes());
+		this.types = types.toArray(new Class<?>[0]);
+		this.superType = Utils.getSuperType(this.types);
+
+		return true;
 	}
 
 	@Override
-	public <R> Expression<? extends R> getConvertedExpression(Class<R>... to) {
-		return new ExprDefaultValue<>(this, to);
+	protected Object[] get(Event event) {
+		Object[] values = this.values.getArray(event);
+		if (values.length != 0) {
+			return values;
+		}
+		return defaultValues.getArray(event);
 	}
 
 	@Override
-	public Expression<?> getSource() {
-		return source == null ? this : source;
+	public boolean isSingle() {
+		return values.isSingle() && defaultValues.isSingle();
 	}
 
 	@Override
-	public Class<? extends T> getReturnType() {
+	public Class<?> getReturnType() {
 		return superType;
 	}
 
 	@Override
-	public boolean isSingle() {
-		return first.isSingle() && second.isSingle();
+	public Class<?>[] possibleReturnTypes() {
+		return Arrays.copyOf(types, types.length);
 	}
 
 	@Override
-	public String toString(Event e, boolean debug) {
-		return first.toString(e, debug) + " or else " + second.toString(e, debug);
+	public String toString(Event event, boolean debug) {
+		return values.toString(event, debug) + " or else " + defaultValues.toString(event, debug);
 	}
 
 }

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

@@ -237,6 +237,22 @@ public Class<? extends T> getReturnType() {
 		return expr.getReturnType();
 	}
 
+	@Override
+	public Class<? extends T>[] possibleReturnTypes() {
+		if (!queue) {
+			return expr.possibleReturnTypes();
+		}
+		return super.possibleReturnTypes();
+	}
+
+	@Override
+	public boolean canReturn(Class<?> returnType) {
+		if (!queue) {
+			return expr.canReturn(returnType);
+		}
+		return super.canReturn(returnType);
+	}
+
 	@Override
 	public String toString(@Nullable Event event, boolean debug) {
 		String prefix;