Version 1.4.33

Cherry-pick:
Clarify test expectations of non-rebound field renaming.
https://r8-review.googlesource.com/c/r8/+/33740

Cherry-pick:
Rename non-rebound field references according to their definitions.
https://r8-review.googlesource.com/c/r8/+/33701

Cherry-pick:
Reproduce b/123068484: no field rebinding due to visibility restriction.
https://r8-review.googlesource.com/c/r8/+/33680

Bug: 123068484
Change-Id: Iecddaf96ad9a3145a8149ffa2ce39ca23e05d7d5
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index ec1ec47..20093ab 100644
--- a/src/main/java/com/android/tools/r8/Version.java
+++ b/src/main/java/com/android/tools/r8/Version.java
@@ -11,7 +11,7 @@
 
   // This field is accessed from release scripts using simple pattern matching.
   // Therefore, changing this field could break our release scripts.
-  public static final String LABEL = "1.4.32";
+  public static final String LABEL = "1.4.33";
 
   private Version() {
   }
diff --git a/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java b/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
index 3b546a8..b589aa4 100644
--- a/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/FieldNameMinifier.java
@@ -12,6 +12,7 @@
 import com.android.tools.r8.shaking.RootSetBuilder.RootSet;
 import com.android.tools.r8.utils.InternalOptions;
 import com.android.tools.r8.utils.Timing;
+import com.google.common.collect.Sets;
 import java.util.Map;
 import java.util.function.Function;
 
@@ -45,11 +46,15 @@
     DexType.forAllInterfaces(appInfo.dexItemFactory,
         iface -> reserveNamesInSubtypes(iface, globalState));
     timing.end();
-    // Now rename the rest.
-    timing.begin("rename");
+    // Rename the definitions.
+    timing.begin("rename-definitions");
     renameFieldsInSubtypes(appInfo.dexItemFactory.objectType);
     DexType.forAllInterfaces(appInfo.dexItemFactory, this::renameFieldsInSubtypes);
     timing.end();
+    // Rename the references that are not rebound to definitions for some reasons.
+    timing.begin("rename-references");
+    renameNonReboundReferences();
+    timing.end();
     return renaming;
   }
 
@@ -92,4 +97,44 @@
           state.assignNewNameFor(field.name, field.type, useUniqueMemberNames));
     }
   }
+
+  private void renameNonReboundReferences() {
+    // TODO(b/123068484): Collect non-rebound references instead of visiting all references.
+    Sets.union(
+        Sets.union(appInfo.staticFieldReads.keySet(), appInfo.staticFieldWrites.keySet()),
+        Sets.union(appInfo.instanceFieldReads.keySet(), appInfo.instanceFieldWrites.keySet()))
+        .forEach(this::renameNonReboundReference);
+  }
+
+  private void renameNonReboundReference(DexField field) {
+    // Already renamed
+    if (renaming.containsKey(field)) {
+      return;
+    }
+    DexEncodedField definition = appInfo.definitionFor(field);
+    if (definition != null) {
+      assert definition.field == field;
+      return;
+    }
+    // Now, `field` is reference. Find its definition and check if it's renamed.
+    DexType holderType = field.getHolder();
+    DexClass holder = appInfo.definitionFor(holderType);
+    // We don't care pruned types or library classes.
+    if (holder == null || holder.isLibraryClass()) {
+      return;
+    }
+    definition = appInfo.resolveFieldOn(holderType, field);
+    if (definition == null) {
+      // The program is already broken in the sense that it has an unresolvable field reference.
+      // Leave it as-is.
+      return;
+    }
+    assert definition.field != field;
+    assert definition.field.getHolder() != holderType;
+    // If the definition is renamed,
+    if (renaming.containsKey(definition.field)) {
+      // Assign the same, renamed name as the definition to the reference.
+      renaming.put(field, renaming.get(definition.field));
+    }
+  }
 }
