Clean up SkyFunction interface method docs

Specifies function return values. Replaces old "builder" terminology. Removes bad advice.

--
MOS_MIGRATED_REVID=90417023

1 files changed, 100 insertions, 65 deletions

diff --git a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java
index 324c03d8eb..58bb846487 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SkyFunction.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.skyframe;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.events.EventHandler;
 
 import java.util.Map;
@@ -23,51 +24,58 @@ import javax.annotation.Nullable;
 /**
  * Machinery to evaluate a single value.
  *
- * <p>The builder is supposed to access only direct dependencies of the value. However, the direct
- * dependencies need not be known in advance. The builder can request arbitrary values using
- * {@link Environment#getValue}. If the values are not ready, the call will return null; in that
- * case the builder can either try to proceed (and potentially indicate more dependencies by
- * additional {@code getValue} calls), or just return null, in which case the missing dependencies
- * will be computed and the builder will be started again.
- */
+ * <p>The SkyFunction {@link #compute} implementation is supposed to access only direct
+ * dependencies of the value. However, the direct dependencies need not be known in advance. The
+ * implementation can request arbitrary values using {@link Environment#getValue}. If the values
+ * are not ready, the call will return {@code null}; in that case the implementation should just
+ * return {@code null}, in which case the missing dependencies will be computed and the {@link
+ * #compute} method will be started again.
+ * */
 public interface SkyFunction {
 
   /**
-   * When a value is requested, this method is called with the name of the value and a value
-   * building environment.
+   * When a value is requested, this method is called with the name of the value and a
+   * dependency-tracking environment.
    *
-   * <p>This method should return a constructed value, or null if any dependencies were missing
-   * ({@link Environment#valuesMissing} was true before returning). In that case the missing
-   * dependencies will be computed and the value builder restarted.
+   * <p>This method should return a non-{@code null} value, or {@code null} if any dependencies
+   * were missing ({@link Environment#valuesMissing} was true before returning). In that case the
+   * missing dependencies will be computed and the {@code compute} method called again.
    *
-   * <p>Implementations must be threadsafe and reentrant.
+   * <p>This method should throw if it fails, or if one of its dependencies fails with an
+   * exception and this method cannot recover. If one of its dependencies fails and this method can
+   * enrich the exception with additional context, then this method should catch that exception and
+   * throw another containing that additional context. If it has no such additional context, then
+   * it should allow its dependency's exception to be thrown through it.
    *
    * @throws SkyFunctionException on failure
-   * @throws InterruptedException when the user interrupts the build
+   * @throws InterruptedException if interrupted
    */
-  @Nullable SkyValue compute(SkyKey skyKey, Environment env) throws SkyFunctionException,
-      InterruptedException;
+  @ThreadSafe
+  @Nullable
+  SkyValue compute(SkyKey skyKey, Environment env)
+      throws SkyFunctionException, InterruptedException;
 
   /**
-   * Extracts a tag (target label) from a SkyKey if it has one. Otherwise return null.
+   * Extracts a tag (target label) from a SkyKey if it has one. Otherwise return {@code null}.
    *
-   * <p>The tag is used for filtering out non-error event messages that do not match --output_filter
-   * flag. If a SkyFunction returns null in this method it means that all the info/warning messages
-   * associated with this value will be shown, no matter what --output_filter says.
+   * <p>The tag is used for filtering out non-error event messages that do not match
+   * --output_filter flag. If a SkyFunction returns {@code null} in this method it means that all
+   * the info/warning messages associated with this value will be shown, no matter what
+   * --output_filter says.
    */
   @Nullable
   String extractTag(SkyKey skyKey);
 
   /**
-   * The services provided to the value builder by the graph implementation.
+   * The services provided to the {@link SkyFunction} implementation by the graph implementation.
    */
   interface Environment {
     /**
      * Returns a direct dependency. If the specified value is not in the set of already evaluated
-     * direct dependencies, returns null. Also returns null if the specified value has already been
-     * evaluated and found to be in error.
+     * direct dependencies, returns {@code null}. Also returns {@code null} if the specified
+     * value has already been evaluated and found to be in error.
      *
-     * <p>On a subsequent build, if any of this value's dependencies have changed they will be
+     * <p>On a subsequent evaluation, if any of this value's dependencies have changed they will be
      * re-evaluated in the same order as originally requested by the {@code SkyFunction} using
      * this {@code getValue} call (see {@link #getValues} for when preserving the order is not
      * important).
@@ -77,12 +85,13 @@ public interface SkyFunction {
 
     /**
      * Returns a direct dependency. If the specified value is not in the set of already evaluated
-     * direct dependencies, returns null. If the specified value has already been evaluated and
-     * found to be in error, throws the exception coming from the error. Value builders may
-     * use this method to continue evaluation even if one of their children is in error by catching
-     * the thrown exception and proceeding. The caller must specify the exception that might be
-     * thrown using the {@code exceptionClass} argument. If the child's exception is not an instance
-     * of {@code exceptionClass}, returns null without throwing.
+     * direct dependencies, returns {@code null}. If the specified value has already been
+     * evaluated and found to be in error, throws the exception coming from the error, so long as
+     * the exception is of one of the specified types. SkyFunction implementations may use this
+     * method to continue evaluation even if one of their dependencies is in error by catching
+     * the thrown exception and proceeding. The caller must specify the exception type(s) that
+     * might be thrown using the {@code exceptionClass} argument(s). If the dependency's
+     * exception is not an instance of {@code exceptionClass}, {@code null} is returned.
      *
      * <p>The exception class given cannot be a supertype or a subtype of {@link RuntimeException},
      * or a subtype of {@link InterruptedException}. See
@@ -104,62 +113,71 @@ public interface SkyFunction {
             throws E1, E2, E3, E4;
 
     /**
-     * Returns true iff any of the past {@link #getValue}(s) or {@link #getValueOrThrow} method
-     * calls for this instance returned null (because the value was not yet present and done in the
-     * graph).
-     *
-     * <p>If this returns true, the {@link SkyFunction} must return {@code null}.
-     */
-    boolean valuesMissing();
-
-    /**
      * Requests {@code depKeys} "in parallel", independent of each others' values. These keys may be
      * thought of as a "dependency group" -- they are requested together by this value.
      *
      * <p>In general, if the result of one getValue call can affect the argument of a later getValue
      * call, the two calls cannot be merged into a single getValues call, since the result of the
-     * first call might change on a later build. Inversely, if the result of one getValue call
+     * first call might change on a later evaluation. Inversely, if the result of one getValue call
      * cannot affect the parameters of the next getValue call, the two keys can form a dependency
      * group and the two getValue calls merged into one getValues call.
      *
-     * <p>This means that on subsequent builds, when checking to see if a value requires rebuilding,
-     * all the values in this group may be simultaneously checked. A SkyFunction should request a
-     * dependency group if checking the deps serially on a subsequent build would take too long, and
-     * if the builder would request all deps anyway as long as no earlier deps had changed.
-     * SkyFunction.Environment implementations may also choose to request these deps in
-     * parallel on the first build, potentially speeding up the build.
+     * <p>This means that on subsequent evaluations, when checking to see if dependencies require
+     * re-evaluation, all the values in this group may be simultaneously checked. A SkyFunction
+     * should request a dependency group if checking the deps serially on a subsequent evaluation
+     * would take too long, and if the {@link #compute} method would request all deps anyway as
+     * long as no earlier deps had changed. SkyFunction.Environment implementations may also
+     * choose to request these deps in parallel on the first evaluation, potentially speeding it up.
      *
-     * <p>While re-evaluating every value in the group may take longer than re-evaluating just the
-     * first one and finding that it has changed, no extra work is done: the contract of the
-     * dependency group means that the builder, when called to rebuild this value, will request all
-     * values in the group again anyway, so they would have to have been built in any case.
+     * <p>While re-evaluating every value in the group may take longer than re-evaluating just
+     * the first one and finding that it has changed, no extra work is done: the contract of the
+     * dependency group means that the {@link #compute} method, when called to re-evaluate this
+     * value, will request all values in the group again anyway, so they would have to have been
+     * built in any case.
      *
      * <p>Example of when to use getValues: A ListProcessor value is built with key inputListRef.
-     * The builder first calls getValue(InputList.key(inputListRef)), and retrieves inputList. It
-     * then iterates through inputList, calling getValue on each input. Finally, it processes the
-     * whole list and returns. Say inputList is (a, b, c). Since the builder will unconditionally
-     * call getValue(a), getValue(b), and getValue(c), the builder can instead just call
-     * getValues({a, b, c}). If the value is later dirtied the evaluator will build a, b, and c in
-     * parallel (assuming the inputList value was unchanged), and re-evaluate the ListProcessor
-     * value only if at least one of them was changed. On the other hand, if the InputList changes
-     * to be (a, b, d), then the evaluator will see that the first dep has changed, and call the
-     * builder to rebuild from scratch, without considering the dep group of {a, b, c}.
+     * The {@link #compute} method first calls getValue(InputList.key(inputListRef)), and
+     * retrieves inputList. It then iterates through inputList, calling getValue on each input.
+     * Finally, it processes the whole list and returns. Say inputList is (a, b, c). Since the
+     * {@link #compute} method will unconditionally call getValue(a), getValue(b), and getValue
+     * (c), the {@link #compute} method can instead just call getValues({a, b, c}). If the value
+     * is later dirtied the evaluator will evaluate a, b, and c in parallel (assuming the inputList
+     * value was unchanged), and re-evaluate the ListProcessor value only if at least one of them
+     * was changed. On the other hand, if the InputList changes to be (a, b, d), then the
+     * evaluator will see that the first dep has changed, and call the {@link #compute} method to
+     * re-evaluate from scratch, without considering the dep group of {a, b, c}.
      *
      * <p>Example of when not to use getValues: A BestMatch value is built with key
-     * &lt;potentialMatchesRef, matchCriterion&gt;. The builder first calls
+     * &lt;potentialMatchesRef, matchCriterion&gt;. The {@link #compute} method first calls
      * getValue(PotentialMatches.key(potentialMatchesRef) and retrieves potentialMatches. It then
      * iterates through potentialMatches, calling getValue on each potential match until it finds
      * one that satisfies matchCriterion. In this case, if potentialMatches is (a, b, c), it would
      * be <i>incorrect</i> to call getValues({a, b, c}), because it is not known yet whether
      * requesting b or c will be necessary -- if a matches, then we will never call b or c.
+     *
+     * <p>Returns a map, {@code m}. For all {@code k} in {@code depKeys}, {@code m.containsKey(k)}
+     * is {@code true}, and, {@code m.get(k) != null} iff the dependency was already evaluated and
+     * was not in error.
      */
     Map<SkyKey, SkyValue> getValues(Iterable<SkyKey> depKeys);
 
     /**
-     * The same as {@link #getValues} but the returned objects may throw when attempting to retrieve
-     * their value. Note that even if the requested values can throw different kinds of exceptions,
-     * only exceptions of type {@code E} will be preserved in the returned objects. All others will
-     * be null.
+     * Similar to {@link #getValues} but allows the caller to specify a set of types that are
+     * proper subtypes of Exception (see {@link SkyFunctionException} for more details) to find
+     * out whether any of the dependencies' evaluations resulted in exceptions of those types.
+     * The returned objects may throw when attempting to retrieve their value.
+     *
+     * <p>Callers should prioritize their responsibility to detect and handle errors in the
+     * returned map over their responsibility to return {@code null} if values are missing. This
+     * is because in nokeep_going evaluations, an error from a low level dependency is given a
+     * chance to be enriched by its reverse-dependencies, if possible.
+     *
+     * <p>Returns a map, {@code m}. For all {@code k} in {@code depKeys}, {@code m.get(k) !=
+     * null}. For all {@code v} such that there is some {@code k} such that {@code m.get(k) ==
+     * v}, the following is true: {@code v.get() != null} iff the dependency {@code k} was
+     * already evaluated and was not in error. {@code v.get()} throws {@code E} iff the
+     * dependency {@code k} was already evaluated with an error in the specified set of {@link
+     * Exception} types.
      */
     <E extends Exception> Map<SkyKey, ValueOrException<E>> getValuesOrThrow(
         Iterable<SkyKey> depKeys, Class<E> exceptionClass);
@@ -175,8 +193,25 @@ public interface SkyFunction {
         Class<E4> exceptionClass4);
 
     /**
+     * Returns whether there was a previous getValue[s][OrThrow] that indicated a missing
+     * dependency. Formally, returns true iff at least one of the following occurred:
+     *
+     * <ul>
+     *   <li>getValue[OrThrow](k[, c]) returned {@code null} for some k</li>
+     *   <li>getValues(ks).get(k) == {@code null} for some ks and k such that ks.contains(k)</li>
+     *   <li>
+     *     getValuesOrThrow(ks, c).get(k).get() == {@code null} for some ks and k such that
+     *     ks.contains(k)
+     *   </li>
+     * </ul>
+     *
+     * <p>If this returns true, the {@link SkyFunction} must return {@code null}.
+     */
+    boolean valuesMissing();
+
+    /**
      * Returns the {@link EventHandler} that a SkyFunction should use to print any errors,
-     * warnings, or progress messages while building.
+     * warnings, or progress messages during execution of {@link SkyFunction#compute}.
      */
     EventHandler getListener();