Deprecate use of option category to describe documentation level / usage restrictions.

Prevent the old category strings "undocumented," "hidden," or "internal" from being used as categories, to prevent developers from relying on deprecated behavior.

PiperOrigin-RevId: 153525499

6 files changed, 51 insertions, 57 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java
index 84573299af..68ec20e017 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitiveOptionDetails.java
@@ -17,7 +17,6 @@ package com.google.devtools.build.lib.analysis.config;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.common.options.Option;
 import com.google.devtools.common.options.OptionsBase;
-import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParser.OptionUsageRestrictions;
 import java.io.Serializable;
 import java.lang.reflect.Field;
@@ -45,7 +44,7 @@ public final class TransitiveOptionDetails implements Serializable {
         for (Field field : options.getClass().getFields()) {
           if (field.isAnnotationPresent(Option.class)) {
             Option option = field.getAnnotation(Option.class);
-            if (OptionsParser.documentationLevel(option).equals(OptionUsageRestrictions.INTERNAL)) {
+            if (option.optionUsageRestrictions() == OptionUsageRestrictions.INTERNAL) {
               // ignore internal options
               continue;
             }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
index 1cd49e800c..230b095fd8 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.view.config.crosstool.CrosstoolConfig.LipoM
 import com.google.devtools.common.options.Converter;
 import com.google.devtools.common.options.EnumConverter;
 import com.google.devtools.common.options.Option;
+import com.google.devtools.common.options.OptionsParser.OptionUsageRestrictions;
 import com.google.devtools.common.options.OptionsParsingException;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -525,7 +526,7 @@ public class CppOptions extends FragmentOptions {
   @Option(
     name = "lipo configuration state",
     defaultValue = "apply_lipo",
-    category = "internal",
+    optionUsageRestrictions = OptionUsageRestrictions.INTERNAL,
     converter = LipoConfigurationStateConverter.class
   )
   public LipoConfigurationState lipoConfigurationState;
diff --git a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
index 0dc787cefb..9c4c3a671a 100644
--- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
+++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java
@@ -17,6 +17,7 @@ package com.google.devtools.common.options;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Ordering;
+import com.google.devtools.common.options.OptionsParser.ConstructionException;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
@@ -93,6 +94,10 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
    */
   private final ImmutableMap<Field, Boolean> allowMultiple;
 
+  /** These categories used to indicate OptionUsageRestrictions, but no longer. */
+  private static final ImmutableList<String> DEPRECATED_CATEGORIES = ImmutableList.of(
+      "undocumented", "hidden", "internal");
+
   private IsolatedOptionsData(
       Map<Class<? extends OptionsBase>,
       Constructor<?>> optionsClasses,
@@ -183,11 +188,11 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
     if (annotation.allowMultiple()) {
       // If the type isn't a List<T>, this is an error in the option's declaration.
       if (!(fieldType instanceof ParameterizedType)) {
-        throw new AssertionError("Type of multiple occurrence option must be a List<...>");
+        throw new ConstructionException("Type of multiple occurrence option must be a List<...>");
       }
       ParameterizedType pfieldType = (ParameterizedType) fieldType;
       if (pfieldType.getRawType() != List.class) {
-        throw new AssertionError("Type of multiple occurrence option must be a List<...>");
+        throw new ConstructionException("Type of multiple occurrence option must be a List<...>");
       }
       fieldType = pfieldType.getActualTypeArguments()[0];
     }
@@ -234,7 +239,7 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
       Type type = getFieldSingularType(optionField, annotation);
       Converter<?> converter = Converters.DEFAULT_CONVERTERS.get(type);
       if (converter == null) {
-        throw new AssertionError(
+        throw new ConstructionException(
             "No converter found for "
                 + type
                 + "; possible fix: add "
@@ -251,7 +256,7 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
     } catch (Exception e) {
       // This indicates an error in the Converter, and should be discovered the first time it is
       // used.
-      throw new AssertionError(e);
+      throw new ConstructionException(e);
     }
   }
 
@@ -378,7 +383,13 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
         Option annotation = field.getAnnotation(Option.class);
         String optionName = annotation.name();
         if (optionName == null) {
-          throw new AssertionError("Option cannot have a null name");
+          throw new ConstructionException("Option cannot have a null name");
+        }
+
+        if (DEPRECATED_CATEGORIES.contains(annotation.category())) {
+          throw new ConstructionException(
+              "Documentation level is no longer read from the option category. Category \""
+                  + annotation.category() + "\" in option \"" + optionName + "\" is disallowed.");
         }
 
         Type fieldType = getFieldSingularType(field, annotation);
@@ -389,14 +400,14 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
         if (converter == Converter.class) {
           Converter<?> actualConverter = Converters.DEFAULT_CONVERTERS.get(fieldType);
           if (actualConverter == null) {
-            throw new AssertionError("Cannot find converter for field of type "
+            throw new ConstructionException("Cannot find converter for field of type "
                 + field.getType() + " named " + field.getName()
                 + " in class " + field.getDeclaringClass().getName());
           }
           converter = actualConverter.getClass();
         }
         if (Modifier.isAbstract(converter.getModifiers())) {
-          throw new AssertionError("The converter type " + converter
+          throw new ConstructionException("The converter type " + converter
               + " must be a concrete type");
         }
         Type converterResultType;
@@ -404,8 +415,8 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
           Method convertMethod = converter.getMethod("convert", String.class);
           converterResultType = GenericTypeHelper.getActualReturnType(converter, convertMethod);
         } catch (NoSuchMethodException e) {
-          throw new AssertionError("A known converter object doesn't implement the convert"
-              + " method");
+          throw new ConstructionException(
+              "A known converter object doesn't implement the convert method");
         }
 
         if (annotation.allowMultiple()) {
@@ -413,22 +424,33 @@ public class IsolatedOptionsData extends OpaqueOptionsData {
             Type elementType =
                 ((ParameterizedType) converterResultType).getActualTypeArguments()[0];
             if (!GenericTypeHelper.isAssignableFrom(fieldType, elementType)) {
-              throw new AssertionError("If the converter return type of a multiple occurrence " +
-                  "option is a list, then the type of list elements (" + fieldType + ") must be " +
-                  "assignable from the converter list element type (" + elementType + ")");
+              throw new ConstructionException(
+                  "If the converter return type of a multiple occurrence option is a list, then "
+                      + "the type of list elements ("
+                      + fieldType
+                      + ") must be assignable from the converter list element type ("
+                      + elementType
+                      + ")");
             }
           } else {
             if (!GenericTypeHelper.isAssignableFrom(fieldType, converterResultType)) {
-              throw new AssertionError("Type of list elements (" + fieldType +
-                  ") for multiple occurrence option must be assignable from the converter " +
-                  "return type (" + converterResultType + ")");
+              throw new ConstructionException(
+                  "Type of list elements ("
+                      + fieldType
+                      + ") for multiple occurrence option must be assignable from the converter "
+                      + "return type ("
+                      + converterResultType
+                      + ")");
             }
           }
         } else {
           if (!GenericTypeHelper.isAssignableFrom(fieldType, converterResultType)) {
-            throw new AssertionError("Type of field (" + fieldType +
-                ") must be assignable from the converter " +
-                "return type (" + converterResultType + ")");
+            throw new ConstructionException(
+                "Type of field ("
+                    + fieldType
+                    + ") must be assignable from the converter return type ("
+                    + converterResultType
+                    + ")");
           }
         }
 
diff --git a/src/main/java/com/google/devtools/common/options/Option.java b/src/main/java/com/google/devtools/common/options/Option.java
index c43966c971..e040046624 100644
--- a/src/main/java/com/google/devtools/common/options/Option.java
+++ b/src/main/java/com/google/devtools/common/options/Option.java
@@ -88,9 +88,6 @@ public @interface Option {
    */
   String category() default "misc";
 
-  // TODO(b/37353610) the old convention was to include documentation level in the category(),
-  // which is still permitted for backwards compatibility. This field should be used for any new
-  // options, as the old category use will be removed.
   /**
    * Options have multiple uses, some flags, some not. For user-visible flags, they are
    * "documented," but otherwise, there are 3 types of undocumented options.
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParser.java b/src/main/java/com/google/devtools/common/options/OptionsParser.java
index 9574a90162..a871fd18e2 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParser.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParser.java
@@ -444,8 +444,7 @@ public class OptionsParser implements OptionsProvider {
     }
 
     private OptionUsageRestrictions optionUsageRestrictions() {
-      Option option = field.getAnnotation(Option.class);
-      return OptionsParser.documentationLevel(option);
+      return field.getAnnotation(Option.class).optionUsageRestrictions();
     }
 
     public boolean isDocumented() {
@@ -556,12 +555,12 @@ public class OptionsParser implements OptionsProvider {
           if (description == null) {
             description = "Options category '" + category + "'";
           }
-          if (documentationLevel(option) == OptionUsageRestrictions.DOCUMENTED) {
+          if (option.optionUsageRestrictions() == OptionUsageRestrictions.DOCUMENTED) {
             desc.append("\n").append(description).append(":\n");
           }
         }
 
-        if (documentationLevel(option) == OptionUsageRestrictions.DOCUMENTED) {
+        if (option.optionUsageRestrictions() == OptionUsageRestrictions.DOCUMENTED) {
           OptionsUsage.getUsage(optionField, desc, helpVerbosity, impl.getOptionsData());
         }
       }
@@ -594,8 +593,8 @@ public class OptionsParser implements OptionsProvider {
       for (Field optionField : allFields) {
         Option option = optionField.getAnnotation(Option.class);
         String category = option.category();
-        OptionUsageRestrictions level = documentationLevel(option);
-        if (!category.equals(prevCategory) && level == OptionUsageRestrictions.DOCUMENTED) {
+        if (!category.equals(prevCategory)
+            && option.optionUsageRestrictions() == OptionUsageRestrictions.DOCUMENTED) {
           String description = categoryDescriptions.get(category);
           if (description == null) {
             description = "Options category '" + category + "'";
@@ -608,7 +607,7 @@ public class OptionsParser implements OptionsProvider {
           prevCategory = category;
         }
 
-        if (level == OptionUsageRestrictions.DOCUMENTED) {
+        if (option.optionUsageRestrictions() == OptionUsageRestrictions.DOCUMENTED) {
           OptionsUsage.getUsageHtml(optionField, desc, escaper, impl.getOptionsData());
         }
       }
@@ -642,7 +641,7 @@ public class OptionsParser implements OptionsProvider {
     });
     for (Field optionField : allFields) {
       Option option = optionField.getAnnotation(Option.class);
-      if (documentationLevel(option) == OptionUsageRestrictions.DOCUMENTED) {
+      if (option.optionUsageRestrictions() == OptionUsageRestrictions.DOCUMENTED) {
         OptionsUsage.getCompletion(optionField, desc);
       }
     }
@@ -674,30 +673,6 @@ public class OptionsParser implements OptionsProvider {
     return impl.getOptionValueDescription(name);
   }
 
-  @Deprecated
-  // TODO(b/37353610) the old convention was to include documentation level in the category(),
-  // which is still permitted for backwards compatibility. The enum field should be used for any new
-  // options, as the old category, and this function, will be removed.
-  public static OptionUsageRestrictions documentationLevel(Option option) {
-    // Until all options use the new documentationLabel attribute of an option, only rely on it if
-    // it is not set to the default value.
-    if (option.optionUsageRestrictions() != OptionUsageRestrictions.DOCUMENTED) {
-      return option.optionUsageRestrictions();
-    }
-
-    // Otherwise, continue reading from the category.
-    String category = option.category();
-    if ("undocumented".equals(category)) {
-      return OptionUsageRestrictions.UNDOCUMENTED;
-    } else if ("hidden".equals(category)) {
-      return OptionUsageRestrictions.HIDDEN;
-    } else if ("internal".equals(category)) {
-      return OptionUsageRestrictions.INTERNAL;
-    } else {
-      return OptionUsageRestrictions.DOCUMENTED;
-    }
-  }
-
   /**
    * A convenience method, equivalent to
    * {@code parse(OptionPriority.COMMAND_LINE, null, Arrays.asList(args))}.
diff --git a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
index 5c635fb524..2d442a14fa 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -674,7 +674,7 @@ class OptionsParserImpl {
     Option option = field == null ? null : field.getAnnotation(Option.class);
 
     if (option == null
-        || OptionsParser.documentationLevel(option) == OptionUsageRestrictions.INTERNAL) {
+        || option.optionUsageRestrictions() == OptionUsageRestrictions.INTERNAL) {
       // This also covers internal options, which are treated as if they did not exist.
       throw new OptionsParsingException("Unrecognized option: " + arg, arg);
     }