Normalize the value of features to match its semantics.

Why do we care?
- The values of the options are used in computing the ST-hash for configurations, which influence the cache keys used when actions are executed remotely.
- normalizing the fragment options reduces the number of different ST-hashes produced

Future changes will normalize other parts of the config.

PiperOrigin-RevId: 504097256
Change-Id: I5570482171daef977b4aa4a956c69151e3c670b5

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

@@ -34,9 +34,12 @@
 import com.google.devtools.common.options.OptionsParsingException;
 import com.google.devtools.common.options.TriState;
 import java.util.AbstractMap.SimpleEntry;
+import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.TreeSet;
 
 /**
  * Core options affecting a {@link BuildConfigurationValue} that don't belong in domain-specific
@@ -1016,6 +1019,40 @@ public LinkedHashMap<String, String> getNormalizedCommandLineBuildVariables() {
     return flagValueByName;
   }
 
+  /// Normalizes --features flags by sorting the values and having disables win over enables.
+  private static List<String> getNormalizedFeatures(List<String> features) {
+    // Parse out the features into a Map<String, boolean>, where the boolean represents whether
+    // the feature is enabled or disabled.
+    Map<String, Boolean> featureToState = new HashMap<>();
+    for (String feature : features) {
+      if (feature.startsWith("-")) {
+        // disable always wins.
+        featureToState.put(feature.substring(1), false);
+      } else if (!featureToState.containsKey(feature)) {
+        // enable feature only if it does not already have a state.
+        // If existing state is enabled, no need to do extra work.
+        // If existing state is disabled, it wins.
+        featureToState.put(feature, true);
+      }
+    }
+    // Partition into enabled/disabled features.
+    TreeSet<String> enabled = new TreeSet<>();
+    TreeSet<String> disabled = new TreeSet<>();
+    for (Map.Entry<String, Boolean> entry : featureToState.entrySet()) {
+      if (entry.getValue()) {
+        enabled.add(entry.getKey());
+      } else {
+        disabled.add(entry.getKey());
+      }
+    }
+    // Rebuild the set of features.
+    // Since we used TreeSet the features come out in a deterministic order.
+    List<String> result = new ArrayList<>(enabled);
+    disabled.stream().map(x -> "-" + x).forEach(result::add);
+    // If we made no changes, return the same instance we got to reduce churn.
+    return result.equals(features) ? features : result;
+  }
+
   @Override
   public CoreOptions getNormalized() {
     CoreOptions result = (CoreOptions) clone();
@@ -1033,6 +1070,8 @@ public CoreOptions getNormalized() {
                 .collect(toImmutableList());
       }
     }
+    // Normalize features.
+    result.defaultFeatures = getNormalizedFeatures(defaultFeatures);
 
     return result;
   }

src/test/java/com/google/devtools/build/lib/analysis/config/CoreOptionsTest.java

@@ -0,0 +1,82 @@
+// Copyright 2023 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.
+package com.google.devtools.build.lib.analysis.config;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.analysis.util.OptionsTestCase;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** A test for {@link CoreOptions}. */
+@RunWith(JUnit4.class)
+public final class CoreOptionsTest extends OptionsTestCase<CoreOptions> {
+
+  private static final String FEATURES_PREFIX = "--features=";
+  private static final String DEFINE_PREFIX = "--define=";
+
+  private static final ImmutableList<String> NO_COLLAPSE_DEFINES =
+      ImmutableList.of("--nocollapse_duplicate_defines");
+  private static final ImmutableList<String> COLLAPSE_DEFINES =
+      ImmutableList.of("--collapse_duplicate_defines");
+
+  @Test
+  public void testFeatures_orderingOfPositiveFeatures() throws Exception {
+    CoreOptions one = createWithPrefix(FEATURES_PREFIX, "foo", "bar");
+    CoreOptions two = createWithPrefix(FEATURES_PREFIX, "bar", "foo");
+    assertSame(one, two);
+  }
+
+  @Test
+  public void testFeatures_duplicateFeatures() throws Exception {
+    CoreOptions one = createWithPrefix(FEATURES_PREFIX, "foo", "bar");
+    CoreOptions two = createWithPrefix(FEATURES_PREFIX, "bar", "foo", "bar");
+    assertSame(one, two);
+  }
+
+  @Test
+  public void testFeatures_disablingWins() throws Exception {
+    CoreOptions one = createWithPrefix(FEATURES_PREFIX, "foo", "-foo", "bar");
+    CoreOptions two = createWithPrefix(FEATURES_PREFIX, "-foo", "bar");
+    assertSame(one, two);
+  }
+
+  @Test
+  public void testDefines_ordering_effectOnCacheKey() throws Exception {
+    CoreOptions one = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "a=1", "b=2");
+    CoreOptions two = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "b=2", "a=1");
+    // Due to NO_COLLAPSE_DEFINES, the two configurations are not equivalent.
+    assertDifferent(one, two);
+  }
+
+  @Test
+  public void testDefines_duplicates_effectOnCacheKey() throws Exception {
+    CoreOptions one = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "a=2", "a=2");
+    CoreOptions two = createWithPrefix(NO_COLLAPSE_DEFINES, DEFINE_PREFIX, "a=2");
+    // Due to NO_COLLAPSE_DEFINES, the two configurations are not equivalent.
+    assertDifferent(one, two);
+  }
+
+  @Test
+  public void testDefines_duplicatesWithCollapsingEnabled_effectOnCacheKey() throws Exception {
+    CoreOptions one = createWithPrefix(COLLAPSE_DEFINES, DEFINE_PREFIX, "a=1", "a=2");
+    CoreOptions two = createWithPrefix(COLLAPSE_DEFINES, DEFINE_PREFIX, "a=2");
+    assertSame(one, two);
+  }
+
+  @Override
+  protected Class<CoreOptions> getOptionsClass() {
+    return CoreOptions.class;
+  }
+}

