Move the canonicalization of an option value to the option value itself.

Ideally, the canonical form we output from OptionUtils would be the same as for the command canonicalize-flags, but that must wait for dependencies to be cleaned up. Still, in the meantime, keep the --foo=1 normalization of --foo, and apply it to all other boolean flag values (t,true,yes,y, and false equivalents), so that the canoncalize-flags output is more canonical, even if it isn't using the --[no]foo form yet.

RELNOTES: Boolean flag values will now get normalized to 1 or 0 in canonicalize-flags output.
PiperOrigin-RevId: 170084599

4 files changed, 69 insertions, 42 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/util/OptionsUtils.java b/src/main/java/com/google/devtools/build/lib/util/OptionsUtils.java
index bae41d869f..c94863ed86 100644
--- a/src/main/java/com/google/devtools/build/lib/util/OptionsUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/util/OptionsUtils.java
@@ -17,8 +17,6 @@ package com.google.devtools.build.lib.util;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.common.options.Converter;
-import com.google.devtools.common.options.Converters;
-import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.OptionsProvider;
 import com.google.devtools.common.options.ParsedOptionDescription;
 import java.util.ArrayList;
@@ -43,23 +41,7 @@ public final class OptionsUtils {
       if (result.length() != 0) {
         result.append(' ');
       }
-      String value = option.getUnconvertedValue();
-      if (option.isBooleanOption()) {
-        boolean isEnabled = false;
-        try {
-          isEnabled = new Converters.BooleanConverter().convert(value);
-        } catch (OptionsParsingException e) {
-          throw new RuntimeException("Unexpected parsing exception", e);
-        }
-        result
-            .append(isEnabled ? "--" : "--no")
-            .append(option.getOptionDefinition().getOptionName());
-      } else {
-        result.append("--").append(option.getOptionDefinition().getOptionName());
-        if (value != null) { // Can be null for Void options.
-          result.append("=").append(ShellEscaper.escapeString(value));
-        }
-      }
+      result.append(option.getCanonicalFormWithValueEscaper(ShellEscaper::escapeString));
     }
     return result.toString();
   }
@@ -82,22 +64,7 @@ public final class OptionsUtils {
       if (option.isHidden()) {
         continue;
       }
-      String value = option.getUnconvertedValue();
-      if (option.isBooleanOption()) {
-        boolean isEnabled = false;
-        try {
-          isEnabled = new Converters.BooleanConverter().convert(value);
-        } catch (OptionsParsingException e) {
-          throw new RuntimeException("Unexpected parsing exception", e);
-        }
-        builder.add((isEnabled ? "--" : "--no") + option.getOptionDefinition().getOptionName());
-      } else {
-        String optionString = "--" + option.getOptionDefinition().getOptionName();
-        if (value != null) { // Can be null for Void options.
-          optionString += "=" + value;
-        }
-        builder.add(optionString);
-      }
+      builder.add(option.getCanonicalForm());
     }
     return builder.build();
   }
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 176d51e66a..221dcf030f 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -158,12 +158,7 @@ class OptionsParserImpl {
             })
         // Ignore expansion options.
         .filter(value -> !value.getOptionDefinition().isExpansionOption())
-        .map(
-            value ->
-                "--"
-                    + value.getOptionDefinition().getOptionName()
-                    + "="
-                    + value.getUnconvertedValue())
+        .map(ParsedOptionDescription::getDeprecatedCanonicalForm)
         .collect(toCollection(ArrayList::new));
   }
 
diff --git a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java
index 0910579541..1f43172fb2 100644
--- a/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java
+++ b/src/main/java/com/google/devtools/common/options/ParsedOptionDescription.java
@@ -15,6 +15,7 @@
 package com.google.devtools.common.options;
 
 import com.google.common.collect.ImmutableList;
