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

5 files changed, 44 insertions, 10 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/Package.java b/src/main/java/com/google/devtools/build/lib/packages/Package.java
index c6c5d038ec..4019e871f6 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Package.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Package.java
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.events.ExtendedEventHandler.Postable;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.packages.AttributeMap.AcceptsLabelAttribute;
 import com.google.devtools.build.lib.packages.License.DistributionType;
+import com.google.devtools.build.lib.syntax.SkylarkSemantics;
 import com.google.devtools.build.lib.util.Preconditions;
 import com.google.devtools.build.lib.util.SpellChecker;
 import com.google.devtools.build.lib.vfs.Canonicalizer;
@@ -713,9 +714,10 @@ public class Package {
 
       /**
        * Called after {@link com.google.devtools.build.lib.skyframe.PackageFunction} is completely
-       * done loading the given {@link Package}.
+       * done loading the given {@link Package}. {@code skylarkSemantics} are the semantics used to
+       * evaluate the build.
        */
-      void onLoadingComplete(Package pkg);
+      void onLoadingComplete(Package pkg, SkylarkSemantics skylarkSemantics);
     }
 
     /** {@link Helper} that simply calls the {@link Package} constructor. */
@@ -731,7 +733,7 @@ public class Package {
       }
 
       @Override
-      public void onLoadingComplete(Package pkg) {
+      public void onLoadingComplete(Package pkg, SkylarkSemantics skylarkSemantics) {
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index 015ac01b74..e144aa8e14 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -1607,8 +1607,8 @@ public final class PackageFactory {
    * Called by a caller of {@link #createPackageFromPreprocessingAst} after this caller has fully
    * loaded the package.
    */
-  public void afterDoneLoadingPackage(Package pkg) {
-    packageBuilderHelper.onLoadingComplete(pkg);
+  public void afterDoneLoadingPackage(Package pkg, SkylarkSemantics skylarkSemantics) {
+    packageBuilderHelper.onLoadingComplete(pkg, skylarkSemantics);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index c5d723293e..4893ac0091 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -423,6 +423,10 @@ public class PackageFunction implements SkyFunction {
    */
   private SkyValue getExternalPackage(Environment env, Path packageLookupPath)
       throws PackageFunctionException, InterruptedException {
+    SkylarkSemantics skylarkSemantics = PrecomputedValue.SKYLARK_SEMANTICS.get(env);
+    if (skylarkSemantics == null) {
+      return null;
+    }
     RootedPath workspacePath = RootedPath.toRootedPath(
         packageLookupPath, Label.EXTERNAL_PACKAGE_FILE_NAME);
     SkyKey workspaceKey = ExternalPackageFunction.key(workspacePath);
@@ -458,7 +462,7 @@ public class PackageFunction implements SkyFunction {
     }
 
     if (packageFactory != null) {
-      packageFactory.afterDoneLoadingPackage(pkg);
+      packageFactory.afterDoneLoadingPackage(pkg, skylarkSemantics);
     }
     return new PackageValue(pkg);
   }
@@ -621,7 +625,7 @@ public class PackageFunction implements SkyFunction {
     // We know this SkyFunction will not be called again, so we can remove the cache entry.
     packageFunctionCache.invalidate(packageId);
 
-    packageFactory.afterDoneLoadingPackage(pkg);
+    packageFactory.afterDoneLoadingPackage(pkg, skylarkSemantics);
     return new PackageValue(pkg);
   }
 
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);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java
index 159b69bedc..e32a18af0b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/packages/BazelPackageLoader.java
@@ -81,7 +81,7 @@ public class BazelPackageLoader extends AbstractPackageLoader {
     }
 
     @Override
-    public BazelPackageLoader build() {
+    public BazelPackageLoader buildImpl() {
       return new BazelPackageLoader(this);
     }