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>

Author: 4 people

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"