Version 1.3.44

Merge: Sanitize quasi-trivial phis in re-processing IR code.
CL: https://r8-review.googlesource.com/c/r8/+/31240/

Merge: Remove the associated Kotlin Metadata if a class is renamed.
CL: https://r8-review.googlesource.com/c/r8/+/30840/

Bug: 119414448, 119626580
Change-Id: Ief02aff8bd8f1a046536da75cd88c680b7321ddc
diff --git a/src/main/java/com/android/tools/r8/Version.java b/src/main/java/com/android/tools/r8/Version.java
index 57a282f..0c63c7b 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.3.43";
+  public static final String LABEL = "1.3.44";
 
   private Version() {
   }
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/lambda/LambdaMerger.java b/src/main/java/com/android/tools/r8/ir/optimize/lambda/LambdaMerger.java
index d1b56f2..4c9b82a 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/lambda/LambdaMerger.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/lambda/LambdaMerger.java
@@ -304,9 +304,11 @@
             .sorted(DexEncodedMethod::slowCompare)
             .collect(Collectors.toList());
     for (DexEncodedMethod method : methods) {
-      converter.processMethod(method, feedback,
+      DexEncodedMethod mappedMethod =
+          converter.getGraphLense().mapDexEncodedMethod(converter.appInfo, method);
+      converter.processMethod(mappedMethod, feedback,
           x -> false, CallSiteInformation.empty(), Outliner::noProcessing);
-      assert method.isProcessed();
+      assert mappedMethod.isProcessed();
     }
   }
 
diff --git a/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java b/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java
index 18a9920..79f18ed 100644
--- a/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java
+++ b/src/main/java/com/android/tools/r8/ir/optimize/staticizer/StaticizingProcessor.java
@@ -20,6 +20,7 @@
 import com.android.tools.r8.ir.code.InvokeMethodWithReceiver;
 import com.android.tools.r8.ir.code.InvokeStatic;
 import com.android.tools.r8.ir.code.MemberType;
+import com.android.tools.r8.ir.code.Phi;
 import com.android.tools.r8.ir.code.StaticGet;
 import com.android.tools.r8.ir.code.StaticPut;
 import com.android.tools.r8.ir.code.Value;
@@ -35,6 +36,7 @@
 import com.google.common.collect.Streams;
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.IdentityHashMap;
 import java.util.Iterator;
 import java.util.List;
