Adding a plugin verification system detecting common programmer errors

Initially we will detect plugins that attempt to register a listener in 
ProtocolLib without setting "depend" or "soft-depend".

ProtocolLib/src/main/java/com/comphenix/protocol/ProtocolLibrary.java

@@ -147,7 +147,7 @@ public class ProtocolLibrary extends JavaPlugin {
 			
 			unhookTask = new DelayedSingleTask(this);
 			protocolManager = new PacketFilterManager(
-					getClassLoader(), getServer(), version, unhookTask, detailedReporter);
+					getClassLoader(), getServer(), this, version, unhookTask, detailedReporter);			
 			
 			// Setup error reporter
 			detailedReporter.addGlobalParameter("manager", protocolManager);

ProtocolLib/src/main/java/com/comphenix/protocol/injector/PacketFilterManager.java

@@ -148,17 +148,22 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok
 	// Spigot listener, if in use
 	private SpigotPacketInjector spigotInjector;
 	
+	// Plugin verifier
+	private PluginVerifier pluginVerifier;
+	
 	/**
 	 * Only create instances of this class if protocol lib is disabled.
 	 */
-	public PacketFilterManager(ClassLoader classLoader, Server server, DelayedSingleTask unhookTask, ErrorReporter reporter) {
+	public PacketFilterManager(ClassLoader classLoader, Server server, Plugin library, DelayedSingleTask unhookTask, ErrorReporter reporter) {
-		this(classLoader, server, new MinecraftVersion(server), unhookTask, reporter);
+		this(classLoader, server, library, new MinecraftVersion(server), unhookTask, reporter);
 	}
 	
 	/**
 	 * Only create instances of this class if protocol lib is disabled.
 	 */
-	public PacketFilterManager(ClassLoader classLoader, Server server, MinecraftVersion mcVersion, DelayedSingleTask unhookTask, ErrorReporter reporter) {
+	public PacketFilterManager(ClassLoader classLoader, Server server, Plugin library, 
+							   MinecraftVersion mcVersion, DelayedSingleTask unhookTask, ErrorReporter reporter) {
+		
 		if (reporter == null)
 			throw new IllegalArgumentException("reporter cannot be NULL.");
 		if (classLoader == null)
@@ -177,6 +182,9 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok
 		this.classLoader = classLoader;
 		this.reporter = reporter;
 		
+		// The plugin verifier
+		this.pluginVerifier = new PluginVerifier(library);
+		
 		// Used to determine if injection is needed
 		Predicate<GamePhase> isInjectionNecessary = new Predicate<GamePhase>() {
 			@Override
@@ -267,6 +275,20 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok
 		return ImmutableSet.copyOf(packetListeners);
 	}
 
+	/**
+	 * Warn of common programming mistakes.
+	 * @param plugin - plugin to check.
+	 */
+	private void printPluginWarnings(Plugin plugin) {
+		switch (pluginVerifier.verify(plugin)) {
+			case NO_DEPEND:
+				reporter.reportWarning(this, plugin + " doesn't depend on ProtocolLib. Check that its plugin.yml has a 'depend' directive.");
+			case VALID:
+				// Do nothing
+				break;
+		}
+	}
+	
 	@Override
 	public void addPacketListener(PacketListener listener) {
 		if (listener == null)
@@ -275,6 +297,8 @@ public final class PacketFilterManager implements ProtocolManager, ListenerInvok
 		// A listener can only be added once
 		if (packetListeners.contains(listener))
 			return;
+		// Check plugin
+		printPluginWarnings(listener.getPlugin());
 		
 		ListeningWhitelist sending = listener.getSendingWhitelist();
 		ListeningWhitelist receiving = listener.getReceivingWhitelist();

ProtocolLib/src/main/java/com/comphenix/protocol/injector/PluginVerifier.java

@@ -0,0 +1,211 @@
+package com.comphenix.protocol.injector;
+
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+
+import org.bukkit.Bukkit;
+import org.bukkit.plugin.Plugin;
+import org.bukkit.plugin.PluginLoadOrder;
+
+import com.google.common.collect.Sets;
+
+/**
+ * Determine if a plugin using ProtocolLib is correct.
+ * 
+ * @author Kristian
+ */
+class PluginVerifier {
+	/**
+	 * A named plugin cannot be found.
+	 * @author Kristian
+	 */
+	public static class PluginNotFoundException extends RuntimeException {
+		/**
+		 * Generated by Eclipse.
+		 */
+		private static final long serialVersionUID = 8956699101336877611L;
+
+		public PluginNotFoundException() {
+			super();
+		}
+
+		public PluginNotFoundException(String message) {
+			super(message);
+		}
+	}
+	
+	public enum VerificationResult {
+		VALID,
+		
+		/**
+		 * The plugin doesn't depend on ProtocolLib directly or indirectly.
+		 */
+		NO_DEPEND;
+		
+		/**
+		 * Determine if the verification was valid.
+		 */
+		public boolean isValid() {
+			return this == VALID;
+		}
+	}
+	
+	/**
+	 * Set of plugins that have been loaded after ProtocolLib.
+	 */
+	private final Set<String> loadedAfter = new HashSet<String>();
+	
+	/**
+	 * Reference to ProtocolLib.
+	 */
+	private final Plugin dependency;
+
+	/**
+	 * Construct a new plugin verifier.
+	 * @param dependency - reference to ProtocolLib, a dependency we require of plugins.
+	 */
+	public PluginVerifier(Plugin dependency) {
+		if (dependency == null)
+			throw new IllegalArgumentException("dependency cannot be NULL.");
+		// This would screw with the assumption in hasDependency(Plugin, Plugin)
+		if (safeConversion(dependency.getDescription().getLoadBefore()).size() > 0)
+			throw new IllegalArgumentException("dependency cannot have a load directives.");
+		
+		this.dependency = dependency;
+	}
+
+	/**
+	 * Retrieve a plugin by name.
+	 * @param pluginName - the non-null name of the plugin to retrieve.
+	 * @return The retrieved plugin.
+	 * @throws PluginNotFoundException If a plugin with the given name cannot be found.
+	 */
+	private Plugin getPlugin(String pluginName) {
+		Plugin plugin = Bukkit.getPluginManager().getPlugin(pluginName);
+		
+		// Ensure that the plugin exists
+		if (plugin != null)
+			return plugin;
+		else
+			throw new PluginNotFoundException("Cannot find plugin " + pluginName); 
+	}
+	
+	/**
+	 * Performs simple verifications on the given plugin.
+	 * <p>
+	 * Results may be cached.
+	 * @param pluginName - the plugin to verify.
+	 * @return A verification result.
+	 * @throws IllegalArgumentException If plugin name is NULL.
+	 * @throws PluginNotFoundException If a plugin with the given name cannot be found.
+	 */
+	public VerificationResult verify(String pluginName) {
+		if (pluginName == null)
+			throw new IllegalArgumentException("pluginName cannot be NULL.");
+		return verify(getPlugin(pluginName));
+	}
+	
+	/**
+	 * Performs simple verifications on the given plugin.
+	 * <p>
+	 * Results may be cached.
+	 * @param plugin - the plugin to verify.
+	 * @return A verification result.
+	 * @throws IllegalArgumentException If plugin name is NULL.
+	 * @throws PluginNotFoundException If a plugin with the given name cannot be found.
+	 */
+	public VerificationResult verify(Plugin plugin) {
+		if (plugin == null)
+			throw new IllegalArgumentException("plugin cannot be NULL.");
+		
+		if (!loadedAfter.contains(plugin.getName())) {
+			if (verifyLoadOrder(dependency, plugin)) {
+				// Memorize
+				loadedAfter.add(plugin.getName());
+			} else {
+				return VerificationResult.NO_DEPEND;
+			}
+		}
+		
+		// Everything seems to be in order
+		return VerificationResult.VALID;
+	}
+	
+	/**
+	 * Determine if a given plugin is guarenteed to be loaded before the other.
+	 * <p>
+	 * Note that the before plugin is assumed to have no "load" directives - that is, plugins to be 
+	 * loaded after itself. The after plugin may have "load" directives, but it is irrelevant for our purposes.
+	 * @param beforePlugin - the plugin that is loaded first.
+	 * @param afterPlugin - the plugin that is loaded last.
+	 * @return TRUE if it will, FALSE if it may or must load in the opposite other.
+	 */
+	private boolean verifyLoadOrder(Plugin beforePlugin, Plugin afterPlugin) {
+		// If a plugin has a dependency, it will be loaded after its dependency
+		if (hasDependency(afterPlugin, beforePlugin)) {
+			return true;
+		}
+		
+		// No dependency - check the load order
+		if (beforePlugin.getDescription().getLoad() == PluginLoadOrder.STARTUP && 
+			afterPlugin.getDescription().getLoad() == PluginLoadOrder.POSTWORLD) {
+			return true;
+		}
+		return false;
+	}
+	
+	/**
+	 * Determine if a plugin has a given dependency, either directly or indirectly.
+	 * @param plugin - the plugin to check.
+	 * @param dependency - the dependency to find.
+	 * @return TRUE if the plugin has the given dependency, FALSE otherwise. 
+	 */
+	private boolean hasDependency(Plugin plugin, Plugin dependency) {
+		return hasDependency(plugin, dependency, Sets.<String>newHashSet());
+	}
+	
+	/**
+	 * Convert a list to a set. 
+	 * <p>
+	 * A null list will be converted to an empty set.
+	 * @param list - the list to convert.
+	 * @return The converted list.
+	 */
+	private Set<String> safeConversion(List<String> list) {
+		if (list == null)
+			return Collections.emptySet();
+		else
+			return Sets.newHashSet(list);
+	}
+	
+	// Avoid cycles
+	private boolean hasDependency(Plugin plugin, Plugin dependency, Set<String> checked) {
+		Set<String> childNames = Sets.union(
+				 safeConversion(plugin.getDescription().getDepend()),
+				 safeConversion(plugin.getDescription().getSoftDepend())
+		);
+		
+		// Ensure that the same plugin isn't processed twice
+		if (!checked.add(plugin.getName())) {
+			throw new IllegalStateException("Cycle detected in dependency graph: " + plugin);
+		}
+		// Look for the dependency in the immediate children
+		if (childNames.contains(dependency.getName())) {
+			return true;
+		}
+		
+		// Recurse through their dependencies
+		for (String childName : childNames) {
+			Plugin childPlugin = getPlugin(childName);
+			
+			if (hasDependency(childPlugin, dependency, checked)) {
+				return true;
+			}
+		}
+		
+		// No dependency found!
+		return false;
+	}
+}