src/test/java/com/google/devtools/build/lib/analysis/util/OptionsTestCase.java

@@ -0,0 +1,84 @@
+// Copyright 2023 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.
+package com.google.devtools.build.lib.analysis.util;
+
+import static com.google.common.truth.Truth.assertThat;
+import static java.util.Arrays.stream;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.analysis.config.FragmentOptions;
+import com.google.devtools.common.options.OptionsParser;
+import java.util.Arrays;
+import java.util.List;
+
+/** A base class for testing cacheKey related functionality of Option classes. */
+public abstract class OptionsTestCase<T extends FragmentOptions> {
+
+  protected abstract Class<T> getOptionsClass();
+
+  /** Construct options parsing the given arguments. */
+  protected T create(List<String> args) throws Exception {
+    Class<T> cls = getOptionsClass();
+    OptionsParser parser = OptionsParser.builder().optionsClasses(ImmutableList.of(cls)).build();
+    parser.parse(args);
+    return parser.getOptions(cls);
+  }
+
+  /** Construct options parsing the given arguments. */
+  protected T create(String... args) throws Exception {
+    return create(Arrays.asList(args));
+  }
+
+  /**
+   * Useful for options which are specified multiple times on the command line. {@code
+   * createWithPrefix("--abc=", "x", "y", "z")} is equivalent to {@code create("--abc=x", "--abc=y",
+   * "--abc=z")}
+   */
+  protected T createWithPrefix(String prefix, String... args) throws Exception {
+    return createWithPrefix(ImmutableList.of(), prefix, args);
+  }
+
+  /**
+   * Variant of {@link #createWithPrefix(String, String...)} with additional fixed set of options.
+   */
+  protected T createWithPrefix(ImmutableList<String> fixed, String prefix, String... args)
+      throws Exception {
+    ImmutableList.Builder<String> builder = ImmutableList.builder();
+    builder.addAll(fixed);
+    stream(args).map(x -> prefix + x).forEach(builder::add);
+    return create(builder.build());
+  }
+
+  protected void assertSame(T one, T two) {
+    // We normalize first, since that is what BuildOptions.checkSum() does.
+    // We do not use BuildOptions.checkSum() because in case of test failure,
+    // the diff on cacheKey is humanreadable.
+    FragmentOptions oneNormalized = one.getNormalized();
+    FragmentOptions twoNormalized = two.getNormalized();
+    assertThat(oneNormalized.cacheKey()).isEqualTo(twoNormalized.cacheKey());
+    // Also check equality of toString() as that influences the ST-hash computation.
+    assertThat(oneNormalized.toString()).isEqualTo(twoNormalized.toString());
+  }
+
+  protected void assertDifferent(T one, T two) {
+    // We normalize first, since that is what BuildOptions.checkSum() does.
+    // We do not use BuildOptions.checkSum() because in case of test failure,
+    // the diff on cacheKey is humanreadable.
+    FragmentOptions oneNormalized = one.getNormalized();
+    FragmentOptions twoNormalized = two.getNormalized();
+    assertThat(oneNormalized.cacheKey()).isNotEqualTo(twoNormalized.cacheKey());
+    // Also check equality of toString() as that influences the ST-hash computation.
+    assertThat(oneNormalized.toString()).isNotEqualTo(twoNormalized.toString());
+  }
+}