From cca446dd040f5f60ac3cb1517d5aec9ce23bf37f Mon Sep 17 00:00:00 2001 From: _tud <98935832+UnderscoreTud@users.noreply.github.com> Date: Mon, 30 Dec 2024 09:24:05 +0300 Subject: [PATCH] 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 Co-authored-by: Efnilite <35348263+Efnilite@users.noreply.github.com> Co-authored-by: Moderocky --- .../ch/njol/skript/lang/SkriptParser.java | 10 ++--- .../skript/lang/util/ConvertedExpression.java | 40 +++++++++++-------- .../skript/lang/util/SimpleExpression.java | 30 -------------- ...7164-expressions sometimes dont convert.sk | 13 ++++++ 4 files changed, 42 insertions(+), 51 deletions(-) create mode 100644 src/test/skript/tests/regressions/7164-expressions sometimes dont convert.sk diff --git a/src/main/java/ch/njol/skript/lang/SkriptParser.java b/src/main/java/ch/njol/skript/lang/SkriptParser.java index cefa92b2549..6cffb2a30fd 100644 --- a/src/main/java/ch/njol/skript/lang/SkriptParser.java +++ b/src/main/java/ch/njol/skript/lang/SkriptParser.java @@ -326,7 +326,6 @@ private static Variable parseVariable(String expr, Class[] r } } - @Nullable @SuppressWarnings({"unchecked", "rawtypes"}) private Expression parseSingleExpr(boolean allowUnparsedLiteral, @Nullable LogEntry error, Class... types) { @@ -368,9 +367,9 @@ private Expression 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 type : types) { - // Check return type against everything that expression accepts - if (parsedExpression.canReturn(type)) { + if (type.isAssignableFrom(parsedReturnType)) { log.printLog(); return (Expression) 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); diff --git a/src/main/java/ch/njol/skript/lang/util/ConvertedExpression.java b/src/main/java/ch/njol/skript/lang/util/ConvertedExpression.java index 4d6228112f0..e6150fda79c 100644 --- a/src/main/java/ch/njol/skript/lang/util/ConvertedExpression.java +++ b/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 source, Class to, Collecti @Nullable public static ConvertedExpression newInstance(Expression from, Class... to) { assert !CollectionUtils.containsSuperclass(to, from.getReturnType()); - // we track a list of converters that may work - List> converters = new ArrayList<>(); - for (Class type : to) { // REMIND try more converters? -> also change WrapperExpression (and maybe ExprLoopValue) - assert type != null; - // casting to is wrong, but since the converter is only used for values returned by the expression - // (which are instances of "") this won't result in any ClassCastExceptions. - for (Class checking : from.possibleReturnTypes()) { - //noinspection unchecked - ConverterInfo converter = (ConverterInfo) 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> infos = new ArrayList<>(); + for (Class 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 toType = (Class) 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 toType : to) { + ConverterInfo 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 to is wrong, but since the converter is used only for values + // returned by the expression (which are instances of ), 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; } diff --git a/src/main/java/ch/njol/skript/lang/util/SimpleExpression.java b/src/main/java/ch/njol/skript/lang/util/SimpleExpression.java index c576cd8d774..eccb3c73ba5 100644 --- a/src/main/java/ch/njol/skript/lang/util/SimpleExpression.java +++ b/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 Expression getConvertedExpression(Class... to) { // check whether this expression is already of type R if (CollectionUtils.containsSuperclass(to, getReturnType())) return (Expression) this; - - // we might be to cast some of the possible return types to R - List> infos = new ArrayList<>(); - for (Class 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 toType = (Class) 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 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); } diff --git a/src/test/skript/tests/regressions/7164-expressions sometimes dont convert.sk b/src/test/skript/tests/regressions/7164-expressions sometimes dont convert.sk new file mode 100644 index 00000000000..f7954b3a642 --- /dev/null +++ b/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"