Move default value & converter finding logic to the OptionDefinition class.

Removes some duplicate computation by memoizing the results. Consolidates caching into a single optionDefinition object, instead of having multiple caches that go from the option name to different parts of what defines an option.

Fly-by cleanup of OptionDescription's contents, all contents that are statically defined as part of an option are in OptionDefintion, while expansion data, which depends on the existence of other options, is more clearly stored separately.

Will move the converter-to-option type matching sanity checks to a compile time check in a later change.

RELNOTES: None.
PiperOrigin-RevId: 166912716

5 files changed, 245 insertions, 52 deletions

diff --git a/src/test/java/com/google/devtools/common/options/BUILD b/src/test/java/com/google/devtools/common/options/BUILD
index a0a4e36fd7..84e3178c4f 100644
--- a/src/test/java/com/google/devtools/common/options/BUILD
+++ b/src/test/java/com/google/devtools/common/options/BUILD
@@ -19,6 +19,7 @@ java_test(
         "//third_party:guava-testlib",
         "//third_party:jsr305",
         "//third_party:junit4",
+        "//third_party:mockito",
         "//third_party:truth",
     ],
 )
diff --git a/src/test/java/com/google/devtools/common/options/OptionDefinitionTest.java b/src/test/java/com/google/devtools/common/options/OptionDefinitionTest.java
new file mode 100644
index 0000000000..d369f33639
--- /dev/null
+++ b/src/test/java/com/google/devtools/common/options/OptionDefinitionTest.java
@@ -0,0 +1,231 @@
+// Copyright 2017 The Bazel Authors. All rights reserved.
+//
+// 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.google.devtools.common.options;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import com.google.devtools.common.options.Converters.IntegerConverter;
+import com.google.devtools.common.options.Converters.StringConverter;
+import com.google.devtools.common.options.OptionsParser.ConstructionException;
+import java.util.Map;
+import java.util.Set;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+import org.mockito.Mockito;
+
+/** Tests for {@link OptionDefinition}. */
+@RunWith(JUnit4.class)
+public class OptionDefinitionTest {
+
+  /** Dummy options class, to test various expected failures of the OptionDefinition. */
+  public static class BrokenOptions extends OptionsBase {
+    @Option(
+      name = "missing_its_converter",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.NO_OP},
+      defaultValue = "1"
+    )
+    public Map<String, String> noConverter;
+
+    @Option(
+      name = "multiple_but_not_a_list",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.NO_OP},
+      defaultValue = "null",
+      allowMultiple = true
+    )
+    public String multipleWithNoList;
+
+    @Option(
+      name = "multiple_with_wrong_collection",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.NO_OP},
+      defaultValue = "1",
+      allowMultiple = true
+    )
+    public Set<String> multipleWithSetType;
+
+    @Option(
+      name = "invalid_default",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.NO_OP},
+      defaultValue = "not a number"
+    )
+    public int invalidDefault;
+  }
+
+  @Test
+  public void errorForMissingOptionConverter() throws Exception {
+    OptionDefinition optionDef =
+        OptionDefinition.extractOptionDefinition(BrokenOptions.class.getField("noConverter"));
+    try {
+      optionDef.getConverter();
+      fail("Missing converter should have caused getConverter to fail.");
+    } catch (ConstructionException e) {
+      assertThat(e)
+          .hasMessageThat()
+          .contains(
+              "Option noConverter expects values of type java.util.Map<java.lang.String, "
+                  + "java.lang.String>, but no converter was found; possible fix: "
+                  + "add converter=... to its @Option annotation.");
+    }
+  }
+
+  @Test
+  public void errorForInvalidOptionTypeForRepeatableOptions() throws Exception {
+    OptionDefinition optionDef =
+        OptionDefinition.extractOptionDefinition(
+            BrokenOptions.class.getField("multipleWithNoList"));
+    try {
+      optionDef.getConverter();
+      fail("Mistyped allowMultiple option did not fail getConverter().");
+    } catch (ConstructionException e) {
+      assertThat(e)
+          .hasMessageThat()
+          .contains
+              ("Option multipleWithNoList allows multiple occurrences, so must be of type "
+                  + "List<...>");
+    }
+  }
+
+  @Test
+  public void errorForInvalidCollectionOptionConverter() throws Exception {
+    OptionDefinition optionDef =
+        OptionDefinition.extractOptionDefinition(
+            BrokenOptions.class.getField("multipleWithSetType"));
+    try {
+      optionDef.getConverter();
+      fail("Mistyped allowMultiple option did not fail getConverter().");
+    } catch (ConstructionException e) {
+      assertThat(e)
+          .hasMessageThat()
+          .contains(
+              "Option multipleWithSetType allows multiple occurrences, so must be of type "
+                  + "List<...>");
+    }
+  }
+
+  @Test
+  public void errorForInvalidDefaultValue() throws Exception {
+    OptionDefinition optionDef =
+        OptionDefinition.extractOptionDefinition(BrokenOptions.class.getField("invalidDefault"));
+    try {
+      optionDef.getDefaultValue();
+      fail("Invalid default value parsed without failure.");
+    } catch (ConstructionException e) {
+      assertThat(e)
+          .hasMessageThat()
+          .contains(
+              "OptionsParsingException while retrieving the default value for invalidDefault: "
+                  + "'not a number' is not an int");
+    }
+  }
+
+  /** The rare valid options. */
+  public static class ValidOptionUsingDefaultConverterForMocking extends OptionsBase {
+    @Option(
+      name = "foo",
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.NO_OP},
+      defaultValue = "42"
+    )
+    public int foo;
+
+    @Option(
+      name = "bar",
+      converter = StringConverter.class,
+      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      effectTags = {OptionEffectTag.NO_OP},
+      defaultValue = "strings"
+    )
+    public int bar;
+  }
+
+  /**
+   * Test that the converter and option default values are only computed once and then are obtained
+   * from the stored values, in the case where a default converter is used.
+   */
+  @Test
+  public void optionDefinitionMemoizesDefaultConverterValue() throws Exception {
+    OptionDefinition optionDefinition =
+        OptionDefinition.extractOptionDefinition(
+            ValidOptionUsingDefaultConverterForMocking.class.getField("foo"));
+    OptionDefinition mockOptionDef = Mockito.spy(optionDefinition);
+
+    // Do a bunch of potentially repeat operations on this option that need to know information
+    // about the converter and default value. Also verify that the values are as expected.
+    boolean isBoolean = mockOptionDef.isBooleanField();
+    assertThat(isBoolean).isFalse();
+
+    Converter<?> converter = mockOptionDef.getConverter();
+    assertThat(converter).isInstanceOf(IntegerConverter.class);
+
+    int value = (int) mockOptionDef.getDefaultValue();
+    assertThat(value).isEqualTo(42);
+
+    // Expect reference equality, since we didn't recompute the value
+    Converter<?> secondConverter = mockOptionDef.getConverter();
+    assertThat(secondConverter).isSameAs(converter);
+
+    mockOptionDef.getDefaultValue();
+
+    // Verify that we didn't re-calculate the converter from the provided class object.
+    verify(mockOptionDef, times(1)).getProvidedConverter();
+    // The first call to getDefaultValue checks isSpecialNullDefault, which called
+    // getUnparsedValueDefault as well, but expect no more calls to it after the initial call.
+    verify(mockOptionDef, times(1)).isSpecialNullDefault();
+    verify(mockOptionDef, times(2)).getUnparsedDefaultValue();
+  }
+
+  /**
+   * Test that the converter and option default values are only computed once and then are obtained
+   * from the stored values, in the case where a converter was provided.
+   */
+  @Test
+  public void optionDefinitionMemoizesProvidedConverterValue() throws Exception {
+    OptionDefinition optionDefinition =
+        OptionDefinition.extractOptionDefinition(
+            ValidOptionUsingDefaultConverterForMocking.class.getField("bar"));
+    OptionDefinition mockOptionDef = Mockito.spy(optionDefinition);
+
+    // Do a bunch of potentially repeat operations on this option that need to know information
+    // about the converter and default value. Also verify that the values are as expected.
+    boolean isBoolean = mockOptionDef.isBooleanField();
+    assertThat(isBoolean).isFalse();
+
+    Converter<?> converter = mockOptionDef.getConverter();
+    assertThat(converter).isInstanceOf(StringConverter.class);
+
+    String value = (String) mockOptionDef.getDefaultValue();
+    assertThat(value).isEqualTo("strings");
+
+    // Expect reference equality, since we didn't recompute the value
+    Converter<?> secondConverter = mockOptionDef.getConverter();
+    assertThat(secondConverter).isSameAs(converter);
+
+    mockOptionDef.getDefaultValue();
+
+    // Verify that we didn't re-calculate the converter from the provided class object.
+    verify(mockOptionDef, times(1)).getProvidedConverter();
+    // The first call to getDefaultValue checks isSpecialNullDefault, which called
+    // getUnparsedValueDefault as well, but expect no more calls to it after the initial call.
+    verify(mockOptionDef, times(1)).isSpecialNullDefault();
+    verify(mockOptionDef, times(2)).getUnparsedDefaultValue();
+  }
+}
diff --git a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
index 10e5a527e8..1266d2e25d 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
@@ -250,53 +250,6 @@ public class OptionsDataTest {
     }
   }
 