diff --git a/src/test/java/com/android/tools/r8/TestBase.java b/src/test/java/com/android/tools/r8/TestBase.java
index 1df6808..2b66df4 100644
--- a/src/test/java/com/android/tools/r8/TestBase.java
+++ b/src/test/java/com/android/tools/r8/TestBase.java
@@ -50,10 +50,13 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.nio.file.StandardOpenOption;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.Collections;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.concurrent.ExecutionException;
 import java.util.function.Consumer;
@@ -903,18 +906,31 @@
     return extractor.getClassInternalType();
   }
 
-  protected Path writeToJar(List<byte[]> classes) throws IOException {
-    Path result = File.createTempFile("junit", ".jar", temp.getRoot()).toPath();
+  protected static void writeToJar(Path output, List<byte[]> classes) throws IOException {
     try (ZipOutputStream out =
         new ZipOutputStream(
             Files.newOutputStream(
-                result, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING))) {
+                output, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING))) {
       for (byte[] clazz : classes) {
         String name = extractClassName(clazz);
         ZipUtils.writeToZipStream(
             out, DescriptorUtils.getPathFromJavaType(name), clazz, ZipEntry.STORED);
       }
     }
+  }
+
+  protected static void writeToJar(Path output, Collection<Path> classes) throws IOException {
+    List<byte[]> bytes = new LinkedList<>();
+    for (Path classPath : classes) {
+      byte[] classBytes = Files.readAllBytes(Paths.get(classPath.toString()));
+      bytes.add(classBytes);
+    }
+    writeToJar(output, bytes);
+  }
+
+  protected Path writeToJar(List<byte[]> classes) throws IOException {
+    Path result = File.createTempFile("junit", ".jar", temp.getRoot()).toPath();
+    writeToJar(result, classes);
     return result;
   }
 
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/FieldRenamingTest.java b/src/test/java/com/android/tools/r8/naming/b123068484/FieldRenamingTest.java
new file mode 100644
index 0000000..4cedaea
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/FieldRenamingTest.java
@@ -0,0 +1,97 @@
+// Copyright (c) 2019, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+package com.android.tools.r8.naming.b123068484;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertThat;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.naming.b123068484.data.Concrete1;
+import com.android.tools.r8.naming.b123068484.runner.Runner;
+import com.android.tools.r8.utils.StringUtils;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.android.tools.r8.utils.codeinspector.FieldSubject;
+import com.android.tools.r8.utils.codeinspector.InstructionSubject;
+import com.android.tools.r8.utils.codeinspector.MethodSubject;
+import com.google.common.collect.ImmutableList;
+import java.nio.file.Path;
+import java.util.List;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class FieldRenamingTest extends TestBase {
+  private static final Class<?> MAIN = Runner.class;
+  private static final Class<?> CONCRETE1 = Concrete1.class;
+  private static List<Path> CLASSES;
+  private static final String EXPECTED_OUTPUT = StringUtils.lines("Runner");
+
+  private final Backend backend;
+
+  @Parameterized.Parameters(name = "Backend: {0}")
+  public static Object[] data() {
+    return Backend.values();
+  }
+
+  public FieldRenamingTest(Backend backend) {
+    this.backend = backend;
+  }
+
+  @BeforeClass
+  public static void setUpClass() throws Exception {
+    CLASSES = ImmutableList.<Path>builder()
+        .addAll(ToolHelper.getClassFilesForTestPackage(MAIN.getPackage()))
+        .addAll(ToolHelper.getClassFilesForTestPackage(CONCRETE1.getPackage()))
+        .build();
+  }
+
+  @Test
+  public void testProguard() throws Exception {
+    Path inJar = temp.newFile("input.jar").toPath().toAbsolutePath();
+    writeToJar(inJar, CLASSES);
+    testForProguard()
+        .addProgramFiles(inJar)
+        .addKeepMainRule(MAIN)
+        .run(MAIN)
+        .assertSuccessWithOutput(EXPECTED_OUTPUT)
+        .inspect(this::inspect);
+  }
+
+  @Test
+  public void testR8() throws Exception {
+    testForR8(backend)
+        .addProgramFiles(CLASSES)
+        .addKeepMainRule(MAIN)
+        .run(MAIN)
+        .assertSuccessWithOutput(EXPECTED_OUTPUT)
+        .inspect(this::inspect);
+  }
+
+  private void inspect(CodeInspector inspector) {
+    FieldSubject fld = inspector.clazz(CONCRETE1.getTypeName().replace("Concrete1", "Abs"))
+        .uniqueFieldWithName("strField");
+    assertThat(fld, isPresent());
+
+    ClassSubject main = inspector.clazz(MAIN);
+    assertThat(main, isPresent());
+    MethodSubject methodSubject = main.mainMethod();
+    assertThat(methodSubject, isPresent());
+
+    methodSubject
+        .iterateInstructions(InstructionSubject::isInstanceGet)
+        .forEachRemaining(instructionSubject -> {
+          String fieldName = instructionSubject.getField().name.toString();
+          // All of those field references will be renamed.
+          assertNotEquals("strField", fieldName);
+          assertEquals(fld.getFinalName(), fieldName);
+        });
+  }
+
+}
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/data/Abs.java b/src/test/java/com/android/tools/r8/naming/b123068484/data/Abs.java
new file mode 100644
index 0000000..d40899a
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/data/Abs.java
@@ -0,0 +1,13 @@
+// Copyright (c) 2019, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+package com.android.tools.r8.naming.b123068484.data;
+
+// Hide the strField via class-level access modifier.
+abstract class Abs extends PublicAbs {
+  public final String strField;
+
+  Abs(String x) {
+    strField = x;
+  }
+}
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete1.java b/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete1.java
new file mode 100644
index 0000000..02ce3b6
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete1.java
@@ -0,0 +1,12 @@
+// Copyright (c) 2019, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+package com.android.tools.r8.naming.b123068484.data;
+
+public class Concrete1 extends Abs {
+  Object otherField;
+  public Concrete1(String x) {
+    super(x);
+    otherField = new Throwable("Concrete1::<init>");
+  }
+}
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete2.java b/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete2.java
new file mode 100644
index 0000000..55e8dce
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/data/Concrete2.java
@@ -0,0 +1,12 @@
+// Copyright (c) 2019, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+package com.android.tools.r8.naming.b123068484.data;
+
+public class Concrete2 extends Abs {
+  Object otherField;
+  public Concrete2(String x) {
+    super(x);
+    otherField = new Exception("Concrete2::<init>");
+  }
+}
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/data/PublicAbs.java b/src/test/java/com/android/tools/r8/naming/b123068484/data/PublicAbs.java
new file mode 100644
index 0000000..d7d0e45
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/data/PublicAbs.java
@@ -0,0 +1,9 @@
+// Copyright (c) 2019, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+package com.android.tools.r8.naming.b123068484.data;
+
+// Just to abstract Concrete(1|2) and make them visible from different packages.
+public abstract class PublicAbs {
+  static Object foo;
+}
diff --git a/src/test/java/com/android/tools/r8/naming/b123068484/runner/Runner.java b/src/test/java/com/android/tools/r8/naming/b123068484/runner/Runner.java
new file mode 100644
index 0000000..84062c9
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/naming/b123068484/runner/Runner.java
@@ -0,0 +1,28 @@
+// Copyright (c) 2019, the R8 project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+package com.android.tools.r8.naming.b123068484.runner;
+
+import com.android.tools.r8.naming.b123068484.data.PublicAbs;
+import com.android.tools.r8.naming.b123068484.data.Concrete1;
+import com.android.tools.r8.naming.b123068484.data.Concrete2;
+
+public class Runner {
+  private static int counter = 0;
+  static PublicAbs create(String x) {
+    if (counter++ % 2 == 0) {
+      return new Concrete1(x);
+    } else {
+      return new Concrete2(x);
+    }
+  }
+
+  public static void main(String[] args) {
+    PublicAbs instance = create("Runner");
+    if (instance instanceof Concrete1) {
+      System.out.println(((Concrete1) instance).strField);
+    } else {
+      System.out.println(((Concrete2) instance).strField);
+    }
+  }
+}