From 3acd4aad506faacd9b4ef9271df9bb0599300cae Mon Sep 17 00:00:00 2001
From: syev <syev@users.noreply.github.com>
Date: Fri, 24 Oct 2014 11:50:17 +0200
Subject: Changed members detection update

Check the members against all the new members, not only the
ones that match the DiffCriteria.
---
 api/src/main/java/org/osjava/jardiff/JarDiff.java | 4 ++--
 1 file changed, 2 insertions(+), 2 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..d391712 100644
--- a/api/src/main/java/org/osjava/jardiff/JarDiff.java
+++ b/api/src/main/java/org/osjava/jardiff/JarDiff.java
@@ -393,12 +393,12 @@ public class JarDiff
                 }
 
                 changedMethods.addAll(removedMethods);
-                changedMethods.retainAll(addedMethods);
+                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);
-- 
cgit v1.2.3


From 32b247d1dc0209faa5cb79b3db72ccc54ebb8991 Mon Sep 17 00:00:00 2001
From: syev <syev@users.noreply.github.com>
Date: Fri, 24 Oct 2014 12:01:39 +0200
Subject: Add access permissions checks

---
 api/src/main/java/org/osjava/jardiff/Tools.java | 48 ++++++++++++++++++++++---
 1 file changed, 44 insertions(+), 4 deletions(-)

diff --git a/api/src/main/java/org/osjava/jardiff/Tools.java b/api/src/main/java/org/osjava/jardiff/Tools.java
index 7ac6a42..41ba63c 100644
--- a/api/src/main/java/org/osjava/jardiff/Tools.java
+++ b/api/src/main/java/org/osjava/jardiff/Tools.java
@@ -65,6 +65,18 @@ public final class Tools
         return (value & mask) == 0;
     }
 
+    private static boolean isLessAccessPermitted(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 {
+            return has(oldAccess, Opcodes.ACC_PUBLIC | Opcodes.ACC_PROTECTED);
+        }
+    }
+
     /**
      * @deprecated Use {@link #isClassAccessChange(int, int)}.
      */
@@ -118,6 +130,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 +175,19 @@ public final class Tools
      * @return
      */
     public static boolean isFieldAccessChange(final int oldAccess, final int newAccess) {
+        if (isLessAccessPermitted(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 +196,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 +244,9 @@ public final class Tools
      * @return
      */
     public static boolean isMethodAccessChange(final int oldAccess, final int newAccess) {
+        if (isLessAccessPermitted(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 +257,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;
 		}
 	}
-- 
cgit v1.2.3


From 28320018e8c6f97a37389aa14af3e14c5b082701 Mon Sep 17 00:00:00 2001
From: syev <syev@users.noreply.github.com>
Date: Fri, 24 Oct 2014 12:11:36 +0200
Subject: Tests on access permissions checks.

---
 .../test/java/org/osjava/jardiff/ToolsTest.java    | 29 +++++++++++++++++++---
 1 file changed, 25 insertions(+), 4 deletions(-)

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));
-- 
cgit v1.2.3


From 327e7fdf59a05f16a050b4132abe4b8b4b0d93be Mon Sep 17 00:00:00 2001
From: syev <syev@users.noreply.github.com>
Date: Fri, 24 Oct 2014 18:46:01 +0200
Subject: Clarifications about access permissions checks

Renamed the Tools method that checks the access permissions /
visibility.
Added comments.
---
 api/src/main/java/org/osjava/jardiff/JarDiff.java | 4 ++++
 api/src/main/java/org/osjava/jardiff/Tools.java   | 7 ++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/api/src/main/java/org/osjava/jardiff/JarDiff.java b/api/src/main/java/org/osjava/jardiff/JarDiff.java
index d391712..24d92ff 100644
--- a/api/src/main/java/org/osjava/jardiff/JarDiff.java
+++ b/api/src/main/java/org/osjava/jardiff/JarDiff.java
@@ -392,7 +392,11 @@ public class JarDiff
                         addedFields.add(entry.getKey());
                 }
 
+                // We add all the old methods that match the criteria
                 changedMethods.addAll(removedMethods);
+                // 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());
diff --git a/api/src/main/java/org/osjava/jardiff/Tools.java b/api/src/main/java/org/osjava/jardiff/Tools.java
index 41ba63c..42ba817 100644
--- a/api/src/main/java/org/osjava/jardiff/Tools.java
+++ b/api/src/main/java/org/osjava/jardiff/Tools.java
@@ -65,7 +65,7 @@ public final class Tools
         return (value & mask) == 0;
     }
 
-    private static boolean isLessAccessPermitted(int oldAccess, int newAccess) {
+    private static boolean isAccessIncompatible(int oldAccess, int newAccess) {
         if (has(newAccess, Opcodes.ACC_PUBLIC)) {
             return false;
         } else if (has(newAccess, Opcodes.ACC_PROTECTED)) {
@@ -73,6 +73,7 @@ public final class Tools
         } 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);
         }
     }
@@ -175,7 +176,7 @@ public final class Tools
      * @return
      */
     public static boolean isFieldAccessChange(final int oldAccess, final int newAccess) {
-        if (isLessAccessPermitted(oldAccess, newAccess)) {
+        if (isAccessIncompatible(oldAccess, newAccess)) {
             return true; // 13.4.7
         }
         if ( not(oldAccess, Opcodes.ACC_FINAL) && has(newAccess, Opcodes.ACC_FINAL) ) {
@@ -244,7 +245,7 @@ public final class Tools
      * @return
      */
     public static boolean isMethodAccessChange(final int oldAccess, final int newAccess) {
-        if (isLessAccessPermitted(oldAccess, newAccess)) {
+        if (isAccessIncompatible(oldAccess, newAccess)) {
             return true; // 13.4.7
         }
         if ( not(oldAccess, Opcodes.ACC_ABSTRACT) && has(newAccess, Opcodes.ACC_ABSTRACT) ) {
-- 
cgit v1.2.3