Make factValue() reject facts that have keys but no value.

It feels icky to me that anyone would pass null as a magic value, and there are already ways to check that a given key is present.
Technically there's no longer a way to test that a given key has no associated value, but it is hard to imagine that people would get this wrong, write tests for it, and fix the bug as a result. (It hasn't come up in Truth itself, at least.) And we can add support later if needed -- always easier than removing support after the fact.

Also, add tests for facts that have keys without values.

RELNOTES=n/a

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

Author: cpovirk

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

@@ -40,6 +40,11 @@
  * caller tries to access the facts of an error that wasn't produced by Truth.
  */
 final class TruthFailureSubject extends ThrowableSubject {
+  static final Fact HOW_TO_TEST_KEYS_WITHOUT_VALUES =
+      factWithoutValue(
+          "To test that a key is present without a value, "
+              + "use factKeys().contains(...) or a similar method.");
+
   /*
    * TODO(cpovirk): Expose this publicly once it can have the right type. That can't happen until we
    * add type parameters to ThrowableSubject, make TruthFailureSubject not extend ThrowableSubject,
@@ -89,12 +94,19 @@ private static ImmutableList<String> getFactKeys(ErrorWithFacts error) {
   /**
    * Returns a subject for the value with the given name.
    *
-   * <p>The value, if present, is always a string, the {@code String.valueOf} representation of the
-   * value passed to {@link Fact#fact}.
+   * <p>The value is always a string, the {@code String.valueOf} representation of the value passed
+   * to {@link Fact#fact}.
+   *
+   * <p>The value is never null:
    *
-   * <p>The value is null in the case of {@linkplain Fact#factWithoutValue facts that have no
-   * value}. By contrast, facts that have a value that is rendered as "null" (such as those created
-   * with {@code fact("key", null)}) are considered to have a value, the string "null."
+   * <ul>
+   *   <li>In the case of {@linkplain Fact#factWithoutValue facts that have no value}, {@code
+   *       factValue} throws an exception. To test for such facts, use {@link #factKeys()}{@code
+   *       .contains(...)} or a similar method.
+   *   <li>In the case of facts that have a value that is rendered as "null" (such as those created
+   *       with {@code fact("key", null)}), {@code factValue} considers them have a string value,
+   *       the string "null."
+   * </ul>
    *
    * <p>If the failure under test contains more than one fact with the given key, this method will
    * fail the test. To assert about such a failure, use {@linkplain #factValue(String, int) the
@@ -145,9 +157,27 @@ private StringSubject doFactValue(String key, @Nullable Integer index) {
           fact("fact count was", factsWithName.size()));
       return ignoreCheck().that("");
     }
+    String value = factsWithName.get(firstNonNull(index, 0)).value;
+    if (value == null) {
+      if (index == null) {
+        failWithoutActual(
+            factWithoutValue("expected to have a value"),
+            fact("for key", key),
+            factWithoutValue("but the key was present with no value"),
+            HOW_TO_TEST_KEYS_WITHOUT_VALUES);
+      } else {
+        failWithoutActual(
+            factWithoutValue("expected to have a value"),
+            fact("for key", key),
+            fact("and index", index),
+            factWithoutValue("but the key was present with no value"),
+            HOW_TO_TEST_KEYS_WITHOUT_VALUES);
+      }
+      return ignoreCheck().that("");
+    }
     StandardSubjectBuilder check =
         index == null ? check("factValue(%s)", key) : check("factValue(%s, %s)", key, index);
-    return check.that(factsWithName.get(firstNonNull(index, 0)).value);
+    return check.that(value);
   }
 
   private static ImmutableList<Fact> factsWithName(ErrorWithFacts error, String key) {

core/src/test/java/com/google/common/truth/TruthFailureSubjectTest.java

@@ -17,6 +17,8 @@
 package com.google.common.truth;
 
 import static com.google.common.truth.Fact.fact;
+import static com.google.common.truth.Fact.factWithoutValue;
+import static com.google.common.truth.TruthFailureSubject.HOW_TO_TEST_KEYS_WITHOUT_VALUES;
 import static com.google.common.truth.TruthFailureSubject.truthFailures;
 import static org.junit.Assert.fail;
 
@@ -35,6 +37,11 @@ public void factKeys() {
     assertThat(fact("foo", "the foo")).factKeys().containsExactly("foo");
   }
 
+  @Test
+  public void factKeysNoValue() {
+    assertThat(factWithoutValue("foo")).factKeys().containsExactly("foo");
+  }
+
   @Test
   public void factKeysFail() {
     expectFailureWhenTestingThat(fact("foo", "the foo")).factKeys().containsExactly("bar");
@@ -78,6 +85,17 @@ public void factValueFailMultipleKeys() {
     assertFailureValue("but contained multiple", "[foo: the foo, foo: the other foo]");
   }
 
+  @Test
+  public void factValueFailNoValue() {
+    Object unused = expectFailureWhenTestingThat(factWithoutValue("foo")).factValue("foo");
+    assertFailureKeys(
+        "expected to have a value",
+        "for key",
+        "but the key was present with no value",
+        HOW_TO_TEST_KEYS_WITHOUT_VALUES.key);
+    assertFailureValue("for key", "foo");
+  }
+
   // factValue(String, int)
 
   @Test
@@ -127,6 +145,19 @@ public void factValueIntFailNotEnoughWithKey() {
     assertFailureValue("fact count was", "1");
   }
 
+  @Test
+  public void factValueIntFailNoValue() {
+    Object unused = expectFailureWhenTestingThat(factWithoutValue("foo")).factValue("foo", 0);
+    assertFailureKeys(
+        "expected to have a value",
+        "for key",
+        "and index",
+        "but the key was present with no value",
+        HOW_TO_TEST_KEYS_WITHOUT_VALUES.key);
+    assertFailureValue("for key", "foo");
+    assertFailureValue("and index", "0");
+  }
+
   // other tests
 
   @Test