Allow/require callers of AbstractPackageLoader to set Skylark semantics explicitly

Previously the default semantics were used unconditionally. Allowing non-default semantics is a feature. Requiring semantics to be specified explicitly helps to avoid unintentional divergence from the caller's intended semantics. We recently did the same thing for Skylark's Environment.Builder (https://github.com/bazelbuild/bazel/commit/b368b39f8ba1e8e8a67af50e5ade9127b2b149d7).

Also pass Skylark semantics through Package.Builder.Helper, so that the extra verification done for shell tests uses the same semantics as the build.

RELNOTES: None
PiperOrigin-RevId: 173544885

1 files changed, 30 insertions, 2 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
index 9ea8e98492..3462284f63 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java
@@ -111,6 +111,7 @@ public abstract class AbstractPackageLoader implements PackageLoader {
   };
   private final Reporter reporter;
   protected final RuleClassProvider ruleClassProvider;
+  protected SkylarkSemantics skylarkSemantics;
   protected final ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions;
   protected final AtomicReference<PathPackageLocator> pkgLocatorRef;
   protected final ExternalFilesHelper externalFilesHelper;
@@ -124,6 +125,7 @@ public abstract class AbstractPackageLoader implements PackageLoader {
     protected final Path workspaceDir;
     protected final BlazeDirectories directories;
     protected RuleClassProvider ruleClassProvider = getDefaultRuleClassProvider();
+    protected SkylarkSemantics skylarkSemantics;
     protected Reporter reporter = new Reporter(new EventBus());
     protected Map<SkyFunctionName, SkyFunction> extraSkyFunctions = new HashMap<>();
     protected List<PrecomputedValue.Injected> extraPrecomputedValues = new ArrayList<>();
@@ -146,6 +148,16 @@ public abstract class AbstractPackageLoader implements PackageLoader {
       return this;
     }
 
+    public Builder setSkylarkSemantics(SkylarkSemantics semantics) {
+      this.skylarkSemantics = semantics;
+      return this;
+    }
+
+    public Builder useDefaultSkylarkSemantics() {
+      this.skylarkSemantics = SkylarkSemantics.DEFAULT_SEMANTICS;
+      return this;
+    }
+
     public Builder setReporter(Reporter reporter) {
       this.reporter = reporter;
       return this;
@@ -177,7 +189,20 @@ public abstract class AbstractPackageLoader implements PackageLoader {
       return this;
     }
 
-    public abstract PackageLoader build();
+    /** Throws {@link IllegalArgumentException} if builder args are incomplete/inconsistent. */
+    protected void validate() {
+      if (skylarkSemantics == null) {
+        throw new IllegalArgumentException(
+            "must call either setSkylarkSemantics or useDefaultSkylarkSemantics");
+      }
+    }
+
+    public final PackageLoader build() {
+      validate();
+      return buildImpl();
+    }
+
+    protected abstract PackageLoader buildImpl();
 
     protected abstract RuleClassProvider getDefaultRuleClassProvider();
 
@@ -189,6 +214,7 @@ public abstract class AbstractPackageLoader implements PackageLoader {
     PathPackageLocator pkgLocator =
         new PathPackageLocator(null, ImmutableList.of(workspaceDir));
     this.ruleClassProvider = builder.ruleClassProvider;
+    this.skylarkSemantics = builder.skylarkSemantics;
     this.reporter = builder.reporter;
     this.extraSkyFunctions = ImmutableMap.copyOf(builder.extraSkyFunctions);
     this.pkgLocatorRef = new AtomicReference<>(pkgLocator);
@@ -210,12 +236,14 @@ public abstract class AbstractPackageLoader implements PackageLoader {
     };
     this.preinjectedDiff =
         makePreinjectedDiff(
+            skylarkSemantics,
             pkgLocator,
             builder.defaultsPackageContents,
             ImmutableList.copyOf(builder.extraPrecomputedValues));
   }
 
   private static ImmutableDiff makePreinjectedDiff(
+      SkylarkSemantics skylarkSemantics,
       PathPackageLocator pkgLocator,
       String defaultsPackageContents,
       ImmutableList<PrecomputedValue.Injected> extraPrecomputedValues) {
@@ -237,7 +265,7 @@ public abstract class AbstractPackageLoader implements PackageLoader {
     }
     PrecomputedValue.PATH_PACKAGE_LOCATOR.set(injectable, pkgLocator);
     PrecomputedValue.DEFAULT_VISIBILITY.set(injectable, ConstantRuleVisibility.PRIVATE);
-    PrecomputedValue.SKYLARK_SEMANTICS.set(injectable, SkylarkSemantics.DEFAULT_SEMANTICS);
+    PrecomputedValue.SKYLARK_SEMANTICS.set(injectable, skylarkSemantics);
     PrecomputedValue.DEFAULTS_PACKAGE_CONTENTS.set(injectable, defaultsPackageContents);
     PrecomputedValue.BLACKLISTED_PACKAGE_PREFIXES_FILE.set(injectable, PathFragment.EMPTY_FRAGMENT);
     return new ImmutableDiff(ImmutableList.<SkyKey>of(), valuesToInject);