Enhance AddsToRuleKey

Summary:
This allows annotating an Immutable with AddsToRuleKey. This
Immutable can then annotate methods with AddToRuleKey. When
constructing the rulekey, the annotated methods will be called and the return
value will be added to the rulekey.

AddToRuleKey can only be used on methods within an Immutable.

This primarily meant as a method to move Immutables off of the
deprecated RuleKeyAppendable.

Test Plan: CI

Reviewed By: jkeljo

fbshipit-source-id: 10120ed

src/com/facebook/buck/android/apkmodule/AbstractAPKModule.java

@@ -15,14 +15,15 @@
  */
 package com.facebook.buck.android.apkmodule;
 
-import com.facebook.buck.rules.RuleKeyAppendable;
-import com.facebook.buck.rules.RuleKeyObjectSink;
+import com.facebook.buck.rules.AddToRuleKey;
+import com.facebook.buck.rules.AddsToRuleKey;
 import com.facebook.buck.util.immutables.BuckStyleTuple;
 import org.immutables.value.Value;
 
 @Value.Immutable
 @BuckStyleTuple
-abstract class AbstractAPKModule implements RuleKeyAppendable, Comparable<AbstractAPKModule> {
+abstract class AbstractAPKModule implements Comparable<AbstractAPKModule>, AddsToRuleKey {
+  @AddToRuleKey
   public abstract String getName();
 
   @Value.Derived
@@ -39,11 +40,6 @@ public String getCanaryClassName() {
     }
   }
 
-  @Override
-  public void appendToRuleKey(RuleKeyObjectSink sink) {
-    sink.setReflectively("name", getName());
-  }
-
   @Override
   public int compareTo(AbstractAPKModule o) {
     if (this == o) {

src/com/facebook/buck/jvm/java/AbstractJavaOptions.java

@@ -16,20 +16,15 @@
 
 package com.facebook.buck.jvm.java;
 
-import com.facebook.buck.rules.RuleKeyAppendable;
-import com.facebook.buck.rules.RuleKeyObjectSink;
+import com.facebook.buck.rules.AddToRuleKey;
+import com.facebook.buck.rules.AddsToRuleKey;
 import com.facebook.buck.rules.Tool;
 import com.facebook.buck.util.immutables.BuckStyleTuple;
 import org.immutables.value.Value;
 
 @Value.Immutable
 @BuckStyleTuple
-abstract class AbstractJavaOptions implements RuleKeyAppendable {
-
+abstract class AbstractJavaOptions implements AddsToRuleKey {
+  @AddToRuleKey
   public abstract Tool getJavaRuntimeLauncher();
-
-  @Override
-  public void appendToRuleKey(RuleKeyObjectSink sink) {
-    sink.setReflectively("java", getJavaRuntimeLauncher());
-  }
 }

src/com/facebook/buck/jvm/java/AbstractJavacOptions.java

@@ -17,9 +17,9 @@
 package com.facebook.buck.jvm.java;
 
 import com.facebook.buck.io.filesystem.ProjectFilesystem;
+import com.facebook.buck.rules.AddToRuleKey;
+import com.facebook.buck.rules.AddsToRuleKey;
 import com.facebook.buck.rules.BuildContext;
-import com.facebook.buck.rules.RuleKeyAppendable;
-import com.facebook.buck.rules.RuleKeyObjectSink;
 import com.facebook.buck.rules.SourcePathResolver;
 import com.facebook.buck.util.immutables.BuckStyleImmutable;
 import com.google.common.annotations.VisibleForTesting;
@@ -47,7 +47,7 @@
  */
 @Value.Immutable
 @BuckStyleImmutable
-abstract class AbstractJavacOptions implements RuleKeyAppendable {
+abstract class AbstractJavacOptions implements AddsToRuleKey {
 
   // Default combined source and target level.
   public static final String TARGETED_JAVA_VERSION = "7";
@@ -56,7 +56,8 @@ abstract class AbstractJavacOptions implements RuleKeyAppendable {
   public enum SpoolMode {
     /**
      * Writes the compiler output directly to a .jar file while retaining the intermediate .class
-     * files in memory. If {@link com.facebook.buck.jvm.java.JavaLibraryDescription.Arg}
+     * files in memory. If {@link
+     * com.facebook.buck.jvm.java.JavaLibraryDescription.AbstractJavaLibraryDescriptionArg}
      * postprocessClassesCommands are present, the builder will resort to writing .class files to
      * disk by necessity.
      */
@@ -70,6 +71,7 @@ public enum SpoolMode {
   }
 
   @Value.Default
+  @AddToRuleKey
   protected SpoolMode getSpoolMode() {
     return SpoolMode.INTERMEDIATE_TO_DISK;
   }
@@ -85,39 +87,49 @@ protected boolean isVerbose() {
   }
 
   @Value.Default
+  @AddToRuleKey
   public String getSourceLevel() {
     return TARGETED_JAVA_VERSION;
   }
 
   @VisibleForTesting
   @Value.Default
+  @AddToRuleKey
   public String getTargetLevel() {
     return TARGETED_JAVA_VERSION;
   }
 
   @Value.Default
+  @AddToRuleKey
   public AnnotationProcessingParams getAnnotationProcessingParams() {
     return AnnotationProcessingParams.EMPTY;
   }
 
+  // TODO(cjhopman): Should this be added to the rulekey?
   public abstract Set<String> getSafeAnnotationProcessors();
 
+  @AddToRuleKey
   public abstract List<String> getExtraArguments();
 
+  @AddToRuleKey
   protected abstract Optional<String> getBootclasspath();
 
+  // TODO(cjhopman): Should this be added to the rulekey?
   protected abstract Map<String, String> getSourceToBootclasspath();
 
+  @AddToRuleKey
   protected boolean isDebug() {
     return !isProductionBuild();
   }
 
   @Value.Default
+  @AddToRuleKey
   public boolean trackClassUsage() {
     return false;
   }
 
   @Value.Default
+  @AddToRuleKey
   public AbiGenerationMode getAbiGenerationMode() {
     return AbiGenerationMode.CLASS;
   }
@@ -231,19 +243,6 @@ public void appendOptionsTo(
     optionsConsumer.addExtras(getExtraArguments());
   }
 
-  @Override
-  public void appendToRuleKey(RuleKeyObjectSink sink) {
-    sink.setReflectively("sourceLevel", getSourceLevel())
-        .setReflectively("targetLevel", getTargetLevel())
-        .setReflectively("extraArguments", Joiner.on(',').join(getExtraArguments()))
-        .setReflectively("debug", isDebug())
-        .setReflectively("bootclasspath", getBootclasspath())
-        .setReflectively("annotationProcessingParams", getAnnotationProcessingParams())
-        .setReflectively("spoolMode", getSpoolMode())
-        .setReflectively("trackClassUsage", trackClassUsage())
-        .setReflectively("abiGenerationMode", getAbiGenerationMode());
-  }
-
   static JavacOptions.Builder builderForUseInJavaBuckConfig() {
     return JavacOptions.builder();
   }

src/com/facebook/buck/rules/AddToRuleKey.java

@@ -17,13 +17,14 @@
 package com.facebook.buck.rules;
 
 import static java.lang.annotation.ElementType.FIELD;
+import static java.lang.annotation.ElementType.METHOD;
 import static java.lang.annotation.RetentionPolicy.RUNTIME;
 
 import java.lang.annotation.Retention;
 import java.lang.annotation.Target;
 
 @Retention(RUNTIME)
-@Target(FIELD)
+@Target({FIELD, METHOD})
 public @interface AddToRuleKey {
   boolean stringify() default false;
 }

src/com/facebook/buck/rules/AddsToRuleKey.java

@@ -16,8 +16,13 @@
 
 package com.facebook.buck.rules;
 
+import org.immutables.value.Value;
+
 /**
- * Identifies a class that uses {@link com.facebook.buck.rules.AddToRuleKey} annotations to indicate
- * fields that should be added to rule keys.
+ * Identifies a class that uses {@link AddToRuleKey} annotations to indicate fields that should be
+ * added to rule keys.
+ *
+ * <p>{@link Value.Immutable} annotated classes can use {@link AddToRuleKey} on methods to indicate
+ * that the method's return value should be added to rule keys.
  */
 public interface AddsToRuleKey {}

src/com/facebook/buck/rules/keys/ReflectiveAlterKeyLoader.java

@@ -17,12 +17,23 @@
 package com.facebook.buck.rules.keys;
 
 import com.facebook.buck.rules.AddToRuleKey;
+import com.facebook.buck.rules.AddsToRuleKey;
+import com.facebook.buck.util.immutables.BuckStyleImmutable;
+import com.facebook.buck.util.immutables.BuckStyleTuple;
+import com.google.common.base.Preconditions;
 import com.google.common.cache.CacheLoader;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSortedMap;
 import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Comparator;
+import java.util.LinkedHashSet;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Queue;
 
 class ReflectiveAlterKeyLoader extends CacheLoader<Class<?>, ImmutableCollection<AlterRuleKey>> {
 
@@ -36,24 +47,65 @@ class ReflectiveAlterKeyLoader extends CacheLoader<Class<?>, ImmutableCollection
   @Override
   public ImmutableCollection<AlterRuleKey> load(Class<?> key) throws Exception {
     ImmutableList.Builder<AlterRuleKey> builder = ImmutableList.builder();
+    List<Class<?>> superClasses = new ArrayList<>();
+
+    // Collect the superclasses first so that they are added before interfaces. That seems more
+    // aesthetically pleasing to me.
     for (Class<?> current = key; !Object.class.equals(current); current = current.getSuperclass()) {
+      superClasses.add(current);
+    }
+
+    LinkedHashSet<Class<?>> superClassesAndInterfaces = new LinkedHashSet<>();
+    Queue<Class<?>> workQueue = new LinkedList<>();
+    workQueue.addAll(superClasses);
+    while (!workQueue.isEmpty()) {
+      Class<?> cls = workQueue.poll();
+      if (superClassesAndInterfaces.add(cls)) {
+        workQueue.addAll(Arrays.asList(cls.getInterfaces()));
+      }
+    }
+
+    for (Class<?> current : superClassesAndInterfaces) {
       ImmutableSortedMap.Builder<ValueExtractor, AlterRuleKey> sortedExtractors =
           ImmutableSortedMap.orderedBy(COMPARATOR);
       for (final Field field : current.getDeclaredFields()) {
         field.setAccessible(true);
         final AddToRuleKey annotation = field.getAnnotation(AddToRuleKey.class);
         if (annotation != null) {
           ValueExtractor valueExtractor = new FieldValueExtractor(field);
-          sortedExtractors.put(valueExtractor, createAlterRuleKey(valueExtractor, annotation));
+          sortedExtractors.put(
+              valueExtractor, createAlterRuleKey(valueExtractor, annotation.stringify()));
+        }
+      }
+      for (final Method method : current.getDeclaredMethods()) {
+        method.setAccessible(true);
+        final AddToRuleKey annotation = method.getAnnotation(AddToRuleKey.class);
+        if (annotation != null) {
+          Preconditions.checkState(
+              hasImmutableAnnotation(current) && AddsToRuleKey.class.isAssignableFrom(current),
+              "AddToRuleKey can only be applied to methods of Immutables. It cannot be applied to %s.%s(...)",
+              current.getName(),
+              method.getName());
+
+          ValueExtractor valueExtractor = new ValueMethodValueExtractor(method);
+          sortedExtractors.put(
+              valueExtractor, createAlterRuleKey(valueExtractor, annotation.stringify()));
         }
       }
       builder.addAll(sortedExtractors.build().values());
     }
     return builder.build();
   }
 
-  private AlterRuleKey createAlterRuleKey(ValueExtractor valueExtractor, AddToRuleKey annotation) {
-    if (annotation.stringify()) {
+  private boolean hasImmutableAnnotation(Class<?> current) {
+    // Value.Immutable only has CLASS retention, so we need to detect this based on our own
+    // annotations.
+    return current.getAnnotation(BuckStyleImmutable.class) != null
+        || current.getAnnotation(BuckStyleTuple.class) != null;
+  }
+
+  private AlterRuleKey createAlterRuleKey(ValueExtractor valueExtractor, boolean stringify) {
+    if (stringify) {
       return new StringifyAlterRuleKey(valueExtractor);
     } else {
       return new DefaultAlterRuleKey(valueExtractor);

src/com/facebook/buck/rules/keys/ValueMethodValueExtractor.java

@@ -0,0 +1,55 @@
+/*
+ * Copyright 2016-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License. You may obtain
+ * a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.facebook.buck.rules.keys;
+
+import com.google.common.base.Preconditions;
+import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
+import javax.annotation.Nullable;
+
+/** Extracts a value of a given field, that is assumed to be accessible. */
+public class ValueMethodValueExtractor implements ValueExtractor {
+  private final Method method;
+
+  public ValueMethodValueExtractor(Method method) {
+    Preconditions.checkArgument(method.getParameterCount() == 0);
+    Preconditions.checkArgument(!method.getReturnType().equals(Void.class));
+    // TODO(cjhopman): Should this do any other verification of the signature/annotations on the
+    // method?
+    this.method = method;
+  }
+
+  @Override
+  public String getFullyQualifiedName() {
+    return method.getDeclaringClass() + "." + method.getName();
+  }
+
+  @Override
+  public String getName() {
+    return method.getName();
+  }
+
+  @Override
+  @Nullable
+  public Object getValue(Object obj) {
+    try {
+      return method.invoke(obj);
+    } catch (IllegalAccessException | InvocationTargetException e) {
+      throw new RuntimeException(e);
+    }
+  }
+}

src/com/facebook/buck/util/immutables/BuckStyleTuple.java

@@ -42,5 +42,5 @@
   additionalJsonAnnotations = {JsonNaming.class}
 )
 @Target({ElementType.TYPE, ElementType.PACKAGE, ElementType.ANNOTATION_TYPE})
-@Retention(RetentionPolicy.SOURCE)
+@Retention(RetentionPolicy.RUNTIME)
 public @interface BuckStyleTuple {}