Automated rollback of commit 2d5b9f35a859bbeec4df66fc6c2256db299dd56a.
*** Reason for rollback ***
http://b/138789815#comment19
*** Original change description ***
Add a flag that makes two Starlark functions equal if they are defined at the same location.
This flag, when set, mostly reverts https://github.com/bazelbuild/bazel/commit/af7fc3b75a62edc3b4b62de25b733b03c3ff925e (only mostly because it only applies to user-defined functions)
BUG=138789815
RELNOTES: None.
PiperOrigin-RevId: 261333026
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
index 061c986..2e384a7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkRuleClassFunctions.java
@@ -243,8 +243,7 @@
};
@Override
- public Provider provider(String doc, Object fields, Location location, Environment env)
- throws EvalException {
+ public Provider provider(String doc, Object fields, Location location) throws EvalException {
Iterable<String> fieldNames = null;
if (fields instanceof SkylarkList<?>) {
@SuppressWarnings("unchecked")
@@ -261,8 +260,7 @@
"Expected list of strings or dictionary of string -> string for 'fields'");
fieldNames = dict.keySet();
}
- return SkylarkProvider.createUnexportedSchemaful(
- fieldNames, location, env.getSemantics().experimentalFunctionEqualityByLocation());
+ return SkylarkProvider.createUnexportedSchemaful(fieldNames, location);
}
// TODO(bazel-team): implement attribute copy and other rule properties
diff --git a/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java b/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
index acc739e..e7375ec 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/NativeProvider.java
@@ -78,7 +78,7 @@
Class<V> valueClass,
String name,
FunctionSignature.WithValues<Object, SkylarkType> signature) {
- super(name, signature, Location.BUILTIN, /* equalityByLocation */ false);
+ super(name, signature, Location.BUILTIN);
Class<? extends NativeProvider<?>> clazz = (Class<? extends NativeProvider<?>>) getClass();
key = new NativeKey(name, clazz);
this.valueClass = valueClass;
diff --git a/src/main/java/com/google/devtools/build/lib/packages/ProviderFromFunction.java b/src/main/java/com/google/devtools/build/lib/packages/ProviderFromFunction.java
index 6edd60e..a293393 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/ProviderFromFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/ProviderFromFunction.java
@@ -42,9 +42,8 @@
protected ProviderFromFunction(
@Nullable String name,
FunctionSignature.WithValues<Object, SkylarkType> signature,
- Location location,
- boolean equalityByLocation) {
- super(name, signature, location, equalityByLocation);
+ Location location) {
+ super(name, signature, location);
}
public SkylarkProviderIdentifier id() {
diff --git a/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java b/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java
index c3d9c33..4d42b45 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/SkylarkProvider.java
@@ -75,10 +75,8 @@
* @param location the location of the Skylark definition for this provider (tests may use {@link
* Location#BUILTIN})
*/
- @VisibleForTesting
public static SkylarkProvider createUnexportedSchemaless(Location location) {
- return new SkylarkProvider(
- /*key=*/ null, /*fields=*/ null, location, /* equalityByLocation */ false);
+ return new SkylarkProvider(/*key=*/ null, /*fields=*/ null, location);
}
/**
@@ -92,12 +90,9 @@
* Location#BUILTIN})
*/
public static SkylarkProvider createUnexportedSchemaful(
- Iterable<String> fields, Location location, boolean equalityByLocation) {
+ Iterable<String> fields, Location location) {
return new SkylarkProvider(
- /*key=*/ null,
- fields == null ? null : ImmutableList.copyOf(fields),
- location,
- equalityByLocation);
+ /*key=*/ null, fields == null ? null : ImmutableList.copyOf(fields), location);
}
/**
@@ -107,9 +102,8 @@
* @param location the location of the Skylark definition for this provider (tests may use {@link
* Location#BUILTIN})
*/
- @VisibleForTesting
public static SkylarkProvider createExportedSchemaless(SkylarkKey key, Location location) {
- return new SkylarkProvider(key, /*fields=*/ null, location, /* equalityByLocation */ false);
+ return new SkylarkProvider(key, /*fields=*/ null, location);
}
/**
@@ -121,14 +115,9 @@
* @param location the location of the Skylark definition for this provider (tests may use {@link
* Location#BUILTIN})
*/
- @VisibleForTesting
public static SkylarkProvider createExportedSchemaful(
SkylarkKey key, Iterable<String> fields, Location location) {
- return new SkylarkProvider(
- key,
- fields == null ? null : ImmutableList.copyOf(fields),
- location, /* equalityByLocation */
- false);
+ return new SkylarkProvider(key, fields == null ? null : ImmutableList.copyOf(fields), location);
}
/**
@@ -138,13 +127,10 @@
* is schemaless.
*/
private SkylarkProvider(
- @Nullable SkylarkKey key,
- @Nullable ImmutableList<String> fields,
- Location location,
- boolean equalityByLocation) {
+ @Nullable SkylarkKey key, @Nullable ImmutableList<String> fields, Location location) {
// We override getName() in order to use the name that is assigned when export() is called.
// Hence BaseFunction's constructor gets a null name.
- super(/*name=*/ null, buildSignature(fields), location, equalityByLocation);
+ super(/*name=*/ null, buildSignature(fields), location);
this.layout = fields == null ? null : new Layout(fields);
this.key = key; // possibly null
this.errorMessageFormatForUnknownField =
diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
index 0a484d1..3211834 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java
@@ -704,15 +704,6 @@
help = "If set to true, unknown string escapes like `\\a` become rejected.")
public boolean incompatibleRestrictStringEscapes;
- @Option(
- name = "experimental_function_equality_by_location",
- defaultValue = "false",
- documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
- effectTags = OptionEffectTag.BUILD_FILE_SEMANTICS,
- help =
- "If set to true, two Starlark functions defined at the same place are considered equal.")
- public boolean experimentalFunctionEqualityByLocation;
-
/**
* An interner to reduce the number of StarlarkSemantics instances. A single Blaze instance should
* never accumulate a large number of these and being able to shortcut on object identity makes a
@@ -783,7 +774,6 @@
.incompatibleAssignmentIdentifiersHaveLocalScope(
incompatibleAssignmentIdentifiersHaveLocalScope)
.incompatibleDisallowHashingFrozenMutables(incompatibleDisallowHashingFrozenMutables)
- .experimentalFunctionEqualityByLocation(experimentalFunctionEqualityByLocation)
.build();
return INTERNER.intern(semantics);
}
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java
index d5b5533..8d85fc7 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkbuildapi/SkylarkRuleFunctionsApi.java
@@ -86,10 +86,8 @@
positional = false,
defaultValue = "None")
},
- useLocation = true,
- useEnvironment = true)
- public ProviderApi provider(String doc, Object fields, Location location, Environment env)
- throws EvalException;
+ useLocation = true)
+ public ProviderApi provider(String doc, Object fields, Location location) throws EvalException;
@SkylarkCallable(
name = "rule",
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
index 575458c..0d3c49a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -26,7 +26,6 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
-import java.util.Objects;
import javax.annotation.Nullable;
/**
@@ -56,7 +55,6 @@
// (that FuncallExpression must supply), optimizing for the all-positional and all-keyword cases.
// Also, use better pure maps to minimize map O(n) re-creation events when processing keyword maps.
public abstract class BaseFunction implements StarlarkFunction {
- private final boolean equalityByLocation;
/**
* The name of the function.
@@ -132,7 +130,7 @@
* {@link #getName}; otherwise it must be non-null.
*/
public BaseFunction(@Nullable String name) {
- this(name, null, null, false);
+ this.name = name;
}
/**
@@ -145,12 +143,10 @@
public BaseFunction(
@Nullable String name,
@Nullable FunctionSignature.WithValues<Object, SkylarkType> signature,
- @Nullable Location location,
- boolean equalityByLocation) {
- this.name = name;
+ @Nullable Location location) {
+ this(name);
this.signature = signature;
this.location = location;
- this.equalityByLocation = equalityByLocation;
}
/**
@@ -162,7 +158,7 @@
public BaseFunction(
@Nullable String name,
@Nullable FunctionSignature.WithValues<Object, SkylarkType> signature) {
- this(name, signature, null, false);
+ this(name, signature, null);
}
/**
@@ -172,7 +168,7 @@
* @param signature the signature, without default values or types
*/
public BaseFunction(@Nullable String name, FunctionSignature signature) {
- this(name, FunctionSignature.WithValues.create(signature), null, false);
+ this(name, FunctionSignature.WithValues.create(signature), null);
}
/**
@@ -182,9 +178,8 @@
* @param defaultValues a list of default values for the optional arguments to be configured.
*/
public BaseFunction(@Nullable String name, @Nullable Iterable<Object> defaultValues) {
- this.name = name;
+ this(name);
this.unconfiguredDefaultValues = defaultValues;
- this.equalityByLocation = false;
}
/** Get parameter documentation as a list corresponding to each parameter */
@@ -579,32 +574,6 @@
return builder.toString();
}
- @Override
- public boolean equals(@Nullable Object other) {
- if (equalityByLocation) {
- if (other instanceof BaseFunction) {
- BaseFunction that = (BaseFunction) other;
- // In theory, the location alone unambiguously identifies a given function. However, in
- // some test cases the location might not have a valid value, thus we also check the name.
- return Objects.equals(this.getName(), that.getName())
- && Objects.equals(this.location, that.location);
- } else {
- return false;
- }
- } else {
- return super.equals(other);
- }
- }
-
- @Override
- public int hashCode() {
- if (equalityByLocation) {
- return Objects.hash(getName(), location);
- } else {
- return super.hashCode();
- }
- }
-
@Nullable
public Location getLocation() {
return location;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
index 35d3350..3199952 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java
@@ -126,7 +126,6 @@
new UserDefinedFunction(
node.getIdentifier().getName(),
node.getIdentifier().getLocation(),
- env.getSemantics().experimentalFunctionEqualityByLocation(),
FunctionSignature.WithValues.create(sig, defaultValues, /*types=*/ null),
node.getStatements(),
env.getGlobals()));
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
index 7454580..9860f6f 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java
@@ -222,8 +222,6 @@
public abstract boolean incompatibleDisallowHashingFrozenMutables();
- public abstract boolean experimentalFunctionEqualityByLocation();
-
@Memoized
@Override
public abstract int hashCode();
@@ -306,7 +304,6 @@
.incompatibleAllowTagsPropagation(false)
.incompatibleAssignmentIdentifiersHaveLocalScope(false)
.incompatibleDisallowHashingFrozenMutables(false)
- .experimentalFunctionEqualityByLocation(false)
.build();
/** Builder for {@link StarlarkSemantics}. All fields are mandatory. */
@@ -413,8 +410,6 @@
public abstract Builder incompatibleDisallowHashingFrozenMutables(boolean value);
- public abstract Builder experimentalFunctionEqualityByLocation(boolean value);
-
public abstract StarlarkSemantics build();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
index c39bdd2..460bc35 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
@@ -36,11 +36,10 @@
public UserDefinedFunction(
String name,
Location location,
- boolean equalityByLocation,
FunctionSignature.WithValues<Object, SkylarkType> signature,
ImmutableList<Statement> statements,
Environment.GlobalFrame definitionGlobals) {
- super(name, signature, location, equalityByLocation);
+ super(name, signature, location);
this.statements = statements;
this.definitionGlobals = definitionGlobals;
}
diff --git a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java
index 121d233..32a5b56 100644
--- a/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java
+++ b/src/main/java/com/google/devtools/build/skydoc/fakebuildapi/FakeSkylarkRuleFunctionsApi.java
@@ -84,8 +84,7 @@
}
@Override
- public ProviderApi provider(String doc, Object fields, Location location, Environment env)
- throws EvalException {
+ public ProviderApi provider(String doc, Object fields, Location location) throws EvalException {
FakeProviderApi fakeProvider = new FakeProviderApi();
// Field documentation will be output preserving the order in which the fields are listed.
ImmutableList.Builder<ProviderFieldInfo> providerFieldInfos = ImmutableList.builder();
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
index 826b89c..37e28b4 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkInfoTest.java
@@ -81,8 +81,7 @@
@Test
public void givenLayoutTakesPrecedenceOverProviderLayout() throws Exception {
SkylarkProvider provider =
- SkylarkProvider.createUnexportedSchemaful(
- ImmutableList.of("f1", "f2"), Location.BUILTIN, /* equalityByLocation */ false);
+ SkylarkProvider.createUnexportedSchemaful(ImmutableList.of("f1", "f2"), Location.BUILTIN);
SkylarkInfo info =
SkylarkInfo.createSchemaful(
provider, invertedLayoutF2F1, new Object[]{5, 4}, Location.BUILTIN);
diff --git a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
index cde858d..0c81e9b 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/SkylarkProviderTest.java
@@ -71,9 +71,8 @@
@Test
public void schemafulProvider_Instantiation() throws Exception {
- SkylarkProvider provider =
- SkylarkProvider.createUnexportedSchemaful(
- ImmutableList.of("a", "b", "c"), /*location=*/ null, false);
+ SkylarkProvider provider = SkylarkProvider.createUnexportedSchemaful(
+ ImmutableList.of("a", "b", "c"), /*location=*/ null);
SkylarkInfo info = instantiateWithA1B2C3(provider);
assertThat(info.isCompact()).isTrue();
assertHasExactlyValuesA1B2C3(info);
@@ -87,9 +86,8 @@
@Test
public void schemafulProvider_GetFields() throws Exception {
- SkylarkProvider provider =
- SkylarkProvider.createUnexportedSchemaful(
- ImmutableList.of("a", "b", "c"), /*location=*/ null, false);
+ SkylarkProvider provider = SkylarkProvider.createUnexportedSchemaful(
+ ImmutableList.of("a", "b", "c"), /*location=*/ null);
assertThat(provider.getFields()).containsExactly("a", "b", "c").inOrder();
}
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 a61b471..ce01c46 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
@@ -172,8 +172,7 @@
"--incompatible_disable_partition_default_parameter=" + rand.nextBoolean(),
"--incompatible_assignment_identifiers_have_local_scope=" + rand.nextBoolean(),
"--incompatible_disallow_hashing_frozen_mutables=" + rand.nextBoolean(),
- "--internal_skylark_flag_test_canary=" + rand.nextBoolean(),
- "--experimental_function_equality_by_location=" + rand.nextBoolean());
+ "--internal_skylark_flag_test_canary=" + rand.nextBoolean());
}
/**
@@ -234,7 +233,6 @@
.incompatibleAssignmentIdentifiersHaveLocalScope(rand.nextBoolean())
.incompatibleDisallowHashingFrozenMutables(rand.nextBoolean())
.internalSkylarkFlagTestCanary(rand.nextBoolean())
- .experimentalFunctionEqualityByLocation(rand.nextBoolean())
.build();
}
