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

7 files changed, 58 insertions, 125 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 8aaaaa44e0..9a114f544f 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -960,6 +960,18 @@ public final class RuleContext extends TargetContext
     return result;
   }
 
+  public void initConfigurationMakeVariableContext(
+      Iterable<? extends MakeVariableSupplier> makeVariableSuppliers) {
+    Preconditions.checkState(configurationMakeVariableContext == null);
+    configurationMakeVariableContext =
+        new ConfigurationMakeVariableContext(
+            this, getRule().getPackage(), getConfiguration(), makeVariableSuppliers);
+  }
+
+  public void initConfigurationMakeVariableContext(MakeVariableSupplier... makeVariableSuppliers) {
+    initConfigurationMakeVariableContext(ImmutableList.copyOf(makeVariableSuppliers));
+  }
+
   /** Indicates whether a string list attribute should be tokenized. */
   public enum Tokenize {
     YES,
@@ -974,22 +986,7 @@ public final class RuleContext extends TargetContext
    * @return a list of strings containing the expanded and tokenized values for the attribute
    */
   public ImmutableList<String> getTokenizedStringListAttr(String attributeName) {
-    return getExpandedStringListAttr(
-        attributeName, Tokenize.YES, getConfigurationMakeVariableContext());
-  }
-
-  /**
-   * Gets an attribute of type STRING_LIST expanding Make variables, $(location) tags into the
-   * dependency location (see {@link LocationExpander} for details) and tokenizes the result.
-   *
-   * @param attributeName the name of the attribute to process
-   * @param makeVariableContext the make variable context
-   * @return a list of strings containing the expanded and tokenized values for the attribute
-   */
-  public ImmutableList<String> getTokenizedStringListAttr(
-      String attributeName, ConfigurationMakeVariableContext makeVariableContext) {
-    return getExpandedStringListAttr(
-        attributeName, Tokenize.YES, makeVariableContext);
+    return getExpandedStringListAttr(attributeName, Tokenize.YES);
   }
 
   /**
@@ -1000,8 +997,7 @@ public final class RuleContext extends TargetContext
    * @return a list of strings containing the processed values for the attribute
    */
   public ImmutableList<String> getExpandedStringListAttr(String attributeName) {
-    return getExpandedStringListAttr(
-        attributeName, Tokenize.NO, getConfigurationMakeVariableContext());
+    return getExpandedStringListAttr(attributeName, Tokenize.NO);
   }
 
   /**
@@ -1009,13 +1005,11 @@ public final class RuleContext extends TargetContext
    * optionally tokenizes the result.
    *
    * @param attributeName the name of the attribute to process
-   * @param makeVariableContext the make variable context
    * @return a list of strings containing the processed values for the attribute
    */
   private ImmutableList<String> getExpandedStringListAttr(
       String attributeName,
-      Tokenize tokenize,
-      ConfigurationMakeVariableContext makeVariableContext) {
+      Tokenize tokenize) {
     if (!getRule().isAttrDefined(attributeName, Type.STRING_LIST)) {
       // TODO(bazel-team): This should be an error.
       return ImmutableList.of();
@@ -1029,27 +1023,21 @@ public final class RuleContext extends TargetContext
         new LocationExpander(this, LocationExpander.Options.ALLOW_DATA);
 
     for (String token : original) {
-      expandValue(tokens, attributeName, token, locationExpander, tokenize, makeVariableContext);
+      expandValue(tokens, attributeName, token, locationExpander, tokenize);
     }
     return ImmutableList.copyOf(tokens);
   }
 
   /**
    * Expands make variables in value and tokenizes the result into tokens.
-   *
-   * @param makeVariableContext the make variable context
-   *     <p>This methods should be called only during initialization.
    */
   public void tokenizeAndExpandMakeVars(
       List<String> result,
       String attributeName,
-      String value,
-      ConfigurationMakeVariableContext makeVariableContext) {
+      String value) {
     LocationExpander locationExpander =
         new LocationExpander(this, Options.ALLOW_DATA, Options.EXEC_PATHS);
-    expandValue(
-        result, attributeName, value, locationExpander, Tokenize.YES,
-        makeVariableContext);
+    expandValue(result, attributeName, value, locationExpander, Tokenize.YES);
   }
 
   /**
@@ -1060,12 +1048,11 @@ public final class RuleContext extends TargetContext
       String attributeName,
       String value,
       @Nullable LocationExpander locationExpander,
-      Tokenize tokenize,
-      ConfigurationMakeVariableContext makeVariableContext) {
+      Tokenize tokenize) {
     if (locationExpander != null) {
       value = locationExpander.expandAttribute(attributeName, value);
     }
-    value = expandMakeVariables(attributeName, value, makeVariableContext);
+    value = expandMakeVariables(attributeName, value);
     if (tokenize == Tokenize.YES) {
       try {
         ShellUtils.tokenize(tokens, value);
@@ -1098,33 +1085,12 @@ public final class RuleContext extends TargetContext
   }
 
   /**
-   * Returns a (cached! read on) context that maps Make variable names (string) to values (string)
-   * without any extra {@link MakeVariableSupplier}.
-   *
-   * <p>Beware!!! {@link ConfigurationMakeVariableContext} instance is cached, so if you call it
-   * first with some list of {@link MakeVariableSupplier} and then with other list, you will always
-   * get the first instance back. TODO(hlopko): Extract Make variable expansion from RuleContext and
-   * fix all the callers
-   *
-   * @return a ConfigurationMakeVariableContext.
+   * Returns a cached context that maps Make variable names (string) to values (string) without any
+   * extra {@link MakeVariableSupplier}.
    */
   public ConfigurationMakeVariableContext getConfigurationMakeVariableContext() {
-    return getConfigurationMakeVariableContext(ImmutableList.<MakeVariableSupplier>of());
-  }
-
-  /**
-   * Returns a (cached! read on) context that maps Make variable names (string) to values (string).
-   *
-   * @see #getConfigurationMakeVariableContext() to understand how the instance is cached!
-   * @param makeVariableSuppliers to be used with {@link ConfigurationMakeVariableContext}
-   * @return a ConfigurationMakeVariableContext.
-   */
-  public ConfigurationMakeVariableContext getConfigurationMakeVariableContext(
-      Iterable<? extends MakeVariableSupplier> makeVariableSuppliers) {
     if (configurationMakeVariableContext == null) {
-      configurationMakeVariableContext =
-          new ConfigurationMakeVariableContext(
-              this, getRule().getPackage(), getConfiguration(), makeVariableSuppliers);
+      initConfigurationMakeVariableContext(ImmutableList.<MakeVariableSupplier>of());
     }
     return configurationMakeVariableContext;
   }
@@ -1138,7 +1104,7 @@ public final class RuleContext extends TargetContext
    */
   public String expandedMakeVariables(String attributeName) {
     String expression = attributes().get(attributeName, Type.STRING);
-    return expandMakeVariables(attributeName, expression, getConfigurationMakeVariableContext());
+    return expandMakeVariables(attributeName, expression);
   }
 
   /**
@@ -1181,8 +1147,7 @@ public final class RuleContext extends TargetContext
   public List<String> expandedMakeVariablesList(String attrName) {
     List<String> variables = new ArrayList<>();
     for (String variable : attributes().get(attrName, Type.STRING_LIST)) {
-      variables.add(
-          expandMakeVariables(attrName, variable, getConfigurationMakeVariableContext()));
+      variables.add(expandMakeVariables(attrName, variable));
     }
     return variables;
   }
@@ -1194,16 +1159,13 @@ public final class RuleContext extends TargetContext
    * @param attrName the name of the attribute from which "expression" comes; used for error
    *     reporting.
    * @param expression the string to expand.
-   * @param makeVariableContext the make variable context
    * @return the expansion of "expression", or null.
    */
   @Nullable
-  public String expandSingleMakeVariable(
-      String attrName,
-      String expression,
-      ConfigurationMakeVariableContext makeVariableContext) {
+  public String expandSingleMakeVariable(String attrName, String expression) {
     try {
-      return MakeVariableExpander.expandSingleVariable(expression, makeVariableContext);
+      return MakeVariableExpander
+          .expandSingleVariable(expression, getConfigurationMakeVariableContext());
     } catch (MakeVariableExpander.ExpansionException e) {
       attributeError(attrName, e.getMessage());
       return expression;
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
index d8cc52f7c2..7f4c12d14a 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
@@ -395,10 +395,7 @@ public final class RunfilesSupport {
         ruleContext,
         executable,
         runfiles,
-        computeArgs(
-            ruleContext,
-            CommandLine.EMPTY,
-            ruleContext.getConfigurationMakeVariableContext()));
+        computeArgs(ruleContext, CommandLine.EMPTY));
   }
 
   /**
@@ -411,10 +408,7 @@ public final class RunfilesSupport {
         ruleContext,
         executable,
         runfiles,
-        computeArgs(
-            ruleContext,
-            CommandLine.of(appendingArgs),
-            ruleContext.getConfigurationMakeVariableContext()));
+        computeArgs(ruleContext, CommandLine.of(appendingArgs)));
   }
 
   /**
@@ -427,26 +421,13 @@ public final class RunfilesSupport {
         ruleContext,
         executable,
         runfiles,
-        computeArgs(ruleContext, appendingArgs, ruleContext.getConfigurationMakeVariableContext()));
-  }
-
-  public static RunfilesSupport withExecutable(
-      RuleContext ruleContext,
-      Runfiles runfiles,
-      Artifact executable,
-      ConfigurationMakeVariableContext makeVariableContext) {
-    return new RunfilesSupport(
-        ruleContext,
-        executable,
-        runfiles,
-        computeArgs(ruleContext, CommandLine.EMPTY, makeVariableContext));
+        computeArgs(ruleContext, appendingArgs));
   }
 
   private static CommandLine computeArgs(
       RuleContext ruleContext,
-      CommandLine additionalArgs,
-      ConfigurationMakeVariableContext makeVariableContext) {
+      CommandLine additionalArgs) {
     return CommandLine.concat(
-        ruleContext.getTokenizedStringListAttr("args", makeVariableContext), additionalArgs);
+        ruleContext.getTokenizedStringListAttr("args"), additionalArgs);
   }
 }
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;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
index e1e779a13b..56c5185dc0 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.rules.python;
 
-import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
@@ -57,6 +56,7 @@ public abstract class PyBinary implements RuleConfiguredTargetFactory {
 
   static RuleConfiguredTargetBuilder init(RuleContext ruleContext, PythonSemantics semantics,
       PyCommon common) throws InterruptedException {
+    ruleContext.initConfigurationMakeVariableContext(new CcFlagsSupplier(ruleContext));
     CcLinkParamsStore ccLinkParamsStore = initializeCcLinkParamStore(ruleContext);
 
     List<Artifact> srcs = common.validateSrcs();
@@ -91,9 +91,7 @@ public abstract class PyBinary implements RuleConfiguredTargetFactory {
         RunfilesSupport.withExecutable(
             ruleContext,
             defaultRunfiles,
-            common.getExecutable(),
-            ruleContext.getConfigurationMakeVariableContext(
-                ImmutableList.of(new CcFlagsSupplier(ruleContext))));
+            common.getExecutable());
 
     if (ruleContext.hasErrors()) {
       return null;