Fix jardiff's Tools.isAccessChange(..): Differentiate between Class, Field and Method and apply all rules of the Java Language Specification

Class, Field and Methods have different binary backward compatibility rules
as specified in the Java Language Specification, Java SE 7 Edition:
  - http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html

For Field 'volatile' the Java Language Specification, first edition has been used:
  - http://www.wsu.edu/UNIX_Systems/java/langspec-1.0/13.doc.html#45194

For each type separate method have been created, i.e. Tools.is<Type>AccessChange().
Each new method has the rules referenced and partially copied into the method
for better documentation.

The now deprecated method Tools.isAccessChange(..) calls Tools.isClassAccessChange(..)
and shall be removed.

Unit test ToolsTest has been expanded for each type and its rules.

3 files changed, 186 insertions, 37 deletions

diff --git a/api/src/main/java/org/osjava/jardiff/PublicDiffCriteria.java b/api/src/main/java/org/osjava/jardiff/PublicDiffCriteria.java
index 9940255..90fb382 100644
--- a/api/src/main/java/org/osjava/jardiff/PublicDiffCriteria.java
+++ b/api/src/main/java/org/osjava/jardiff/PublicDiffCriteria.java
@@ -38,7 +38,7 @@ public class PublicDiffCriteria implements DiffCriteria
     public boolean validClass(ClassInfo info) {
         return !info.isSynthetic() && info.isPublic();
     }
