Add check(String template, Object... args), and use it to provide con…

…text like...

- "value of: throwable.getCause().getMessage()"
- "multimap was: <{1=[5]}>"

This covers:
- "high-level approach" issues A, A1, E
- "Subject.check()" issues A, B, C

This stops short of letting the existing no-arg check() contribute to a chain, thanks to outstanding questions around the right behavior in some edge cases. Any call to no-arg check() will throw away any "value of" string we've computed so far, and it won't trigger the inclusion of a "root." (Calls to check(String, Object...) elsewhere in the chain may still cause us to include one or both of "value of" and "root.")

This also doesn't introduce any newlines into failure messages.

Internally, the main parts of this are:
- Make our custom ComparisonFailure accept a suffix (for use with "value of"). We need this because ComparisonFailure puts the user-specified message *before* the "expected: ...; but was: ..." text, and we want to put part of ours after. I've also added suffix support to our custom AssertionError. This might be avoidable, but it seems reasonable enough for the two classes to have similar APIs, especially because our GWT method for "give me a ComparisonFailure" actually returns a plain AssertionError.
- In FailureMetadata, track the calls to check() alongside the calls to Subject constructors.
- Make FailureMetadata.Message (currently used by withMessage()) available for use from check().
- Change MultimapSubject and ThrowableSubject to use the new API.

RELNOTES=Added `check(String template, Object... args)`. Most users of `check()` should migrate. If the new method doesn't suit your needs, please [file a bug](https://github.com/google/truth/issues/new).

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=186348792

core/src/main/java/com/google/common/truth/ComparisonFailureWithFields.java

@@ -16,15 +16,13 @@
 
 package com.google.common.truth;
 
