Merge remote-tracking branch 'aosp/upstream-master' into master am: 11217a9366 am: e61b9d7d2f am: bf6212f47a

am: eafd445ea0

Change-Id: Ic95e2af4882d544e7b9fff7c8807d7bf0d66b7a3

4 files changed, 157 insertions, 54 deletions

diff --git a/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java b/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java
index 732f064..c42062a 100644
--- a/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java
+++ b/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java
@@ -18,7 +18,10 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 
 import com.google.common.collect.ImmutableList;
+import java.util.Comparator;
 import java.util.HashSet;
+import java.util.Set;
+import java.util.TreeSet;
 import org.objectweb.asm.ClassReader;
 import org.objectweb.asm.ClassVisitor;
 import org.objectweb.asm.MethodVisitor;
@@ -33,18 +36,20 @@ public class DefaultMethodClassFixer extends ClassVisitor {
 
   private final ClassReaderFactory classpath;
   private final ClassReaderFactory bootclasspath;
+  private final ClassLoader targetLoader;
   private final HashSet<String> instanceMethods = new HashSet<>();
-  private final HashSet<String> seenInterfaces = new HashSet<>();
 
   private boolean isInterface;
-  private ImmutableList<String> interfaces;
+  private String internalName;
+  private ImmutableList<String> directInterfaces;
   private String superName;
 
   public DefaultMethodClassFixer(ClassVisitor dest, ClassReaderFactory classpath,
-      ClassReaderFactory bootclasspath) {
+      ClassReaderFactory bootclasspath, ClassLoader targetLoader) {
     super(Opcodes.ASM5, dest);
     this.classpath = classpath;
     this.bootclasspath = bootclasspath;
+    this.targetLoader = targetLoader;
   }
 
   @Override
@@ -55,22 +60,23 @@ public class DefaultMethodClassFixer extends ClassVisitor {
       String signature,
       String superName,
       String[] interfaces) {
-    checkState(this.interfaces == null);
+    checkState(this.directInterfaces == null);
     isInterface = BitFlags.isSet(access, Opcodes.ACC_INTERFACE);
+    internalName = name;
     checkArgument(superName != null || "java/lang/Object".equals(name), // ASM promises this
         "Type without superclass: %s", name);
-    this.interfaces = ImmutableList.copyOf(interfaces);
+    this.directInterfaces = ImmutableList.copyOf(interfaces);
     this.superName = superName;
     super.visit(version, access, name, signature, superName, interfaces);
   }
 
   @Override
   public void visitEnd() {
-    if (!isInterface && defaultMethodsDefined(interfaces)) {
+    if (!isInterface && defaultMethodsDefined(directInterfaces)) {
       // Inherited methods take precedence over default methods, so visit all superclasses and
       // figure out what methods they declare before stubbing in any missing default methods.
       recordInheritedMethods();
-      stubMissingDefaultMethods(interfaces);
+      stubMissingDefaultMethods();
     }
     super.visitEnd();
   }
@@ -85,6 +91,51 @@ public class DefaultMethodClassFixer extends ClassVisitor {
     return super.visitMethod(access, name, desc, signature, exceptions);
   }
 
+  private void stubMissingDefaultMethods() {
+    TreeSet<Class<?>> allInterfaces = new TreeSet<>(InterfaceComparator.INSTANCE);
+    for (String direct : directInterfaces) {
+      // Loading ensures all transitively implemented interfaces can be loaded, which is necessary
+      // to produce correct default method stubs in all cases.  We could do without classloading but
+      // it's convenient to rely on Class.isAssignableFrom to compute subtype relationships, and
+      // we'd still have to insist that all transitively implemented interfaces can be loaded.
+      // We don't load the visited class, however, in case it's a generated lambda class.
+      Class<?> itf = loadFromInternal(direct);
+      collectInterfaces(itf, allInterfaces);
+    }
+
+    Class<?> superclass = loadFromInternal(superName);
+    for (Class<?> interfaceToVisit : allInterfaces) {
+      // if J extends I, J is allowed to redefine I's default methods.  The comparator we used
+      // above makes sure we visit J before I in that case so we can use J's definition.
+      if (superclass != null && interfaceToVisit.isAssignableFrom(superclass)) {
+        // superclass already implements this interface, so we must skip it.  The superclass will
+        // be similarly rewritten or comes from the bootclasspath; either way we don't need to and
+        // shouldn't stub default methods for this interface.
+        continue;
+      }
+      stubMissingDefaultMethods(interfaceToVisit.getName().replace('.', '/'));
+    }
+  }
+
+  private Class<?> loadFromInternal(String internalName) {
+    try {
+      return targetLoader.loadClass(internalName.replace('/', '.'));
+    } catch (ClassNotFoundException e) {
+      throw new IllegalStateException(
+          "Couldn't load " + internalName + ", is the classpath complete?", e);
+    }
+  }
+
+  private void collectInterfaces(Class<?> itf, Set<Class<?>> dest) {
+    checkArgument(itf.isInterface());
+    if (!dest.add(itf)) {
+      return;
+    }
+    for (Class<?> implemented : itf.getInterfaces()) {
+      collectInterfaces(implemented, dest);
+    }
+  }
+
   private void recordInheritedMethods() {
     InstanceMethodRecorder recorder = new InstanceMethodRecorder();
     String internalName = superName;
@@ -145,29 +196,23 @@ public class DefaultMethodClassFixer extends ClassVisitor {
         && !instanceMethods.contains(name + ":" + desc);
   }
 
-  private void stubMissingDefaultMethods(ImmutableList<String> interfaces) {
-    for (String implemented : interfaces) {
-      if (!seenInterfaces.add(implemented)) {
-        // Skip: a superclass already implements this interface, or we've seen it here
-        continue;
-      }
-      ClassReader bytecode = classpath.readIfKnown(implemented);
-      if (bytecode != null && !bootclasspath.isKnown(implemented)) {
-        // Class in classpath and bootclasspath is a bad idea but in any event, assume the
-        // bootclasspath will take precedence like in a classloader.
-        // We can skip code attributes as we just need to find default methods to stub.
-        bytecode.accept(new DefaultMethodStubber(), ClassReader.SKIP_CODE | ClassReader.SKIP_DEBUG);
-      }
+  private void stubMissingDefaultMethods(String implemented) {
+    if (bootclasspath.isKnown(implemented)) {
+      // Default methods on the bootclasspath will be available at runtime, so just ignore them.
+      return;
     }
+    ClassReader bytecode = checkNotNull(classpath.readIfKnown(implemented),
+        "Couldn't find interface %s implemented by %s", implemented, internalName);
+    // We can skip code attributes as we just need to find default methods to stub.
+    bytecode.accept(new DefaultMethodStubber(), ClassReader.SKIP_CODE | ClassReader.SKIP_DEBUG);
   }
 
   /**
    * Visitor for interfaces that produces delegates in the class visited by the outer
    * {@link DefaultMethodClassFixer} for every default method encountered.
    */
-  public class DefaultMethodStubber extends ClassVisitor {
+  private class DefaultMethodStubber extends ClassVisitor {
 
-    @SuppressWarnings("hiding") private ImmutableList<String> interfaces;
     private String interfaceName;
 
     public DefaultMethodStubber() {
@@ -183,20 +228,20 @@ public class DefaultMethodClassFixer extends ClassVisitor {
         String superName,
         String[] interfaces) {
       checkArgument(BitFlags.isSet(access, Opcodes.ACC_INTERFACE));
-      checkState(this.interfaces == null);
-      this.interfaces = ImmutableList.copyOf(interfaces);
+      checkState(interfaceName == null);
       interfaceName = name;
     }
 
     @Override
-    public void visitEnd() {
-      stubMissingDefaultMethods(this.interfaces);
-    }
-
-    @Override
     public MethodVisitor visitMethod(
         int access, String name, String desc, String signature, String[] exceptions) {
       if (shouldStub(access, name, desc)) {
+        // Remember we stubbed this method in case it's also defined by subsequently visited
+        // interfaces.  javac would force the method to be defined explicitly if there any two
+        // definitions conflict, but see stubMissingDefaultMethods() for how we deal with default
+        // methods redefined in interfaces extending another.
+        recordIfInstanceMethod(access, name, desc);
+
         // Add this method to the class we're desugaring and stub in a body to call the default
         // implementation in the interface's companion class. ijar omits these methods when setting
         // ACC_SYNTHETIC modifier, so don't.
@@ -230,7 +275,7 @@ public class DefaultMethodClassFixer extends ClassVisitor {
   /**
    * Visitor for interfaces that recursively searches interfaces for default method declarations.
    */
-  public class DefaultMethodFinder extends ClassVisitor {
+  private class DefaultMethodFinder extends ClassVisitor {
 
     @SuppressWarnings("hiding") private ImmutableList<String> interfaces;
     private boolean found;
@@ -290,13 +335,6 @@ public class DefaultMethodClassFixer extends ClassVisitor {
         String superName,
         String[] interfaces) {
       checkArgument(BitFlags.noneSet(access, Opcodes.ACC_INTERFACE));
-      for (String inheritedInterface : interfaces) {
-        // No point copying default methods that we'll also copy for a superclass.  Note we may
-        // be processing a class in the bootclasspath, in which case the interfaces must also
-        // be in the bootclasspath and we can skip those as well.  Also note this is best-effort,
-        // since these interfaces may extend other interfaces that we're not recording here.
-        seenInterfaces.add(inheritedInterface);
-      }
       super.visit(version, access, name, signature, superName, interfaces);
     }
 
@@ -307,4 +345,26 @@ public class DefaultMethodClassFixer extends ClassVisitor {
       return null;
     }
   }
+
+  /** Comparator for interfaces that compares by whether interfaces extend one another. */
+  enum InterfaceComparator implements Comparator<Class<?>> {
+    INSTANCE;
+
+    @Override
+    public int compare(Class<?> o1, Class<?> o2) {
+      checkArgument(o1.isInterface());
+      checkArgument(o2.isInterface());
+      if (o1 == o2) {
+        return 0;
+      }
+      if (o1.isAssignableFrom(o2)) { // o1 is supertype of o2
+        return 1; // we want o1 to come after o2
+      }
+      if (o2.isAssignableFrom(o1)) { // o2 is supertype of o1
+        return -1; // we want o2 to come after o1
+      }
+      // o1 and o2 aren't comparable so arbitrarily impose lexicographical ordering
+      return o1.getName().compareTo(o2.getName());
+    }
+  }
 }
diff --git a/java/com/google/devtools/build/android/desugar/Desugar.java b/java/com/google/devtools/build/android/desugar/Desugar.java
index ca7032e..b4b4c8e 100644
--- a/java/com/google/devtools/build/android/desugar/Desugar.java
+++ b/java/com/google/devtools/build/android/desugar/Desugar.java
@@ -42,9 +42,11 @@ import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
+import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 import javax.annotation.Nullable;
 import org.objectweb.asm.ClassReader;
@@ -171,6 +173,16 @@ class Desugar {
     public boolean desugarTryWithResourcesIfNeeded;
 
     @Option(
+      name = "desugar_try_with_resources_omit_runtime_classes",
+      defaultValue = "false",
+      category = "misc",
+      help =
+          "Omits the runtime classes necessary to support try-with-resources from the output. "
+              + "This property has effect only if --desugar_try_with_resources_if_needed is used."
+    )
+    public boolean desugarTryWithResourcesOmitRuntimeClasses;
+
+    @Option(
       name = "copy_bridges_from_classpath",
       defaultValue = "false",
       category = "misc",
@@ -193,6 +205,7 @@ class Desugar {
   private final CoreLibraryRewriter rewriter;
   private final LambdaClassMaker lambdas;
   private final GeneratedClassStore store;
+  private final Set<String> visitedExceptionTypes = new HashSet<>();
   /** The counter to record the times of try-with-resources desugaring is invoked. */
   private final AtomicInteger numOfTryWithResourcesInvoked = new AtomicInteger();
 
@@ -306,7 +319,9 @@ class Desugar {
   }
 
   private void copyThrowableExtensionClass(OutputFileProvider outputFileProvider) {
-    if (!outputJava7 || !options.desugarTryWithResourcesIfNeeded) {
+    if (!outputJava7
+        || !options.desugarTryWithResourcesIfNeeded
+        || options.desugarTryWithResourcesOmitRuntimeClasses) {
       // try-with-resources statements are okay in the output jar.
       return;
     }
@@ -441,10 +456,13 @@ class Desugar {
       // null ClassReaderFactory b/c we don't expect to need it for lambda classes
       visitor = new Java7Compatibility(visitor, (ClassReaderFactory) null);
       if (options.desugarTryWithResourcesIfNeeded) {
-        visitor = new TryWithResourcesRewriter(visitor, loader, numOfTryWithResourcesInvoked);
+        visitor =
+            new TryWithResourcesRewriter(
+                visitor, loader, visitedExceptionTypes, numOfTryWithResourcesInvoked);
       }
       if (options.desugarInterfaceMethodBodiesIfNeeded) {
-        visitor = new DefaultMethodClassFixer(visitor, classpathReader, bootclasspathReader);
+        visitor =
+            new DefaultMethodClassFixer(visitor, classpathReader, bootclasspathReader, loader);
         visitor = new InterfaceDesugaring(visitor, bootclasspathReader, store);
       }
     }
@@ -487,10 +505,13 @@ class Desugar {
       if (outputJava7) {
         visitor = new Java7Compatibility(visitor, classpathReader);
         if (options.desugarTryWithResourcesIfNeeded) {
-          visitor = new TryWithResourcesRewriter(visitor, loader, numOfTryWithResourcesInvoked);
+          visitor =
+              new TryWithResourcesRewriter(
+                  visitor, loader, visitedExceptionTypes, numOfTryWithResourcesInvoked);
         }
         if (options.desugarInterfaceMethodBodiesIfNeeded) {
-          visitor = new DefaultMethodClassFixer(visitor, classpathReader, bootclasspathReader);
+          visitor =
+              new DefaultMethodClassFixer(visitor, classpathReader, bootclasspathReader, loader);
           visitor = new InterfaceDesugaring(visitor, bootclasspathReader, store);
         }
       }
diff --git a/java/com/google/devtools/build/android/desugar/TryWithResourcesRewriter.java b/java/com/google/devtools/build/android/desugar/TryWithResourcesRewriter.java
index cde223e..38255a1 100644
--- a/java/com/google/devtools/build/android/desugar/TryWithResourcesRewriter.java
+++ b/java/com/google/devtools/build/android/desugar/TryWithResourcesRewriter.java
@@ -13,6 +13,7 @@
 // limitations under the License.
 package com.google.devtools.build.android.desugar;
 
+import static com.google.common.base.Preconditions.checkNotNull;
 import static org.objectweb.asm.Opcodes.ASM5;
 import static org.objectweb.asm.Opcodes.INVOKESTATIC;
 import static org.objectweb.asm.Opcodes.INVOKEVIRTUAL;
@@ -22,8 +23,11 @@ import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableMultimap;
 import com.google.common.collect.ImmutableSet;
+import java.util.Collections;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicInteger;
 import org.objectweb.asm.ClassVisitor;
+import org.objectweb.asm.Label;
 import org.objectweb.asm.MethodVisitor;
 
 /**
@@ -83,7 +87,7 @@ public class TryWithResourcesRewriter extends ClassVisitor {
           .build();
 
   private final ClassLoader classLoader;
-
+  private final Set<String> visitedExceptionTypes;
   private final AtomicInteger numOfTryWithResourcesInvoked;
   /**
    * Indicate whether the current class being desugared should be ignored. If the current class is
@@ -94,9 +98,11 @@ public class TryWithResourcesRewriter extends ClassVisitor {
   public TryWithResourcesRewriter(
       ClassVisitor classVisitor,
       ClassLoader classLoader,
+      Set<String> visitedExceptionTypes,
       AtomicInteger numOfTryWithResourcesInvoked) {
     super(ASM5, classVisitor);
     this.classLoader = classLoader;
+    this.visitedExceptionTypes = visitedExceptionTypes;
     this.numOfTryWithResourcesInvoked = numOfTryWithResourcesInvoked;
   }
 
@@ -112,23 +118,38 @@ public class TryWithResourcesRewriter extends ClassVisitor {
     shouldCurrentClassBeIgnored = THROWABLE_EXT_CLASS_INTERNAL_NAMES.contains(name);
   }
 
-
   @Override
   public MethodVisitor visitMethod(
       int access, String name, String desc, String signature, String[] exceptions) {
+    if (exceptions != null && exceptions.length > 0) {
+      // collect exception types.
+      Collections.addAll(visitedExceptionTypes, exceptions);
+    }
     MethodVisitor visitor = super.cv.visitMethod(access, name, desc, signature, exceptions);
     return visitor == null || shouldCurrentClassBeIgnored
         ? visitor
-        : new TryWithResourceVisitor(visitor, classLoader);
+        : new TryWithResourceVisitor(name + desc, visitor, classLoader);
   }
 
   private class TryWithResourceVisitor extends MethodVisitor {
 
     private final ClassLoader classLoader;
+    /** For debugging purpose. Enrich exception information. */
+    private final String methodSignature;
 
-    public TryWithResourceVisitor(MethodVisitor methodVisitor, ClassLoader classLoader) {
+    public TryWithResourceVisitor(
+        String methodSignature, MethodVisitor methodVisitor, ClassLoader classLoader) {
       super(ASM5, methodVisitor);
       this.classLoader = classLoader;
+      this.methodSignature = methodSignature;
+    }
+
+    @Override
+    public void visitTryCatchBlock(Label start, Label end, Label handler, String type) {
+      if (type != null) {
+        visitedExceptionTypes.add(type); // type in a try-catch block must extend Throwable.
+      }
+      super.visitTryCatchBlock(start, end, handler, type);
     }
 
     @Override
@@ -138,6 +159,7 @@ public class TryWithResourcesRewriter extends ClassVisitor {
         return;
       }
       numOfTryWithResourcesInvoked.incrementAndGet();
+      visitedExceptionTypes.add(checkNotNull(owner)); // owner extends Throwable.
       super.visitMethodInsn(
           INVOKESTATIC, THROWABLE_EXTENSION_INTERNAL_NAME, name, METHOD_DESC_MAP.get(desc), false);
     }
@@ -149,15 +171,16 @@ public class TryWithResourcesRewriter extends ClassVisitor {
       if (!TARGET_METHODS.containsEntry(name, desc)) {
         return false;
       }
-      if (owner.equals("java/lang/Throwable")) {
-        return true; // early return, for performance.
+      if (visitedExceptionTypes.contains(owner)) {
+        return true; // The owner is an exception that has been visited before.
       }
       try {
         Class<?> throwableClass = classLoader.loadClass("java.lang.Throwable");
         Class<?> klass = classLoader.loadClass(owner.replace('/', '.'));
         return throwableClass.isAssignableFrom(klass);
       } catch (ClassNotFoundException e) {
-        throw new AssertionError(e);
+        throw new AssertionError(
+            "Failed to load class when desugaring method " + methodSignature, e);
       }
     }
   }
diff --git a/java/com/google/devtools/common/options/OptionsParser.java b/java/com/google/devtools/common/options/OptionsParser.java
index c6bd002..728c490 100644
--- a/java/com/google/devtools/common/options/OptionsParser.java
+++ b/java/com/google/devtools/common/options/OptionsParser.java
@@ -553,15 +553,14 @@ public class OptionsParser implements OptionsProvider {
       for (Field optionField : allFields) {
         Option option = optionField.getAnnotation(Option.class);
         String category = option.category();
-        if (!category.equals(prevCategory)) {
-          prevCategory = category;
+        if (!category.equals(prevCategory)
+            && option.optionUsageRestrictions() == OptionUsageRestrictions.DOCUMENTED) {
           String description = categoryDescriptions.get(category);
           if (description == null) {
             description = "Options category '" + category + "'";
           }
-          if (option.optionUsageRestrictions() == OptionUsageRestrictions.DOCUMENTED) {
-            desc.append("\n").append(description).append(":\n");
-          }
+          desc.append("\n").append(description).append(":\n");
+          prevCategory = category;
         }
 
         if (option.optionUsageRestrictions() == OptionUsageRestrictions.DOCUMENTED) {