Bug 483591: API Tools view should filter non-API changes (ApiComparator bug for generic methods)

Change-Id: I1f52f51e0f143add4f0c0592da396b4a08b2240c
Signed-off-by: Markus Keller <markus_keller@ch.ibm.com>

3 files changed, 44 insertions, 39 deletions

diff --git a/apitools/org.eclipse.pde.api.tools.tests/src/org/eclipse/pde/api/tools/comparator/tests/FieldDeltaTests.java b/apitools/org.eclipse.pde.api.tools.tests/src/org/eclipse/pde/api/tools/comparator/tests/FieldDeltaTests.java
index cec05aabb9..855d96eed4 100644
--- a/apitools/org.eclipse.pde.api.tools.tests/src/org/eclipse/pde/api/tools/comparator/tests/FieldDeltaTests.java
+++ b/apitools/org.eclipse.pde.api.tools.tests/src/org/eclipse/pde/api/tools/comparator/tests/FieldDeltaTests.java
@@ -10,9 +10,6 @@
  *******************************************************************************/
 package org.eclipse.pde.api.tools.comparator.tests;
 
-import junit.framework.Test;
-import junit.framework.TestSuite;
-
 import org.eclipse.jdt.core.Flags;
 import org.eclipse.pde.api.tools.internal.provisional.RestrictionModifiers;
 import org.eclipse.pde.api.tools.internal.provisional.VisibilityModifiers;
@@ -23,6 +20,9 @@ import org.eclipse.pde.api.tools.internal.provisional.model.IApiBaseline;
 import org.eclipse.pde.api.tools.internal.provisional.model.IApiComponent;
 import org.eclipse.pde.api.tools.internal.util.Util;
 
+import junit.framework.Test;
+import junit.framework.TestSuite;
+
 /**
  * Delta tests for field
  */
