Error when multiple event-values are present for a default expression

src/main/java/ch/njol/skript/expressions/base/EventValueExpression.java

@@ -20,21 +20,31 @@
 
 import java.lang.reflect.Array;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 
 import org.bukkit.event.Event;
+import org.bukkit.event.player.PlayerEvent;
 import org.eclipse.jdt.annotation.Nullable;
 
+import ch.njol.skript.ScriptLoader;
+import ch.njol.skript.Skript;
 import ch.njol.skript.SkriptAPIException;
 import ch.njol.skript.classes.Changer;
 import ch.njol.skript.classes.Changer.ChangeMode;
 import ch.njol.skript.classes.Changer.ChangerUtils;
 import ch.njol.skript.classes.ClassInfo;
+import ch.njol.skript.config.Node;
+import ch.njol.skript.config.SimpleNode;
 import ch.njol.skript.lang.DefaultExpression;
 import ch.njol.skript.lang.Expression;
+import ch.njol.skript.lang.SkriptParser;
+import ch.njol.skript.lang.Statement;
+import ch.njol.skript.lang.SyntaxElementInfo;
 import ch.njol.skript.lang.SkriptParser.ParseResult;
 import ch.njol.skript.lang.util.SimpleExpression;
+import ch.njol.skript.localization.Noun;
 import ch.njol.skript.log.ParseLogHandler;
 import ch.njol.skript.log.SkriptLogger;
 import ch.njol.skript.registrations.Classes;
@@ -142,7 +152,7 @@ public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelaye
 			throw new SkriptAPIException(this.getClass().getName() + " has expressions in its pattern but does not override init(...)");
 		return init();
 	}