+import static com.google.common.truth.Platform.ComparisonFailureMessageStrategy.OMIT_COMPARISON_FAILURE_GENERATED_MESSAGE;
+
 import com.google.common.truth.Platform.PlatformComparisonFailure;
 
 final class ComparisonFailureWithFields extends PlatformComparisonFailure {
-  ComparisonFailureWithFields(String message, String expected, String actual, Throwable cause) {
-    super(message, expected, actual, cause);
-  }
-
-  @Override
-  public String getMessage() {
-    return getMessagePassedToConstructor();
+  ComparisonFailureWithFields(
+      String message, String expected, String actual, String suffix, Throwable cause) {
+    super(message, expected, actual, suffix, cause, OMIT_COMPARISON_FAILURE_GENERATED_MESSAGE);
   }
 }

core/src/main/java/com/google/common/truth/ExpectFailure.java

@@ -89,7 +89,8 @@ public ExpectFailure() {}
   public StandardSubjectBuilder whenTesting() {
     checkState(inRuleContext, "ExpectFailure must be used as a JUnit @Rule");
     if (failure != null) {
-      throw SimpleAssertionError.create("ExpectFailure already captured a failure", failure);
+      throw SimpleAssertionError.create(
+          "ExpectFailure already captured a failure", /*suffix=*/ null, failure);
     }
     if (failureExpected) {
       throw new AssertionError(

core/src/main/java/com/google/common/truth/FailureMetadata.java

@@ -15,11 +15,14 @@
  */
 package com.google.common.truth;
 
-import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.MoreObjects.firstNonNull;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+import static com.google.common.base.Verify.verifyNotNull;
 import static com.google.common.truth.StackTraceCleaner.cleanStackTrace;
+import static com.google.common.truth.StringUtil.format;
 
-import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
 import com.google.common.truth.Truth.SimpleAssertionError;
@@ -48,39 +51,63 @@
 public final class FailureMetadata {
   static FailureMetadata forFailureStrategy(FailureStrategy failureStrategy) {
     return new FailureMetadata(
-        failureStrategy, ImmutableList.<Message>of(), ImmutableList.<Subject<?, ?>>of());
+        failureStrategy, ImmutableList.<LazyMessage>of(), ImmutableList.<Step>of());
   }
 
   private final FailureStrategy strategy;
 
-  /*
-   * TODO(cpovirk): This implementation is wasteful. Probably it doesn't matter, since the APIs that
-   * construct ImmutableLists are used primarily by APIs that are likely to allocate a lot, anyway.
-   * Specifically, `messages` is used by the withMessage() varargs method, and `chain` is used by
-   * chaining assertions like those for throwables and multimaps. But if it ever does matter, we
-   * could use an immutable cactus stack -- or probably even avoid storing most of the chain
-   * entirely (unless we end up wanting more of the chain to show "telescoping context," as in "the
-   * int value of this optional in this list in this multimap").
+  /**
+   * The data from a call to either (a) a {@link Subject} constructor or (b) {@link Subject#check}.
    */
+  private static final class Step {
+    static Step subjectCreation(Subject<?, ?> subject) {
+      return new Step(checkNotNull(subject), null);
+    }
+
+    static Step checkCall(@Nullable Function<String, String> descriptionUpdate) {
+      return new Step(null, descriptionUpdate);
+    }
+
+    /*
+     * We store Subject, rather than the actual value itself, so that we can call actualAsString(),
+     * which lets subjects customize display through actualCustomStringRepresentation(). Why not
+     * call actualAsString() immediately? First, it might be expensive, and second, the Subject
+     * isn't initialized at the time we receive it. We *might* be able to make it safe to call if it
+     * looks only at actual(), but it might try to look at fields initialized by a subclass, which
+     * aren't ready yet.
+     */
+    @Nullable final Subject<?, ?> subject;
 
-  private final ImmutableList<Message> messages;
+    @Nullable final Function<String, String> descriptionUpdate;
+
+    private Step(
+        @Nullable Subject<?, ?> subject, @Nullable Function<String, String> descriptionUpdate) {
+      this.subject = subject;
+      this.descriptionUpdate = descriptionUpdate;
+    }
+
+    boolean isCheckCall() {
+      return subject == null;
+    }
+  }
 
   /*
-   * We store Subject, rather than the actual value itself, so that we can call actualAsString().
-   * Why not call the method immediately? First, it might be expensive, and second, the Subject
-   * isn't initialized at the time we receive it. We *might* be able to make it safe to call if it
-   * looks only at actual(), but it might try to look at fields initialized by a subclass, which
-   * aren't ready yet.
+   * TODO(cpovirk): This implementation is wasteful, especially because `steps` is used even by
+   * non-chaining assertions. If it ever does matter, we could use an immutable cactus stack -- or
+   * probably even avoid storing most of the chain entirely (unless we end up wanting more of the
+   * chain to show "telescoping context," as in "the int value of this optional in this list in this
+   * multimap").
    */
-  private final ImmutableList<Subject<?, ?>> chain;
+
+  private final ImmutableList<LazyMessage> messages;
+
+  private final ImmutableList<Step> steps;
 
   FailureMetadata(
-      FailureStrategy strategy,
-      ImmutableList<Message> messages,
-      ImmutableList<Subject<?, ?>> chain) {
+      FailureStrategy strategy, ImmutableList<LazyMessage> messages, ImmutableList<Step> steps) {
     this.strategy = checkNotNull(strategy);
     this.messages = checkNotNull(messages);
-    this.chain = checkNotNull(chain);
+    this.steps = checkNotNull(steps);
   }
 
   /**
@@ -90,8 +117,19 @@ static FailureMetadata forFailureStrategy(FailureStrategy failureStrategy) {
    * ThrowableSubject#hasMessageThat}.
    */
   FailureMetadata updateForSubject(Subject<?, ?> subject) {
-    ImmutableList<Subject<?, ?>> chain = append(this.chain, subject);
-    return derive(messages, chain);
+    ImmutableList<Step> steps = append(this.steps, Step.subjectCreation(subject));
+    return derive(messages, steps);
+  }
+
+  FailureMetadata updateForCheckCall() {
+    ImmutableList<Step> steps = append(this.steps, Step.checkCall(null));
+    return derive(messages, steps);
+  }
+
+  FailureMetadata updateForCheckCall(Function<String, String> descriptionUpdate) {
+    checkNotNull(descriptionUpdate);
+    ImmutableList<Step> steps = append(this.steps, Step.checkCall(descriptionUpdate));
+    return derive(messages, steps);
   }
 
   /**
@@ -100,50 +138,37 @@ FailureMetadata updateForSubject(Subject<?, ?> subject) {
    * Subject}) or {@link Truth#assertWithMessage} (for most other calls).
    */
   FailureMetadata withMessage(String format, Object[] args) {
-    ImmutableList<Message> messages = append(this.messages, new Message(format, args));
-    return derive(messages, chain);
-  }
-
-  private static final class Message {
-    private static final String PLACEHOLDER_ERR =
-        "Incorrect number of args (%s) for the given placeholders (%s) in string template:\"%s\"";
-
-    private final String format;
-    private final Object[] args;
-
-    Message(@Nullable String format, @Nullable Object... args) {
-      this.format = format;
-      this.args = args;
-      int placeholders = countPlaceholders(format);
-      checkArgument(
-          placeholders == args.length, PLACEHOLDER_ERR, args.length, placeholders, format);
-    }
-
-    @Override
-    public String toString() {
-      return StringUtil.format(format, args);
-    }
+    ImmutableList<LazyMessage> messages = append(this.messages, new LazyMessage(format, args));
+    return derive(messages, steps);
   }
 
   void fail(String message) {
-    doFail(SimpleAssertionError.create(addToMessage(message), rootCause()));
+    doFail(SimpleAssertionError.create(addToMessage(message), rootUnlessThrowable(), rootCause()));
   }
 
   void fail(String message, Throwable cause) {
-    doFail(SimpleAssertionError.create(addToMessage(message), cause));
+    doFail(SimpleAssertionError.create(addToMessage(message), rootUnlessThrowable(), cause));
     // TODO(cpovirk): add rootCause() as a suppressed exception?
   }
 
   void failComparing(String message, CharSequence expected, CharSequence actual) {
     doFail(
         new JUnitComparisonFailure(
-            addToMessage(message), expected.toString(), actual.toString(), rootCause()));
+            addToMessage(message),
+            expected.toString(),
+            actual.toString(),
+            rootUnlessThrowable(),
+            rootCause()));
   }
 
   void failComparing(String message, CharSequence expected, CharSequence actual, Throwable cause) {
     doFail(
         new JUnitComparisonFailure(
-            addToMessage(message), expected.toString(), actual.toString(), cause));
+            addToMessage(message),
+            expected.toString(),
+            actual.toString(),
+            rootUnlessThrowable(),
+            cause));
     // TODO(cpovirk): add rootCause() as a suppressed exception?
   }
 
