Fix expression conversion

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

@@ -326,7 +326,6 @@ private static <T> Variable<T> parseVariable(String expr, Class<? extends T>[] r
 		}
 	}
 
-
 	@Nullable
 	@SuppressWarnings({"unchecked", "rawtypes"})
 	private <T> Expression<? extends T> parseSingleExpr(boolean allowUnparsedLiteral, @Nullable LogEntry error, Class<? extends T>... types) {
@@ -368,9 +367,9 @@ private <T> Expression<? extends T> parseSingleExpr(boolean allowUnparsedLiteral
 			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;
 						}
@@ -540,13 +539,14 @@ private Expression<?> parseSingleExpr(boolean allowUnparsedLiteral, @Nullable Lo
 			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/util/ConvertedExpression.java

@@ -24,6 +24,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;
@@ -96,24 +97,31 @@ public ConvertedExpression(Expression<? extends F> source, Class<T> to, Collecti
 	@Nullable
 	public static <F, T> 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

@@ -35,13 +35,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.
@@ -206,33 +203,6 @@ public <R> Expression<? extends R> getConvertedExpression(Class<R>... to) {
 		// 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/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"