Fix the legendary compare issue with two same literals

src/main/java/ch/njol/skript/conditions/CondCompare.java

@@ -22,15 +22,20 @@
 import org.eclipse.jdt.annotation.Nullable;
 
 import ch.njol.skript.Skript;
+
 import org.skriptlang.skript.lang.comparator.Comparator;
 import org.skriptlang.skript.lang.comparator.Relation;
+import org.skriptlang.skript.lang.converter.ConverterInfo;
+import org.skriptlang.skript.lang.converter.Converters;
+
 import ch.njol.skript.doc.Description;
 import ch.njol.skript.doc.Examples;
 import ch.njol.skript.doc.Name;
 import ch.njol.skript.doc.Since;
 import ch.njol.skript.lang.Condition;
 import ch.njol.skript.lang.Expression;
 import ch.njol.skript.lang.ExpressionList;
+import ch.njol.skript.lang.Literal;
 import ch.njol.skript.lang.ParseContext;
 import ch.njol.skript.lang.SkriptParser.ParseResult;
 import ch.njol.skript.lang.UnparsedLiteral;
@@ -39,15 +44,13 @@
 import ch.njol.skript.log.RetainingLogHandler;
 import ch.njol.skript.log.SkriptLogger;
 import ch.njol.skript.registrations.Classes;
+
 import org.skriptlang.skript.lang.comparator.Comparators;
 import ch.njol.skript.util.Patterns;
 import ch.njol.skript.util.Utils;
 import ch.njol.util.Checker;
 import ch.njol.util.Kleenean;
 