-	
+
 	@Override
 	public boolean init() {
 		final ParseLogHandler log = SkriptLogger.startParseLogHandler();
@@ -158,6 +168,12 @@ public boolean init() {
 					hasValue = getters.get(event) != null;
 					continue;
 				}
+				if (EventValues.hasMultipleGetters(event, c, getTime()) == Kleenean.TRUE) {
+					Noun typeName = Classes.getExactClassInfo(componentType).getName();
+					log.printError("There are multiple " + typeName.toString(true) + " in " + Utils.a(getParser().getCurrentEventName()) + " event. " +
+							"You must define which " + typeName + " to use.");
+					return false;
+				}
 				Getter<? extends T, ?> getter;
 				if (exact) {
 					getter = EventValues.getExactEventValueGetter(event, c, getTime());

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

@@ -31,6 +31,7 @@
 import org.skriptlang.skript.lang.converter.Converters;
 import ch.njol.skript.expressions.base.EventValueExpression;
 import ch.njol.skript.util.Getter;
+import ch.njol.util.Kleenean;
 
 /**
  * @author Peter Güttinger
@@ -136,46 +137,53 @@ public String getExcludeErrorMessage() {
 			return futureEventValues;
 		throw new IllegalArgumentException("time must be -1, 0, or 1");
 	}
-	
+
 	/**
 	 * Registers an event value.
 	 * 
-	 * @param e the event type
-	 * @param c the type of the default value
-	 * @param g the getter to get the value
-	 * @param time -1 if this is the value before the event, 1 if after, and 0 if it's the default or this value doesn't have distinct states.
+	 * @param event the event type class.
+	 * @param type the return type of the getter for the event value.
+	 * @param getter the getter to get the value with the provided event.
+	 * @param time value of TIME_PAST if this is the value before the event, TIME_FUTURE if after, and TIME_NOW if it's the default or this value doesn't have distinct states.
 	 *            <b>Always register a default state!</b> You can leave out one of the other states instead, e.g. only register a default and a past state. The future state will
 	 *            default to the default state in this case.
 	 */
-	public static <T, E extends Event> void registerEventValue(Class<E> e, Class<T> c, Getter<T, E> g, int time) {
-		registerEventValue(e, c, g, time, null, (Class<? extends E>[]) null);
+	public static <T, E extends Event> void registerEventValue(Class<E> event, Class<T> type, Getter<T, E> getter, int time) {
+		registerEventValue(event, type, getter, time, null, (Class<? extends E>[]) null);
 	}
-	
+
 	/**
-	 * Same as {@link #registerEventValue(Class, Class, Getter, int)}
+	 * Registers an event value and with excluded events.
+	 * Excluded events are events that this event value can't operate in.
 	 * 
-	 * @param e
-	 * @param c
-	 * @param g
-	 * @param time -1 if this is the value before the event, 1 if after, and 0 if it's the default or this value doesn't have distinct states.
+	 * @param event the event type class.
+	 * @param type the return type of the getter for the event value.
+	 * @param getter the getter to get the value with the provided event.
+	 * @param time value of TIME_PAST if this is the value before the event, TIME_FUTURE if after, and TIME_NOW if it's the default or this value doesn't have distinct states.
 	 *            <b>Always register a default state!</b> You can leave out one of the other states instead, e.g. only register a default and a past state. The future state will
 	 *            default to the default state in this case.
-	 * @param excludes Subclasses of the event for which this event value should not be registered for
+	 * @param excludeErrorMessage The error message to display when used in the excluded events.
+	 * @param excludes subclasses of the event for which this event value should not be registered for
 	 */
 	@SafeVarargs
-	public static <T, E extends Event> void registerEventValue(Class<E> e, Class<T> c, Getter<T, E> g, int time, @Nullable String excludeErrorMessage, @Nullable Class<? extends E>... excludes) {
+	public static <T, E extends Event> void registerEventValue(Class<E> event, Class<T> type, Getter<T, E> getter, int time, @Nullable String excludeErrorMessage, @Nullable Class<? extends E>... excludes) {
 		Skript.checkAcceptRegistrations();
 		List<EventValueInfo<?, ?>> eventValues = getEventValuesList(time);
 		for (int i = 0; i < eventValues.size(); i++) {
 			EventValueInfo<?, ?> info = eventValues.get(i);
-			if (info.event != e ? info.event.isAssignableFrom(e) : info.c.isAssignableFrom(c)) {
-				eventValues.add(i, new EventValueInfo<>(e, c, g, excludeErrorMessage, excludes));
+			// We don't care for exact duplicates. Prefer Skript's over any addon.
+			if (info.event.equals(event) && info.c.equals(type))
+				return;
+			// If the events don't match, we prefer the highest subclass event.
+			// If the events match, we prefer the highest subclass type.
+			if (!info.event.equals(event) ? info.event.isAssignableFrom(event) : info.c.isAssignableFrom(type)) {
+				eventValues.add(i, new EventValueInfo<>(event, type, getter, excludeErrorMessage, excludes));
 				return;
 			}
 		}
-		eventValues.add(new EventValueInfo<>(e, c, g, excludeErrorMessage, excludes));
+		eventValues.add(new EventValueInfo<>(event, type, getter, excludeErrorMessage, excludes));
 	}
-	
+
 	/**
 	 * Gets a specific value from an event. Returns null if the event doesn't have such a value (conversions are done to try and get the desired value).
 	 * <p>
@@ -225,110 +233,155 @@ public static <T, E extends Event> T getEventValue(E e, Class<T> c, int time) {
 		return null;
 	}
 
+	/**
+	 * Checks if an event has multiple getters, including default ones.
+	 * 
+	 * @param event the event class the getter will be getting from.
+	 * @param type type of getter.
+	 * @param time the event-value's time.
+	 * @return true or false if the event and type have multiple getters.
+	 */
+	public static <T, E extends Event> Kleenean hasMultipleGetters(Class<E> event, Class<T> type, int time) {
+		List<Getter<? extends T, ? super E>> getters = getEventValueGetters(event, type, time, true);
+		if (getters == null)
+			return Kleenean.UNKNOWN;
+		return Kleenean.get(getters.size() > 1);
+	}
+
 	/**
 	 * Returns a getter to get a value from in an event.
 	 * <p>
 	 * Can print an error if the event value is blocked for the given event.
 	 * 
-	 * @param event the event class the getter will be getting from
-	 * @param c type of getter
-	 * @param time the event-value's time
-	 * @return A getter to get values for a given type of events
+	 * @param event the event class the getter will be getting from.
+	 * @param type type of getter.
+	 * @param time the event-value's time.
+	 * @return A getter to get values for a given type of events.
 	 * @see #registerEventValue(Class, Class, Getter, int)
 	 * @see EventValueExpression#EventValueExpression(Class)
 	 */
 	@Nullable
-	public static <T, E extends Event> Getter<? extends T, ? super E> getEventValueGetter(Class<E> event, Class<T> c, int time) {
-		return getEventValueGetter(event, c, time, true);
+	public static <T, E extends Event> Getter<? extends T, ? super E> getEventValueGetter(Class<E> event, Class<T> type, int time) {
+		return getEventValueGetter(event, type, time, true);
+	}
+
+	@Nullable
+	private static <T, E extends Event> Getter<? extends T, ? super E> getEventValueGetter(Class<E> event, Class<T> type, int time, boolean allowDefault) {
+		List<Getter<? extends T, ? super E>> list = getEventValueGetters(event, type, time, allowDefault);
+		if (list == null || list.isEmpty())
+			return null;
+		return list.get(0);
 	}
 
+	/*
+	 * We need to be able to collect all possible event-values to a list for determining problematic collisions.
+	 * Always return after the loop check if the list is not empty.
+	 */
 	@Nullable
 	@SuppressWarnings("unchecked")
-	private static <T, E extends Event> Getter<? extends T, ? super E> getEventValueGetter(Class<E> event, Class<T> c, int time, boolean allowDefault) {
+	private static <T, E extends Event> List<Getter<? extends T, ? super E>> getEventValueGetters(Class<E> event, Class<T> type, int time, boolean allowDefault) {
 		List<EventValueInfo<?, ?>> eventValues = getEventValuesList(time);
+		List<Getter<? extends T, ? super E>> list = new ArrayList<>();
 		// First check for exact classes matching the parameters.
-		Getter<? extends T, ? super E> exact = (Getter<? extends T, ? super E>) getExactEventValueGetter(event, c, time);
-		if (exact != null)
-			return exact;
+		Getter<? extends T, ? super E> exact = (Getter<? extends T, ? super E>) getExactEventValueGetter(event, type, time);
+		if (exact != null) {
+			list.add(exact);
+			return list;
+		}
 		// Second check for assignable subclasses.
 		for (EventValueInfo<?, ?> eventValueInfo : eventValues) {
-			if (!c.isAssignableFrom(eventValueInfo.c))
+			if (!type.isAssignableFrom(eventValueInfo.c))
 				continue;
 			if (!checkExcludes(eventValueInfo, event))
 				return null;
-			if (eventValueInfo.event.isAssignableFrom(event))
-				return (Getter<? extends T, ? super E>) eventValueInfo.getter;
+			if (eventValueInfo.event.isAssignableFrom(event)) {
+				list.add((Getter<? extends T, ? super E>) eventValueInfo.getter);
+				continue;
+			}
 			if (!event.isAssignableFrom(eventValueInfo.event))
 				continue;
-			return new Getter<T, E>() {
+			list.add(new Getter<T, E>() {
 				@Override
 				@Nullable
 				public T get(E event) {
 					if (!eventValueInfo.event.isInstance(event))
 						return null;
 					return ((Getter<? extends T, E>) eventValueInfo.getter).get(event);
 				}
-			};
+			});
+			continue;
 		}
+		if (!list.isEmpty())
+			return list;
 		// Most checks have returned before this below is called, but Skript will attempt to convert or find an alternative.
 		// Third check is if the returned object matches the class.
 		for (EventValueInfo<?, ?> eventValueInfo : eventValues) {
-			if (!eventValueInfo.c.isAssignableFrom(c))
+			if (!eventValueInfo.c.isAssignableFrom(type))
 				continue;
 			boolean checkInstanceOf = !eventValueInfo.event.isAssignableFrom(event);
 			if (checkInstanceOf && !event.isAssignableFrom(eventValueInfo.event))
 				continue;
 			if (!checkExcludes(eventValueInfo, event))
 				return null;
-			return new Getter<T, E>() {
+			list.add(new Getter<T, E>() {
 				@Override
 				@Nullable
 				public T get(E event) {
 					if (checkInstanceOf && !eventValueInfo.event.isInstance(event))
 						return null;
 					Object object = ((Getter<? super T, ? super E>) eventValueInfo.getter).get(event);
-					if (c.isInstance(object))
+					if (type.isInstance(object))
 						return (T) object;
 					return null;
 				}
-			};
+			});
+			continue;
 		}
+		if (!list.isEmpty())
+			return list;
 		// Fourth check will attempt to convert the event value to the requesting type.
 		// This first for loop will check that the events are exact. See issue #5016
 		for (EventValueInfo<?, ?> eventValueInfo : eventValues) {
 			if (!event.equals(eventValueInfo.event))
 				continue;
 
-			Getter<? extends T, ? super E> getter = (Getter<? extends T, ? super E>) getConvertedGetter(eventValueInfo, c, false);
+			Getter<? extends T, ? super E> getter = (Getter<? extends T, ? super E>) getConvertedGetter(eventValueInfo, type, false);
 			if (getter == null)
 				continue;
 
 			if (!checkExcludes(eventValueInfo, event))
 				return null;
-			return getter;
+			list.add(getter);
+			continue;
 		}
+		if (!list.isEmpty())
+			return list;
 		// This loop will attempt to look for converters assignable to the class of the provided event.
 		for (EventValueInfo<?, ?> eventValueInfo : eventValues) {
 			// The requesting event must be assignable to the event value's event. Otherwise it'll throw an error.
 			if (!event.isAssignableFrom(eventValueInfo.event))
 				continue;
 
-			Getter<? extends T, ? super E> getter = (Getter<? extends T, ? super E>) getConvertedGetter(eventValueInfo, c, true);
+			Getter<? extends T, ? super E> getter = (Getter<? extends T, ? super E>) getConvertedGetter(eventValueInfo, type, true);
 			if (getter == null)
 				continue;
 
 			if (!checkExcludes(eventValueInfo, event))
 				return null;
-			return getter;
+			list.add(getter);
+			continue;
 		}
+		if (!list.isEmpty())
+			return list;
 		// If the check should try again matching event values with a 0 time (most event values).
 		if (allowDefault && time != 0)
-			return getEventValueGetter(event, c, 0, false);
+			return getEventValueGetters(event, type, 0, false);
 		return null;
 	}
 
 	/**
 	 * Check if the event value states to exclude events.
+	 * False if the current EventValueInfo cannot operate in the provided event.
 	 * 
 	 * @param info The event value info that will be used to grab the value from
 	 * @param event The event class to check the excludes against.

src/main/resources/lang/default.lang

@@ -1973,7 +1973,7 @@ types:
 	inventory: inventor¦y¦ies @an
 	player: player¦s @a
 	offlineplayer: offline player¦s @a
-	commandsender: player¦¦s¦/console @a
+	commandsender: command sender¦s @a
 	inventoryholder: inventory holder¦s @an
 	gamemode: gamemode¦s @a
 	material: material¦s @a