JPS mappings for incremental compilation refactoring: rules for added fields

GitOrigin-RevId: 7b15d686e337638b4a0de921deb0c75b88d57c61

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

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

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

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

java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass/BaseServer.java.new

@@ -0,0 +1,3 @@
+public class BaseServer {
+  public Integer field = 42;
+}

java/java-tests/testData/compileServer/incremental/membersChange/addFieldOfSameKindToBaseClass/src/BaseServer.java

@@ -0,0 +1,2 @@
+public class BaseServer {
+}

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);
+}

java/java-tests/testData/compileServer/incremental/membersChange/addFieldToBaseClass/data.xml

@@ -1,12 +0,0 @@
-<testData>
-
-  <deleted_by_make>
-    <file path="classes/BaseServer.class" />
-    <file path="classes/Server.class" />
-  </deleted_by_make>
-
-  <recompile>
-    <file path="source/Server.java" />
-  </recompile>
-
-</testData>

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<ClassRepr> reprs = Iterators.collect(myFuture.reprsByName(subClass, ClassRepr.class), new SmartList<>());
             if (!Iterators.isEmpty(reprs)) {
               final Iterable<File> sourceFileNames = classToSourceFileGet(subClass);
@@ -1709,35 +1711,9 @@ public final class Mappings {
         for (final Pair<FieldRepr, ClassRepr> 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<UsageRepr.Usage> 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);
           }
         }
       }

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;

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<JvmNodeReferenceID> 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<JvmClass, JvmField> 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<Node<?, ?>> 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");

jps/jps-builders/src/org/jetbrains/jps/dependency/java/PackageConstraint.java

@@ -15,6 +15,6 @@ public class PackageConstraint implements Predicate<Node<?, ?>> {
 
   @Override
   public boolean test(Node<?, ?> node) {
-    return node instanceof JvmClass && !myPackageName.equals(((JvmClass)node).getPackageName());
+    return !(node instanceof JvmClass) || !myPackageName.equals(((JvmClass)node).getPackageName());
   }
 }

jps/jps-builders/src/org/jetbrains/jps/dependency/java/Utils.java

@@ -140,7 +140,7 @@ public final class Utils {
 
   public Iterable<Pair<JvmClass, JvmField>> getOverriddenFields(JvmClass fromCls, JvmField field) {
     Function<JvmClass, Iterable<Pair<JvmClass, JvmField>>> 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<Pair<JvmClass, JvmMethod>> getOverriddenMethods(JvmClass fromCls, Predicate<JvmMethod> searchCond) {
     Function<JvmClass, Iterable<Pair<JvmClass, JvmMethod>>> 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<Pair<JvmClass, JvmMethod>> getOverridingMethods(JvmClass fromCls, JvmMethod method, Predicate<JvmMethod> searchCond) {
-    Function<JvmClass, Iterable<Pair<JvmClass, JvmMethod>>> dataGetter = cl -> isVisibleIn(fromCls, method, cl)? Iterators.collect(
+    Function<JvmClass, Iterable<Pair<JvmClass, JvmMethod>>> 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) {

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();
   }