Add integration test for --all_incompatible_changes flag conflicts

This adds a new warning when the same flag is expanded to by multiple expansion flags. This extends an existing suite of warnings, e.g. for when an expansion flag conflicts with an explicit option.

The blaze canonicalize-flags command now takes a new flag --show_warnings. This flag causes any warnings encountered while parsing the given command line to be printed to stderr.

RELNOTES: blaze canonicalize-flags now takes a --show_warnings flag

PiperOrigin-RevId: 152186672

4 files changed, 137 insertions, 12 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java
index 77fb5e669a..ee43018010 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/CanonicalizeCommand.java
@@ -14,6 +14,7 @@
 package com.google.devtools.build.lib.runtime.commands;
 
 import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.flags.InvocationPolicyEnforcer;
 import com.google.devtools.build.lib.flags.InvocationPolicyParser;
@@ -28,7 +29,6 @@ import com.google.devtools.common.options.OptionsBase;
 import com.google.devtools.common.options.OptionsParser;
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.OptionsProvider;
-
 import java.util.Collection;
 import java.util.List;
 
@@ -46,16 +46,53 @@ import java.util.List;
 public final class CanonicalizeCommand implements BlazeCommand {
 
   public static class Options extends OptionsBase {
-    @Option(name = "for_command",
-            defaultValue = "build",
-            category = "misc",
-            help = "The command for which the options should be canonicalized.")
+    @Option(
+      name = "for_command",
+      defaultValue = "build",
+      category = "misc",
+      help = "The command for which the options should be canonicalized."
+    )
     public String forCommand;
 
-    @Option(name = "invocation_policy",
-        defaultValue = "",
-        help = "Applies an invocation policy to the options to be canonicalized.")
+    @Option(
+      name = "invocation_policy",
+      defaultValue = "",
+      help = "Applies an invocation policy to the options to be canonicalized."
+    )
     public String invocationPolicy;
+
+    @Option(
+      name = "show_warnings",
+      defaultValue = "false",
+      help = "Output parser warnings to standard error (e.g. for conflicting flag options)."
+    )
+    public boolean showWarnings;
+  }
+
+  /**
+   * These options are used by the incompatible_changes_conflict_test.sh integration test, which
+   * confirms that the warning for conflicting expansion options is working correctly. These flags
+   * are undocumented no-ops, and are not to be used by anything outside of that test.
+   */
+  public static class FlagClashCanaryOptions extends OptionsBase {
+    @Option(name = "flag_clash_canary", defaultValue = "false", category = "undocumented")
+    public boolean flagClashCanary;
+
+    @Option(
+      name = "flag_clash_canary_expander1",
+      defaultValue = "null",
+      category = "undocumented",
+      expansion = {"--flag_clash_canary=1"}
+    )
+    public Void flagClashCanaryExpander1;
+
+    @Option(
+      name = "flag_clash_canary_expander2",
+      defaultValue = "null",
+      category = "undocumented",
+      expansion = {"--flag_clash_canary=0"}
+    )
+    public Void flagClashCanaryExpander2;
   }
 
   @Override
@@ -70,10 +107,12 @@ public final class CanonicalizeCommand implements BlazeCommand {
       return ExitCode.COMMAND_LINE_ERROR;
     }
     Collection<Class<? extends OptionsBase>> optionsClasses =
-        BlazeCommandUtils.getOptions(
-            command.getClass(), runtime.getBlazeModules(), runtime.getRuleClassProvider());
+        ImmutableList.<Class<? extends OptionsBase>>builder()
+            .addAll(BlazeCommandUtils.getOptions(
+                command.getClass(), runtime.getBlazeModules(), runtime.getRuleClassProvider()))
+            .add(FlagClashCanaryOptions.class)
+            .build();
     try {
-      
       OptionsParser parser = OptionsParser.newOptionsParser(optionsClasses);
       parser.setAllowResidue(false);
       parser.parse(options.getResidue());
@@ -83,8 +122,13 @@ public final class CanonicalizeCommand implements BlazeCommand {
               InvocationPolicyParser.parsePolicy(canonicalizeOptions.invocationPolicy));
       invocationPolicyEnforcer.enforce(parser, commandName);
 
-      List<String> result = parser.canonicalize();
+      if (canonicalizeOptions.showWarnings) {
+        for (String warning : parser.getWarnings()) {
+          env.getReporter().handle(Event.warn(warning));
+        }
+      }
 
+      List<String> result = parser.canonicalize();
       for (String piece : result) {
         env.getReporter().getOutErr().printOutLn(piece);
       }
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 94702c93bb..fd3a8cea13 100644
--- a/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
+++ b/src/main/java/com/google/devtools/common/options/OptionsParserImpl.java
@@ -296,6 +296,15 @@ class OptionsParserImpl {
           // Create a warning if an expansion option overrides an explicit option:
           warnings.add("The option '" + expandedFrom + "' was expanded and now overrides a "
               + "previous explicitly specified option '" + name + "'");
+        } else if ((entry.getExpansionParent() != null) && (expandedFrom != null)) {
+          warnings.add(
+              "The option '"
+                  + name
+                  + "' was expanded to from both options '"
+                  + entry.getExpansionParent()
+                  + "' and '"
+                  + expandedFrom
+                  + "'");
         }
 
         // Record the new value:
@@ -633,6 +642,8 @@ class OptionsParserImpl {
       field = optionsData.getFieldFromName(name);
 
       // Look for a "no"-prefixed option name: "no<optionName>" or "no_<optionName>".
+      // Note: It is impossible to specify "--no_foo" for a flag named "--_foo", since that'll be
+      // interpreted as the "no_" negating prefix for "--foo".
       if (field == null && name.startsWith("no")) {
         name = name.substring(name.startsWith("no_") ? 3 : 2);
         field = optionsData.getFieldFromName(name);
diff --git a/src/test/shell/integration/BUILD b/src/test/shell/integration/BUILD
index eb91a7301d..b083b44b8e 100644
--- a/src/test/shell/integration/BUILD
+++ b/src/test/shell/integration/BUILD
@@ -156,6 +156,13 @@ sh_test(
 )
 
 sh_test(
+    name = "incompatible_changes_conflict_test",
+    size = "small",
+    srcs = ["incompatible_changes_conflict_test.sh"],
+    data = [":test-deps"],
+)
+
+sh_test(
     name = "ide_info_generation_skylark",
     size = "large",
     srcs = ["ide_info_generation.sh"],
diff --git a/src/test/shell/integration/incompatible_changes_conflict_test.sh b/src/test/shell/integration/incompatible_changes_conflict_test.sh
new file mode 100755
index 0000000000..955e8bdfe2
--- /dev/null
+++ b/src/test/shell/integration/incompatible_changes_conflict_test.sh
@@ -0,0 +1,63 @@
+#!/bin/bash
+#
+# Copyright 2017 The Bazel Authors. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# Tests that there are no conflicts among all of the flags in the transitive
+# expansion of --all_incompatible_changes. This is an integration test because
+# it is difficult to know in a unit test exactly what features (OptionsBase
+# subclasses) are passed to the parser from within a unit test.
+
+# Load the test setup defined in the parent directory
+CURRENT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+source "${CURRENT_DIR}/../integration_test_setup.sh" \
+  || { echo "integration_test_setup.sh not found!" >&2; exit 1; }
+
+# The clash canary flags are built into the canonicalize-flags command
+# specifically for this test suite.
+canary_clash_error="The option 'flag_clash_canary' was expanded to from both "
+canary_clash_error+="options 'flag_clash_canary_expander1' and "
+canary_clash_error+="'flag_clash_canary_expander2'."
+
+# Ensures that we didn't change the formatting of the warning message or
+# disable the warning.
+function test_conflict_warning_is_working() {
+  bazel canonicalize-flags --show_warnings -- \
+    --flag_clash_canary_expander1 --flag_clash_canary_expander2 \
+    &>$TEST_log || fail "bazel canonicalize-flags failed";
+  fail_msg = "Did not find expected flag conflict warning"
+  expect_log "$canary_clash_error" "$fail_msg"
+}
+
+# Ensures that canonicalize-flags doesn't emit warnings unless requested.
+function test_canonicalize_flags_suppresses_warnings() {
+  bazel canonicalize-flags -- \
+    --flag_clash_canary_expander1 --flag_clash_canary_expander2 \
+    &>$TEST_log || fail "bazel canonicalize-flags failed";
+  fail_msg = "canonicalize-flags should have suppressed parser warnings since "
+  fail_msg += "--show_warnings wasn't specified"
+  expect_not_log "$canary_clash_error" "$fail_msg"
+}
+
+function test_no_conflicts_among_incompatible_changes() {
+  bazel canonicalize-flags --show_warnings -- --all_incompatible_changes \
+    &>$TEST_log || fail "bazel canonicalize-flags failed";
+  expected="The option '.*' was expanded to from both options "
+  expected+="'.*' and '.*'."
+  fail_msg="Options conflict in expansion of --all_incompatible_changes"
+  expect_not_log "$expected" "$fail_msg"
+}
+
+
+run_suite "incompatible_changes_conflict_test"