Merge patches into feature (before release). (#7306)

* Fix the REMOVE changer of variables (#7163)

* Fix the REMOVE changer of variables

* Fix wording

* Fix tests

* Update Variable.java

---------

Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>

* Fix Improperly Typed Array in ExprXOf (#7268)

* Add boat data check to prevent error. (#7301)

* Fix BlockIgniteEvent  (#7252)

Update BukkitEventValues.java

Co-authored-by: Moderocky <admin@moderocky.com>

* Fix expression conversion (#7165)

* Fix expression conversion

* Extract duplicate code into a separate helper method

* improve conversion strategy

* Add .sk to test file

* Simplify conversion usage

We need to use conversion whenever there are multiple return types. If the expression does not accept our supertype, then we can attempt to convert it, which will already handle safety checks for multiple return types

* SimpleExpression: fix possible return type conversion

This fixes SimpleExpression not converting  possible return types that are not contained in the desired types array. For example, if an expression can return a Number or a String, and we want an Expression that is a Number or an World, it will now include converters for String->Number and String->World

* Use safety checks of ConvertedExpression

* Remove incorrect converter remake

* Move logic from SimpleExpression to ConvertedExpression

---------

Co-authored-by: APickledWalrus <apickledwalrus@gmail.com>
Co-authored-by: Efnilite <35348263+Efnilite@users.noreply.github.com>
Co-authored-by: Moderocky <admin@moderocky.com>

---------

Co-authored-by: _tud <98935832+UnderscoreTud@users.noreply.github.com>
Co-authored-by: sovdee <10354869+sovdeeth@users.noreply.github.com>
Co-authored-by: Patrick Miller <apickledwalrus@gmail.com>
Co-authored-by: kyoshuske <65446070+kyoshuske@users.noreply.github.com>
Co-authored-by: Efnilite <35348263+Efnilite@users.noreply.github.com>

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

@@ -244,7 +244,7 @@ public Player get(final BlockIgniteEvent e) {
 			@Override
 			@Nullable
 			public Block get(final BlockIgniteEvent e) {
-				return e.getIgnitingBlock();
+				return e.getBlock();
 			}
 		}, 0);
 		// BlockDispenseEvent

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

@@ -50,7 +50,7 @@ public BoatData(@Nullable Boat.Type type){
 	private BoatData(int type) {
 		matchedPattern = type;
 	}
-	
+
 	@Override
 	protected boolean init(Literal<?>[] exprs, int matchedPattern, ParseResult parseResult) {
 		return true;

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

@@ -18,6 +18,8 @@
 import org.bukkit.inventory.ItemStack;
 import org.jetbrains.annotations.Nullable;
 
+import java.lang.reflect.Array;
+
 @Name("X of Item")
 @Description("An expression to be able to use a certain amount of items where the amount can be any expression. Please note that this expression is not stable and might be replaced in the future.")
 @Examples("give level of player of pickaxes to the player")
@@ -44,7 +46,7 @@ public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelaye
 	protected Object[] get(Event e, Object[] source) {
 		Number a = amount.getSingle(e);
 		if (a == null)
-			return new Object[0];
+			return (Object[]) Array.newInstance(getReturnType(), 0);
 
 		return get(source, o -> {
 			if (o instanceof ItemStack) {

src/main/java/ch/njol/skript/lang/SkriptParser.java

@@ -351,9 +351,9 @@ public boolean hasTag(String tag) {
 			if ((flags & PARSE_EXPRESSIONS) != 0) {
 				Expression<?> parsedExpression = parseExpression(types, expr);
 				if (parsedExpression != null) { // Expression/VariableString parsing success
+					Class<?> parsedReturnType = parsedExpression.getReturnType();
 					for (Class<? extends T> type : types) {
-						// Check return type against everything that expression accepts
-						if (parsedExpression.canReturn(type)) {
+						if (type.isAssignableFrom(parsedReturnType)) {
 							log.printLog();
 							return (Expression<? extends T>) parsedExpression;
 						}
@@ -519,13 +519,14 @@ public boolean hasTag(String tag) {
 			if ((flags & PARSE_EXPRESSIONS) != 0) {
 				Expression<?> parsedExpression = parseExpression(types, expr);
 				if (parsedExpression != null) { // Expression/VariableString parsing success
+					Class<?> parsedReturnType = parsedExpression.getReturnType();
 					for (int i = 0; i < types.length; i++) {
 						Class<?> type = types[i];
 						if (type == null) // Ignore invalid (null) types
 							continue;
 
-						// Check return type against everything that expression accepts
-						if (parsedExpression.canReturn(type)) {
+						// Check return type against the expression's return type
+						if (type.isAssignableFrom(parsedReturnType)) {
 							if (!exprInfo.isPlural[i] && !parsedExpression.isSingle()) { // Wrong number of arguments
 								if (context == ParseContext.COMMAND) {
 									Skript.error(Commands.m_too_many_arguments.toString(exprInfo.classes[i].getName().getIndefiniteArticle(), exprInfo.classes[i].getName().toString()), ErrorQuality.SEMANTIC_ERROR);

src/main/java/ch/njol/skript/lang/Variable.java

@@ -555,14 +555,15 @@ public void change(Event event, Object @Nullable [] delta, ChangeMode mode) thro
 					if (mode == ChangeMode.REMOVE) {
 						if (map == null)
 							return;
-						ArrayList<String> toRemove = new ArrayList<>(); // prevents CMEs
+						Set<String> toRemove = new HashSet<>(); // prevents CMEs
 						for (Object value : delta) {
 							for (Entry<String, Object> entry : map.entrySet()) {
+								String key = entry.getKey();
+								if (key == null)
+									continue; // This is NOT a part of list variable
+								if (toRemove.contains(key))
+									continue; // Skip if we've already marked this key to be removed
 								if (Relation.EQUAL.isImpliedBy(Comparators.compare(entry.getValue(), value))) {
-									String key = entry.getKey();
-									if (key == null)
-										continue; // This is NOT a part of list variable
-
 									// Otherwise, we'll mark that key to be set to null
 									toRemove.add(key);
 									break;
@@ -576,7 +577,7 @@ public void change(Event event, Object @Nullable [] delta, ChangeMode mode) thro
 					} else if (mode == ChangeMode.REMOVE_ALL) {
 						if (map == null)
 							return;
-						ArrayList<String> toRemove = new ArrayList<>(); // prevents CMEs
+						Set<String> toRemove = new HashSet<>(); // prevents CMEs
 						for (Entry<String, Object> i : map.entrySet()) {
 							for (Object value : delta) {
 								if (Relation.EQUAL.isImpliedBy(Comparators.compare(i.getValue(), value)))

src/main/java/ch/njol/skript/lang/util/ConvertedExpression.java

@@ -6,6 +6,7 @@
 import ch.njol.skript.lang.Expression;
 import ch.njol.skript.lang.SkriptParser.ParseResult;
 import ch.njol.skript.registrations.Classes;
+import ch.njol.skript.util.Utils;
 import ch.njol.util.Checker;
 import ch.njol.util.Kleenean;
 import ch.njol.util.coll.CollectionUtils;
@@ -77,24 +78,31 @@ public ConvertedExpression(Expression<? extends F> source, Class<T> to, Collecti
 	@SafeVarargs
 	public static <F, T> @Nullable ConvertedExpression<F, T> newInstance(Expression<F> from, Class<T>... to) {
 		assert !CollectionUtils.containsSuperclass(to, from.getReturnType());
-		// we track a list of converters that may work
-		List<ConverterInfo<? super F, ? extends T>> converters = new ArrayList<>();
-		for (Class<T> type : to) { // REMIND try more converters? -> also change WrapperExpression (and maybe ExprLoopValue)
-			assert type != null;
-			// casting <? super ? extends F> to <? super F> is wrong, but since the converter is only used for values returned by the expression
-			// (which are instances of "<? extends F>") this won't result in any ClassCastExceptions.
-			for (Class<? extends F> checking : from.possibleReturnTypes()) {
-				//noinspection unchecked
-				ConverterInfo<? super F, ? extends T> converter = (ConverterInfo<? super F, ? extends T>) Converters.getConverterInfo(checking, type);
-				if (converter != null)
-					converters.add(converter);
+
+		// we might be able to cast some (or all) of the possible return types to T
+		// for possible return types that can't be directly cast, regular converters will be used
+		List<ConverterInfo<? extends F, ? extends T>> infos = new ArrayList<>();
+		for (Class<? extends F> type : from.possibleReturnTypes()) {
+			if (CollectionUtils.containsSuperclass(to, type)) { // this type is of T, build a converter simply casting
+				// noinspection unchecked - 'type' is a desired type in 'to'
+				Class<T> toType = (Class<T>) type;
+				infos.add(new ConverterInfo<>(type, toType, toType::cast, 0));
+			} else { // this possible return type is not included in 'to'
+				// build all converters for converting the possible return type into any of the types of 'to'
+				for (Class<T> toType : to) {
+					ConverterInfo<? extends F, T> converter = Converters.getConverterInfo(type, toType);
+					if (converter != null)
+						infos.add(converter);
+				}
 			}
-			int size = converters.size();
-			if (size == 1) // if there is only one info, there is no need to wrap it in a list
-				return new ConvertedExpression<>(from, type, converters.get(0));
-			if (size > 1)
-				return new ConvertedExpression<>(from, type, converters, true);
 		}
+		if (!infos.isEmpty()) { // there are converters for (at least some of) the return types
+			// a note: casting <? extends F> to <? super F> is wrong, but since the converter is used only for values
+			//         returned by the expression (which are instances of <? extends F>), this won't result in any CCEs
+			// noinspection rawtypes, unchecked
+			return new ConvertedExpression(from, Utils.getSuperType(infos.stream().map(ConverterInfo::getTo).toArray(Class[]::new)), infos, true);
+		}
+
 		return null;
 	}
 

src/main/java/ch/njol/skript/lang/util/SimpleExpression.java

@@ -19,13 +19,10 @@
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.skriptlang.skript.lang.converter.Converter;
-import org.skriptlang.skript.lang.converter.ConverterInfo;
 
 import java.lang.reflect.Array;
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Iterator;
-import java.util.List;
 
 /**
  * An implementation of the {@link Expression} interface. You should usually extend this class to make a new expression.
@@ -189,33 +186,6 @@ public static <T> boolean check(T @Nullable [] values, Checker<? super T> checke
 		// check whether this expression is already of type R
 		if (CollectionUtils.containsSuperclass(to, getReturnType()))
 			return (Expression<? extends R>) this;
-
-		// we might be to cast some of the possible return types to R
-		List<ConverterInfo<? extends T, R>> infos = new ArrayList<>();
-		for (Class<? extends T> type : this.possibleReturnTypes()) {
-			if (CollectionUtils.containsSuperclass(to, type)) { // this type is of R
-				// build a converter that for casting to R
-				// safety check is present in the event that we do not get this type at runtime
-				final Class<R> toType = (Class<R>) type;
-				infos.add(new ConverterInfo<>(getReturnType(), toType, fromObject -> {
-					if (toType.isInstance(fromObject))
-						return (R) fromObject;
-					return null;
-				}, 0));
-			}
-		}
-		int size = infos.size();
-		if (size == 1) { // if there is only one info, there is no need to wrap it in a list
-			ConverterInfo<? extends T, R> info = infos.get(0);
-			//noinspection rawtypes
-			return new ConvertedExpression(this, info.getTo(), info);
-		}
-		if (size > 1) {
-			//noinspection rawtypes
-			return new ConvertedExpression(this, Utils.getSuperType(infos.stream().map(ConverterInfo::getTo).toArray(Class[]::new)), infos, false);
-		}
-
-		// attempt traditional conversion with proper converters
 		return this.getConvertedExpr(to);
 	}
 

src/test/skript/tests/regressions/6817-null of x incorrect type.sk

@@ -0,0 +1,3 @@
+test "null of x incorrect type":
+	# would cause an exception
+	spawn {_null} of pig above {_null}

src/test/skript/tests/regressions/7162-removing from variable skips duplicates.sk

@@ -0,0 +1,6 @@
+test "removing from variables skips duplicates":
+	set {_list::*} to 1, 1, 2, 3, 4, 5, 6 and 6
+	remove 1 and 1 from {_list::*}
+	assert {_list::*} is 2, 3, 4, 5, 6 and 6 with "didn't remove all instances of 1 from the list"
+	remove first 6 elements out of {_list::*} from {_list::*}
+	assert {_list::*} is not set with "didn't remove all elements"

src/test/skript/tests/regressions/7164-expressions sometimes dont convert.sk

@@ -0,0 +1,13 @@
+test "expressions sometimes dont convert":
+	assert formatted ({_foo} + {_bar}) is not set with "formatting nothing shouldn't throw an error"
+
+	set {_foo} to "test"
+	assert formatted ({_foo} + {_bar}) is not set with "formatting string + none shouldn't throw an error"
+
+	set {_foo} to 1
+	set {_bar} to 2
+	assert formatted ({_foo} + {_bar}) is not set with "formatting number + number shouldn't throw an error"
+
+	set {_foo} to "foo"
+	set {_bar} to "bar"
+	assert formatted ({_foo} + {_bar}) is "foobar" with "formatting strings doesn't work"