Merge pull request #51 from syev/master

Fix #50: Handling of members access permission changes

3 files changed, 76 insertions, 10 deletions

diff --git a/api/src/main/java/org/osjava/jardiff/JarDiff.java b/api/src/main/java/org/osjava/jardiff/JarDiff.java
index 432993a..24d92ff 100644
--- a/api/src/main/java/org/osjava/jardiff/JarDiff.java
+++ b/api/src/main/java/org/osjava/jardiff/JarDiff.java
@@ -392,13 +392,17 @@ public class JarDiff
                         addedFields.add(entry.getKey());
                 }
 
+                // We add all the old methods that match the criteria
                 changedMethods.addAll(removedMethods);
-                changedMethods.retainAll(addedMethods);
+                // We keep the intersection of these with all the new methods
+                // to detect as changed a method that no longer match the
+                // criteria (i.e. a method that was public and is now private)
+                changedMethods.retainAll(newMethods.keySet());
                 removedMethods.removeAll(changedMethods);
                 removedMethods.removeAll(extNewMethods.keySet());
                 addedMethods.removeAll(changedMethods);
                 changedFields.addAll(removedFields);
-                changedFields.retainAll(addedFields);
+                changedFields.retainAll(newFields.keySet());
                 removedFields.removeAll(changedFields);
                 removedFields.removeAll(extNewFields.keySet());
                 addedFields.removeAll(changedFields);
diff --git a/api/src/main/java/org/osjava/jardiff/Tools.java b/api/src/main/java/org/osjava/jardiff/Tools.java
index 7ac6a42..42ba817 100644
--- a/api/src/main/java/org/osjava/jardiff/Tools.java
+++ b/api/src/main/java/org/osjava/jardiff/Tools.java
@@ -65,6 +65,19 @@ public final class Tools
         return (value & mask) == 0;
     }
 
+    private static boolean isAccessIncompatible(int oldAccess, int newAccess) {
+        if (has(newAccess, Opcodes.ACC_PUBLIC)) {
+            return false;
+        } else if (has(newAccess, Opcodes.ACC_PROTECTED)) {
+            return has(oldAccess, Opcodes.ACC_PUBLIC);
+        } else if (has(newAccess, Opcodes.ACC_PRIVATE)) {
+            return not(oldAccess, Opcodes.ACC_PRIVATE);
+        } else {
+            // new access is package, it is incompatible if old access was public or protected
+            return has(oldAccess, Opcodes.ACC_PUBLIC | Opcodes.ACC_PROTECTED);
+        }
+    }
+
     /**
      * @deprecated Use {@link #isClassAccessChange(int, int)}.
      */
@@ -118,6 +131,14 @@ public final class Tools
      * 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.7 Access to Members and Constructors</a><ul>
+     *     <li>Changing the declared access of a member or constructor to permit less access
+     *        <b>may break compatibility</b> with pre-existing binaries, causing a linkage error to be thrown when these binaries are resolved.
+     *     </li>
+     *     <li>The binary format is defined so that changing a member or constructor to be more accessible does not cause a
+     *         linkage error when a subclass (already) defines a method to have less access.
+     *     </li>
+     *     </ul></li>
      *   <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>
@@ -155,13 +176,19 @@ public final class Tools
      * @return
      */
     public static boolean isFieldAccessChange(final int oldAccess, final int newAccess) {
+        if (isAccessIncompatible(oldAccess, newAccess)) {
+            return true; // 13.4.7
+        }
         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;
+            final int accessPermissions = Opcodes.ACC_PUBLIC |
+                                          Opcodes.ACC_PROTECTED |
+                                          Opcodes.ACC_PRIVATE;
+            final int oldAccess2 = oldAccess & ~compatibleChanges & ~accessPermissions;
+            final int newAccess2 = newAccess & ~compatibleChanges & ~accessPermissions;
             return oldAccess2 != newAccess2;
         }
     }
@@ -170,6 +197,14 @@ public final class Tools
      * 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.9">13.4.7 Access to Members and Constructors</a><ul>
+     *     <li>Changing the declared access of a member or constructor to permit less access
+     *        <b>may break compatibility</b> with pre-existing binaries, causing a linkage error to be thrown when these binaries are resolved.
+     *     </li>
+     *     <li>The binary format is defined so that changing a member or constructor to be more accessible does not cause a
+     *         linkage error when a subclass (already) defines a method to have less access.
+     *     </li>
+     *     </ul></li>
      *   <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>
@@ -210,6 +245,9 @@ public final class Tools
      * @return
      */
     public static boolean isMethodAccessChange(final int oldAccess, final int newAccess) {
+        if (isAccessIncompatible(oldAccess, newAccess)) {
+            return true; // 13.4.7
+        }
         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) &&
@@ -220,8 +258,11 @@ public final class Tools
 		                                  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;
+            final int accessPermissions = Opcodes.ACC_PUBLIC |
+                                          Opcodes.ACC_PROTECTED |
+                                          Opcodes.ACC_PRIVATE;
+			final int oldAccess2 = oldAccess & ~compatibleChanges & ~accessPermissions;
+			final int newAccess2 = newAccess & ~compatibleChanges & ~accessPermissions;
 			return oldAccess2 != newAccess2;
 		}
 	}
