From b318d5f500a3883a099948bc15c5ad5be3db25e8 Mon Sep 17 00:00:00 2001 From: Eugene Zhuravlev Date: Thu, 7 Feb 2013 20:18:53 +0100 Subject: [PATCH] external build make: correctly handle situations when more accessible method was removed from hierarchy --- .../removeMoreAccessibleMethod.log | 12 ++++ .../src/x/Abstract.java | 6 ++ .../src/x/Concrete.java | 7 +++ .../src/x/Concrete.java.new | 4 ++ .../src/x/y/Client.java | 11 ++++ .../java/dependencyView/ClassRepr.java | 8 +-- .../java/dependencyView/Difference.java | 7 ++- .../java/dependencyView/Mappings.java | 48 ++++++-------- .../builders/java/dependencyView/Proto.java | 63 +++++++++++++++++-- .../org/jetbrains/ether/MemberChangeTest.java | 4 ++ 10 files changed, 129 insertions(+), 41 deletions(-) create mode 100644 java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod.log create mode 100644 java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Abstract.java create mode 100644 java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Concrete.java create mode 100644 java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Concrete.java.new create mode 100644 java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/y/Client.java diff --git a/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod.log b/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod.log new file mode 100644 index 000000000000..ea3129552780 --- /dev/null +++ b/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod.log @@ -0,0 +1,12 @@ +Cleaning output files: +out/production/RemoveMoreAccessibleMethod/x/Concrete.class +End of files +Compiling files: +src/x/Concrete.java +End of files +Cleaning output files: +out/production/RemoveMoreAccessibleMethod/x/y/Client.class +End of files +Compiling files: +src/x/y/Client.java +End of files diff --git a/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Abstract.java b/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Abstract.java new file mode 100644 index 000000000000..ab43a5b2ae00 --- /dev/null +++ b/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Abstract.java @@ -0,0 +1,6 @@ +package x; + +public class Abstract { + protected void method(int a) { + } +} diff --git a/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Concrete.java b/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Concrete.java new file mode 100644 index 000000000000..8a992bb4ca8d --- /dev/null +++ b/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Concrete.java @@ -0,0 +1,7 @@ +package x; + +public class Concrete extends Abstract{ + @Override + public void method(int a) { + } +} diff --git a/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Concrete.java.new b/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Concrete.java.new new file mode 100644 index 000000000000..1502b7ddf13b --- /dev/null +++ b/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/Concrete.java.new @@ -0,0 +1,4 @@ +package x; + +public class Concrete extends Abstract{ +} diff --git a/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/y/Client.java b/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/y/Client.java new file mode 100644 index 000000000000..6a759deba6bc --- /dev/null +++ b/java/java-tests/testData/compileServer/incremental/membersChange/removeMoreAccessibleMethod/src/x/y/Client.java @@ -0,0 +1,11 @@ +package x.y; + +import x.*; + +public class Client { + private static Concrete concrete; + + public static void main(String[] args) { + concrete.method(10); + } +} diff --git a/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/ClassRepr.java b/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/ClassRepr.java index d0df19162937..83b1ce253569 100644 --- a/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/ClassRepr.java +++ b/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/ClassRepr.java @@ -80,6 +80,10 @@ public class ClassRepr extends Proto { return myUsages.add(usage); } + public boolean isInterface() { + return (access & Opcodes.ACC_INTERFACE) != 0; + } + public abstract static class Diff extends Difference { public abstract Specifier interfaces(); @@ -288,10 +292,6 @@ public class ClassRepr extends Proto { } } - public boolean isAnnotation() { - return (access & Opcodes.ACC_ANNOTATION) > 0; - } - @Override public boolean equals(Object o) { if (this == o) return true; diff --git a/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Difference.java b/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Difference.java index 0c59a0d0789a..e95258d74823 100644 --- a/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Difference.java +++ b/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Difference.java @@ -25,9 +25,6 @@ import java.util.*; * Date: 01.03.11 */ abstract class Difference { - public static boolean isPackageLocal(final int access) { - return (access & (Opcodes.ACC_PRIVATE | Opcodes.ACC_PROTECTED | Opcodes.ACC_PUBLIC)) == 0; - } public static boolean weakerAccess(final int me, final int then) { return ((me & Opcodes.ACC_PRIVATE) > 0 && (then & Opcodes.ACC_PRIVATE) == 0) || @@ -35,6 +32,10 @@ abstract class Difference { (isPackageLocal(me) && (then & Opcodes.ACC_PROTECTED) > 0); } + private static boolean isPackageLocal(final int access) { + return (access & (Opcodes.ACC_PRIVATE | Opcodes.ACC_PROTECTED | Opcodes.ACC_PUBLIC)) == 0; + } + public static final int NONE = 0; public static final int ACCESS = 1; public static final int TYPE = 2; diff --git a/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Mappings.java b/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Mappings.java index 062cf1c95c63..fe3f28831ea1 100644 --- a/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Mappings.java +++ b/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Mappings.java @@ -695,8 +695,8 @@ public class Mappings { } private static boolean isVisibleIn(final ClassRepr c, final ProtoMember m, final ClassRepr scope) { - final boolean privacy = ((m.access & Opcodes.ACC_PRIVATE) > 0) && c.name != scope.name; - final boolean packageLocality = Difference.isPackageLocal(m.access) && !c.getPackageName().equals(scope.getPackageName()); + final boolean privacy = m.isPrivate() && c.name != scope.name; + final boolean packageLocality = m.isPackageLocal() && !c.getPackageName().equals(scope.getPackageName()); return !privacy && !packageLocality; } @@ -733,13 +733,13 @@ public class Mappings { final Util self = new Util(); // Public branch --- hopeless - if ((member.access & Opcodes.ACC_PUBLIC) > 0) { + if (member.isPublic()) { debug("Public access, switching to a non-incremental mode"); return false; } // Protected branch - if ((member.access & Opcodes.ACC_PROTECTED) > 0) { + if (member.isProtected()) { debug("Protected access, softening non-incremental decision: adding all relevant subclasses for a recompilation"); debug("Root class: ", owner); @@ -1008,16 +1008,14 @@ public class Mappings { Ref oldItRef = null; for (final MethodRepr m : added) { debug("Method: ", m.name); - if ((it.access & Opcodes.ACC_INTERFACE) > 0 || - (it.access & Opcodes.ACC_ABSTRACT) > 0 || - (m.access & Opcodes.ACC_ABSTRACT) > 0) { + if (it.isInterface() || it.isAbstract() || m.isAbstract()) { debug("Class is abstract, or is interface, or added method in abstract => affecting all subclasses"); myFuture.affectSubclasses(it.name, myAffectedFiles, state.myAffectedUsages, state.myDependants, false); } TIntHashSet propagated = null; - if ((m.access & Opcodes.ACC_PRIVATE) == 0 && m.name != myInitName) { + if (!m.isPrivate() && m.name != myInitName) { if (oldItRef == null) { oldItRef = new Ref(getReprByName(null, it.name)); // lazy init } @@ -1035,7 +1033,7 @@ public class Mappings { } } - if ((m.access & Opcodes.ACC_PRIVATE) == 0) { + if (!m.isPrivate()) { final Collection> affectedMethods = myFuture.findAllMethodsBySpecificity(m, it); final MethodRepr.Predicate overrides = MethodRepr.equalByJavaRules(m); @@ -1147,7 +1145,7 @@ public class Mappings { for (final Pair overriden : overridenMethods) { final MethodRepr mm = overriden.first; - if (mm == MOCK_METHOD || !mm.myType.equals(m.myType) || !isEmpty(mm.signature) || !isEmpty(m.signature)) { + if (mm == MOCK_METHOD || !mm.myType.equals(m.myType) || !isEmpty(mm.signature) || !isEmpty(m.signature) || m.isMoreAccessibleThan(mm)) { clear = false; break loop; } @@ -1171,7 +1169,7 @@ public class Mappings { } } - if ((m.access & Opcodes.ACC_ABSTRACT) == 0) { + if (!m.isAbstract()) { propagated.forEach(new TIntProcedure() { @Override public boolean execute(int p) { @@ -1199,7 +1197,7 @@ public class Mappings { } visited = true; - allAbstract = ((pp.first.access & Opcodes.ACC_ABSTRACT) > 0) || ((cc.access & Opcodes.ACC_INTERFACE) > 0); + allAbstract = pp.first.isAbstract() || cc.isInterface(); if (!allAbstract) { break; @@ -1345,12 +1343,7 @@ public class Mappings { for (final FieldRepr f : added) { debug("Field: ", f.name); - final boolean fPrivate = (f.access & Opcodes.ACC_PRIVATE) > 0; - final boolean fProtected = (f.access & Opcodes.ACC_PROTECTED) > 0; - final boolean fPublic = (f.access & Opcodes.ACC_PUBLIC) > 0; - final boolean fPLocal = !fPrivate && !fProtected && !fPublic; - - if (!fPrivate) { + if (!f.isPrivate()) { final TIntHashSet subClasses = getAllSubclasses(classRepr.name); subClasses.forEach(new TIntProcedure() { @Override @@ -1394,28 +1387,23 @@ public class Mappings { final FieldRepr ff = p.first; final ClassRepr cc = p.second; - final boolean ffPrivate = (ff.access & Opcodes.ACC_PRIVATE) > 0; - final boolean ffProtected = (ff.access & Opcodes.ACC_PROTECTED) > 0; - final boolean ffPublic = (ff.access & Opcodes.ACC_PUBLIC) > 0; - final boolean ffPLocal = Difference.isPackageLocal(ff.access); - - if (!ffPrivate) { + if (!ff.isPrivate()) { final TIntHashSet propagated = myPresent.propagateFieldAccess(ff.name, cc.name); final Set localUsages = new HashSet(); debug("Affecting usages of overridden field in class ", cc.name); myFuture.affectFieldUsages(ff, propagated, ff.createUsage(myContext, cc.name), localUsages, state.myDependants); - if (fPrivate || (fPublic && (ffPublic || ffPLocal)) || (fProtected && ffProtected) || (fPLocal && ffPLocal)) { - + if (f.isPrivate() || (f.isPublic() && (ff.isPublic() || ff.isPackageLocal())) || (f.isProtected() && ff.isProtected()) || (f.isPackageLocal() && ff.isPackageLocal())) { + // nothing } else { Util.UsageConstraint constaint; - if ((ffProtected && fPublic) || (fProtected && ffPublic) || (ffPLocal && fProtected)) { + if ((ff.isProtected() && f.isPublic()) || (f.isProtected() && ff.isPublic()) || (ff.isPackageLocal() && f.isProtected())) { constaint = myFuture.new NegationConstraint(myFuture.new InheritanceConstraint(cc.name)); } - else if (ffPublic && ffPLocal) { + else if (ff.isPublic() && ff.isPackageLocal()) { constaint = myFuture.new NegationConstraint(myFuture.new PackageConstraint(cc.getPackageName())); } else { @@ -1449,7 +1437,7 @@ public class Mappings { for (final FieldRepr f : removed) { debug("Field: ", f.name); - if ((f.access & Opcodes.ACC_PRIVATE) == 0 && (f.access & DESPERATE_MASK) == DESPERATE_MASK && f.hasValue()) { + if (!f.isPrivate() && (f.access & DESPERATE_MASK) == DESPERATE_MASK && f.hasValue()) { debug("Field had value and was (non-private) final static => a switch to non-incremental mode requested"); if (myConstantSearch != null) { myDelayedWorks.addConstantWork(it.name, f, true, false); @@ -1483,7 +1471,7 @@ public class Mappings { debug("Field: ", field.name); - if ((field.access & Opcodes.ACC_PRIVATE) == 0 && (field.access & DESPERATE_MASK) == DESPERATE_MASK) { + if (!field.isPrivate() && (field.access & DESPERATE_MASK) == DESPERATE_MASK) { final int changedModifiers = d.addedModifiers() | d.removedModifiers(); final boolean harmful = (changedModifiers & (Opcodes.ACC_STATIC | Opcodes.ACC_FINAL)) > 0; final boolean accessChanged = (changedModifiers & (Opcodes.ACC_PUBLIC | Opcodes.ACC_PRIVATE | Opcodes.ACC_PROTECTED)) > 0; diff --git a/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Proto.java b/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Proto.java index 7a4351542bde..593128edb982 100644 --- a/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Proto.java +++ b/jps/jps-builders/src/org/jetbrains/jps/builders/java/dependencyView/Proto.java @@ -60,6 +60,64 @@ class Proto implements RW.Savable, Streamable { } } + public final boolean isPublic() { + return (Opcodes.ACC_PUBLIC & access) != 0; + } + + public final boolean isProtected() { + return (Opcodes.ACC_PROTECTED & access) != 0; + } + + public final boolean isPackageLocal() { + return (access & (Opcodes.ACC_PRIVATE | Opcodes.ACC_PROTECTED | Opcodes.ACC_PUBLIC)) == 0; + } + + public final boolean isPrivate() { + return (Opcodes.ACC_PRIVATE & access) != 0; + } + + public final boolean isAbstract() { + return (Opcodes.ACC_ABSTRACT & access) != 0; + } + + public final boolean isBridge() { + return (Opcodes.ACC_BRIDGE & access) != 0; + } + + public final boolean isSynthetic() { + return (Opcodes.ACC_SYNTHETIC & access) != 0; + } + + public final boolean isAnnotation() { + return (Opcodes.ACC_ANNOTATION & access) != 0; + } + + public final boolean isFinal() { + return (Opcodes.ACC_FINAL & access) != 0; + } + + public final boolean isStatic() { + return (Opcodes.ACC_STATIC & access) != 0; + } + + /** + * tests if the accessibility of this Proto is less restricted than the accessibility of the given Proto + * @return true means this Proto is less restricted than the proto passed as parameter
+ * false means this Proto has more restricted access than the parameter Proto or they have equal accessibility + */ + public final boolean isMoreAccessibleThan(Proto anotherProto) { + if (anotherProto.isPrivate()) { + return this.isPackageLocal() || this.isProtected() || this.isPublic(); + } + if (anotherProto.isPackageLocal()) { + return this.isProtected() || this.isPublic(); + } + if (anotherProto.isProtected()) { + return this.isPublic(); + } + return false; + } + public Difference difference(final Proto past) { int diff = Difference.NONE; @@ -96,10 +154,7 @@ class Proto implements RW.Savable, Streamable { @Override public boolean packageLocalOn() { - return ((past.access & Opcodes.ACC_PRIVATE) != 0 || - (past.access & Opcodes.ACC_PUBLIC) != 0 || - (past.access & Opcodes.ACC_PROTECTED) != 0) && - Difference.isPackageLocal(access); + return (past.isPrivate() || past.isPublic() || past.isProtected()) && Proto.this.isPackageLocal(); } @Override diff --git a/jps/jps-builders/testSrc/org/jetbrains/ether/MemberChangeTest.java b/jps/jps-builders/testSrc/org/jetbrains/ether/MemberChangeTest.java index 1fd356da4a32..c1c1f81c3cca 100644 --- a/jps/jps-builders/testSrc/org/jetbrains/ether/MemberChangeTest.java +++ b/jps/jps-builders/testSrc/org/jetbrains/ether/MemberChangeTest.java @@ -176,6 +176,10 @@ public class MemberChangeTest extends IncrementalTestCase { doTest(); } + public void testRemoveMoreAccessibleMethod() { + doTest(); + } + public void testRenameMethod() { doTest(); }