GITHUB-34: final methods, interfaces and enums

springloaded/notes.md

@@ -1,10 +1,28 @@
 
+Implementation details:
+
+catchers
+- A catcher is added to a reloadable type as it is being loaded for the first time. A reloadable type gets
+a catcher for each method it inherits from a parent but does not override, or for an abstract class each
+method it receives from an interface.  They basically stand in for methods that could be added in later
+reloads. The catchers 'catch' virtual method dispatches. Their job is either to (a) make a simple super
+call if the reloadable type does not implement the specified method (yet), or (b) if the method
+has been implemented by a reload, forward to the new implementation.  Catchers are not created for
+private, final or static methods since those cannot be overridden.  If you modify a final method in a reloadable
+type to make it non final and then override it, it will be handled in a different way than catchers.
+
+superdispatchers
+- Where a method is protected in a non-reloadable type it is necessary to add a superdispatcher method to the
+subtype so that when the executor for a new version is running it can access that protected method. The 
+superdispatcher is simply a public method on a type that calls super.
+
 
 
 Helpful snippets when debugging tests:
 	ClassPrinter.print(z.getLatestExecutorBytes());
 	Utils.dump("foo/SubControllerB", rtype.bytesLoaded);
 		
-		
+
+
 		
 		

springloaded/src/main/java/org/springsource/loaded/AbstractMember.java

@@ -109,7 +109,7 @@ public abstract class AbstractMember implements Constants {
 		return Modifier.isPublic(getModifiers());
 	}
 
