diff --git a/java/java-tests/testData/compileServer/incremental/fieldModifiers/hideProtectedWithPackagePrivate.log b/java/java-tests/testData/compileServer/incremental/fieldModifiers/hideProtectedWithPackagePrivate.log index f19b59652c22..da96fc0e4237 100644 --- a/java/java-tests/testData/compileServer/incremental/fieldModifiers/hideProtectedWithPackagePrivate.log +++ b/java/java-tests/testData/compileServer/incremental/fieldModifiers/hideProtectedWithPackagePrivate.log @@ -5,8 +5,10 @@ Compiling files: src/ppp/Derived.java End of files Cleaning output files: +out/production/HideProtectedWithPackagePrivate/ppp/ClientInside.class out/production/HideProtectedWithPackagePrivate/qqq/DerivedOutside.class End of files Compiling files: +src/ppp/ClientInside.java src/qqq/DerivedOutside.java End of files \ No newline at end of file diff --git a/java/java-tests/testData/compileServer/incremental/fieldModifiers/hidePublicWithPackagePrivate.log b/java/java-tests/testData/compileServer/incremental/fieldModifiers/hidePublicWithPackagePrivate.log index ee14d2409df0..1c1daf54fd8e 100644 --- a/java/java-tests/testData/compileServer/incremental/fieldModifiers/hidePublicWithPackagePrivate.log +++ b/java/java-tests/testData/compileServer/incremental/fieldModifiers/hidePublicWithPackagePrivate.log @@ -5,10 +5,12 @@ Compiling files: src/ppp/Derived.java End of files Cleaning output files: +out/production/HidePublicWithPackagePrivate/ppp/ClientInside.class out/production/HidePublicWithPackagePrivate/qqq/ClientOutside.class out/production/HidePublicWithPackagePrivate/qqq/DerivedOutside.class End of files Compiling files: +src/ppp/ClientInside.java src/qqq/ClientOutside.java src/qqq/DerivedOutside.java End of files \ No newline at end of file diff --git a/java/java-tests/testData/compileServer/incremental/fieldModifiers/hidePublicWithProtected.log b/java/java-tests/testData/compileServer/incremental/fieldModifiers/hidePublicWithProtected.log index d991690d3cf6..c42fb6090368 100644 --- a/java/java-tests/testData/compileServer/incremental/fieldModifiers/hidePublicWithProtected.log +++ b/java/java-tests/testData/compileServer/incremental/fieldModifiers/hidePublicWithProtected.log @@ -5,8 +5,12 @@ Compiling files: src/ppp/Derived.java End of files Cleaning output files: +out/production/HidePublicWithProtected/ppp/ClientInside.class out/production/HidePublicWithProtected/qqq/ClientOutside.class +out/production/HidePublicWithProtected/qqq/DerivedOutside.class End of files Compiling files: +src/ppp/ClientInside.java src/qqq/ClientOutside.java +src/qqq/DerivedOutside.java End of files \ No newline at end of file diff --git a/java/java-tests/testData/compileServer/incremental/membersChange/addFieldToBaseClass/addFieldToBaseClass.log b/java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass.log similarity index 58% rename from java/java-tests/testData/compileServer/incremental/membersChange/addFieldToBaseClass/addFieldToBaseClass.log rename to java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass.log index 5d06361ae503..a675908faa89 100644 --- a/java/java-tests/testData/compileServer/incremental/membersChange/addFieldToBaseClass/addFieldToBaseClass.log +++ b/java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass.log @@ -1,5 +1,5 @@ Cleaning output files: -out/production/AddFieldToBaseClass/BaseServer.class +out/production/AddFieldOfSameKindToBaseClass/BaseServer.class End of files Compiling files: src/BaseServer.java diff --git a/java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass/BaseServer.java.new b/java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass/BaseServer.java.new new file mode 100644 index 000000000000..7007da41942c --- /dev/null +++ b/java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass/BaseServer.java.new @@ -0,0 +1,3 @@ +public class BaseServer { + public Integer field = 42; +} diff --git a/java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass/src/BaseServer.java b/java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass/src/BaseServer.java new file mode 100644 index 000000000000..33f9b7be23e4 --- /dev/null +++ b/java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass/src/BaseServer.java @@ -0,0 +1,2 @@ +public class BaseServer { +} diff --git a/java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass/src/Server.java b/java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass/src/Server.java new file mode 100644 index 000000000000..97094658dd9d --- /dev/null +++ b/java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass/src/Server.java @@ -0,0 +1,3 @@ +public class Server extends BaseServer { + public Integer field = new Integer(10); +} diff --git a/java/java-tests/testData/compileServer/incremental/membersChange/addFieldToBaseClass/data.xml b/java/java-tests/testData/compileServer/incremental/membersChange/addFieldToBaseClass/data.xml deleted file mode 100644 index 559781e76f7e..000000000000 --- a/java/java-tests/testData/compileServer/incremental/membersChange/addFieldToBaseClass/data.xml +++ /dev/null @@ -1,12 +0,0 @@ - - - - - - - - - - - - \ No newline at end of file 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 f5f603b74508..d9403ff62a3a 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 @@ -1667,7 +1667,9 @@ public final class Mappings { debug("Field: ", addedField.name); if (!addedField.isPrivate()) { - getAllSubclasses(classRepr.name).forEach(subClass -> { + IntSet changedClassWithSubclasses = myFuture.propagateFieldAccess(addedField.name, classRepr.name); + changedClassWithSubclasses.add(classRepr.name); + changedClassWithSubclasses.forEach(subClass -> { final Iterable reprs = Iterators.collect(myFuture.reprsByName(subClass, ClassRepr.class), new SmartList<>()); if (!Iterators.isEmpty(reprs)) { final Iterable sourceFileNames = classToSourceFileGet(subClass); @@ -1709,35 +1711,9 @@ public final class Mappings { for (final Pair p : overriddenFields) { final FieldRepr overridden = p.first; final ClassRepr cc = p.second; - if (overridden.isPrivate()) { - continue; - } - final boolean sameKind = addedField.myType.equals(overridden.myType) && addedField.isStatic() == overridden.isStatic() && addedField.isSynthetic() == overridden.isSynthetic() && addedField.isFinal() == overridden.isFinal(); - if (!sameKind || Difference.weakerAccess(addedField.access, overridden.access)) { - final IntSet propagated = myPresent.propagateFieldAccess(overridden.name, cc.name); - - final Set affectedUsages = new HashSet<>(); + if (!overridden.isPrivate()) { debug("Affecting usages of overridden field in class ", cc.name); - myFuture.affectFieldUsages(overridden, propagated, overridden.createUsage(myContext, cc.name), affectedUsages, state.myDependants); - - if (sameKind) { - // check if we can reduce the number of usages going to be recompiled - UsageConstraint constraint = null; - if (addedField.isProtected()) { - // no need to recompile usages in field class' package and hierarchy, since newly added field is accessible in this scope - constraint = myFuture.new InheritanceConstraint(cc); - } - else if (addedField.isPackageLocal()) { - // no need to recompile usages in field class' package, since newly added field is accessible in this scope - constraint = myFuture.new PackageConstraint(cc.getPackageName()); - } - if (constraint != null) { - for (final UsageRepr.Usage usage : affectedUsages) { - state.myUsageConstraints.put(usage, constraint); - } - } - } - state.myAffectedUsages.addAll(affectedUsages); + myFuture.affectFieldUsages(overridden, myPresent.propagateFieldAccess(overridden.name, cc.name), overridden.createUsage(myContext, cc.name), state.myAffectedUsages, state.myDependants); } } } diff --git a/jps/jps-builders/src/org/jetbrains/jps/dependency/java/InheritanceConstraint.java b/jps/jps-builders/src/org/jetbrains/jps/dependency/java/InheritanceConstraint.java index 312a64627469..b2f36a90e207 100644 --- a/jps/jps-builders/src/org/jetbrains/jps/dependency/java/InheritanceConstraint.java +++ b/jps/jps-builders/src/org/jetbrains/jps/dependency/java/InheritanceConstraint.java @@ -18,9 +18,11 @@ public final class InheritanceConstraint extends PackageConstraint{ if (!super.test(node)) { return false; } - for (JvmNodeReferenceID s : myUtils.allSupertypes(((JvmClass)node).getReferenceID())) { - if (myRootClass.equals(s)) { - return false; + if (node instanceof JvmClass) { + for (JvmNodeReferenceID s : myUtils.allSupertypes(((JvmClass)node).getReferenceID())) { + if (myRootClass.equals(s)) { + return false; + } } } return true; diff --git a/jps/jps-builders/src/org/jetbrains/jps/dependency/java/JavaDifferentiateStrategy.java b/jps/jps-builders/src/org/jetbrains/jps/dependency/java/JavaDifferentiateStrategy.java index 04696e01ea0f..b57276fcfd76 100644 --- a/jps/jps-builders/src/org/jetbrains/jps/dependency/java/JavaDifferentiateStrategy.java +++ b/jps/jps-builders/src/org/jetbrains/jps/dependency/java/JavaDifferentiateStrategy.java @@ -594,15 +594,11 @@ public final class JavaDifferentiateStrategy implements DifferentiateStrategy { for (JvmField addedField : added) { debug("Field: " + addedField.getName()); - if (!addedField.isPrivate()) { - - for (ReferenceID id : future.withAllSubclasses(changedClass.getReferenceID())) { - if (!(id instanceof JvmNodeReferenceID)) { - continue; - } - JvmNodeReferenceID subClass = (JvmNodeReferenceID)id; - - String affectReason = null; + Set changedClassWithSubclasses = future.collectSubclassesWithoutField(changedClass.getReferenceID(), addedField.getName()); + changedClassWithSubclasses.add(changedClass.getReferenceID()); + for (JvmNodeReferenceID subClass : changedClassWithSubclasses) { + String affectReason = null; + if (!addedField.isPrivate()) { for (JvmClass cl : future.getNodes(subClass, JvmClass.class)) { if (cl.isLocal()) { affectReason = "Affecting local subclass (introduced field can potentially hide surrounding method parameters/local variables): "; @@ -619,40 +615,30 @@ public final class JavaDifferentiateStrategy implements DifferentiateStrategy { } } } + } - if (affectReason != null) { - affectNodeSources(context, subClass, affectReason); - } + if (affectReason != null) { + affectNodeSources(context, subClass, affectReason); + } - debug("Affecting field usages referenced from subclass ", subClass.getNodeName()); - affectMemberUsages(context, subClass, addedField, Collections.emptyList()); - if (addedField.isStatic()) { - affectStaticMemberOnDemandUsages(context, subClass, Collections.emptyList()); - } + if (!addedField.isPrivate() && addedField.isStatic()) { + affectStaticMemberOnDemandUsages(context, subClass, Collections.emptyList()); + } + else { + // ensure analysis scope includes classes that depend on the subclass + context.affectUsage(new AffectionScopeMetaUsage(subClass)); } } - for (Pair p : future.getOverriddenFields(changedClass, addedField)) { - JvmClass overriddenCls = p.getFirst(); - JvmField overriddenField = p.getSecond(); - if (!addedField.isSameKind(overriddenField) || addedField.isWeakerAccessThan(overriddenField)) { - debug("Affecting usages of overridden field in class ", overriddenCls.getName()); - - Predicate> constraint = null; - if (addedField.isSameKind(overriddenField)) { - if (addedField.isProtected()) { - // no need to recompile usages in field class' package and hierarchy, since newly added field is accessible in this scope - constraint = new InheritanceConstraint(future, overriddenCls); - } - else if (addedField.isPackageLocal()) { - // no need to recompile usages in field class' package, since newly added field is accessible in this scope - constraint = new PackageConstraint(overriddenCls.getPackageName()); - } - } - affectMemberUsages(context, overriddenCls.getReferenceID(), overriddenField, present.collectSubclassesWithoutField(overriddenCls.getReferenceID(), overriddenField.getName()), constraint); + context.affectUsage((n, u) -> { + // affect all clients that access fields with the same name via subclasses, + // if the added field is not visible to the client + if (!(u instanceof FieldUsage) || !(n instanceof JvmClass)) { + return false; } - } - + FieldUsage fieldUsage = (FieldUsage)u; + return Objects.equals(fieldUsage.getName(), addedField.getName()) && changedClassWithSubclasses.contains(fieldUsage.getElementOwner()); + }); } debug("End of added fields processing"); diff --git a/jps/jps-builders/src/org/jetbrains/jps/dependency/java/PackageConstraint.java b/jps/jps-builders/src/org/jetbrains/jps/dependency/java/PackageConstraint.java index 46ac2540926f..c23298acb916 100644 --- a/jps/jps-builders/src/org/jetbrains/jps/dependency/java/PackageConstraint.java +++ b/jps/jps-builders/src/org/jetbrains/jps/dependency/java/PackageConstraint.java @@ -15,6 +15,6 @@ public class PackageConstraint implements Predicate> { @Override public boolean test(Node node) { - return node instanceof JvmClass && !myPackageName.equals(((JvmClass)node).getPackageName()); + return !(node instanceof JvmClass) || !myPackageName.equals(((JvmClass)node).getPackageName()); } } diff --git a/jps/jps-builders/src/org/jetbrains/jps/dependency/java/Utils.java b/jps/jps-builders/src/org/jetbrains/jps/dependency/java/Utils.java index 57a33763c267..6b608a419fd7 100644 --- a/jps/jps-builders/src/org/jetbrains/jps/dependency/java/Utils.java +++ b/jps/jps-builders/src/org/jetbrains/jps/dependency/java/Utils.java @@ -140,7 +140,7 @@ public final class Utils { public Iterable> getOverriddenFields(JvmClass fromCls, JvmField field) { Function>> dataGetter = cl -> Iterators.collect( - Iterators.map(Iterators.filter(cl.getFields(), f -> Objects.equals(f.getName(), field.getName()) && isVisibleIn(cl, f, fromCls)), ff -> Pair.create(cl, ff)), + Iterators.map(Iterators.filter(cl.getFields(), f -> Objects.equals(f.getName(), field.getName()) && isVisibleInHierarchy(cl, f, fromCls)), ff -> Pair.create(cl, ff)), new SmartList<>() ); return Iterators.flat( @@ -150,7 +150,7 @@ public final class Utils { public Iterable> getOverriddenMethods(JvmClass fromCls, Predicate searchCond) { Function>> dataGetter = cl -> Iterators.collect( - Iterators.map(Iterators.filter(cl.getMethods(), m -> searchCond.test(m) && isVisibleIn(cl, m, fromCls)), mm -> Pair.create(cl, mm)), + Iterators.map(Iterators.filter(cl.getMethods(), m -> searchCond.test(m) && isVisibleInHierarchy(cl, m, fromCls)), mm -> Pair.create(cl, mm)), new SmartList<>() ); return Iterators.flat( @@ -159,7 +159,7 @@ public final class Utils { } public Iterable> getOverridingMethods(JvmClass fromCls, JvmMethod method, Predicate searchCond) { - Function>> dataGetter = cl -> isVisibleIn(fromCls, method, cl)? Iterators.collect( + Function>> dataGetter = cl -> isVisibleInHierarchy(fromCls, method, cl)? Iterators.collect( Iterators.map(Iterators.filter(cl.getMethods(), searchCond::test), mm -> Pair.create(cl, mm)), new SmartList<>() ) : Collections.emptyList(); @@ -192,11 +192,26 @@ public final class Utils { return !Iterators.isEmpty(Iterators.filter(cls.getMethods(), method::isSameByJavaRules)) || !Iterators.isEmpty(getOverriddenMethods(cls, method::isSameByJavaRules)); } - // tests visibility within a class hierarchy - private static boolean isVisibleIn(final JvmClass cls, final ProtoMember member, final JvmClass scope) { - final boolean privacy = member.isPrivate() && !Objects.equals(cls.getName(), scope.getName()); - final boolean packageLocality = member.isPackageLocal() && !Objects.equals(cls.getPackageName(), scope.getPackageName()); - return !privacy && !packageLocality; + private boolean isVisibleInHierarchy(final JvmClass cls, final ProtoMember clsMember, final JvmClass subClass) { + // optimized version, allows skipping isInheritor check + return clsMember.isProtected() || isVisibleIn(cls, clsMember, subClass); + } + + public boolean isVisibleIn(final JvmClass cls, final ProtoMember clsMember, final JvmClass scope) { + if (clsMember.isPrivate()) { + return Objects.equals(cls.getReferenceID(), scope.getReferenceID()); + } + if (clsMember.isPackageLocal()) { + return Objects.equals(cls.getPackageName(), scope.getPackageName()); + } + if (clsMember.isProtected()) { + return Objects.equals(cls.getPackageName(), scope.getPackageName()) || isInheritorOf(scope, cls); + } + return true; + } + + public boolean isInheritorOf(JvmClass who, JvmClass whom) { + return !Iterators.isEmpty(Iterators.filter(Iterators.recurseDepth(who, cl -> Iterators.flat(Iterators.map(who.getSuperTypes(), st -> getClassesByName(st))), true), cl -> cl.getReferenceID().equals(whom.getReferenceID()))); } public boolean inheritsFromLibraryClass(JvmClass cls) { diff --git a/jps/jps-builders/testSrc/org/jetbrains/ether/MemberChangeTest.java b/jps/jps-builders/testSrc/org/jetbrains/ether/MemberChangeTest.java index 64e7d1f7c8cb..b815b48f73a7 100644 --- a/jps/jps-builders/testSrc/org/jetbrains/ether/MemberChangeTest.java +++ b/jps/jps-builders/testSrc/org/jetbrains/ether/MemberChangeTest.java @@ -42,6 +42,10 @@ public class MemberChangeTest extends IncrementalTestCase { doTest(); } + public void testAddFieldOfSameKindToBaseClass() { + doTest(); + } + public void testAddFieldToDerived() { doTest(); }