@@ -238,7 +240,7 @@
   }
 
   private void removeReferencesToThis(DexEncodedMethod method, IRCode code) {
-    fixupStaticizedValueUsers(code, code.getThis());
+    fixupStaticizedThisUsers(code, code.getThis());
   }
 
   private void rewriteReferences(DexEncodedMethod method, IRCode code) {
@@ -251,11 +253,12 @@
             .collect(Collectors.toList());
 
     singletonFieldReads.forEach(read -> {
-      CandidateInfo candidateInfo = singletonFields.get(read.getField());
+      DexField field = read.getField();
+      CandidateInfo candidateInfo = singletonFields.get(field);
       assert candidateInfo != null;
       Value value = read.dest();
       if (value != null) {
-        fixupStaticizedValueUsers(code, value);
+        fixupStaticizedFieldReadUsers(code, value, field);
       }
       if (!candidateInfo.preserveRead.get()) {
         read.removeOrReplaceByDebugLocalRead();
@@ -267,12 +270,116 @@
     }
   }
 
-  // Fixup value usages: rewrites all method calls so that they point to static methods.
-  private void fixupStaticizedValueUsers(IRCode code, Value thisValue) {
+  // Fixup `this` usages: rewrites all method calls so that they point to static methods.
+  private void fixupStaticizedThisUsers(IRCode code, Value thisValue) {
     assert thisValue != null;
     assert thisValue.numberOfPhiUsers() == 0;
 
-    for (Instruction user : thisValue.uniqueUsers()) {
+    fixupStaticizedValueUsers(code, thisValue.uniqueUsers());
+
+    assert thisValue.numberOfUsers() == 0;
+  }
+
+  // Re-processing finalized code may create slightly different IR code than what the examining
+  // phase has seen. For example,
+  //
+  //  b1:
+  //    s1 <- static-get singleton
+  //    ...
+  //    invoke-virtual { s1, ... } mtd1
+  //    goto Exit
+  //  b2:
+  //    s2 <- static-get singleoton
+  //    ...
+  //    invoke-virtual { s2, ... } mtd1
+  //    goto Exit
+  //  ...
+  //  Exit: ...
+  //
+  // ~>
+  //
+  //  b1:
+  //    s1 <- static-get singleton
+  //    ...
+  //    goto Exit
+  //  b2:
+  //    s2 <- static-get singleton
+  //    ...
+  //    goto Exit
+  //  Exit:
+  //    sp <- phi(s1, s2)
+  //    invoke-virtual { sp, ... } mtd1
+  //    ...
+  //
+  // From staticizer's viewpoint, `sp` is trivial in the sense that it is composed of values that
+  // refer to the same singleton field. If so, we can safely relax the assertion; remove uses of
+  // field reads; remove quasi-trivial phis; and then remove original field reads.
+  private boolean testAndcollectPhisComposedOfSameFieldRead(
+      Set<Phi> phisToCheck, DexField field, Set<Phi> trivialPhis) {
+    for (Phi phi : phisToCheck) {
+      Set<Phi> chainedPhis = Sets.newIdentityHashSet();
+      for (Value operand : phi.getOperands()) {
+        if (operand.isPhi()) {
+          chainedPhis.add(operand.asPhi());
+        } else {
+          if (!operand.definition.isStaticGet()) {
+            return false;
+          }
+          if (operand.definition.asStaticGet().getField() != field) {
+            return false;
+          }
+        }
+      }
+      if (!chainedPhis.isEmpty()) {
+        if (!testAndcollectPhisComposedOfSameFieldRead(chainedPhis, field, trivialPhis)) {
+          return false;
+        }
+      }
+      trivialPhis.add(phi);
+    }
+    return true;
+  }
+
+  // Fixup field read usages. Same as {@link #fixupStaticizedThisUsers} except this one is handling
+  // quasi-trivial phis that might be introduced while re-processing finalized code.
+  private void fixupStaticizedFieldReadUsers(IRCode code, Value dest, DexField field) {
+    assert dest != null;
+    // During the examine phase, field reads with any phi users have been invalidated, hence zero.
+    // However, it may be not true if re-processing introduces phis after optimizing common suffix.
+    Set<Phi> trivialPhis = Sets.newIdentityHashSet();
+    boolean hasTrivialPhis =
+        testAndcollectPhisComposedOfSameFieldRead(dest.uniquePhiUsers(), field, trivialPhis);
+    assert dest.numberOfPhiUsers() == 0 || hasTrivialPhis;
+    Set<Instruction> users = new HashSet<>(dest.uniqueUsers());
+    // If that is the case, method calls we want to fix up include users of those phis.
+    if (hasTrivialPhis) {
+      for (Phi phi : trivialPhis) {
+        users.addAll(phi.uniqueUsers());
+      }
+    }
+
+    fixupStaticizedValueUsers(code, users);
+
+    if (hasTrivialPhis) {
+      // We can't directly use Phi#removeTrivialPhi because they still refer to different operands.
+      for (Phi phi : trivialPhis) {
+        // First, make sure phi users are indeed uses of field reads and removed via fixup.
+        assert phi.numberOfUsers() == 0;
+        // Then, manually clean up this from all of the operands.
+        for (Value operand : phi.getOperands()) {
+          operand.removePhiUser(phi);
+        }
+        // And remove it from the containing block.
+        phi.getBlock().removePhi(phi);
+      }
+    }
+
+    // No matter what, number of phi users should be zero too.
+    assert dest.numberOfUsers() == 0 && dest.numberOfPhiUsers() == 0;
+  }
+
+  private void fixupStaticizedValueUsers(IRCode code, Set<Instruction> users) {
+    for (Instruction user : users) {
       assert user.isInvokeVirtual() || user.isInvokeDirect();
       InvokeMethodWithReceiver invoke = user.asInvokeMethodWithReceiver();
       Value newValue = null;
@@ -288,8 +395,6 @@
       invoke.replace(new InvokeStatic(
           invoke.getInvokedMethod(), newValue, args.subList(1, args.size())));
     }
-
-    assert thisValue.numberOfUsers() == 0;
   }
 
   private void remapMovedCandidates(IRCode code) {
diff --git a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
index 37e6de9..ecf7158 100644
--- a/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
+++ b/src/main/java/com/android/tools/r8/naming/ClassNameMinifier.java
@@ -124,6 +124,7 @@
     timing.begin("rename-classes");
     for (DexClass clazz : classes) {
       if (!renaming.containsKey(clazz.type)) {
+        clazz.annotations = clazz.annotations.keepIf(this::isNotKotlinMetadata);
         DexString renamed = computeName(clazz.type);
         renaming.put(clazz.type, renamed);
       }
@@ -581,4 +582,8 @@
     }
     return packagePrefix.substring(0, i);
   }
+
+  private boolean isNotKotlinMetadata(DexAnnotation annotation) {
+    return annotation.annotation.type != appInfo.dexItemFactory.kotlin.metadata.kotlinMetadataType;
+  }
 }
diff --git a/src/test/java/com/android/tools/r8/ToolHelper.java b/src/test/java/com/android/tools/r8/ToolHelper.java
index e185b33..c1ba2c5 100644
--- a/src/test/java/com/android/tools/r8/ToolHelper.java
+++ b/src/test/java/com/android/tools/r8/ToolHelper.java
@@ -99,6 +99,7 @@
 
   public static final String JAVA_8_RUNTIME = "third_party/openjdk/openjdk-rt-1.8/rt.jar";
   public static final String KT_RUNTIME = "third_party/kotlin/kotlinc/lib/kotlin-runtime.jar";
+  public static final String KT_REFLECT = "third_party/kotlin/kotlinc/lib/kotlin-reflect.jar";
   private static final String ANDROID_JAR_PATTERN = "third_party/android_jar/lib-v%d/android.jar";
   private static final AndroidApiLevel DEFAULT_MIN_SDK = AndroidApiLevel.I;
 
@@ -573,6 +574,12 @@
     return path;
   }
 
+  public static Path getKotlinReflectJar() {
+    Path path = Paths.get(KT_REFLECT);
+    assert Files.exists(path) : "Expected kotlin reflect jar";
+    return path;
+  }
+
   public static Path getJdwpTestsCfJarPath(AndroidApiLevel minSdk) {
     if (minSdk.getLevel() >= AndroidApiLevel.N.getLevel()) {
       return Paths.get("third_party", "jdwp-tests", "apache-harmony-jdwp-tests-host.jar");
diff --git a/src/test/java/com/android/tools/r8/kotlin/MetadataStripTest.java b/src/test/java/com/android/tools/r8/kotlin/MetadataStripTest.java
new file mode 100644
index 0000000..c762aa8
--- /dev/null
+++ b/src/test/java/com/android/tools/r8/kotlin/MetadataStripTest.java
@@ -0,0 +1,111 @@
+// Copyright (c) 2018, 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.kotlin;
+
+import static com.android.tools.r8.utils.codeinspector.Matchers.isPresent;
+import static com.android.tools.r8.utils.codeinspector.Matchers.isRenamed;
+import static org.hamcrest.CoreMatchers.not;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assume.assumeTrue;
+
+import com.android.tools.r8.TestBase;
+import com.android.tools.r8.TestRunResult;
+import com.android.tools.r8.ToolHelper;
+import com.android.tools.r8.ToolHelper.KotlinTargetVersion;
+import com.android.tools.r8.graph.DexAnnotation;
+import com.android.tools.r8.graph.DexClass;
+import com.android.tools.r8.utils.FileUtils;
+import com.android.tools.r8.utils.InternalOptions;
+import com.android.tools.r8.utils.codeinspector.ClassSubject;
+import com.android.tools.r8.utils.codeinspector.CodeInspector;
+import com.google.common.collect.ImmutableList;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Collection;
+import java.util.function.Consumer;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+
+@RunWith(Parameterized.class)
+public class MetadataStripTest extends TestBase {
+  private static final String METADATA_DESCRIPTOR = "Lkotlin/Metadata;";
+  private static final String KEEP_ANNOTATIONS = "-keepattributes *Annotation*";
+
+  private final Backend backend;
+  private final KotlinTargetVersion targetVersion;
+
+  private Consumer<InternalOptions> optionsModifier =
+      o -> {
+        o.enableTreeShaking = true;
+        o.enableMinification = true;
+      };
+
+  @Parameterized.Parameters(name = "Backend: {0} target: {1}")
+  public static Collection<Object[]> data() {
+    ImmutableList.Builder<Object[]> builder = new ImmutableList.Builder<>();
+    for (Backend backend : Backend.values()) {
+      for (KotlinTargetVersion targetVersion : KotlinTargetVersion.values()) {
+        builder.add(new Object[]{backend, targetVersion});
+      }
+    }
+    return builder.build();
+  }
+
+  public MetadataStripTest(Backend backend, KotlinTargetVersion targetVersion) {
+    this.backend = backend;
+    this.targetVersion = targetVersion;
+  }
+
+  @Test
+  public void testJstyleRunnable() throws Exception {
+    assumeTrue("Non-trivial changes between 1.3 and master.", backend != Backend.CF);
+    final String folder = "lambdas_jstyle_runnable";
+    final String mainClassName = "lambdas_jstyle_runnable.MainKt";
+    final String implementer1ClassName = "lambdas_jstyle_runnable.Implementer1Kt";
+    TestRunResult result = testForR8(backend)
+        .addProgramFiles(getKotlinJarFile(folder))
+        .addProgramFiles(getJavaJarFile(folder))
+        .addProgramFiles(ToolHelper.getKotlinReflectJar())
+        .enableProguardTestOptions()
+        .enableInliningAnnotations()
+        .addKeepMainRule(mainClassName)
+        .addKeepRules(KEEP_ANNOTATIONS)
+        .addKeepRules("-keep class kotlin.Metadata")
+        .addOptionsModification(optionsModifier)
+        .run(mainClassName);
+    CodeInspector inspector = result.inspector();
+    ClassSubject clazz = inspector.clazz(mainClassName);
+    assertThat(clazz, isPresent());
+    assertThat(clazz, not(isRenamed()));
+    // Main class is kept, hence the presence of Metadata.
+    assertNotNull(retrieveMetadata(clazz.getDexClass()));
+    ClassSubject impl1 = inspector.clazz(implementer1ClassName);
+    assertThat(impl1, isPresent());
+    assertThat(impl1, isRenamed());
+    // All other classes can be renamed, hence the absence of Metadata;
+    assertNull(retrieveMetadata(impl1.getDexClass()));
+  }
+
+  private DexAnnotation retrieveMetadata(DexClass dexClass) {
+    for (DexAnnotation annotation : dexClass.annotations.annotations) {
+      if (annotation.annotation.type.toDescriptorString().equals(METADATA_DESCRIPTOR)) {
+        return annotation;
+      }
+    }
+    return null;
+  }
+
+  private Path getKotlinJarFile(String folder) {
+    return Paths.get(ToolHelper.TESTS_BUILD_DIR, "kotlinR8TestResources",
+        targetVersion.getFolderName(), folder + FileUtils.JAR_EXTENSION);
+  }
+
+  private Path getJavaJarFile(String folder) {
+    return Paths.get(ToolHelper.TESTS_BUILD_DIR, "kotlinR8TestResources",
+        targetVersion.getFolderName(), folder + ".java" + FileUtils.JAR_EXTENSION);
+  }
+}