Make the state in RuleContext explicit

This isn't ideal - RuleContext should not have state, but this ended up
happening between adding a cache and refactoring how make variables are
discovered.

I have carefully traced back all callers that provide custom make variable
suppliers and added an init call to their rule initialization. Note that the
ConfigurationMakeVariableContext is _cached_, so callers that call in without
any make variable suppliers and then call again with them would get the context
from the previous call.

We now enforce that the ConfigurationMakeVariableContext is only initialized
once, and that this happens before any usage, which is slightly better than the
previous state, where initialization was silently ignored on any subsequent
call.

Progress on #2475.

PiperOrigin-RevId: 170312285

4 files changed, 23 insertions, 31 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
index b341c4b825..742e5dd37c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcBinary.java
@@ -45,6 +45,7 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.TargetUtils;
 import com.google.devtools.build.lib.rules.apple.ApplePlatform;
+import com.google.devtools.build.lib.rules.cpp.CcCommon.CcFlagsSupplier;
 import com.google.devtools.build.lib.rules.cpp.CcLibraryHelper.Info;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
 import com.google.devtools.build.lib.rules.cpp.CppConfiguration.DynamicMode;
@@ -166,6 +167,8 @@ public abstract class CcBinary implements RuleConfiguredTargetFactory {
   public static ConfiguredTarget init(CppSemantics semantics, RuleContext ruleContext, boolean fake)
       throws InterruptedException, RuleErrorException {
     ruleContext.checkSrcsSamePackage(true);
+    ruleContext.initConfigurationMakeVariableContext(new CcFlagsSupplier(ruleContext));
+
     CcCommon common = new CcCommon(ruleContext);
     CcToolchainProvider ccToolchain = common.getToolchain();
     FdoSupportProvider fdoSupport = common.getFdoSupport();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
index 602b5d4ed6..36278ef9c9 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcCommon.java
@@ -150,8 +150,6 @@ public final class CcCommon {
   }
 
   public ImmutableList<String> getCopts() {
-    Preconditions.checkState(hasAttribute("copts", Type.STRING_LIST));
-
     if (!getCoptsFilter(ruleContext).apply("-Wno-future-warnings")) {
       ruleContext.attributeWarning(
           "nocopts",
@@ -164,8 +162,8 @@ public final class CcCommon {
     }
 
     return ImmutableList.<String>builder()
-        .addAll(getPackageCopts(ruleContext))
-        .addAll(CppHelper.getAttributeCopts(ruleContext, "copts"))
+        .addAll(CppHelper.getPackageCopts(ruleContext))
+        .addAll(CppHelper.getAttributeCopts(ruleContext))
         .build();
   }
 
@@ -336,11 +334,6 @@ public final class CcCommon {
     }
   }
 
-  private static ImmutableList<String> getPackageCopts(RuleContext ruleContext) {
-    List<String> unexpanded = ruleContext.getRule().getPackage().getDefaultCopts();
-    return ImmutableList.copyOf(CppHelper.expandMakeVariables(ruleContext, "copts", unexpanded));
-  }
-
   /** Returns copts filter built from the make variable expanded nocopts attribute. */
   Predicate<String> getCoptsFilter() {
     return getCoptsFilter(ruleContext);
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
index 0b4c2dfef2..96681370bd 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CcLibrary.java
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.packages.AttributeMap;
 import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.ImplicitOutputsFunction;
 import com.google.devtools.build.lib.packages.RawAttributeMapper;
+import com.google.devtools.build.lib.rules.cpp.CcCommon.CcFlagsSupplier;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
 import com.google.devtools.build.lib.rules.cpp.Link.LinkTargetType;
 import com.google.devtools.build.lib.rules.cpp.LinkerInputs.LibraryToLink;
@@ -104,6 +105,8 @@ public abstract class CcLibrary implements RuleConfiguredTargetFactory {
       boolean linkStatic,
       boolean addDynamicRuntimeInputArtifactsToRunfiles)
       throws RuleErrorException, InterruptedException {
+    ruleContext.initConfigurationMakeVariableContext(new CcFlagsSupplier(ruleContext));
+
     final CcCommon common = new CcCommon(ruleContext);
     CcToolchainProvider ccToolchain = common.getToolchain();
     FdoSupportProvider fdoSupport = common.getFdoSupport();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
index feaad45b72..5bc125633e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppHelper.java
@@ -25,9 +25,7 @@ import com.google.devtools.build.lib.actions.ActionOwner;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.MiddlemanFactory;
 import com.google.devtools.build.lib.analysis.AnalysisUtils;
-import com.google.devtools.build.lib.analysis.ConfigurationMakeVariableContext;
 import com.google.devtools.build.lib.analysis.FileProvider;
-import com.google.devtools.build.lib.analysis.MakeVariableSupplier;
 import com.google.devtools.build.lib.analysis.PlatformConfiguration;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleContext;
@@ -46,7 +44,6 @@ import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.RuleErrorConsumer;
-import com.google.devtools.build.lib.rules.cpp.CcCommon.CcFlagsSupplier;
 import com.google.devtools.build.lib.rules.cpp.CcLinkParams.Linkstamp;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.FeatureConfiguration;
 import com.google.devtools.build.lib.rules.cpp.CcToolchainFeatures.Tool;
@@ -149,32 +146,25 @@ public class CppHelper {
    * @return a list of strings containing the expanded and tokenized values for the
    *         attribute
    */
-  // TODO(bazel-team): Move to CcCommon; refactor CcPlugin to use either CcLibraryHelper or
-  // CcCommon.
-  static List<String> expandMakeVariables(
+  private static List<String> expandMakeVariables(
       RuleContext ruleContext, String attributeName, List<String> input) {
     boolean tokenization =
         !ruleContext.getFeatures().contains("no_copts_tokenization");
 
     List<String> tokens = new ArrayList<>();
-    ImmutableList<? extends MakeVariableSupplier> makeVariableSuppliers =
-        ImmutableList.of(new CcFlagsSupplier(ruleContext));
-    ConfigurationMakeVariableContext makeVariableContext =
-        ruleContext.getConfigurationMakeVariableContext(makeVariableSuppliers);
     for (String token : input) {
       try {
         // Legacy behavior: tokenize all items.
         if (tokenization) {
-          ruleContext.tokenizeAndExpandMakeVars(
-              tokens, attributeName, token, makeVariableContext);
+          ruleContext.tokenizeAndExpandMakeVars(tokens, attributeName, token);
         } else {
           String exp =
-              ruleContext.expandSingleMakeVariable(attributeName, token, makeVariableContext);
+              ruleContext.expandSingleMakeVariable(attributeName, token);
           if (exp != null) {
             ShellUtils.tokenize(tokens, exp);
           } else {
             tokens.add(
-                ruleContext.expandMakeVariables(attributeName, token, makeVariableContext));
+                ruleContext.expandMakeVariables(attributeName, token));
           }
         }
       } catch (ShellUtils.TokenizationException e) {
@@ -185,15 +175,22 @@ public class CppHelper {
   }
 
   /**
-   * Appends the tokenized values of the copts attribute to copts.
+   * Returns the tokenized values of the copts attribute to copts.
    */
-  public static ImmutableList<String> getAttributeCopts(RuleContext ruleContext, String attr) {
+  // Called from CcCommon and CcSupport (Google's internal version of proto_library).
+  public static ImmutableList<String> getAttributeCopts(RuleContext ruleContext) {
+    String attr = "copts";
     Preconditions.checkArgument(ruleContext.getRule().isAttrDefined(attr, Type.STRING_LIST));
     List<String> unexpanded = ruleContext.attributes().get(attr, Type.STRING_LIST);
-
     return ImmutableList.copyOf(expandMakeVariables(ruleContext, attr, unexpanded));
   }
 
+  // Called from CcCommon.
+  static ImmutableList<String> getPackageCopts(RuleContext ruleContext) {
+    List<String> unexpanded = ruleContext.getRule().getPackage().getDefaultCopts();
+    return ImmutableList.copyOf(expandMakeVariables(ruleContext, "copts", unexpanded));
+  }
+
   /**
    * Expands attribute value either using label expansion
    * (if attemptLabelExpansion == {@code true} and it does not look like make
@@ -202,9 +199,6 @@ public class CppHelper {
   public static List<String> expandLinkopts(
       RuleContext ruleContext, String attrName, Iterable<String> values) {
     List<String> result = new ArrayList<>();
-    ConfigurationMakeVariableContext makeVariableContext =
-        ruleContext.getConfigurationMakeVariableContext(
-            ImmutableList.of(new CcFlagsSupplier(ruleContext)));
     for (String value : values) {
       if (isLinkoptLabel(value)) {
         if (!expandLabel(ruleContext, result, value)) {
@@ -215,8 +209,7 @@ public class CppHelper {
             .tokenizeAndExpandMakeVars(
                 result,
                 attrName,
-                value,
-                makeVariableContext);
+                value);
       }
     }
     return result;