From 1cbce0fad88d02c76386e8525e03d0f427515ad2 Mon Sep 17 00:00:00 2001
From: laurentlb <laurentlb@google.com>
Date: Tue, 27 Mar 2018 12:43:22 -0700
Subject: [PATCH] New incompatible flag to disable PACKAGE_NAME and
 REPOSITORY_NAME.

RELNOTES[INC]:
  Variables PACKAGE_NAME and REPOSITORY_NAME are deprecated in favor of
  functions `package_name()` and `repository_name()`.
  https://docs.bazel.build/versions/master/skylark/lib/native.html#package_name

PiperOrigin-RevId: 190657188
---
 .../build/lib/packages/PackageFactory.java    | 19 +++++---
 .../lib/packages/SkylarkSemanticsCodec.java   |  2 +
 .../lib/packages/SkylarkSemanticsOptions.java | 14 ++++++
 .../devtools/build/lib/syntax/Identifier.java | 20 +++++++++
 .../build/lib/syntax/SkylarkSemantics.java    | 29 ++++++++++++
 .../lib/packages/PackageFactoryTest.java      | 22 +++++++++
 .../SkylarkSemanticsConsistencyTest.java      |  2 +
 .../util/PackageFactoryApparatus.java         | 45 ++++++++++++++-----
 .../TransitiveTraversalFunctionTest.java      |  8 +++-
 9 files changed, 145 insertions(+), 16 deletions(-)

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 dce457b195d7e4..25fc689d8e316b 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
@@ -1331,7 +1331,13 @@ public Package createPackageForTesting(
       throws NoSuchPackageException, InterruptedException {
     Package externalPkg = newExternalPackageBuilder(
         buildFile.getRelative(Label.WORKSPACE_FILE_NAME), "TESTING").build();
-    return createPackageForTesting(packageId, externalPkg, buildFile, locator, eventHandler);
+    return createPackageForTesting(
+        packageId,
+        externalPkg,
+        buildFile,
+        locator,
+        eventHandler,
+        SkylarkSemantics.DEFAULT_SEMANTICS);
   }
 
   /**
@@ -1344,7 +1350,8 @@ public Package createPackageForTesting(
       Package externalPkg,
       Path buildFile,
       CachingPackageLocator locator,
-      ExtendedEventHandler eventHandler)
+      ExtendedEventHandler eventHandler,
+      SkylarkSemantics semantics)
       throws NoSuchPackageException, InterruptedException {
     String error =
         LabelValidator.validatePackageName(packageId.getPackageFragment().getPathString());
@@ -1372,7 +1379,7 @@ public Package createPackageForTesting(
                 /*imports=*/ ImmutableMap.<String, Extension>of(),
                 /*skylarkFileDependencies=*/ ImmutableList.<Label>of(),
                 /*defaultVisibility=*/ ConstantRuleVisibility.PUBLIC,
-                SkylarkSemantics.DEFAULT_SEMANTICS,
+                semantics,
                 globber)
             .build();
     for (Postable post : result.getPosts()) {
@@ -1544,8 +1551,10 @@ private void buildPkgEnv(
     }
 
     pkgEnv.setupDynamic(PKG_CONTEXT, context);
-    pkgEnv.setupDynamic(Runtime.PKG_NAME, packageId.getPackageFragment().getPathString());
-    pkgEnv.setupDynamic(Runtime.REPOSITORY_NAME, packageId.getRepository().toString());
+    if (!pkgEnv.getSemantics().incompatiblePackageNameIsAFunction()) {
+      pkgEnv.setupDynamic(Runtime.PKG_NAME, packageId.getPackageFragment().getPathString());
+      pkgEnv.setupDynamic(Runtime.REPOSITORY_NAME, packageId.getRepository().toString());
+    }
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java
index 9056342bacee2b..21d5c49aadeb50 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsCodec.java
@@ -52,6 +52,7 @@ public void serialize(
     codedOut.writeBoolNoTag(semantics.incompatibleDisallowThreeArgVardef());
     codedOut.writeBoolNoTag(semantics.incompatibleDisallowToplevelIfStatement());
     codedOut.writeBoolNoTag(semantics.incompatibleNewActionsApi());
+    codedOut.writeBoolNoTag(semantics.incompatiblePackageNameIsAFunction());
     codedOut.writeBoolNoTag(semantics.incompatibleRemoveNativeGitRepository());
     codedOut.writeBoolNoTag(semantics.incompatibleRemoveNativeHttpArchive());
     codedOut.writeBoolNoTag(semantics.incompatibleStringIsNotIterable());
@@ -73,6 +74,7 @@ public SkylarkSemantics deserialize(DeserializationContext context, CodedInputSt
     builder.incompatibleDisallowThreeArgVardef(codedIn.readBool());
     builder.incompatibleDisallowToplevelIfStatement(codedIn.readBool());
     builder.incompatibleNewActionsApi(codedIn.readBool());
+    builder.incompatiblePackageNameIsAFunction(codedIn.readBool());
     builder.incompatibleRemoveNativeGitRepository(codedIn.readBool());
     builder.incompatibleRemoveNativeHttpArchive(codedIn.readBool());
     builder.incompatibleStringIsNotIterable(codedIn.readBool());
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
index afb730c75c1b2d..003012962e454a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkSemanticsOptions.java
@@ -168,6 +168,19 @@ public class SkylarkSemanticsOptions extends OptionsBase implements Serializable
   )
   public boolean incompatibleNewActionsApi;
 
+  @Option(
+    name = "incompatible_package_name_is_a_function",
+    defaultValue = "false",
+    category = "incompatible changes",
+    documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+    effectTags = {OptionEffectTag.UNKNOWN},
+    metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+    help =
+        "If set to true, the values PACKAGE_NAME and REPOSITORY_NAME are not available. "
+            + "Use the package_name() or repository_name() functions instead."
+  )
+  public boolean incompatiblePackageNameIsAFunction;
+
   @Option(
     name = "incompatible_remove_native_git_repository",
     defaultValue = "false",
@@ -229,6 +242,7 @@ public SkylarkSemantics toSkylarkSemantics() {
         .incompatibleDisallowThreeArgVardef(incompatibleDisallowThreeArgVardef)
         .incompatibleDisallowToplevelIfStatement(incompatibleDisallowToplevelIfStatement)
         .incompatibleNewActionsApi(incompatibleNewActionsApi)
+        .incompatiblePackageNameIsAFunction(incompatiblePackageNameIsAFunction)
         .incompatibleRemoveNativeGitRepository(incompatibleRemoveNativeGitRepository)
         .incompatibleRemoveNativeHttpArchive(incompatibleRemoveNativeHttpArchive)
         .incompatibleStringIsNotIterable(incompatibleStringIsNotIterable)
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
index e201a0e865a6d5..419cc99621d4f1 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -96,6 +96,26 @@ EvalException createInvalidIdentifierException(Set<String> symbols) {
     if (name.equals("$error$")) {
       return new EvalException(getLocation(), "contains syntax error(s)", true);
     }
+
+    if (name.equals("PACKAGE_NAME")) {
+      return new EvalException(
+          getLocation(),
+          "The value 'PACKAGE_NAME' has been removed in favor of 'package_name()', "
+              + "please use the latter ("
+              + "https://docs.bazel.build/versions/master/skylark/lib/native.html#package_name). "
+              + "You can temporarily allow the old name "
+              + "by using --incompatiblePackageNameIsAFunction=false");
+    }
+    if (name.equals("REPOSITORY_NAME")) {
+      return new EvalException(
+          getLocation(),
+          "The value 'REPOSITORY_NAME' has been removed in favor of 'repository_name()', "
+              + "please use the latter ("
+              + "https://docs.bazel.build/versions/master/skylark/lib/native.html#repository_name)."
+              + " You can temporarily allow the old name "
+              + "by using --incompatiblePackageNameIsAFunction=false");
+    }
+
     String suggestion = SpellChecker.didYouMean(name, symbols);
     return new EvalException(getLocation(), "name '" + name + "' is not defined" + suggestion);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
index 05208dcdd06ab9..46f3f1ae941009 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkSemantics.java
@@ -40,17 +40,31 @@ public abstract class SkylarkSemantics {
 
   // <== Add new options here in alphabetic order ==>
   public abstract boolean incompatibleBzlDisallowLoadAfterStatement();
+
   public abstract boolean incompatibleDepsetIsNotIterable();
+
   public abstract boolean incompatibleDepsetUnion();
+
   public abstract boolean incompatibleDisableGlobTracking();
+
   public abstract boolean incompatibleDisableObjcProviderResources();
+
   public abstract boolean incompatibleDisallowDictPlus();
+
   public abstract boolean incompatibleDisallowThreeArgVardef();
+
   public abstract boolean incompatibleDisallowToplevelIfStatement();
+
   public abstract boolean incompatibleNewActionsApi();
+
+  public abstract boolean incompatiblePackageNameIsAFunction();
+
   public abstract boolean incompatibleRemoveNativeGitRepository();
+
   public abstract boolean incompatibleRemoveNativeHttpArchive();
+
   public abstract boolean incompatibleStringIsNotIterable();
+
   public abstract boolean internalSkylarkFlagTestCanary();
 
   /** Returns a {@link Builder} initialized with the values of this instance. */
@@ -77,6 +91,7 @@ public static Builder builderWithDefaults() {
           .incompatibleDisallowThreeArgVardef(false)
           .incompatibleDisallowToplevelIfStatement(true)
           .incompatibleNewActionsApi(false)
+          .incompatiblePackageNameIsAFunction(false)
           .incompatibleRemoveNativeGitRepository(false)
           .incompatibleRemoveNativeHttpArchive(false)
           .incompatibleStringIsNotIterable(false)
@@ -89,17 +104,31 @@ public abstract static class Builder {
 
     // <== Add new options here in alphabetic order ==>
     public abstract Builder incompatibleBzlDisallowLoadAfterStatement(boolean value);
+
     public abstract Builder incompatibleDepsetIsNotIterable(boolean value);
+
     public abstract Builder incompatibleDepsetUnion(boolean value);
+
     public abstract Builder incompatibleDisableGlobTracking(boolean value);
+
     public abstract Builder incompatibleDisableObjcProviderResources(boolean value);
+
     public abstract Builder incompatibleDisallowDictPlus(boolean value);
+
     public abstract Builder incompatibleDisallowThreeArgVardef(boolean value);
+
     public abstract Builder incompatibleDisallowToplevelIfStatement(boolean value);
+
     public abstract Builder incompatibleNewActionsApi(boolean value);
+
+    public abstract Builder incompatiblePackageNameIsAFunction(boolean value);
+
     public abstract Builder incompatibleRemoveNativeGitRepository(boolean value);
+
     public abstract Builder incompatibleRemoveNativeHttpArchive(boolean value);
+
     public abstract Builder incompatibleStringIsNotIterable(boolean value);
+
     public abstract Builder internalSkylarkFlagTestCanary(boolean value);
 
     public abstract SkylarkSemantics build();
diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
index bbb8be7e8205b2..501037574a36b5 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/PackageFactoryTest.java
@@ -246,6 +246,14 @@ public void testPackageConstant() throws Exception {
     assertThat(Sets.newHashSet(pkg.getTargets(Rule.class)).size()).isSameAs(1);
   }
 
+  @Test
+  public void testPackageConstantIsForbidden() throws Exception {
+    events.setFailFast(false);
+    Path buildFile = scratch.file("/pina/BUILD", "cc_library(name=PACKAGE_NAME + '-colada')");
+    packages.createPackage("pina", buildFile, "--incompatible_package_name_is_a_function=true");
+    events.assertContainsError("The value 'PACKAGE_NAME' has been removed");
+  }
+
   @Test
   public void testPackageNameFunction() throws Exception {
     Path buildFile = scratch.file("/pina/BUILD", "cc_library(name=package_name() + '-colada')");
@@ -271,6 +279,20 @@ public void testPackageConstantInExternalRepository() throws Exception {
     assertThat(AggregatingAttributeMapper.of(c).get("cmd", Type.STRING)).isEqualTo("@a b");
   }
 
+  @Test
+  public void testPackageConstantInExternalRepositoryIsForbidden() throws Exception {
+    events.setFailFast(false);
+    Path buildFile =
+        scratch.file(
+            "/external/a/b/BUILD", "genrule(name='c', srcs=[], outs=['ao'], cmd=REPOSITORY_NAME)");
+    packages.createPackage(
+        PackageIdentifier.create("@a", PathFragment.create("b")),
+        buildFile,
+        events.reporter(),
+        "--incompatible_package_name_is_a_function=true");
+    events.assertContainsError("The value 'REPOSITORY_NAME' has been removed");
+  }
+
   @Test
   public void testPackageFunctionInExternalRepository() throws Exception {
     Path buildFile =
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
index a9a3d517f7662d..7cfedfec14716d 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java
@@ -128,6 +128,7 @@ private static SkylarkSemanticsOptions buildRandomOptions(Random rand) throws Ex
         "--incompatible_disallow_three_arg_vardef=" + rand.nextBoolean(),
         "--incompatible_disallow_toplevel_if_statement=" + rand.nextBoolean(),
         "--incompatible_new_actions_api=" + rand.nextBoolean(),
+        "--incompatible_package_name_is_a_function=" + rand.nextBoolean(),
         "--incompatible_remove_native_git_repository=" + rand.nextBoolean(),
         "--incompatible_remove_native_http_archive=" + rand.nextBoolean(),
         "--incompatible_string_is_not_iterable=" + rand.nextBoolean(),
@@ -150,6 +151,7 @@ private static SkylarkSemantics buildRandomSemantics(Random rand) {
         .incompatibleDisallowThreeArgVardef(rand.nextBoolean())
         .incompatibleDisallowToplevelIfStatement(rand.nextBoolean())
         .incompatibleNewActionsApi(rand.nextBoolean())
+        .incompatiblePackageNameIsAFunction(rand.nextBoolean())
         .incompatibleRemoveNativeGitRepository(rand.nextBoolean())
         .incompatibleRemoveNativeHttpArchive(rand.nextBoolean())
         .incompatibleStringIsNotIterable(rand.nextBoolean())
diff --git a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
index e91b2ded27a2b0..6fa2949ec4105e 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/util/PackageFactoryApparatus.java
@@ -30,6 +30,7 @@
 import com.google.devtools.build.lib.packages.PackageFactory;
 import com.google.devtools.build.lib.packages.PackageFactory.LegacyGlobber;
 import com.google.devtools.build.lib.packages.RuleClassProvider;
+import com.google.devtools.build.lib.packages.SkylarkSemanticsOptions;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
 import com.google.devtools.build.lib.syntax.Environment.Extension;
 import com.google.devtools.build.lib.syntax.ParserInputSource;
@@ -39,6 +40,7 @@
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.common.options.OptionsParser;
 import java.io.IOException;
 
 /**
@@ -76,12 +78,15 @@ private CachingPackageLocator getPackageLocator() {
     return createEmptyLocator();
   }
 
-  /**
-   * Parses and evaluates {@code buildFile} and returns the resulting {@link Package} instance.
-   */
+  /** Parses and evaluates {@code buildFile} and returns the resulting {@link Package} instance. */
   public Package createPackage(String packageName, Path buildFile) throws Exception {
-    return createPackage(PackageIdentifier.createInMainRepo(packageName), buildFile,
-        eventHandler);
+    return createPackage(PackageIdentifier.createInMainRepo(packageName), buildFile, eventHandler);
+  }
+
+  public Package createPackage(String packageName, Path buildFile, String skylarkOption)
+      throws Exception {
+    return createPackage(
+        PackageIdentifier.createInMainRepo(packageName), buildFile, eventHandler, skylarkOption);
   }
 
   /**
@@ -89,21 +94,41 @@ public Package createPackage(String packageName, Path buildFile) throws Exceptio
    * resulting {@link Package} instance.
    */
   public Package createPackage(
-      PackageIdentifier packageIdentifier, Path buildFile, ExtendedEventHandler reporter)
+      PackageIdentifier packageIdentifier,
+      Path buildFile,
+      ExtendedEventHandler reporter,
+      String skylarkOption)
       throws Exception {
+
+    OptionsParser parser = OptionsParser.newOptionsParser(SkylarkSemanticsOptions.class);
+    parser.parse(
+        skylarkOption == null
+            ? ImmutableList.<String>of()
+            : ImmutableList.<String>of(skylarkOption));
+    SkylarkSemantics semantics =
+        parser.getOptions(SkylarkSemanticsOptions.class).toSkylarkSemantics();
+
     try {
+      Package externalPkg =
+          factory
+              .newExternalPackageBuilder(
+                  buildFile.getRelative(Label.WORKSPACE_FILE_NAME), "TESTING")
+              .build();
       Package pkg =
           factory.createPackageForTesting(
-              packageIdentifier,
-              buildFile,
-              getPackageLocator(),
-              reporter);
+              packageIdentifier, externalPkg, buildFile, getPackageLocator(), reporter, semantics);
       return pkg;
     } catch (InterruptedException e) {
       throw new IllegalStateException(e);
     }
   }
 
+  public Package createPackage(
+      PackageIdentifier packageIdentifier, Path buildFile, ExtendedEventHandler reporter)
+      throws Exception {
+    return createPackage(packageIdentifier, buildFile, reporter, null);
+  }
+
   /**
    * Parses the {@code buildFile} into a {@link BuildFileAST}.
    */
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java
index 80a07e0b8bb670..7749f9e9ff8439 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TransitiveTraversalFunctionTest.java
@@ -24,6 +24,7 @@
 import com.google.devtools.build.lib.packages.NoSuchTargetException;
 import com.google.devtools.build.lib.packages.Package;
 import com.google.devtools.build.lib.skyframe.TransitiveBaseTraversalFunction.TargetAndErrorIfAnyImpl;
+import com.google.devtools.build.lib.syntax.SkylarkSemantics;
 import com.google.devtools.build.lib.util.GroupedList;
 import com.google.devtools.build.lib.util.GroupedList.GroupedListHelper;
 import com.google.devtools.build.lib.vfs.Path;
@@ -104,6 +105,11 @@ private Package scratchPackage(String workspaceName, PackageIdentifier packageId
             Package.Builder.DefaultHelper.INSTANCE, buildFile.getRelative("WORKSPACE"), "TESTING");
     externalPkg.setWorkspaceName(workspaceName);
     return pkgFactory.createPackageForTesting(
-        packageId, externalPkg.build(), buildFile, packageIdentifier -> buildFile, reporter);
+        packageId,
+        externalPkg.build(),
+        buildFile,
+        packageIdentifier -> buildFile,
+        reporter,
+        SkylarkSemantics.DEFAULT_SEMANTICS);
   }
 }