From 5d1042629e94e278dcc49db5f640829acc3bff1a Mon Sep 17 00:00:00 2001 From: ccalvarin Date: Wed, 19 Apr 2017 01:15:45 +0200 Subject: 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 --- .../analysis/config/TransitiveOptionDetails.java | 3 +- .../devtools/build/lib/rules/cpp/CppOptions.java | 3 +- .../common/options/IsolatedOptionsData.java | 58 +++++++++++++++------- .../com/google/devtools/common/options/Option.java | 3 -- .../devtools/common/options/OptionsParser.java | 39 +++------------ .../devtools/common/options/OptionsParserImpl.java | 2 +- 6 files changed, 51 insertions(+), 57 deletions(-) (limited to 'src/main/java/com/google') 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 allowMultiple; + /** These categories used to indicate OptionUsageRestrictions, but no longer. */ + private static final ImmutableList DEPRECATED_CATEGORIES = ImmutableList.of( + "undocumented", "hidden", "internal"); + private IsolatedOptionsData( Map, Constructor> optionsClasses, @@ -183,11 +188,11 @@ public class IsolatedOptionsData extends OpaqueOptionsData { if (annotation.allowMultiple()) { // If the type isn't a List, 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); } -- cgit v1.2.3