-  /** Dummy options class. */
-  public static class InvalidOptionConverter extends OptionsBase {
-    @Option(
-      name = "foo",
-      converter = StringConverter.class,
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.NO_OP},
-      defaultValue = "1"
-    )
-    public Integer foo;
-  }
-
-  @Test
-  public void errorForInvalidOptionConverter() throws Exception {
-    try {
-      construct(InvalidOptionConverter.class);
-    } catch (ConstructionException e) {
-      // Expected exception
-      return;
-    }
-    fail();
-  }
-
-  /** Dummy options class. */
-  public static class InvalidListOptionConverter extends OptionsBase {
-    @Option(
-      name = "foo",
-      converter = StringConverter.class,
-      documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
-      effectTags = {OptionEffectTag.NO_OP},
-      defaultValue = "1",
-      allowMultiple = true
-    )
-    public List<Integer> foo;
-  }
-
-  @Test
-  public void errorForInvalidListOptionConverter() throws Exception {
-    try {
-      construct(InvalidListOptionConverter.class);
-    } catch (ConstructionException e) {
-      // Expected exception
-      return;
-    }
-    fail();
-  }
-
   /** Dummy options class using deprecated category. */
   public static class InvalidUndocumentedCategory extends OptionsBase {
     @Option(
diff --git a/src/test/java/com/google/devtools/common/options/OptionsTest.java b/src/test/java/com/google/devtools/common/options/OptionsTest.java
index 32f257b3a0..ac2be6af90 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsTest.java
@@ -491,12 +491,11 @@ public class OptionsTest {
       Options.parse(K.class, NO_ARGS).getOptions();
       fail();
     } catch (OptionsParser.ConstructionException e) {
-      assertThat(e).hasCauseThat().isInstanceOf(IllegalStateException.class);
+      assertThat(e).hasCauseThat().isInstanceOf(OptionsParsingException.class);
       assertThat(e)
-          .hasCauseThat()
           .hasMessageThat()
           .isEqualTo(
-              "OptionsParsingException while retrieving default for "
+              "OptionsParsingException while retrieving the default value for "
                   + "int1: 'null' is not an int");
     }
   }
diff --git a/src/test/shell/integration/execution_phase_tests.sh b/src/test/shell/integration/execution_phase_tests.sh
index 50369831b1..21d6dc8da1 100755
--- a/src/test/shell/integration/execution_phase_tests.sh
+++ b/src/test/shell/integration/execution_phase_tests.sh
@@ -209,13 +209,22 @@ function test_cache_computed_file_digests_ui() {
 }
 
 function test_jobs_default_auto() {
+  # The default flag value is only read if --jobs is not set explicitly.
+  # Do not use a bazelrc here, this would break the test.
+  # TODO(b/65166983) this should be --bazelrc=/dev/null, since this is a bazel
+  # test and we want to encourage bazel-specific naming, but that would
+  # currently break the test because --bazelrc and --blazerc are treated
+  # separately.
   mkdir -p package || fail "mkdir failed"
   echo "cc_library(name = 'foo', srcs = ['foo.cc'])" >package/BUILD
   echo "int foo(void) { return 0; }" >package/foo.cc
 
-  local java_log="$(bazel info output_base 2>/dev/null)/java.log"
+  local output_base="$(bazel --nomaster_bazelrc --blazerc=/dev/null info \
+      output_base 2>/dev/null)" || fail "bazel info should work"
+  local java_log="${output_base}/java.log"
+  bazel --nomaster_bazelrc --blazerc=/dev/null build package:foo \
+      >>"${TEST_log}" 2>&1 || fail "Should build"
 
-  bazel build package:foo >>"${TEST_log}" 2>&1 || fail "Should build"
   assert_last_log "BuildRequest" 'Flag "jobs" was set to "auto"' "${java_log}" \
       "--jobs was not set to auto by default"
 }