Improve UnparsedLiteral Handling (#6360)

APickledWalrus · web-flow · commit 6be9375ab85c · 2024-02-01T18:09:32.000-05:00
diff --git a/src/main/java/ch/njol/skript/expressions/arithmetic/ExprArithmetic.java b/src/main/java/ch/njol/skript/expressions/arithmetic/ExprArithmetic.java
@@ -28,6 +28,7 @@
 import ch.njol.skript.lang.ExpressionType;
 import ch.njol.skript.lang.Literal;
 import ch.njol.skript.lang.SkriptParser.ParseResult;
+import ch.njol.skript.lang.UnparsedLiteral;
 import ch.njol.skript.lang.util.SimpleExpression;
 import ch.njol.skript.lang.util.SimpleLiteral;
 import ch.njol.skript.registrations.Classes;
@@ -117,43 +118,151 @@ public PatternInfo(Operator operator, boolean leftGrouped, boolean rightGrouped)
 	private boolean leftGrouped, rightGrouped;
 
 	@Override
-	@SuppressWarnings({"ConstantConditions", "unchecked"})
+	@SuppressWarnings({"ConstantConditions", "rawtypes", "unchecked"})
 	public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) {
-		first = LiteralUtils.defendExpression(exprs[0]);
-		second = LiteralUtils.defendExpression(exprs[1]);
-
-		if (!LiteralUtils.canInitSafely(first, second))
-			return false;
-
-		Class<? extends L> firstClass = first.getReturnType();
-		Class<? extends R> secondClass = second.getReturnType();
+		first = (Expression<L>) exprs[0];
+		second = (Expression<R>) exprs[1];
 
 		PatternInfo patternInfo = patterns.getInfo(matchedPattern);
 		leftGrouped = patternInfo.leftGrouped;
 		rightGrouped = patternInfo.rightGrouped;
 		operator = patternInfo.operator;
 
-		if (firstClass != Object.class && secondClass == Object.class && Arithmetics.getOperations(operator, firstClass).isEmpty()) {
-			// If the first class is known but doesn't have any operations, then we fail
-			return error(firstClass, secondClass);
-		} else if (firstClass == Object.class && secondClass != Object.class && Arithmetics.getOperations(operator).stream()
-				.noneMatch(info -> info.getRight().isAssignableFrom(secondClass))) {
-			// If the second class is known but doesn't have any operations, then we fail
-			return error(firstClass, secondClass);
+		/*
+		 * Step 1: UnparsedLiteral Resolving
+		 *
+		 * Since Arithmetic may be performed on a variety of types, it is possible that 'first' or 'second'
+		 *  will represent unparsed literals. That is, the parser could not determine what their literal contents represent.
+		 * Thus, it is now up to this expression to determine what they mean.
+		 *
+		 * If there are no unparsed literals, nothing happens at this step.
+		 * If there are unparsed literals, one of three possible execution flows will occur:
+		 *
+	 	 * Case 1. 'first' and 'second' are unparsed literals
+	 	 * In this case, there is not a lot of information to work with.
+	 	 * 'first' and 'second' are attempted to be converted to fit one of all operations using 'operator'.
+	 	 * If they cannot be matched with the types of a known operation, init will fail.
+	 	 *
+	 	 * Case 2. 'first' is an unparsed literal, 'second' is not
+	 	 * In this case, 'first' needs to be converted into the "left" type of
+	 	 *  any operation using 'operator' with the type of 'second' as the right type.
+	 	 * If 'first' cannot be converted, init will fail.
+	 	 * If no operations are found for converting 'first', init will fail, UNLESS the type of 'second' is object,
+	 	 *  where operations will be searched again later with the context of the type of first.
+	 	 * TODO When 'first' can represent multiple literals, it might be worth checking which of those can work with 'operator' and 'second'
+	 	 *
+	 	 * Case 3. 'second' is an unparsed literal, 'first' is not
+	 	 * In this case, 'second' needs to be converted into the "right" type of
+		 *  any operation using 'operator' with the type of 'first' as the "left" type.
+		 * If 'second' cannot be converted, init will fail.
+		 * If no operations are found for converting 'second', init will fail, UNLESS the type of 'first' is object,
+		 *  where operations will be searched again later with the context of the type of second.
+		 * TODO When 'second' can represent multiple literals, it might be worth checking which of those can work with 'first' and 'operator'
+		 */
+
+		if (first instanceof UnparsedLiteral) {
+			if (second instanceof UnparsedLiteral) { // first and second need converting
+				for (OperationInfo<?, ?, ?> operation : Arithmetics.getOperations(operator)) {
+					// match left type with 'first'
+					Expression<?> convertedFirst = first.getConvertedExpression(operation.getLeft());
+					if (convertedFirst == null)
+						continue;
+					// match right type with 'second'
+					Expression<?> convertedSecond = second.getConvertedExpression(operation.getRight());
+					if (convertedSecond == null)
+						continue;
+					// success, set the values
+					first = (Expression<L>) convertedFirst;
+					second = (Expression<R>) convertedSecond;
+					returnType = (Class<? extends T>) operation.getReturnType();
+				}
+			} else { // first needs converting
+				// attempt to convert <first> to types that make valid operations with <second>
+				Class<?> secondClass = second.getReturnType();
+				Class[] leftTypes = Arithmetics.getOperations(operator).stream()
+					.filter(info -> info.getRight().isAssignableFrom(secondClass))
+					.map(OperationInfo::getLeft)
+					.toArray(Class[]::new);
+				if (leftTypes.length == 0) { // no known operations with second's type
+					if (secondClass != Object.class) // there won't be any operations
+						return error(first.getReturnType(), secondClass);
+					first = (Expression<L>) first.getConvertedExpression(Object.class);
+				} else {
+					first = (Expression<L>) first.getConvertedExpression(leftTypes);
+				}
+			}
+		} else if (second instanceof UnparsedLiteral) { // second needs converting
+			// attempt to convert <second> to types that make valid operations with <first>
+			Class<?> firstClass = first.getReturnType();
+			List<? extends OperationInfo<?, ?, ?>> operations = Arithmetics.getOperations(operator, firstClass);
+			if (operations.isEmpty()) { // no known operations with first's type
+				if (firstClass != Object.class) // there won't be any operations
+					return error(firstClass, second.getReturnType());
+				second = (Expression<R>) second.getConvertedExpression(Object.class);
+			} else {
+				second = (Expression<R>) second.getConvertedExpression(operations.stream()
+						.map(OperationInfo::getRight)
+						.toArray(Class[]::new)
+				);
+			}
 		}
 
-		OperationInfo<L, R, T> operationInfo;
+		if (!LiteralUtils.canInitSafely(first, second)) // checks if there are still unparsed literals present
+			return false;
+
+		/*
+		 * Step 2: Return Type Calculation
+		 *
+		 * After the first step, everything that can be known about 'first' and 'second' during parsing is known.
+		 * As a result, it is time to determine the return type of the operation.
+		 *
+		 * If the types of 'first' or 'second' are object, it is possible that multiple operations with different return types
+		 *  will be found. If that is the case, the supertype of these operations will be the return type (can be object).
+		 * If the types of both are object (e.g. variables), the return type will be object (have to wait until runtime and hope it works).
+		 * Of course, if no operations are found, init will fail.
+		 *
+		 * After these checks, it is safe to assume returnType has a value, as init should have failed by now if not.
+		 * One final check is performed specifically for numerical types.
+		 * Any numerical operation involving division or exponents have a return type of Double.
+		 * Other operations will also return Double, UNLESS 'first' and 'second' are of integer types, in which case the return type will be Long.
+		 *
+		 * If the types of both are something meaningful, the search for a registered operation commences.
+		 * If no operation can be found, init will fail.
+		 */
+
+		Class<? extends L> firstClass = first.getReturnType();
+		Class<? extends R> secondClass = second.getReturnType();
+
 		if (firstClass == Object.class || secondClass == Object.class) {
-			// If either of the types is unknown, then we resolve the operation at runtime
-			operationInfo = null;
-		} else {
-			operationInfo = (OperationInfo<L, R, T>) Arithmetics.lookupOperationInfo(operator, firstClass, secondClass);
-			if (operationInfo == null) // We error if we couldn't find an operation between the two types
+			// if either of the types is unknown, then we resolve the operation at runtime
+			Class<?>[] returnTypes = null;
+			if (!(firstClass == Object.class && secondClass == Object.class)) { // both aren't object
+				if (firstClass == Object.class) {
+					returnTypes = Arithmetics.getOperations(operator).stream()
+							.filter(info -> info.getRight().isAssignableFrom(secondClass))
+							.map(OperationInfo::getReturnType)
+							.toArray(Class[]::new);
+				} else { // secondClass is Object
+					returnTypes = Arithmetics.getOperations(operator, firstClass).stream()
+						.map(OperationInfo::getReturnType)
+						.toArray(Class[]::new);
+				}
+			}
+			if (returnTypes == null) { // both are object; can't determine anything
+				returnType = (Class<? extends T>) Object.class;
+			} else if (returnTypes.length == 0) { // one of the classes is known but doesn't have any operations
+				return error(firstClass, secondClass);
+			} else {
+				returnType = (Class<? extends T>) Classes.getSuperClassInfo(returnTypes).getC();
+			}
+		} else if (returnType == null) { // lookup
+			OperationInfo<L, R, T> operationInfo = (OperationInfo<L, R, T>) Arithmetics.lookupOperationInfo(operator, firstClass, secondClass);
+			if (operationInfo == null) // we error if we couldn't find an operation between the two types
 				return error(firstClass, secondClass);
+			returnType = operationInfo.getReturnType();
 		}
 
-		returnType = operationInfo == null ? (Class<? extends T>) Object.class : operationInfo.getReturnType();
-
+		// ensure proper return types for numerical operations
 		if (Number.class.isAssignableFrom(returnType)) {
 			if (operator == Operator.DIVISION || operator == Operator.EXPONENTIATION) {
 				returnType = (Class<? extends T>) Double.class;
@@ -164,19 +273,31 @@ public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelaye
 					firstIsInt |= i.isAssignableFrom(first.getReturnType());
 					secondIsInt |= i.isAssignableFrom(second.getReturnType());
 				}
-
 				returnType = (Class<? extends T>) (firstIsInt && secondIsInt ? Long.class : Double.class);
 			}
 		}
 
-		// Chaining
-		if (first instanceof ExprArithmetic && !leftGrouped) {
+		/*
+		 * Step 3: Chaining and Parsing
+		 *
+		 * This step builds the arithmetic chain that will be parsed into an ordered operation to be executed at runtime.
+		 * With larger operations, it is possible that 'first' or 'second' will be instances of ExprArithmetic.
+		 * As a result, their chains need to be incorporated into this instance's chain.
+		 * This is to ensure that, during parsing, a "gettable" that follows the order of operations is built.
+		 * However, in the case of parentheses, the chains will not be combined as the
+		 *  order of operations dictates that the result of that chain be determined first.
+		 *
+		 * The chain (a list of values and operators) will then be parsed into a "gettable" that
+		 *  can be evaluated during runtime for a final result.
+		 */
+
+		if (first instanceof ExprArithmetic && !leftGrouped) { // combine chain of 'first' if we do not have parentheses
 			chain.addAll(((ExprArithmetic<?, ?, L>) first).chain);
 		} else {
 			chain.add(first);
 		}
 		chain.add(operator);
-		if (second instanceof ExprArithmetic && !rightGrouped) {
+		if (second instanceof ExprArithmetic && !rightGrouped) { // combine chain of 'second' if we do not have parentheses
 			chain.addAll(((ExprArithmetic<?, ?, R>) second).chain);
 		} else {
 			chain.add(second);
@@ -197,7 +318,8 @@ protected T[] get(Event event) {
 
 	private boolean error(Class<?> firstClass, Class<?> secondClass) {
 		ClassInfo<?> first = Classes.getSuperClassInfo(firstClass), second = Classes.getSuperClassInfo(secondClass);
-		Skript.error(operator.getName() + " can't be performed on " + first.getName().withIndefiniteArticle() + " and " + second.getName().withIndefiniteArticle());
+		if (first.getC() != Object.class && second.getC() != Object.class) // errors with "object" are not very useful and often misleading
+			Skript.error(operator.getName() + " can't be performed on " + first.getName().withIndefiniteArticle() + " and " + second.getName().withIndefiniteArticle());
 		return false;
 	}
 
diff --git a/src/main/java/ch/njol/skript/log/ParseLogHandler.java b/src/main/java/ch/njol/skript/log/ParseLogHandler.java
@@ -18,8 +18,9 @@
  */
 package ch.njol.skript.log;
 
-import ch.njol.skript.Skript;
 import org.eclipse.jdt.annotation.Nullable;
+import org.jetbrains.annotations.ApiStatus;
+import org.jetbrains.annotations.Contract;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -31,6 +32,29 @@ public class ParseLogHandler extends LogHandler {
 	private LogEntry error = null;
 	
 	private final List<LogEntry> log = new ArrayList<>();
+
+	/**
+	 * Internal method for creating a backup of this log.
+	 * @return A new ParseLogHandler containing the contents of this ParseLogHandler.
+	 */
+	@ApiStatus.Internal
+	@Contract("-> new")
+	public ParseLogHandler backup() {
+		ParseLogHandler copy = new ParseLogHandler();
+		copy.error = this.error;
+		copy.log.addAll(this.log);
+		return copy;
+	}
+
+	/**
+	 * Internal method for restoring a backup of this log.
+	 */
+	@ApiStatus.Internal
+	public void restore(ParseLogHandler parseLogHandler) {
+		this.error = parseLogHandler.error;
+		this.log.clear();
+		this.log.addAll(parseLogHandler.log);
+	}
 	
 	@Override
 	public LogResult log(LogEntry entry) {
diff --git a/src/main/java/ch/njol/skript/patterns/TypePatternElement.java b/src/main/java/ch/njol/skript/patterns/TypePatternElement.java
@@ -24,6 +24,7 @@
 import ch.njol.skript.lang.Literal;
 import ch.njol.skript.lang.SkriptParser;
 import ch.njol.skript.lang.SkriptParser.ExprInfo;
+import ch.njol.skript.lang.UnparsedLiteral;
 import ch.njol.skript.lang.parser.ParserInstance;
 import ch.njol.skript.log.ErrorQuality;
 import ch.njol.skript.log.ParseLogHandler;
@@ -138,6 +139,10 @@ public MatchResult match(String expr, MatchResult matchResult) {
 
 		ExprInfo exprInfo = getExprInfo();
 
+		MatchResult matchBackup = null;
+		ParseLogHandler loopLogHandlerBackup = null;
+		ParseLogHandler expressionLogHandlerBackup = null;
+
 		ParseLogHandler loopLogHandler = SkriptLogger.startParseLogHandler();
 		try {
 			while (newExprOffset != -1) {
@@ -168,11 +173,37 @@ public MatchResult match(String expr, MatchResult matchResult) {
 								}
 							}
 
-							expressionLogHandler.printLog();
-							loopLogHandler.printLog();
-
 							newMatchResult.expressions[expressionIndex] = expression;
-							return newMatchResult;
+
+							/*
+							 * the parser will return unparsed literals in cases where it cannot interpret an input and object is the desired return type.
+							 * in those cases, it is up to the expression to interpret the input.
+							 * however, this presents a problem for input that is not intended as being one of these object-accepting expressions.
+							 * these object-accepting expressions will be matched instead but their parsing will fail as they cannot interpret the unparsed literals.
+							 * even though it can't interpret them, this loop will have returned a match and thus parsing has ended (and the correct interpretation never attempted).
+							 * to avoid this issue, while also permitting unparsed literals in cases where they are justified,
+							 *  the code below forces the loop to continue in hopes of finding a match without unparsed literals.
+							 * if it is unsuccessful, a backup of the first successful match (with unparsed literals) is saved to be returned.
+							 */
+							boolean hasUnparsedLiteral = false;
+							for (int i = expressionIndex + 1; i < newMatchResult.expressions.length; i++) {
+								if (newMatchResult.expressions[i] instanceof UnparsedLiteral) {
+									hasUnparsedLiteral = Classes.parse(((UnparsedLiteral) newMatchResult.expressions[i]).getData(), Object.class, newMatchResult.parseContext) == null;
+									if (hasUnparsedLiteral) {
+										break;
+									}
+								}
+							}
+
+							if (!hasUnparsedLiteral) {
+								expressionLogHandler.printLog();
+								loopLogHandler.printLog();
+								return newMatchResult;
+							} else if (matchBackup == null) { // only backup the first occurrence of unparsed literals
+								matchBackup = newMatchResult;
+								loopLogHandlerBackup = loopLogHandler.backup();
+								expressionLogHandlerBackup = expressionLogHandler.backup();
+							}
 						}
 					} finally {
 						expressionLogHandler.printError();
@@ -193,11 +224,19 @@ public MatchResult match(String expr, MatchResult matchResult) {
 				}
 			}
 		} finally {
-			if (!loopLogHandler.isStopped())
+			if (loopLogHandlerBackup != null) { // print backup logs if applicable
+				loopLogHandler.restore(loopLogHandlerBackup);
+				assert expressionLogHandlerBackup != null;
+				expressionLogHandlerBackup.printLog();
+			}
+			if (!loopLogHandler.isStopped()) {
 				loopLogHandler.printError();
+			}
 		}
 
-		return null;
+		// if there were unparsed literals, we will return the backup now
+		// if there were not, this returns null
+		return matchBackup;
 	}
 
 	@Override
diff --git a/src/test/skript/tests/misc/invalid unparsed literals.sk b/src/test/skript/tests/misc/invalid unparsed literals.sk
@@ -0,0 +1,6 @@
+test "invalid unparsed literals":
+
+	parse:
+		loop 9 times:
+			set {_i} to loop-value - 1
+	assert last parse logs is not set with "skript should be able to understand 'loop-value - 1' but did not"
diff --git a/src/test/skript/tests/syntaxes/expressions/ExprArithmetic.sk b/src/test/skript/tests/syntaxes/expressions/ExprArithmetic.sk
@@ -255,3 +255,10 @@ local function component_equals(a: number, b: number) :: boolean:
 	then:
 		return true
 	return true if {_a} is {_b}, else false
+
+test "arithmetic return types":
+	# the issue below is that <none> is now returned for the function
+	# skript interprets this as "y-coordinate of ({_location} - 4)" which is valid due to Object.class return types
+	# however, we can get more specific return types by returning the superclass of the return types of all Object-Number operations
+	set {_location} to location(0,10,0,"world")
+	assert (y-coordinate of {_location} - 4) is 6 with "y-coordinate of {_location} - 4 is not 6 (got '%y-coordinate of {_location} - 4%')"
