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
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 8457329..68ec20e 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 @@
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 @@
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 1cd49e8..230b095 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.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 @@
@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 0dc787c..9c4c3a6 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 @@
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 @@
*/
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 @@
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 @@
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 @@
} 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 @@
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 @@
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 @@
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 @@
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 c43966c..e040046 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 @@
*/
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 9574a90..a871fd1 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 @@
}
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 @@
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 @@
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 @@
prevCategory = category;
}
- if (level == OptionUsageRestrictions.DOCUMENTED) {
+ if (option.optionUsageRestrictions() == OptionUsageRestrictions.DOCUMENTED) {
OptionsUsage.getUsageHtml(optionField, desc, escaper, impl.getOptionsData());
}
}
@@ -642,7 +641,7 @@
});
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 @@
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 5c635fb..2d442a1 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 @@
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);
}
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 5fc531a..0066ca3 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsDataTest.java
@@ -18,6 +18,7 @@
import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableList;
+import com.google.devtools.common.options.OptionsParser.ConstructionException;
import java.lang.reflect.Field;
import java.util.ArrayList;
import java.util.List;
@@ -242,7 +243,7 @@
public void errorForInvalidOptionConverter() throws Exception {
try {
construct(InvalidOptionConverter.class);
- } catch (AssertionError e) {
+ } catch (ConstructionException e) {
// Expected exception
return;
}
@@ -264,13 +265,37 @@
public void errorForInvalidListOptionConverter() throws Exception {
try {
construct(InvalidListOptionConverter.class);
- } catch (AssertionError e) {
+ } catch (ConstructionException e) {
// Expected exception
return;
}
fail();
}
+ /** Dummy options class using deprecated category. */
+ public static class InvalidUndocumentedCategory extends OptionsBase {
+ @Option(
+ name = "experimental_foo",
+ category = "undocumented",
+ defaultValue = "true"
+ )
+ public boolean experimentalFoo;
+ }
+
+ @Test
+ public void invalidUndocumentedCategoryFails() {
+ try {
+ construct(InvalidUndocumentedCategory.class);
+ fail();
+ } catch (ConstructionException e) {
+ // Expected exception
+ assertThat(e).hasMessageThat().contains(
+ "Documentation level is no longer read from the option category.");
+ assertThat(e).hasMessageThat().contains("undocumented");
+ assertThat(e).hasMessageThat().contains("experimental_foo");
+ }
+ }
+
/**
* Dummy options class.
*
diff --git a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
index d7e3915..3d85315 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
@@ -27,6 +27,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.common.options.Converters.CommaSeparatedOptionListConverter;
+import com.google.devtools.common.options.OptionsParser.ConstructionException;
import com.google.devtools.common.options.OptionsParser.OptionUsageRestrictions;
import com.google.devtools.common.options.OptionsParser.OptionValueDescription;
import com.google.devtools.common.options.OptionsParser.UnparsedOptionValueDescription;
@@ -1403,7 +1404,7 @@
public void illegalListType() throws Exception {
try {
OptionsParser.newOptionsParser(IllegalListTypeExample.class);
- } catch (AssertionError e) {
+ } catch (ConstructionException e) {
// Expected exception
return;
}