-/**
- * @author Peter Güttinger
- */
 @Name("Comparison")
 @Description({"A very general condition, it simply compares two values. Usually you can only compare for equality (e.g. block is/isn't of <type>), " +
 		"but some values can also be compared using greater than/less than. In that case you can also test for whether an object is between two others.",
@@ -93,19 +96,18 @@ public class CondCompare extends Condition {
 		Skript.registerCondition(CondCompare.class, patterns.getPatterns());
 	}
 
-	@SuppressWarnings("null")
 	private Expression<?> first;
-	@SuppressWarnings("null")
 	private Expression<?> second;
+
 	@Nullable
 	private Expression<?> third;
-	@SuppressWarnings("null")
+
 	private Relation relation;
-	@SuppressWarnings("rawtypes")
+
 	@Nullable
-	private Comparator comp;
+	@SuppressWarnings("rawtypes")
+	private Comparator comparator;
 
-	@SuppressWarnings("null")
 	@Override
 	public boolean init(final Expression<?>[] vars, final int matchedPattern, final Kleenean isDelayed, final ParseResult parser) {
 		first = vars[0];
@@ -134,7 +136,7 @@ public boolean init(final Expression<?>[] vars, final int matchedPattern, final
 			}
 		}
 		@SuppressWarnings("rawtypes")
-		final Comparator comp = this.comp;
+		final Comparator comp = this.comparator;
 		if (comp != null) {
 			if (third == null) {
 				if (!relation.isImpliedBy(Relation.EQUAL, Relation.NOT_EQUAL) && !comp.supportsOrdering()) {
@@ -160,58 +162,57 @@ public static String f(final Expression<?> e) {
 
 	@SuppressWarnings({"unchecked"})
 	private boolean init(String expr) {
-		final RetainingLogHandler log = SkriptLogger.startRetainingLog();
+		RetainingLogHandler log = SkriptLogger.startRetainingLog();
 		Expression<?> third = this.third;
 		try {
 			if (first.getReturnType() == Object.class) {
-				final Expression<?> e = first.getConvertedExpression(Object.class);
-				if (e == null) {
+				Expression<?> expression = null;
+				if (first instanceof UnparsedLiteral)
+					expression = attemptReconstruct((UnparsedLiteral) first, second);
+				if (expression == null)
+					expression = first.getConvertedExpression(Object.class);
+				if (expression == null) {
 					log.printErrors();
 					return false;
 				}
-				first = e;
+				first = expression;
 			}
 			if (second.getReturnType() == Object.class) {
-				final Expression<?> e = second.getConvertedExpression(Object.class);
-				if (e == null) {
+				Expression<?> expression = null;
+				if (second instanceof UnparsedLiteral)
+					expression = attemptReconstruct((UnparsedLiteral) second, first);
+				if (expression == null)
+					expression = second.getConvertedExpression(Object.class);
+				if (expression == null) {
 					log.printErrors();
 					return false;
 				}
-				second = e;
+				second = expression;
 			}
 			if (third != null && third.getReturnType() == Object.class) {
-				final Expression<?> e = third.getConvertedExpression(Object.class);
-				if (e == null) {
+				Expression<?> expression = null;
+				if (third instanceof UnparsedLiteral)
+					expression = attemptReconstruct((UnparsedLiteral) third, first);
+				if (expression == null)
+					expression = third.getConvertedExpression(Object.class);
+				if (expression == null) {
 					log.printErrors();
 					return false;
 				}
-				this.third = third = e;
+				this.third = third = expression;
 			}
 			log.printLog();
 		} finally {
 			log.stop();
 		}
-		final Class<?> f = first.getReturnType(), s = third == null ? second.getReturnType() : Utils.getSuperType(second.getReturnType(), third.getReturnType());
-		if (f == Object.class || s == Object.class)
+		Class<?> firstReturnType = first.getReturnType();
+		Class<?> secondReturnType = third == null ? second.getReturnType() : Utils.getSuperType(second.getReturnType(), third.getReturnType());
+		if (firstReturnType == Object.class || secondReturnType == Object.class)
 			return true;
-		/*
-		 * https://github.com/Mirreski/Skript/issues/10
-		 * 
-		if(Entity.class.isAssignableFrom(s)){
-			String[] split = expr.split(" ");
-			System.out.println(expr);
-			if(!split[split.length - 1].equalsIgnoreCase("player") && EntityData.parseWithoutIndefiniteArticle(split[split.length - 1]) != null){
-				comp = Comparators.getComparator(f, EntityData.class);
-				second = SkriptParser.parseLiteral(split[split.length - 1], EntityData.class, ParseContext.DEFAULT);
-			}else
-				comp = Comparators.getComparator(f, s);
-
-		}
-		*///else
-
-		comp = Comparators.getComparator(f, s);
-
-		if (comp == null) { // Try to re-parse with more context
+
+		comparator = Comparators.getComparator(firstReturnType, secondReturnType);
+
+		if (comparator == null) { // Try to re-parse with more context
 			/*
 			 * SkriptParser sees that CondCompare takes two objects. Most of the time,
 			 * this works fine. However, when there are multiple conflicting literals,
@@ -224,23 +225,23 @@ private boolean init(String expr) {
 			 * Some damage types not working (issue #2184) would be a good example
 			 * of issues that SkriptParser's lack of context can cause.
 			 */
-			SimpleLiteral<?> reparsedSecond = reparseLiteral(first.getReturnType(), second);
+			SimpleLiteral<?> reparsedSecond = reparseLiteral(firstReturnType, second);
 			if (reparsedSecond != null) {
 				second = reparsedSecond;
-				comp = Comparators.getComparator(f, second.getReturnType());
+				comparator = Comparators.getComparator(firstReturnType, second.getReturnType());
 			} else {
 				SimpleLiteral<?> reparsedFirst = reparseLiteral(second.getReturnType(), first);
 				if (reparsedFirst != null) {
 					first = reparsedFirst;
-					comp = Comparators.getComparator(first.getReturnType(), s);
+					comparator = Comparators.getComparator(first.getReturnType(), secondReturnType);
 				}
 			}
 
 		}
-		
-		return comp != null;
+
+		return comparator != null;
 	}
-	
+
 	/**
 	 * Attempts to parse given expression again as a literal of given type.
 	 * This will only work if the expression is a literal and its unparsed
@@ -251,22 +252,53 @@ private boolean init(String expr) {
 	 * @return A literal value, or null if parsing failed.
 	 */
 	@Nullable
-	private <T> SimpleLiteral<T> reparseLiteral(Class<T> type, Expression<?> expr) {
-		if (expr instanceof SimpleLiteral) { // Only works for simple literals
-			Expression<?> source = expr.getSource();
-
-			// Try to get access to unparsed content of it
-			if (source instanceof UnparsedLiteral) {
-				String unparsed = ((UnparsedLiteral) source).getData();
-				T data = Classes.parse(unparsed, type, ParseContext.DEFAULT);
-				if (data != null) { // Success, let's make a literal of it
-					return new SimpleLiteral<>(data, false, new UnparsedLiteral(unparsed));
-				}
+	private <T> SimpleLiteral<T> reparseLiteral(Class<T> type, Expression<?> expression) {
+		Expression<?> source = expression;
+		if (expression instanceof SimpleLiteral) // Only works for simple literals
+			source = expression.getSource();
+
+		// Try to get access to unparsed content of it
+		if (source instanceof UnparsedLiteral) {
+			String unparsed = ((UnparsedLiteral) source).getData();
+			T data = Classes.parse(unparsed, type, ParseContext.DEFAULT);
+			if (data != null) { // Success, let's make a literal of it
+				return new SimpleLiteral<>(data, false, new UnparsedLiteral(unparsed));
 			}
 		}
 		return null; // Context-sensitive parsing failed; can't really help it
 	}
-
+
+	/**
+	 * Attempts to transform an UnparsedLiteral into a type that is comparable to another.
+	 * For example 'fire' will be VisualEffect without this, but if the user attempts to compare 'fire'
+	 * with a block. This method will see if 'fire' can be compared to the block, and it will find ItemType.
+	 * Essentially solving something a human sees as comparable, but Skript doesn't understand.
+	 * 
+	 * @param one The UnparsedLiteral expression to attempt to reconstruct.
+	 * @param two any expression to grab the return type from.
+	 * @return The newly formed Literal, will be SimpleLiteral in most cases.
+	 */
+	@SuppressWarnings("unchecked")
+	private Literal<?> attemptReconstruct(UnparsedLiteral one, Expression<?> two) {
+		Expression<?> expression = null;
+		// Must handle numbers first.
+		expression = one.getConvertedExpression(Number.class);
+		if (expression == null) {
+			for (ConverterInfo<?, ?> converter : Converters.getConverterInfos()) {
+				// We're comparing the 'two' expression, so we need the 'from' for comparing.
+				if (!converter.getFrom().isAssignableFrom(two.getReturnType()))
+					continue;
+				// Must be comparable if we're going to attempt to convert this UnparsedLiteral. Otherwise why attempt something that won't work.
+				if (Comparators.getComparator(two.getReturnType(), converter.getTo()) == null)
+					continue;
+				expression = reparseLiteral(converter.getTo(), one);
+			}
+		}
+		if (expression == null)
+			expression = one.getConvertedExpression(two.getReturnType());
+		return (Literal<?>) expression;
+	}
+
 	/*
 	 * # := condition (e.g. is, is less than, contains, is enchanted with, has permission, etc.)
 	 * !# := not #
@@ -302,20 +334,20 @@ private <T> SimpleLiteral<T> reparseLiteral(Class<T> type, Expression<?> expr) {
 	 * neither a nor b # x or y === a !# x or y && b !# x or y			// nor = and
 	 */
 	@Override
-	@SuppressWarnings({"null", "unchecked"})
+	@SuppressWarnings("unchecked")
 	public boolean check(final Event e) {
 		final Expression<?> third = this.third;
 		return first.check(e, (Checker<Object>) o1 ->
 			second.check(e, (Checker<Object>) o2 -> {
 				if (third == null)
-					return relation.isImpliedBy(comp != null ? comp.compare(o1, o2) : Comparators.compare(o1, o2));
+					return relation.isImpliedBy(comparator != null ? comparator.compare(o1, o2) : Comparators.compare(o1, o2));
 				return third.check(e, (Checker<Object>) o3 -> {
 					boolean isBetween;
-					if (comp != null) {
+					if (comparator != null) {
 						isBetween =
-							(Relation.GREATER_OR_EQUAL.isImpliedBy(comp.compare(o1, o2)) && Relation.SMALLER_OR_EQUAL.isImpliedBy(comp.compare(o1, o3)))
+							(Relation.GREATER_OR_EQUAL.isImpliedBy(comparator.compare(o1, o2)) && Relation.SMALLER_OR_EQUAL.isImpliedBy(comparator.compare(o1, o3)))
 							// Check OPPOSITE (switching o2 / o3)
-							|| (Relation.GREATER_OR_EQUAL.isImpliedBy(comp.compare(o1, o3)) && Relation.SMALLER_OR_EQUAL.isImpliedBy(comp.compare(o1, o2)));
+							|| (Relation.GREATER_OR_EQUAL.isImpliedBy(comparator.compare(o1, o3)) && Relation.SMALLER_OR_EQUAL.isImpliedBy(comparator.compare(o1, o2)));
 					} else {
 						isBetween =
 							(Relation.GREATER_OR_EQUAL.isImpliedBy(Comparators.compare(o1, o2)) && Relation.SMALLER_OR_EQUAL.isImpliedBy(Comparators.compare(o1, o3)))
@@ -337,7 +369,7 @@ public String toString(final @Nullable Event e, final boolean debug) {
 		else
 			s = first.toString(e, debug) + " is " + (isNegated() ? "not " : "") + "between " + second.toString(e, debug) + " and " + third.toString(e, debug);
 		if (debug)
-			s += " (comparator: " + comp + ")";
+			s += " (comparator: " + comparator + ")";
 		return s;
 	}
 

src/main/java/ch/njol/skript/registrations/Classes.java

@@ -504,9 +504,9 @@ public static <T> T parse(final String s, final Class<T> c, final ParseContext c
 					continue;
 				if (c.isAssignableFrom(conv.getTo())) {
 					log.clear();
-					final Object o = parseSimple(s, conv.getFrom(), context);
-					if (o != null) {
-						t = (T) ((Converter) conv.getConverter()).convert(o);
+					Object object = parseSimple(s, conv.getFrom(), context);
+					if (object != null) {
+						t = (T) ((Converter) conv.getConverter()).convert(object);
 						if (t != null) {
 							log.printLog();
 							return t;

src/main/java/ch/njol/skript/test/runner/EffAssert.java

@@ -20,6 +20,7 @@
 
 import org.bukkit.event.Event;
 import org.eclipse.jdt.annotation.Nullable;
+import org.skriptlang.skript.lang.script.Script;
 
 import ch.njol.skript.Skript;
 import ch.njol.skript.doc.Description;
@@ -46,6 +47,7 @@ public class EffAssert extends Effect  {
 
 	@Nullable
 	private Condition condition;
+	private Script script;
 
 	private Expression<String> errorMsg;
 	private boolean shouldFail;
@@ -56,6 +58,7 @@ public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelaye
 		String conditionString = parseResult.regexes.get(0).group();
 		errorMsg = (Expression<String>) exprs[0];
 		shouldFail = parseResult.mark != 0;
+		script = getParser().getCurrentScript();
 
 		ParseLogHandler logHandler = SkriptLogger.startParseLogHandler();
 		try {
@@ -91,7 +94,7 @@ public TriggerItem walk(Event event) {
 			if (SkriptJUnitTest.getCurrentJUnitTest() != null) {
 				TestTracker.junitTestFailed(SkriptJUnitTest.getCurrentJUnitTest(), message);
 			} else {
-				TestTracker.testFailed(message);
+				TestTracker.testFailed(message, script);
 			}
 			return null;
 		}