diff options
author | ccalvarin <ccalvarin@google.com> | 2017-03-30 19:57:20 +0000 |
---|---|---|
committer | Philipp Wollermann <philwo@google.com> | 2017-03-31 17:10:44 +0200 |
commit | 584dc717e64df4e50d7f657eb2878f53295eab12 (patch) | |
tree | a3769f986f61cd2bbee44528631d4a8fd3c16b6f /src/main/java | |
parent | 098054ebb95da6d678a86e4faf025e22118ca856 (diff) |
Watch for --no and --no_ flag name conflicts.
Prevent OptionsBases with conflicting names due to --no boolean flag aliases to
successfully combine into parser.
Also remove the comment about --no_ not being documented, since it has been documented since Bazel was open-sourced.
PiperOrigin-RevId: 151738453
Diffstat (limited to 'src/main/java')
3 files changed, 65 insertions, 18 deletions
diff --git a/src/main/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java b/src/main/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java index 5c636b8a7d..ad4c975713 100644 --- a/src/main/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java +++ b/src/main/java/com/google/devtools/common/options/DuplicateOptionDeclarationException.java @@ -14,9 +14,7 @@ package com.google.devtools.common.options; -/** - * Indicates that an option is declared in more than one class. - */ +/** Indicates that a flag is declared more than once. */ public class DuplicateOptionDeclarationException extends RuntimeException { DuplicateOptionDeclarationException(String message) { 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 27f42f48b1..60a32a907f 100644 --- a/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java +++ b/src/main/java/com/google/devtools/common/options/IsolatedOptionsData.java @@ -260,6 +260,45 @@ class IsolatedOptionsData extends OpaqueOptionsData { return convertedValue; } + private static <A> void checkForCollisions( + Map<A, Field> aFieldMap, + A optionName, + String description) { + if (aFieldMap.containsKey(optionName)) { + throw new DuplicateOptionDeclarationException( + "Duplicate option name, due to " + description + ": --" + optionName); + } + } + + private static void checkForBooleanAliasCollisions( + Map<String, String> booleanAliasMap, + String optionName, + String description) { + if (booleanAliasMap.containsKey(optionName)) { + throw new DuplicateOptionDeclarationException( + "Duplicate option name, due to " + + description + + " --" + + optionName + + ", it conflicts with a negating alias for boolean flag --" + + booleanAliasMap.get(optionName)); + } + } + + private static void checkAndUpdateBooleanAliases( + Map<String, Field> nameToFieldMap, + Map<String, String> booleanAliasMap, + String optionName) { + // Check that the two aliases do not conflict with existing flags. + checkForCollisions(nameToFieldMap, "no_" + optionName, "boolean option alias"); + checkForCollisions(nameToFieldMap, "no" + optionName, "boolean option alias"); + + // Record that the boolean option takes up additional namespace for its two negating + // aliases. + booleanAliasMap.put("no_" + optionName, optionName); + booleanAliasMap.put("no" + optionName, optionName); + } + /** * Constructs an {@link IsolatedOptionsData} object for a parser that knows about the given * {@link OptionsBase} classes. No inter-option analysis is done. Performs basic sanity checking @@ -274,6 +313,9 @@ class IsolatedOptionsData extends OpaqueOptionsData { Map<Field, Converter<?>> convertersBuilder = Maps.newHashMap(); Map<Field, Boolean> allowMultipleBuilder = Maps.newHashMap(); + // Maps the negated boolean flag aliases to the original option name. + Map<String, String> booleanAliasMap = Maps.newHashMap(); + // Read all Option annotations: for (Class<? extends OptionsBase> parsedOptionsClass : classes) { try { @@ -289,8 +331,8 @@ class IsolatedOptionsData extends OpaqueOptionsData { for (Field field : fields) { Option annotation = field.getAnnotation(Option.class); - - if (annotation.name() == null) { + String optionName = annotation.name(); + if (optionName == null) { throw new AssertionError("Option cannot have a null name"); } @@ -326,7 +368,7 @@ 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 occurance " + + 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 + ")"); } @@ -345,21 +387,28 @@ class IsolatedOptionsData extends OpaqueOptionsData { } } - if (nameToFieldBuilder.put(annotation.name(), field) != null) { - throw new DuplicateOptionDeclarationException( - "Duplicate option name: --" + annotation.name()); + if (isBooleanField(field)) { + checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, optionName); } + + checkForCollisions(nameToFieldBuilder, optionName, "option"); + checkForBooleanAliasCollisions(booleanAliasMap, optionName, "option"); + nameToFieldBuilder.put(optionName, field); + if (!annotation.oldName().isEmpty()) { - if (nameToFieldBuilder.put(annotation.oldName(), field) != null) { - throw new DuplicateOptionDeclarationException( - "Old option name duplicates option name: --" + annotation.oldName()); + String oldName = annotation.oldName(); + checkForCollisions(nameToFieldBuilder, oldName, "old option name"); + checkForBooleanAliasCollisions(booleanAliasMap, oldName, "old option name"); + nameToFieldBuilder.put(annotation.oldName(), field); + + // If boolean, repeat the alias dance for the old name. + if (isBooleanField(field)) { + checkAndUpdateBooleanAliases(nameToFieldBuilder, booleanAliasMap, oldName); } } if (annotation.abbrev() != '\0') { - if (abbrevToFieldBuilder.put(annotation.abbrev(), field) != null) { - throw new DuplicateOptionDeclarationException( - "Duplicate option abbrev: -" + annotation.abbrev()); - } + checkForCollisions(abbrevToFieldBuilder, annotation.abbrev(), "option abbreviation"); + abbrevToFieldBuilder.put(annotation.abbrev(), field); } optionDefaultsBuilder.put(field, retrieveDefaultFromAnnotation(field)); @@ -379,4 +428,5 @@ class IsolatedOptionsData extends OpaqueOptionsData { convertersBuilder, allowMultipleBuilder); } + } 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 a818ec477f..aeb0da08f9 100644 --- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java +++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java @@ -636,8 +636,7 @@ class OptionsParserImpl { value = equalsAt == -1 ? null : arg.substring(equalsAt + 1); field = optionsData.getFieldFromName(name); - // look for a "no"-prefixed option name: "no<optionname>"; - // (Undocumented: we also allow --no_foo. We're generous like that.) + // Look for a "no"-prefixed option name: "no<optionName>" or "no_<optionName>". if (field == null && name.startsWith("no")) { name = name.substring(name.startsWith("no_") ? 3 : 2); field = optionsData.getFieldFromName(name); |