@@ -1629,14 +1629,8 @@ public class FieldDeltaTests extends DeltaTestSetup {
 		IDelta delta = ApiComparator.compare(beforeApiComponent, afterApiComponent, before, after, VisibilityModifiers.API, null);
 		assertNotNull("No delta", delta); //$NON-NLS-1$
 		IDelta[] allLeavesDeltas = collectLeaves(delta);
-		assertEquals("Wrong size", 2, allLeavesDeltas.length); //$NON-NLS-1$
+		assertEquals("Wrong size", 1, allLeavesDeltas.length); //$NON-NLS-1$
 		IDelta child = allLeavesDeltas[0];
-		assertEquals("Wrong kind", IDelta.ADDED, child.getKind()); //$NON-NLS-1$
-		assertEquals("Wrong flag", IDelta.CLINIT, child.getFlags()); //$NON-NLS-1$
-		assertFalse("Is visible", Util.isVisible(child.getNewModifiers())); //$NON-NLS-1$
-		assertEquals("Wrong element type", IDelta.CLASS_ELEMENT_TYPE, child.getElementType()); //$NON-NLS-1$
-		assertTrue("Not compatible", DeltaProcessor.isCompatible(child)); //$NON-NLS-1$
-		child = allLeavesDeltas[1];
 		assertEquals("Wrong kind", IDelta.REMOVED, child.getKind()); //$NON-NLS-1$
 		assertEquals("Wrong flag", IDelta.VALUE, child.getFlags()); //$NON-NLS-1$
 		assertTrue("Not visible", Util.isVisible(child.getOldModifiers())); //$NON-NLS-1$
@@ -1713,14 +1707,8 @@ public class FieldDeltaTests extends DeltaTestSetup {
 		IDelta delta = ApiComparator.compare(beforeApiComponent, afterApiComponent, before, after, VisibilityModifiers.API, null);
 		assertNotNull("No delta", delta); //$NON-NLS-1$
 		IDelta[] allLeavesDeltas = collectLeaves(delta);
-		assertEquals("Wrong size", 2, allLeavesDeltas.length); //$NON-NLS-1$
+		assertEquals("Wrong size", 1, allLeavesDeltas.length); //$NON-NLS-1$
 		IDelta child = allLeavesDeltas[0];
-		assertEquals("Wrong kind", IDelta.ADDED, child.getKind()); //$NON-NLS-1$
-		assertEquals("Wrong flag", IDelta.CLINIT, child.getFlags()); //$NON-NLS-1$
-		assertFalse("Is visible", Util.isVisible(child.getNewModifiers())); //$NON-NLS-1$
-		assertEquals("Wrong element type", IDelta.CLASS_ELEMENT_TYPE, child.getElementType()); //$NON-NLS-1$
-		assertTrue("Not compatible", DeltaProcessor.isCompatible(child)); //$NON-NLS-1$
-		child = allLeavesDeltas[1];
 		assertEquals("Wrong kind", IDelta.REMOVED, child.getKind()); //$NON-NLS-1$
 		assertEquals("Wrong flag", IDelta.VALUE, child.getFlags()); //$NON-NLS-1$
 		assertTrue("Not visible", Util.isVisible(child.getOldModifiers())); //$NON-NLS-1$
@@ -1738,7 +1726,7 @@ public class FieldDeltaTests extends DeltaTestSetup {
 		assertNotNull("no api component", beforeApiComponent); //$NON-NLS-1$
 		IApiComponent afterApiComponent = after.getApiComponent(BUNDLE_NAME);
 		assertNotNull("no api component", afterApiComponent); //$NON-NLS-1$
-		IDelta delta = ApiComparator.compare(beforeApiComponent, afterApiComponent, before, after, VisibilityModifiers.API, null);
+		IDelta delta = ApiComparator.compare(beforeApiComponent, afterApiComponent, before, after, VisibilityModifiers.ALL_VISIBILITIES, null);
 		assertNotNull("No delta", delta); //$NON-NLS-1$
 		IDelta[] allLeavesDeltas = collectLeaves(delta);
 		assertEquals("Wrong size", 2, allLeavesDeltas.length); //$NON-NLS-1$
@@ -1767,7 +1755,7 @@ public class FieldDeltaTests extends DeltaTestSetup {
 		assertNotNull("no api component", beforeApiComponent); //$NON-NLS-1$
 		IApiComponent afterApiComponent = after.getApiComponent(BUNDLE_NAME);
 		assertNotNull("no api component", afterApiComponent); //$NON-NLS-1$
-		IDelta delta = ApiComparator.compare(beforeApiComponent, afterApiComponent, before, after, VisibilityModifiers.API, null);
+		IDelta delta = ApiComparator.compare(beforeApiComponent, afterApiComponent, before, after, VisibilityModifiers.ALL_VISIBILITIES, null);
 		assertNotNull("No delta", delta); //$NON-NLS-1$
 		IDelta[] allLeavesDeltas = collectLeaves(delta);
 		assertEquals("Wrong size", 1, allLeavesDeltas.length); //$NON-NLS-1$
diff --git a/apitools/org.eclipse.pde.api.tools.tests/src/org/eclipse/pde/api/tools/comparator/tests/MethodDeltaTests.java b/apitools/org.eclipse.pde.api.tools.tests/src/org/eclipse/pde/api/tools/comparator/tests/MethodDeltaTests.java
index ca0c2e5b1b..0b50a6cbc4 100644
--- a/apitools/org.eclipse.pde.api.tools.tests/src/org/eclipse/pde/api/tools/comparator/tests/MethodDeltaTests.java
+++ b/apitools/org.eclipse.pde.api.tools.tests/src/org/eclipse/pde/api/tools/comparator/tests/MethodDeltaTests.java
@@ -2240,15 +2240,7 @@ public class MethodDeltaTests extends DeltaTestSetup {
 		IApiComponent afterApiComponent = after.getApiComponent(BUNDLE_NAME);
 		assertNotNull("no api component", afterApiComponent); //$NON-NLS-1$
 		IDelta delta = ApiComparator.compare(beforeApiComponent, afterApiComponent, before, after, VisibilityModifiers.API, null);
-		assertNotNull("No delta", delta); //$NON-NLS-1$
-		IDelta[] allLeavesDeltas = collectLeaves(delta);
-		assertEquals("Wrong size", 1, allLeavesDeltas.length); //$NON-NLS-1$
-		IDelta child = allLeavesDeltas[0];
-		assertEquals("Wrong kind", IDelta.ADDED, child.getKind()); //$NON-NLS-1$
-		assertEquals("Wrong flag", IDelta.METHOD, child.getFlags()); //$NON-NLS-1$
-		assertFalse("Is visible", Util.isVisible(child.getNewModifiers())); //$NON-NLS-1$
-		assertEquals("Wrong element type", IDelta.CLASS_ELEMENT_TYPE, child.getElementType()); //$NON-NLS-1$
-		assertTrue("Not compatible", DeltaProcessor.isCompatible(child)); //$NON-NLS-1$
+		assertTrue("Not no delta", delta == ApiComparator.NO_DELTA); //$NON-NLS-1$
 	}
 	/**
 	 * https://bugs.eclipse.org/bugs/show_bug.cgi?id=244620
diff --git a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/comparator/ClassFileComparator.java b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/comparator/ClassFileComparator.java
index 342634e01b..723f1a8a75 100644
--- a/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/comparator/ClassFileComparator.java
+++ b/apitools/org.eclipse.pde.api.tools/src/org/eclipse/pde/api/tools/internal/comparator/ClassFileComparator.java
@@ -1079,8 +1079,11 @@ public class ClassFileComparator {
 		IApiField field2 = this.type2.getField(name);
 		if (field2 == null) {
 			if (Flags.isPrivate(access) || Util.isDefault(access)) {
-				this.addDelta(getElementType(this.type1), IDelta.REMOVED, IDelta.FIELD, this.currentDescriptorRestrictions, access, 0, this.type1, name, new String[] {
-						Util.getDescriptorName(this.type1), name });
+				if (!(this.visibilityModifiers == VisibilityModifiers.API && component.hasApiDescription())) {
+					// report non-API delta:
+					this.addDelta(getElementType(this.type1), IDelta.REMOVED, IDelta.FIELD, this.currentDescriptorRestrictions, access, 0, this.type1, name, new String[] {
+							Util.getDescriptorName(this.type1), name });
+				}
 			} else {
 				boolean found = false;
 				if (this.component2 != null) {
@@ -1219,6 +1222,11 @@ public class ClassFileComparator {
 				}
 				return;
 			}
+			if ((Flags.isPrivate(access) || Util.isDefault(access) || (Flags.isProtected(access) && RestrictionModifiers.isExtendRestriction(this.initialDescriptorRestrictions)))
+					&& (Flags.isPrivate(access2) || Util.isDefault(access2) || (Flags.isProtected(access2) && RestrictionModifiers.isExtendRestriction(this.currentDescriptorRestrictions)))) {
+				// don't report non-API deltas
+				return;
+			}
 		}
 
 		restrictions |= this.currentDescriptorRestrictions;
@@ -1359,12 +1367,17 @@ public class ClassFileComparator {
 		String methodDisplayName = getMethodDisplayName(method, this.type1);
 		if (method2 == null) {
 			if (method.isClassInitializer()) {
-				// report delta: removal of a clinit method
-				this.addDelta(getElementType(this.type1), IDelta.REMOVED, IDelta.CLINIT, this.currentDescriptorRestrictions, access, 0, this.type1, this.type1.getName(), Util.getDescriptorName(type1));
+				if (!(this.visibilityModifiers == VisibilityModifiers.API && component.hasApiDescription())) {
+					// report non-API delta: removal of a clinit method
+					this.addDelta(getElementType(this.type1), IDelta.REMOVED, IDelta.CLINIT, this.currentDescriptorRestrictions, access, 0, this.type1, this.type1.getName(), Util.getDescriptorName(type1));
+				}
 				return;
 			} else if (Flags.isPrivate(access) || Util.isDefault(access)) {
-				this.addDelta(getElementType(this.type1), IDelta.REMOVED, getTargetType(method), Flags.isAbstract(this.type2.getModifiers()) ? this.currentDescriptorRestrictions | RestrictionModifiers.NO_INSTANTIATE : this.currentDescriptorRestrictions, access, 0, this.type1, getKeyForMethod(method, this.type1), new String[] {
-						Util.getDescriptorName(this.type1), methodDisplayName });
+				if (!(this.visibilityModifiers == VisibilityModifiers.API && component.hasApiDescription())) {
+					// report non-API delta:
+					this.addDelta(getElementType(this.type1), IDelta.REMOVED, getTargetType(method), Flags.isAbstract(this.type2.getModifiers()) ? this.currentDescriptorRestrictions | RestrictionModifiers.NO_INSTANTIATE : this.currentDescriptorRestrictions, access, 0, this.type1, getKeyForMethod(method, this.type1), new String[] {
+							Util.getDescriptorName(this.type1), methodDisplayName });
+				}
 				return;
 			}
 			// if null we need to walk the hierarchy of descriptor2
@@ -1578,6 +1591,11 @@ public class ClassFileComparator {
 					return;
 				}
 			}
+			if ((Flags.isPrivate(access) || Util.isDefault(access) || (Flags.isProtected(access) && RestrictionModifiers.isExtendRestriction(this.initialDescriptorRestrictions)))
+					&& (Flags.isPrivate(access2) || Util.isDefault(access2) || (Flags.isProtected(access2) && RestrictionModifiers.isExtendRestriction(this.currentDescriptorRestrictions)))) {
+				// don't report non-API deltas
+				return;
+			}
 		}
 		if (this.component.hasApiDescription() && !method.isConstructor() && !method.isClassInitializer() && !(type1.isInterface() || type1.isAnnotation())) {
 			if (restrictions != referenceRestrictions) {
@@ -1948,16 +1966,21 @@ public class ClassFileComparator {
 			this.addDelta(getElementType(type), IDelta.ADDED, IDelta.ENUM_CONSTANT, this.currentDescriptorRestrictions, this.initialDescriptorRestrictions, 0, access, this.type1, name, new String[] {
 					Util.getDescriptorName(type), name });
 		} else {
-			this.addDelta(getElementType(type), IDelta.ADDED, IDelta.FIELD, this.currentDescriptorRestrictions, this.initialDescriptorRestrictions, 0, access, this.type1, name, new String[] {
-					Util.getDescriptorName(type), name });
+			if (!(this.visibilityModifiers == VisibilityModifiers.API && component.hasApiDescription()) || Flags.isPublic(access) || Flags.isProtected(access)) {
+				// report non-API delta:
+				this.addDelta(getElementType(type), IDelta.ADDED, IDelta.FIELD, this.currentDescriptorRestrictions, this.initialDescriptorRestrictions, 0, access, this.type1, name, new String[] {
+						Util.getDescriptorName(type), name });
+			}
 		}
 	}
 
 	private void reportMethodAddition(IApiMethod method, IApiType type) {
 		int access = method.getModifiers();
 		if (method.isClassInitializer()) {
-			// report delta: addition of clinit method
-			this.addDelta(getElementType(type), IDelta.ADDED, IDelta.CLINIT, this.currentDescriptorRestrictions, 0, access, this.type1, type.getName(), Util.getDescriptorName(type1));
+			if (!(this.visibilityModifiers == VisibilityModifiers.API && component.hasApiDescription())) {
+				// report non-API delta: addition of clinit method
+				this.addDelta(getElementType(type), IDelta.ADDED, IDelta.CLINIT, this.currentDescriptorRestrictions, 0, access, this.type1, type.getName(), Util.getDescriptorName(type1));
+			}
 			return;
 		}
 		if (Flags.isSynthetic(access)) {
@@ -2144,7 +2167,9 @@ public class ClassFileComparator {
 							Util.getDescriptorName(type), methodDisplayName });
 				}
 			}
-		} else {
+		} else if (!(this.visibilityModifiers == VisibilityModifiers.API && component.hasApiDescription())) {
+			// report non-API deltas for private and package-accessible methods
+			// as well:
 			this.addDelta(getElementType(type), IDelta.ADDED, method.isConstructor() ? IDelta.CONSTRUCTOR : IDelta.METHOD, restrictionsForMethodAddition, this.initialDescriptorRestrictions, 0, method.getModifiers(), this.type1, getKeyForMethod(method, type), new String[] {
 					Util.getDescriptorName(type), methodDisplayName });
 		}