+import java.util.function.Function;
 import javax.annotation.Nullable;
 
 /**
@@ -49,6 +50,46 @@ public final class ParsedOptionDescription {
     return commandLineForm;
   }
 
+  public String getCanonicalForm() {
+    return getCanonicalFormWithValueEscaper(s -> s);
+  }
+
+  public String getCanonicalFormWithValueEscaper(Function<String, String> escapingFunction) {
+    // For boolean flags (note that here we do not check for TriState flags, only flags with actual
+    // boolean values, so that we know the return type of getConvertedValue), use the --[no]flag
+    // form for the canonical value.
+    if (optionDefinition.getType().equals(boolean.class)) {
+      try {
+        return ((boolean) getConvertedValue() ? "--" : "--no") + optionDefinition.getOptionName();
+      } catch (OptionsParsingException e) {
+        throw new RuntimeException("Unexpected parsing exception", e);
+      }
+    } else {
+      String optionString = "--" + optionDefinition.getOptionName();
+      if (unconvertedValue != null) { // Can be null for Void options.
+        optionString += "=" + escapingFunction.apply(unconvertedValue);
+      }
+      return optionString;
+    }
+  }
+
+  @Deprecated
+  // TODO(b/65646296) Once external dependencies are cleaned up, use getCanonicalForm()
+  String getDeprecatedCanonicalForm() {
+    String value = unconvertedValue;
+    // For boolean flags (note that here we do not check for TriState flags, only flags with actual
+    // boolean values, so that we know the return type of getConvertedValue), set them all to 1 or
+    // 0, instead of keeping the wide variety of values we accept in their original form.
+    if (optionDefinition.getType().equals(boolean.class)) {
+      try {
+        value = (boolean) getConvertedValue() ? "1" : "0";
+      } catch (OptionsParsingException e) {
+        throw new RuntimeException("Unexpected parsing exception", e);
+      }
+    }
+    return String.format("--%s=%s", optionDefinition.getOptionName(), value);
+  }
+
   public boolean isBooleanOption() {
     return optionDefinition.getType().equals(boolean.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 77bcc36412..41ad1f30a5 100644
--- a/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
+++ b/src/test/java/com/google/devtools/common/options/OptionsParserTest.java
@@ -1886,6 +1886,30 @@ public class OptionsParserTest {
     assertThat(canonicalize(Yesterday.class, "-h")).containsExactly("--g=1");
   }
 
+  /**
+   * Check that all forms of boolean flags are canonicalizes to the same form.
+   *
+   * The list of accepted values is from
+   * {@link com.google.devtools.common.options.Converters.BooleanConverter}, and the value-less
+   * --[no] form is controlled by {@link OptionsParserImpl#identifyOptionAndPossibleArgument}.
+   */
+  @Test
+  public void canonicalizeNormalizesBooleanFlags() throws Exception {
+    assertThat(canonicalize(Yesterday.class, "--g")).containsExactly("--g=1");
+    assertThat(canonicalize(Yesterday.class, "--g=1")).containsExactly("--g=1");
+    assertThat(canonicalize(Yesterday.class, "--g=true")).containsExactly("--g=1");
+    assertThat(canonicalize(Yesterday.class, "--g=t")).containsExactly("--g=1");
+    assertThat(canonicalize(Yesterday.class, "--g=yes")).containsExactly("--g=1");
+    assertThat(canonicalize(Yesterday.class, "--g=y")).containsExactly("--g=1");
+
+    assertThat(canonicalize(Yesterday.class, "--nog")).containsExactly("--g=0");
+    assertThat(canonicalize(Yesterday.class, "--g=0")).containsExactly("--g=0");
+    assertThat(canonicalize(Yesterday.class, "--g=false")).containsExactly("--g=0");
+    assertThat(canonicalize(Yesterday.class, "--g=f")).containsExactly("--g=0");
+    assertThat(canonicalize(Yesterday.class, "--g=no")).containsExactly("--g=0");
+    assertThat(canonicalize(Yesterday.class, "--g=n")).containsExactly("--g=0");
+  }
+
   public static class LongValueExample extends OptionsBase {
     @Option(
       name = "longval",
@@ -2050,7 +2074,7 @@ public class OptionsParserTest {
   public void testWrapperCanonicalization() throws OptionsParsingException {
     List<String> canonicalized = canonicalize(WrapperOptionExample.class,
         "--wrapper=--flag1=true", "--wrapper=--flag2=87", "--wrapper=--flag3=bar");
-    assertThat(canonicalized).isEqualTo(Arrays.asList("--flag1=true", "--flag2=87", "--flag3=bar"));
+    assertThat(canonicalized).isEqualTo(Arrays.asList("--flag1=1", "--flag2=87", "--flag3=bar"));
   }
 
   /** Dummy options that declares it uses only core types. */