diff --git a/api/src/test/java/org/osjava/jardiff/ToolsTest.java b/api/src/test/java/org/osjava/jardiff/ToolsTest.java
index 39683bd..7af6d45 100644
--- a/api/src/test/java/org/osjava/jardiff/ToolsTest.java
+++ b/api/src/test/java/org/osjava/jardiff/ToolsTest.java
@@ -58,10 +58,20 @@ public class ToolsTest {
         assertFalse(Tools.isFieldAccessChange(Opcodes.ACC_FINAL, 0));
         assertFalse(Tools.isFieldAccessChange(Opcodes.ACC_FINAL + Opcodes.ACC_PUBLIC, Opcodes.ACC_PUBLIC));
 
-        // No matter the final access, can't become protected or private or
-        // package if it was public.
+        // A field can become more accessible
+        assertFalse(Tools.isFieldAccessChange(Opcodes.ACC_PROTECTED, Opcodes.ACC_PUBLIC));
+        assertFalse(Tools.isFieldAccessChange(0, Opcodes.ACC_PUBLIC));
+        assertFalse(Tools.isFieldAccessChange(0, Opcodes.ACC_PROTECTED));
+        assertFalse(Tools.isFieldAccessChange(Opcodes.ACC_PRIVATE, Opcodes.ACC_PUBLIC));
+        assertFalse(Tools.isFieldAccessChange(Opcodes.ACC_PRIVATE, Opcodes.ACC_PROTECTED));
+        assertFalse(Tools.isFieldAccessChange(Opcodes.ACC_PRIVATE, 0));
+        // ...but can't become less accessible
         assertTrue(Tools.isFieldAccessChange(Opcodes.ACC_FINAL + Opcodes.ACC_PUBLIC, 0));
         assertTrue(Tools.isFieldAccessChange(Opcodes.ACC_PUBLIC, Opcodes.ACC_PROTECTED));
+        assertTrue(Tools.isFieldAccessChange(Opcodes.ACC_PUBLIC, Opcodes.ACC_PRIVATE));
+        assertTrue(Tools.isFieldAccessChange(Opcodes.ACC_PROTECTED, 0));
+        assertTrue(Tools.isFieldAccessChange(Opcodes.ACC_PROTECTED, Opcodes.ACC_PRIVATE));
+        assertTrue(Tools.isFieldAccessChange(0, Opcodes.ACC_PRIVATE));
 
         // A field can't change static
         assertTrue(Tools.isFieldAccessChange(Opcodes.ACC_STATIC, 0));
@@ -96,10 +106,21 @@ public class ToolsTest {
         assertFalse(Tools.isMethodAccessChange(Opcodes.ACC_STATIC + Opcodes.ACC_PUBLIC,
                                                Opcodes.ACC_STATIC + Opcodes.ACC_PUBLIC + Opcodes.ACC_FINAL));
 
-        // No matter the final access, can't become protected or private or
-        // package if it was public.
+        // A method can become more accessible
+        assertFalse(Tools.isMethodAccessChange(Opcodes.ACC_PROTECTED, Opcodes.ACC_PUBLIC));
+        assertFalse(Tools.isMethodAccessChange(0, Opcodes.ACC_PUBLIC));
+        assertFalse(Tools.isMethodAccessChange(0, Opcodes.ACC_PROTECTED));
+        assertFalse(Tools.isMethodAccessChange(Opcodes.ACC_PRIVATE, Opcodes.ACC_PUBLIC));
+        assertFalse(Tools.isMethodAccessChange(Opcodes.ACC_PRIVATE, Opcodes.ACC_PROTECTED));
+        assertFalse(Tools.isMethodAccessChange(Opcodes.ACC_PRIVATE, 0));
+        // ...but can't become less accessible
         assertTrue(Tools.isMethodAccessChange(Opcodes.ACC_FINAL + Opcodes.ACC_PUBLIC, 0));
         assertTrue(Tools.isMethodAccessChange(Opcodes.ACC_PUBLIC, Opcodes.ACC_PROTECTED));
+        assertTrue(Tools.isMethodAccessChange(Opcodes.ACC_PUBLIC, Opcodes.ACC_PRIVATE));
+        assertTrue(Tools.isMethodAccessChange(Opcodes.ACC_PROTECTED, 0));
+        assertTrue(Tools.isMethodAccessChange(Opcodes.ACC_PROTECTED, Opcodes.ACC_PRIVATE));
+        assertTrue(Tools.isMethodAccessChange(0, Opcodes.ACC_PRIVATE));
+
         // A class or method can become concrete.
         assertFalse(Tools.isMethodAccessChange(Opcodes.ACC_ABSTRACT, 0));
         assertFalse(Tools.isMethodAccessChange(Opcodes.ACC_ABSTRACT + Opcodes.ACC_PUBLIC, Opcodes.ACC_PUBLIC));