@@ -153,9 +178,10 @@ private void doFail(AssertionError failure) {
   }
 
   private String addToMessage(String body) {
+    Iterable<?> messages = allPrefixMessages();
     StringBuilder result = new StringBuilder(body.length());
     Joiner.on(": ").appendTo(result, messages);
-    if (!messages.isEmpty()) {
+    if (messages.iterator().hasNext()) {
       if (body.isEmpty()) {
         /*
          * The only likely case of an empty body is with failComparing(). In that case, we still
@@ -176,41 +202,126 @@ private String addToMessage(String body) {
     return result.toString();
   }
 
-  private FailureMetadata derive(
-      ImmutableList<Message> messages, ImmutableList<Subject<?, ?>> chain) {
-    return new FailureMetadata(strategy, messages, chain);
+  private Iterable<?> allPrefixMessages() {
+    String description = description();
+    return (description == null) ? messages : append(messages, "value of: " + description);
+  }
+
+  private FailureMetadata derive(ImmutableList<LazyMessage> messages, ImmutableList<Step> steps) {
+    return new FailureMetadata(strategy, messages, steps);
   }
 
   /**
-   * Returns the first {@link Throwable} in the chain of actual values or {@code null} if none are
-   * present. Typically, we'll have a root cause only if the assertion chain contains a {@link
-   * ThrowableSubject}.
+   * Returns a description of how the final actual value was derived from earlier actual values in
+   * the chain, if the chain ends with at least one derivation that we have a name for.
+   *
+   * <p>We don't want to say: "value of string: expected [foo] but was [bar]" (OK, we might still
+   * decide to say this, but for now, we don't.)
+   *
+   * <p>We do want to say: "value of throwable.getMessage(): expected [foo] but was [bar]"
+   *
+   * <p>To support that, {@code descriptionWasDerived} tracks whether we've been given context
+   * through {@code check} calls <i>that include names</i>.
+   *
+   * <p>If we're missing a naming function halfway through, we have to reset: We don't want to claim
+   * that the value is "foo.bar.baz" when it's "foo.bar.somethingelse.baz." We have to go back to
+   * "object.baz." (But note that {@link #rootUnlessThrowable} will still provide the value of the
+   * root foo to the user as long as we had at least one naming function: We might not know the
+   * root's exact relationship to the final object, but we know it's some object "different enough"
+   * to be worth displaying.)
    */
-  private Throwable rootCause() {
-    for (Subject<?, ?> subject : chain) {
-      if (subject.actual() instanceof Throwable) {
-        return (Throwable) subject.actual();
+  @Nullable
+  private String description() {
+    String description = null;
+    boolean descriptionWasDerived = false;
+    for (Step step : steps) {
+      if (step.isCheckCall()) {
+        checkState(description != null);
+        if (step.descriptionUpdate == null) {
+          description = null;
+          descriptionWasDerived = false;
+        } else {
+          description = verifyNotNull(step.descriptionUpdate.apply(description));
+          descriptionWasDerived = true;
+        }
+        continue;
+      }
+
+      if (description == null) {
+        description =
+            firstNonNull(step.subject.internalCustomName(), step.subject.typeDescription());
       }
     }
-    return null;
+    return descriptionWasDerived ? description : null;
   }
 
-  @VisibleForTesting
-  static int countPlaceholders(@Nullable String template) {
-    if (template == null) {
-      return 0;
+  /**
+   * Returns the root actual value, if we know it's "different enough" from the final actual value.
+   *
+   * <p>We don't want to say: "expected [foo] but was [bar]. string: [bar]"
+   *
+   * <p>We do want to say: "expected [foo] but was [bar]. myObject: MyObject[string=bar, i=0]"
+   *
+   * <p>To support that, {@code seenDerivation} tracks whether we've seen multiple actual values,
+   * which is equivalent to whether we've seen multiple Subject instances or, more informally,
+   * whether the user is making a chained assertion.
+   *
+   * <p>There's one wrinkle: Sometimes chaining doesn't add information. This is often true with
+   * "internal" chaining, like when StreamSubject internally creates an IterableSubject to delegate
+   * to. The two subjects' string representations will be identical (or, in some cases, _almost_
+   * identical), so there is no value in showing both. In such cases, implementations can call the
+   * no-arg {@code check()}, which instructs this method that that particular chain link "doesn't
+   * count." (Note that plenty code calls the no-arg {@code check} even for "real" chaining, since
+   * the {@code check} overload that accepts a name didn't use to exist. Note also that there are
+   * some edge cases that we're not sure how to handle yet, for which we might introduce additional
+   * {@code check}-like methods someday.)
+   */
+  @Nullable
+  private String rootUnlessThrowable() {
+    Step rootSubject = null;
+    boolean seenDerivation = false;
+    for (Step step : steps) {
+      if (step.isCheckCall()) {
+        seenDerivation |= step.descriptionUpdate != null;
+        continue;
+      }
+
+      if (rootSubject == null) {
+        if (step.subject.actual() instanceof Throwable) {
+          /*
+           * We'll already include the Throwable as a cause of the AssertionError (see rootCause()),
+           * so we don't need to include it again in the message.
+           */
+          return null;
+        }
+        rootSubject = step;
+      }
     }
-    int index = 0;
-    int count = 0;
-    while (true) {
-      index = template.indexOf("%s", index);
-      if (index == -1) {
-        break;
+    /*
+     * TODO(cpovirk): Maybe say "root foo: ..." instead of just "foo: ..." if there's more than one
+     * foo in the chain, if the description string doesn't start with "foo," and/or if the name we
+     * have is just "object?"
+     */
+    return seenDerivation
+        ? format(
+            "%s was: %s",
+            rootSubject.subject.typeDescription(), rootSubject.subject.actualAsString())
+        : null;
+  }
+
+  /**
+   * Returns the first {@link Throwable} in the chain of actual values or {@code null} if none are
+   * present. Typically, we'll have a root cause only if the assertion chain contains a {@link
+   * ThrowableSubject}.
+   */
+  @Nullable
+  private Throwable rootCause() {
+    for (Step step : steps) {
+      if (!step.isCheckCall() && step.subject.actual() instanceof Throwable) {
+        return (Throwable) step.subject.actual();
       }
-      index++;
-      count++;
     }
-    return count;
+    return null;
   }
 
   private static <E> ImmutableList<E> append(ImmutableList<? extends E> list, E object) {