-	public boolean isPrivateStaticFinal() {
+	public boolean isPrivateOrStaticOrFinal() {
 		return (modifiers & ACC_PRIVATE_STATIC_FINAL) != 0;
 	}
 

springloaded/src/main/java/org/springsource/loaded/TypeDescriptorExtractor.java

@@ -36,6 +36,8 @@ import org.objectweb.asm.Opcodes;
  */
 public class TypeDescriptorExtractor {
 
+	private final static boolean DEBUG_TYPE_DESCRIPTOR_EXTRACTOR = false;
+	
 	private TypeRegistry registry;
 
 	public TypeDescriptorExtractor(TypeRegistry registry) {
@@ -50,7 +52,7 @@ public class TypeDescriptorExtractor {
 	}
 
 	/**
-	 * Visit a class and accumulate enough information to build a TypeDescriptor.
+	 * Visit a class and accumulate sufficient information to build a TypeDescriptor.
 	 */
 	class ExtractionVisitor implements ClassVisitor, Opcodes {
 
@@ -60,7 +62,9 @@ public class TypeDescriptorExtractor {
 		private String superclassName;
 		private String[] interfaceNames;
 		private boolean isGroovy = false;
+		private boolean isEnum = false;
 		private boolean hasClinit = false;
+		// TODO [perf - reduce garbage] make these collections lazily initialize
 		private List<MethodMember> constructors = new ArrayList<MethodMember>();
 		private List<MethodMember> methods = new ArrayList<MethodMember>();
 		private List<FieldMember> fieldsRequiringAccessors = new ArrayList<FieldMember>();
@@ -71,7 +75,6 @@ public class TypeDescriptorExtractor {
 			this.isReloadableType = isReloadableType;
 		}
 		
-
 		public TypeDescriptor getTypeDescriptor() {
 			if (isReloadableType) {
 				computeCatchersAndSuperdispatchers();
@@ -87,7 +90,7 @@ public class TypeDescriptorExtractor {
 		}
 
 		/**
-		 * Determine if there are clashes. A clash is where a static method takes the this reloadable type as its first parameter
+		 * Determine if there are clashes. A clash is where a static method takes the reloadable type as its first parameter
 		 * but in all other ways is the same as an existing instance method. For example this instance method A.foo(String) clashes
 		 * with this static method A.foo(A, String). 'clashing' means the executor will have to do something to avoid a duplicate
 		 * method problem and we'll have to differentiate between the two.
@@ -120,6 +123,7 @@ public class TypeDescriptorExtractor {
 			return registry.getDescriptorFor(slashedname);
 		}
 
+		// TODO [refactor] extract the type registry relationship code into a central helper class
 		private TypeDescriptor findTypeDescriptor(TypeRegistry registry, String typename) {
 			// follow the pattern for a classloader: recurse up trying to find it, then recurse down trying to load it
 			TypeRegistry regToTry = registry;
@@ -138,77 +142,92 @@ public class TypeDescriptorExtractor {
 		}
 
 		/**
-		 * Create catcher methods for methods from our super-hierarchy that we don't yet override (but may after the initial define
-		 * has happened).
+		 * Algorithm: Go up the superclass hierarchy for a type and determine what should be caught in this type (see 
+		 * 'catchers' in notes.md). Methods that are private, static or final do *not* get a catcher. This method
+		 * also computes superdispatchers - see 'superdispatchers' in notes.md
+		 * 
+		 */
+		private void walkHierarchyForCatchersAndSuperDispatchers(String superclass, List<String> superDispatchers, List<String> finalInHierarchy) {
+			TypeDescriptor supertypeDescriptor = superclass==null?null:findTypeDescriptor(registry, superclass);
+			if (DEBUG_TYPE_DESCRIPTOR_EXTRACTOR) {
+				System.out.println("Computing catchers on "+this.typename+" from superclass "+superclass);
+			}
+			boolean isReloadable = supertypeDescriptor.isReloadable();
+			for (MethodMember method: supertypeDescriptor.getMethods()) {
+				if (shouldCreateSuperDispatcherFor(method) && !superDispatchers.contains(method.nameAndDescriptor)) {
+					// need a public super dispatcher - so that we can reach that super method
+					// from a reloaded instance of this type
+					MethodMember superdispatcher = method.superDispatcherFor();
+					methods.add(superdispatcher);
+					superDispatchers.add(method.nameAndDescriptor);
+				}
+				if (shouldCatchMethod(method) && !finalInHierarchy.contains(method.getNameAndDescriptor())) {
+					// don't need the catcher if method is already defined since when the existing method is rewritten
+					// it will be kind of morphed into a catcher
+					// TODO what about a private method that is overridden by a static method (same name/descriptor but not
+					// an overrides relationship)
+					if (!isReloadable && Modifier.isFinal(method.getModifiers())) {
+						// Do not create a catcher, the supertype is not reloadable and so an implementation cannot be
+						// added lower in the type hierarchy
+						finalInHierarchy.add(method.getNameAndDescriptor());
+						continue;
+					}
+					MethodMember found = null;
+					for (MethodMember existingMethod : methods) {
+						if (existingMethod.equalsApartFromModifiers(method)) {
+							found = existingMethod;
+							break;
+						}
+					}
+					if (found != null) {
+						continue;
+					}
+					MethodMember catcherCopy = method.catcherCopyOf();
+					if (DEBUG_TYPE_DESCRIPTOR_EXTRACTOR) {
+						System.out.println("Adding catcher for "+method.nameAndDescriptor);
+					}
+					methods.add(catcherCopy);
+				} else {
+					if (method.isFinal()) {
+						finalInHierarchy.add(method.getNameAndDescriptor());
+					}
+				}
+			}
+			if (supertypeDescriptor.supertypeName != null) {
+				walkHierarchyForCatchersAndSuperDispatchers(supertypeDescriptor.supertypeName, superDispatchers, finalInHierarchy);
+			}
+			if (Modifier.isAbstract(this.flags) && !this.isEnum/* && !Modifier.isInterface(this.flags)*/) {
+				// abstract class may be missing methods that it can implement from the interfaces
+				for (String interfaceName : supertypeDescriptor.superinterfaceNames) {
+					addCatchersForNonImplementedMethodsFrom(interfaceName, finalInHierarchy);
+				}
+			}
+		}
+		
+		/**
+		 * Compute and add the catch methods and super dispatch methods that apply to this type.
 		 */
 		private void computeCatchersAndSuperdispatchers() {
-			// When walking up the hierarchy we may hit a 'final' method which means we must not catch it.
-			// The 'shouldNotCatch' list stores things we discover like this that should not be caught
-			List<String> shouldNotCatch = new ArrayList<String>();
-
-			String type = superclassName;
-			// Don't need catchers in interfaces
-			if (Modifier.isInterface(this.flags)) {
+			if (Modifier.isInterface(this.flags)) { // Don't need catchers in interfaces
 				return;
 			}
-			List<String> superDispatcherAddedFor = new ArrayList<String>();
-			while (type != null) {
-				TypeDescriptor supertypeDescriptor = findTypeDescriptor(registry, type);
-				// TODO review the need to create catchers for methods where the supertype is reloadable.  In this situation we are already going to
-				// be intercepting the call side of these methods so we don't need the catcher.  Could be a large performance increase and reduction in 
-				// permgen, and simplification of stack traces
-				//				if (!supertypeDescriptor.isReloadable()) {
-				for (MethodMember method : supertypeDescriptor.getMethods()) {
-					if (shouldCreateSuperDispatcherFor(method) && !superDispatcherAddedFor.contains(method.nameAndDescriptor)) {
-						// need a public super dispatcher - so that we can reach that super method
-						// from a reloaded instance of this type
-						MethodMember superdispatcher = method.superDispatcherFor();
-						methods.add(superdispatcher);
-						superDispatcherAddedFor.add(method.nameAndDescriptor);
-					}
-					
-					if (shouldCatchMethod(method) && !shouldNotCatch.contains(method.getNameAndDescriptor())) {
-						// don't need the catcher if method is already defined since when the existing method is rewritten
-						// it will be kind of morphed into a catcher
-						// TODO what about a private method that is overridden by a static method (same name/descriptor but not
-						// an overrides relationship)
-						//						if (supertypeDescriptor.isGroovyType() && !isGroovy) {
-						//							if (method.getName().startsWith("super$")) {
-						//								continue;
-						//							}
-						//						}
-						MethodMember found = null;
-						for (MethodMember existingMethod : methods) {
-							if (existingMethod.equalsApartFromModifiers(method)) {
-								found = existingMethod;
-								break;
-							}
-						}
-						if (found != null) {
-							continue;
-						}
-						MethodMember catcherCopy = method.catcherCopyOf();
-						//						System.out.println("catcher is " + catcherCopy + "  is groovy type? " + this.isGroovy);
-						methods.add(catcherCopy);
-					} else {
-						if (method.isFinal()) {
-							shouldNotCatch.add(method.getNameAndDescriptor());
-						}
-					}
-				}
-				//				}
-				type = supertypeDescriptor.supertypeName;
-			}
+			
+			// TODO [review design] review the need to create catchers for methods where the supertype is reloadable.
+			// Can we just add them to the topmost reloadable type?
+			List<String> doNotCatch = new ArrayList<String>();
+			walkHierarchyForCatchersAndSuperDispatchers(superclassName, new ArrayList<String>(), doNotCatch);
 
-			// ought to look in interfaces *if* we are an abstract class
-			if (Modifier.isAbstract(this.flags)/* && !Modifier.isInterface(this.flags)*/) {
-				// abstract class
+			// ought to look in interfaces if we are an abstract class
+			if (Modifier.isAbstract(this.flags) && !this.isEnum/* && !Modifier.isInterface(this.flags)*/) {
+				// abstract class may be missing methods that it can implement from the interfaces
 				for (String interfaceName : interfaceNames) {
-					addCatchersForNonImplementedMethodsFrom(interfaceName);
+					addCatchersForNonImplementedMethodsFrom(interfaceName, doNotCatch);
 				}
 			}
-
-			finalInHierarchy.addAll(shouldNotCatch);
+			if (DEBUG_TYPE_DESCRIPTOR_EXTRACTOR) {
+				System.out.println("For "+this.typename+" setting finalsInHierarchy to "+doNotCatch);
+			}
+			finalInHierarchy.addAll(doNotCatch);
 		}
 
 		// TODO should clone and finalize be in here?
@@ -218,8 +237,7 @@ public class TypeDescriptorExtractor {
 					(method.getName().equals("clone") && method.getDescriptor().equals("()Ljava/lang/Object;")));
 		}
 
-
-		private void addCatchersForNonImplementedMethodsFrom(String interfacename) {
+		private void addCatchersForNonImplementedMethodsFrom(String interfacename,List<String> finalInNonReloadableType) {
 			TypeDescriptor interfaceDescriptor = findTypeDescriptor(registry, interfacename);
 			for (MethodMember method : interfaceDescriptor.getMethods()) {
 				// If this class doesn't implement this interface method, add it
@@ -230,17 +248,21 @@ public class TypeDescriptorExtractor {
 						break;
 					}
 				}
-				if (!found) {
+				if (!found && !finalInNonReloadableType.contains(method.getNameAndDescriptor())) {
+					if (DEBUG_TYPE_DESCRIPTOR_EXTRACTOR) {
+						Log.log("adding catcher for ["+method+"] from ["+interfacename+"] to ["+this.typename+"]");
+					}
 					methods.add(method.catcherCopyOfWithAbstractRemoved());
 				}
 			}
 			for (String interfaceName : interfaceDescriptor.superinterfaceNames) {
-				addCatchersForNonImplementedMethodsFrom(interfaceName);
+				addCatchersForNonImplementedMethodsFrom(interfaceName,finalInNonReloadableType);
 			}
 		}
 
 		/**
-		 * Field
+		 * Protected fields in reloadable parents of a class need an accessor adding to the reloadable
+		 * type so that the fields can be reached from the executor.
 		 */
 		private void computeFieldsRequiringAccessors() {
 			String type = superclassName;
@@ -274,13 +296,17 @@ public class TypeDescriptorExtractor {
 		 * @return true if it should be caught
 		 */
 		private boolean shouldCatchMethod(MethodMember method) {
-			return !(method.isPrivateStaticFinal() || method.getName().endsWith(Constants.methodSuffixSuperDispatcher) || (method.getName().equals("finalize") && method.getDescriptor().equals("()V")));
+			return !(method.isPrivateOrStaticOrFinal() || method.getName().endsWith(Constants.methodSuffixSuperDispatcher) || 
+					 (method.getName().equals("finalize") && method.getDescriptor().equals("()V")));
 		}
 
 		public void visit(int version, int flags, String name, String signature, String superclassName, String[] interfaceNames) {
 			this.flags = flags;
 			this.superclassName = superclassName;
 			this.interfaceNames = interfaceNames;
+			if (superclassName!=null && superclassName.equals("java/lang/Enum")) {
+				this.isEnum = true;
+			}
 			this.typename = name;
 		}
 
@@ -304,7 +330,7 @@ public class TypeDescriptorExtractor {
 			}
 			return null;
 		}
-		// For each method, copy it into the new class making appropriate adjustments
+
 		/**
 		 * Visit a method in the class and build an appropriate representation for it to include in the extracted output.
 		 */

springloaded/src/test/java/org/springsource/loaded/test/CrossLoaderTests.java

@@ -459,4 +459,18 @@ public class CrossLoaderTests extends SpringLoadedTests {
 	// set the superclass for a reloadabletype when the clinit runs for the subtype, for quicker lookup of exactly the type we need (getRegistryFor(clazz.getClassLoader())
 	// use those sparse arrays and ID numbers more so that type lookups can be quicker.  Once we truly discover the right super type reference from a subtype, fill it in in the array
 	// optimize for the case where there will only be one type around of a given name *usually*
+	
+	@Test
+	public void github34() throws Exception {
+		ReloadableType rtypeA = subLoader.loadAsReloadableType("issue34.Implementation3");
+		result = runUnguarded(rtypeA.getClazz(), "run");
+		assertEquals("Hello World!", result.stdout);
+
+//		ReloadableType rtypeB = subLoader.loadAsReloadableType("subpkg.Bottom");
+//		result = runUnguarded(rtypeB.getClazz(), "m");
+//		assertEquals("Bottom.m() running", result.stdout);
+//		assertNotSame(rtypeA.getTypeRegistry(), rtypeB.getTypeRegistry());
+	}
+	
+	
 }

springloaded/src/test/java/org/springsource/loaded/test/SpringLoadedTestsInSeparateJVM.java

@@ -18,6 +18,7 @@ package org.springsource.loaded.test;
 import static org.junit.Assert.fail;
 
 import org.junit.AfterClass;
+import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.springsource.loaded.test.ReloadingJVM.JVMOutput;
@@ -54,6 +55,25 @@ public class SpringLoadedTestsInSeparateJVM extends SpringLoadedTests {
 		JVMOutput output = jvm.run("jvmtwo.Runner");
 		assertStdout("jvmtwo.Runner.run() running", output);
 	}
+	
+	@Test
+	public void githubIssue34() throws Exception {
+		jvm.copyToTestdataDirectory("issue34.Interface1"); 
+		jvm.copyToTestdataDirectory("issue34.Interface2"); 
+		jvm.copyToTestdataDirectory("issue34.Implementation1"); 
+		jvm.copyToTestdataDirectory("issue34.Implementation2"); 
+		jvm.copyToTestdataDirectory("issue34.Implementation3"); 
+		JVMOutput output = jvm.run("issue34.Implementation3");
+		assertStdout("Hello World!\n", output);		
+	}
+
+	@Test
+	public void githubIssue34_2() throws Exception {
+		jvm.copyToTestdataDirectory("issue34.InnerEnum$sorters"); 
+		jvm.copyToTestdataDirectory("issue34.InnerEnum$sorters$1"); 
+		JVMOutput output = jvm.run("issue34.InnerEnum");
+		assertStdout("Hello World!\n", output);		
+	}
 
 	@Test
 	public void testCreatingAndInvokingMethodsOnInstance() throws Exception {

springloaded/src/test/java/org/springsource/loaded/test/infra/SubLoader.java

@@ -40,6 +40,8 @@ import org.springsource.loaded.test.TestUtils;
  */
 public class SubLoader extends ClassLoader {
 
+	public static final boolean DEBUG_LOADING = false;
+	
 	// @formatter:off
 	static String[] folders = new String[] { 
 		TestUtils.getPathToClasses("../testdata-subloader")
@@ -60,6 +62,9 @@ public class SubLoader extends ClassLoader {
 	}
 
 	public ReloadableType loadAsReloadableType(String typename) throws ClassNotFoundException {
+		if (DEBUG_LOADING) {
+			System.out.println("SubLoader: loading "+typename);
+		}
 		if (tr == null) {
 			tr = TypeRegistry.getTypeRegistryFor(this);
 		}

springloaded/src/test/java/org/springsource/loaded/test/infra/SuperLoader.java

@@ -73,7 +73,9 @@ public class SuperLoader extends ClassLoader {
 	}
 
 	public Class<?> findClass(String name) throws ClassNotFoundException {
-		//		System.out.println(">> SuperLoader.findClass(" + name + ")");
+		if (SubLoader.DEBUG_LOADING) {
+			System.out.println("> SuperLoader: findClass(" + name + ")");
+		}
 		Class<?> c = null;
 		// Look in the filesystem first
 		try {
@@ -87,6 +89,9 @@ public class SuperLoader extends ClassLoader {
 						ReloadableType rtype = tr.addType(name, data);
 						data = rtype.bytesLoaded;
 					}
+					if (SubLoader.DEBUG_LOADING) {
+						System.out.println("  SuperLoader: found in folder: '"+folders[i]+"'");
+					}
 					c = defineClass(name, data, 0, data.length);
 					break;
 				}
@@ -105,6 +110,7 @@ public class SuperLoader extends ClassLoader {
 					if (zipentry != null) {
 						byte[] data = Utils.loadBytesFromStream(zipfile.getInputStream(zipentry));
 						TypeRegistry tr = TypeRegistry.getTypeRegistryFor(this);
+//						data = new SpringLoadedPreProcessor().preProcess(this, slashedClassName, null, data);
 						if (tr != null) {
 
 							// Give the plugins a chance to rewrite stuff too
@@ -120,7 +126,9 @@ public class SuperLoader extends ClassLoader {
 							//System.out.println("Transforming " + name);
 							data = tr.methodCallRewrite(data);
 						}
-
+						if (SubLoader.DEBUG_LOADING) {
+							System.out.println("  SuperLoader: found in zip: '"+jars[i]+"'");
+						}
 						c = defineClass(name, data, 0, data.length);
 						break;
 					}

testdata-subloader/src/main/java/issue34/Implementation2.java

@@ -0,0 +1,4 @@
+package issue34;
+public abstract class Implementation2<T1,T2> extends grails.Implementation1<T2> implements Interface2<T1,T2> {
+
+}

testdata-subloader/src/main/java/issue34/Implementation3.java

@@ -0,0 +1,12 @@
+package issue34;
+public class Implementation3 extends Implementation2<String, Integer> {
+
+    public static void main(String [] args) {
+        System.out.println("Hello World!");
+    }
+    
+    public static void run() {
+    	Implementation3.main(null);
+    }
+
+}

testdata-subloader/src/main/java/issue34/Interface2.java

@@ -0,0 +1,4 @@
+package issue34;
+public interface Interface2<T1, T2> extends grails.Interface1<T2> {
+
+}

testdata-superloader/src/main/java/grails/Implementation1.java

@@ -0,0 +1,10 @@
+package grails;
+
+public abstract class Implementation1<T> implements Interface1<T> {
+
+    @Override
+    public final T process1(T type) {
+        return type;
+    }
+
+}

testdata-superloader/src/main/java/grails/Interface1.java

@@ -0,0 +1,6 @@
+package grails;
+public interface Interface1<T> {
+
+    public T process1(T type);
+
+}

testdata/src/main/java/issue34/Implementation1.java

@@ -0,0 +1,10 @@
+package issue34;
+
+public abstract class Implementation1<T> implements Interface1<T> {
+
+    @Override
+    public final T process1(T type) {
+        return type;
+    }
+
+}

testdata/src/main/java/issue34/Implementation2.java

@@ -0,0 +1,4 @@
+package issue34;
+public abstract class Implementation2<T1,T2> extends Implementation1<T2> implements Interface2<T1,T2> {
+
+}

testdata/src/main/java/issue34/Implementation3.java

@@ -0,0 +1,12 @@
+package issue34;
+public class Implementation3 extends Implementation2<String, Integer> {
+
+    public static void main(String [] args) {
+        System.out.println("Hello World!");
+    }
+    
+    public static void run() {
+    	Implementation3.main(null);
+    }
+
+}

testdata/src/main/java/issue34/InnerEnum.java

@@ -0,0 +1,29 @@
+package issue34;
+
+import java.util.Comparator;
+import java.util.Map;
+import java.util.TreeMap;
+
+public class InnerEnum {
+	
+	public static void run() {
+		InnerEnum.main(null);
+	}
+
+	@SuppressWarnings("unused")
+	public static void main(String[] args) {
+		System.out.println("Hello World!");
+		Map<String, String> map = new TreeMap<String, String>(sorters.string);
+	}
+
+	private static enum sorters implements Comparator<String> {
+		string {
+			private static final long serialVersionUID = 1L;
+
+			@Override
+			public int compare(String o1, String o2) {
+				return o1.compareTo(o2);
+			}
+		}
+	}
+}

testdata/src/main/java/issue34/Interface1.java

@@ -0,0 +1,6 @@
+package issue34;
+public interface Interface1<T> {
+
+    public T process1(T type);
+
+}

testdata/src/main/java/issue34/Interface2.java

@@ -0,0 +1,4 @@
+package issue34;
+public interface Interface2<T1, T2> extends Interface1<T2> {
+
+}