-    
+
     /**
      * Check if a method is valid.
      * If the method is not synthetic and public, return true.
@@ -49,7 +49,7 @@ public class PublicDiffCriteria implements DiffCriteria
     public boolean validMethod(MethodInfo info) {
         return !info.isSynthetic() && info.isPublic();
     }
-    
+
     /**
      * Check if a field is valid.
      * If the method is not synthetic and public, return true.
@@ -60,7 +60,7 @@ public class PublicDiffCriteria implements DiffCriteria
     public boolean validField(FieldInfo info) {
         return !info.isSynthetic() && info.isPublic();
     }
-    
+
     /**
      * Check if there is a change between two versions of a class.
      * Returns true if the access flags differ, or if the superclass differs
@@ -71,7 +71,7 @@ public class PublicDiffCriteria implements DiffCriteria
      * @return True if the classes differ, false otherwise.
      */
     public boolean differs(ClassInfo oldInfo, ClassInfo newInfo) {
-        if (Tools.isAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
+        if (Tools.isClassAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
             return true;
         // Yes classes can have a null supername, e.g. java.lang.Object !
         if(oldInfo.getSupername() == null) {
@@ -81,15 +81,15 @@ public class PublicDiffCriteria implements DiffCriteria
         } else if (!oldInfo.getSupername().equals(newInfo.getSupername())) {
             return true;
         }
-        Set<String> oldInterfaces
+        final Set<String> oldInterfaces
             = new HashSet(Arrays.asList(oldInfo.getInterfaces()));
-        Set<String> newInterfaces
+        final Set<String> newInterfaces
             = new HashSet(Arrays.asList(newInfo.getInterfaces()));
         if (!oldInterfaces.equals(newInterfaces))
             return true;
         return false;
     }
-    
+
     /**
      * Check if there is a change between two versions of a method.
      * Returns true if the access flags differ, or if the thrown
@@ -100,23 +100,23 @@ public class PublicDiffCriteria implements DiffCriteria
      * @return True if the methods differ, false otherwise.
      */
     public boolean differs(MethodInfo oldInfo, MethodInfo newInfo) {
-        if (Tools.isAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
+        if (Tools.isMethodAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
             return true;
         if (oldInfo.getExceptions() == null
             || newInfo.getExceptions() == null) {
             if (oldInfo.getExceptions() != newInfo.getExceptions())
                 return true;
         } else {
-            Set<String> oldExceptions
+            final Set<String> oldExceptions
                 = new HashSet(Arrays.asList(oldInfo.getExceptions()));
-            Set<String> newExceptions
+            final Set<String> newExceptions
                 = new HashSet(Arrays.asList(newInfo.getExceptions()));
             if (!oldExceptions.equals(newExceptions))
                 return true;
         }
         return false;
     }
-    
+
     /**
      * Check if there is a change between two versions of a field.
      * Returns true if the access flags differ, or if the inital value
@@ -127,7 +127,7 @@ public class PublicDiffCriteria implements DiffCriteria
      * @return True if the fields differ, false otherwise.
      */
     public boolean differs(FieldInfo oldInfo, FieldInfo newInfo) {
-        if (Tools.isAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
+        if (Tools.isFieldAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
             return true;
         if (oldInfo.getValue() == null || newInfo.getValue() == null) {
             if (oldInfo.getValue() != newInfo.getValue())
diff --git a/api/src/main/java/org/osjava/jardiff/SimpleDiffCriteria.java b/api/src/main/java/org/osjava/jardiff/SimpleDiffCriteria.java
index 5b82266..ceaf0d2 100644
--- a/api/src/main/java/org/osjava/jardiff/SimpleDiffCriteria.java
+++ b/api/src/main/java/org/osjava/jardiff/SimpleDiffCriteria.java
@@ -38,7 +38,7 @@ public class SimpleDiffCriteria implements DiffCriteria
     public boolean validClass(ClassInfo info) {
         return !info.isSynthetic() && (info.isPublic() || info.isProtected());
     }
-    
+
     /**
      * Check if a method is valid.
      * If the method is not synthetic and is public or protected, return true.
@@ -49,7 +49,7 @@ public class SimpleDiffCriteria implements DiffCriteria
     public boolean validMethod(MethodInfo info) {
         return !info.isSynthetic() && (info.isPublic() || info.isProtected());
     }
-    
+
     /**
      * Check if a field is valid.
      * If the method is not synthetic and is public or protected, return true.
@@ -60,7 +60,7 @@ public class SimpleDiffCriteria implements DiffCriteria
     public boolean validField(FieldInfo info) {
         return !info.isSynthetic() && (info.isPublic() || info.isProtected());
     }
-    
+
     /**
      * Check if there is a change between two versions of a class.
      * Returns true if the access flags differ, or if the superclass differs
@@ -71,7 +71,7 @@ public class SimpleDiffCriteria implements DiffCriteria
      * @return True if the classes differ, false otherwise.
      */
     public boolean differs(ClassInfo oldInfo, ClassInfo newInfo) {
-        if (Tools.isAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
+        if (Tools.isClassAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
             return true;
         // Yes classes can have a null supername, e.g. java.lang.Object !
         if(oldInfo.getSupername() == null) {
@@ -81,15 +81,15 @@ public class SimpleDiffCriteria implements DiffCriteria
         } else if (!oldInfo.getSupername().equals(newInfo.getSupername())) {
             return true;
         }
-        Set<String> oldInterfaces
+        final Set<String> oldInterfaces
             = new HashSet(Arrays.asList(oldInfo.getInterfaces()));
-        Set<String> newInterfaces
+        final Set<String> newInterfaces
             = new HashSet(Arrays.asList(newInfo.getInterfaces()));
         if (!oldInterfaces.equals(newInterfaces))
             return true;
         return false;
     }
-    
+
     /**
      * Check if there is a change between two versions of a method.
      * Returns true if the access flags differ, or if the thrown
@@ -100,23 +100,23 @@ public class SimpleDiffCriteria implements DiffCriteria
      * @return True if the methods differ, false otherwise.
      */
     public boolean differs(MethodInfo oldInfo, MethodInfo newInfo) {
-        if (Tools.isAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
+        if (Tools.isMethodAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
             return true;
         if (oldInfo.getExceptions() == null
             || newInfo.getExceptions() == null) {
             if (oldInfo.getExceptions() != newInfo.getExceptions())
                 return true;
         } else {
-            Set<String> oldExceptions
+            final Set<String> oldExceptions
                 = new HashSet(Arrays.asList(oldInfo.getExceptions()));
-            Set<String> newExceptions
+            final Set<String> newExceptions
                 = new HashSet(Arrays.asList(newInfo.getExceptions()));
             if (!oldExceptions.equals(newExceptions))
                 return true;
         }
         return false;
     }
-    
+
     /**
      * Check if there is a change between two versions of a field.
      * Returns true if the access flags differ, or if the inital value
@@ -127,7 +127,7 @@ public class SimpleDiffCriteria implements DiffCriteria
      * @return True if the fields differ, false otherwise.
      */
     public boolean differs(FieldInfo oldInfo, FieldInfo newInfo) {
-        if (Tools.isAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
+        if (Tools.isFieldAccessChange(oldInfo.getAccess(), newInfo.getAccess()))
             return true;
         if (oldInfo.getValue() == null || newInfo.getValue() == null) {
             if (oldInfo.getValue() != newInfo.getValue())
diff --git a/api/src/main/java/org/osjava/jardiff/Tools.java b/api/src/main/java/org/osjava/jardiff/Tools.java
index e159541..7ac6a42 100644
--- a/api/src/main/java/org/osjava/jardiff/Tools.java
+++ b/api/src/main/java/org/osjava/jardiff/Tools.java
@@ -32,20 +32,20 @@ public final class Tools
     private Tools() {
         /* empty */
     }
-    
+
     /**
      * Get the java class name given an internal class name.
      * This method currently replaces all instances of $ and / with . this
      * may not be according to the java language spec, and will almost
      * certainly fail for some inner classes.
-     * 
+     *
      * @param internalName The internal name of the class.
      * @return The java class name.
      */
     public static final String getClassName(String internalName) {
-        StringBuffer ret = new StringBuffer(internalName.length());
+        final StringBuffer ret = new StringBuffer(internalName.length());
         for (int i = 0; i < internalName.length(); i++) {
-            char ch = internalName.charAt(i);
+            final char ch = internalName.charAt(i);
             switch (ch) {
             case '$':
             case '/':
@@ -58,22 +58,171 @@ public final class Tools
         return ret.toString();
     }
 
+    private static boolean has(final int value, final int mask) {
+        return (value & mask) != 0;
+    }
+    private static boolean not(final int value, final int mask) {
+        return (value & mask) == 0;
+    }
+
     /**
-     * Returns whether newAccess is incompatible with oldAccess
+     * @deprecated Use {@link #isClassAccessChange(int, int)}.
+     */
+    public static boolean isAccessChange(int oldAccess, int newAccess) {
+        return isClassAccessChange(oldAccess, newAccess);
+    }
+
+    /**
+     * Returns whether a class's newAccess is incompatible with oldAccess
+     * following <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html">Java Language Specification, Java SE 7 Edition</a>:
+     * <ul>
+     *   <li><a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.1">13.4.1 abstract Classes</a><ul>
+     *     <li>If a class that was not declared abstract is changed to be declared abstract,
+     *         then pre-existing binaries that attempt to create new instances of that class
+     *         will throw either an InstantiationError at link time,
+     *         or (if a reflective method is used) an InstantiationException at run time.
+     *         Such changes <b>break backward compatibility</b>!</li>
+     *     <li>Changing a class that is declared abstract to no longer be declared abstract
+     *         <b>does not break compatibility</b> with pre-existing binaries.</li>
+     *     </ul></li>
+     *   <li><a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.2">13.4.2 final Classes</a><ul>
+     *     <li>If a class that was not declared final is changed to be declared final,
+     *         then a VerifyError is thrown if a binary of a pre-existing subclass of this class is loaded,
+     *         because final classes can have no subclasses.
+     *         Such changes <b>break functional backward compatibility</b>!</li>
+     *     <li>Changing a class that is declared final to no longer be declared final
+     *         <b>does not break compatibility</b> with pre-existing binaries.</li>
+     *     </ul></li>
+     * </ul>
      *
      * @param oldAccess
      * @param newAccess
      * @return
      */
-    public static boolean isAccessChange(int oldAccess, int newAccess) {
-		if ((oldAccess & Opcodes.ACC_FINAL) == 0 && (newAccess & Opcodes.ACC_FINAL) > 0) {
-			return true;
-		} else if ((oldAccess & Opcodes.ACC_ABSTRACT) == 0 && (newAccess & Opcodes.ACC_ABSTRACT) > 0) {
-			return true;
+    public static boolean isClassAccessChange(final int oldAccess, final int newAccess) {
+        if ( not(oldAccess, Opcodes.ACC_ABSTRACT) && has(newAccess, Opcodes.ACC_ABSTRACT) ) {
+            return true; // 13.4.1 #1
+        } else  if ( not(oldAccess, Opcodes.ACC_FINAL) && has(newAccess, Opcodes.ACC_FINAL) ) {
+            return true; // 13.4.2 #1
+        } else {
+            final int compatibleChanges = Opcodes.ACC_ABSTRACT |     // 13.4.1 #2
+                                          Opcodes.ACC_FINAL ;        // 13.4.2 #2
+            // FIXME Opcodes.ACC_VOLATILE ?
+            final int oldAccess2 = oldAccess & ~compatibleChanges;
+            final int newAccess2 = newAccess & ~compatibleChanges;
+            return oldAccess2 != newAccess2;
+        }
+    }
+
+    /**
+     * Returns whether a field's newAccess is incompatible with oldAccess
+     * following <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html">Java Language Specification, Java SE 7 Edition</a>:
+     * <ul>
+     *   <li><a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.9">13.4.9 final Fields and Constants</a><ul>
+     *     <li>If a field that was not declared final is changed to be declared final,
+     *         then it <b>can break compatibility</b> with pre-existing binaries that attempt to assign new values to the field.</li>
+     *     <li>Deleting the keyword final or changing the value to which a <i>non-final</i> field is initialized
+     *         <b>does not break compatibility</b> with existing binaries.</li>
+     *     <li>If a field is a constant variable (§4.12.4),
+     *         then deleting the keyword final or changing its value
+     *         will <i>not break compatibility</i> with pre-existing binaries by causing them not to run,
+     *         but they will not see any new value for the usage of the field unless they are recompiled.
+     *         This is true even if the usage itself is not a compile-time constant expression (§15.28).
+     *         Such changes <b>break functional backward compatibility</b>!</li>
+     *     </ul></li>
+     *   <li><a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.10">13.4.10 static Fields</a><ul>
+     *     <li>If a field that is not declared private was not declared static
+     *         and is changed to be declared static, or vice versa,
+     *         then a linkage error, specifically an IncompatibleClassChangeError,
+     *         will result if the field is used by a pre-existing binary which expected a field of the other kind.
+     *         Such changes <b>break backward compatibility</b>!</li>
+     *     </ul></li>
+     *   <li><a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.11">13.4.11. transient Fields </a><ul>
+     *     <li>Adding or deleting a transient modifier of a field
+     *         <b>does not break compatibility</b> with pre-existing binaries.</li>
+     *   </ul></li>
+     *   <li><a href="http://www.wsu.edu/UNIX_Systems/java/langspec-1.0/13.doc.html#45194">13.4.11 volatile Fields (JLS 1.0)</a><ul>
+     *     <li>If a field that is not declared private was not declared volatile
+     *         and is changed to be declared volatile, or vice versa, then a linkage time error,
+     *         specifically an IncompatibleClassChangeError, may result if the field is used
+     *         by a preexisting binary that expected a field of the opposite volatility.
+     *         Such changes <b>break backward compatibility</b>!</li>
+     *   </ul></li>
+     * </ul>
+     *
+     * @param oldAccess
+     * @param newAccess
+     * @return
+     */
+    public static boolean isFieldAccessChange(final int oldAccess, final int newAccess) {
+        if ( not(oldAccess, Opcodes.ACC_FINAL) && has(newAccess, Opcodes.ACC_FINAL) ) {
+            return true; // 13.4.9 #1
+        } else {
+            final int compatibleChanges = Opcodes.ACC_FINAL |         // 13.4.9 #2
+                                          Opcodes.ACC_TRANSIENT;      // 13.4.11 #1
+            final int oldAccess2 = oldAccess & ~compatibleChanges;
+            final int newAccess2 = newAccess & ~compatibleChanges;
+            return oldAccess2 != newAccess2;
+        }
+    }
+
+    /**
+     * Returns whether a method's newAccess is incompatible with oldAccess
+     * following <a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html">Java Language Specification, Java SE 7 Edition</a>:
+     * <ul>
+     *   <li><a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.16">13.4.16 abstract Methods</a><ul>
+     *     <li>Changing a method that is declared abstract to no longer be declared abstract
+     *         <b>does not break compatibility</b> with pre-existing binaries.</li>
+     *     <li>Changing a method that is not declared abstract to be declared abstract
+     *         <b>will break compatibility</b> with pre-existing binaries that previously invoked the method, causing an AbstractMethodError.</li>
+     *     </ul></li>
+     *   <li><a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.17">13.4.17 final</a><ul>
+     *     <li>Changing a method that is declared final to no longer be declared final
+     *         <b>does not break compatibility</b> with pre-existing binaries.</li>
+     *     <li>Changing an instance method that is not declared final to be declared final
+     *         <b>may break compatibility</b> with existing binaries that depend on the ability to override the method.</li>
+     *     <li>Changing a class (static) method that is not declared final to be declared final
+     *         <b>does not break compatibility</b> with existing binaries, because the method could not have been overridden.</li>
+     *     </ul></li>
+     *   <li><a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.18">13.4.18 native Methods</a><ul>
+     *     <li>Adding or deleting a native modifier of a method
+     *         <b>does not break compatibility</b> with pre-existing binaries.</li>
+     *     </ul></li>
+     *   <li><a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.19">13.4.19 static Methods</a><ul>
+     *     <li>If a method that is not declared private is also declared static (that is, a class method)
+     *         and is changed to not be declared static (that is, to an instance method), or vice versa,
+     *         then <i>compatibility with pre-existing binaries may be broken</i>, resulting in a linkage time error,
+     *         namely an IncompatibleClassChangeError, if these methods are used by the pre-existing binaries.
+     *         Such changes <b>break functional backward compatibility</b>!</li>
+     *     </ul></li>
+     *   <li><a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.20">13.4.20 synchronized Methods</a><ul>
+     *     <li>Adding or deleting a synchronized modifier of a method
+     *         <b>does not break compatibility</b> with pre-existing binaries.</li>
+     *   </ul></li>
+     *   <li><a href="http://docs.oracle.com/javase/specs/jls/se7/html/jls-13.html#jls-13.4.21">13.4.21 Method and Constructor Throws</a><ul>
+     *     <li>Changes to the throws clause of methods or constructors
+     *         <b>do not break compatibility</b> with pre-existing binaries; these clauses are checked only at compile time.</li>
+     *   </ul></li>
+     * </ul>
+     *
+     * @param oldAccess
+     * @param newAccess
+     * @return
+     */
+    public static boolean isMethodAccessChange(final int oldAccess, final int newAccess) {
+        if ( not(oldAccess, Opcodes.ACC_ABSTRACT) && has(newAccess, Opcodes.ACC_ABSTRACT) ) {
+            return true; // 13.4.16 #2
+        } else  if ( not(oldAccess, Opcodes.ACC_FINAL) && not(oldAccess, Opcodes.ACC_STATIC) &&
+                     has(newAccess, Opcodes.ACC_FINAL) ) {
+			return true; // 13.4.17 #2 excluding and #3
 		} else {
-			oldAccess = oldAccess & ~Opcodes.ACC_FINAL & ~Opcodes.ACC_ABSTRACT;
-			newAccess = newAccess & ~Opcodes.ACC_FINAL & ~Opcodes.ACC_ABSTRACT;
+		    final int compatibleChanges = Opcodes.ACC_ABSTRACT |     // 13.4.16 #1
+		                                  Opcodes.ACC_FINAL |        // 13.4.17 #1
+		                                  Opcodes.ACC_NATIVE |       // 13.4.18 #1
+		                                  Opcodes.ACC_SYNCHRONIZED;  // 13.4.20 #1
+			final int oldAccess2 = oldAccess & ~compatibleChanges;
+			final int newAccess2 = newAccess & ~compatibleChanges;
+			return oldAccess2 != newAccess2;
 		}
-		return oldAccess != newAccess;
 	}
 }