Skip to content

Commit

Permalink
Remove the flag --incompatible_disallow_dict_lookup_unhashable_keys
Browse files Browse the repository at this point in the history
    The flag was flipped to True in November.
    bazelbuild/bazel#9184

    RELNOTES: The flag --incompatible_disallow_dict_lookup_unhashable_keys is removed.
    PiperOrigin-RevId: 289431363
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent cd311ce commit 3346072
Show file tree
Hide file tree
Showing 5 changed files with 64 additions and 97 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -150,15 +150,6 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl
+ "pertaining to Google legacy code.")
public boolean experimentalGoogleLegacyApi;

@Option(
name = "experimental_ninja_actions",
defaultValue = "false",
documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
effectTags = {OptionEffectTag.BAZEL_INTERNAL_CONFIGURATION},
metadataTags = {OptionMetadataTag.EXPERIMENTAL},
help = "If set to true, enables Ninja execution functionality.")
public boolean experimentalNinjaActions;

@Option(
name = "experimental_platforms_api",
defaultValue = "false",
Expand Down Expand Up @@ -639,7 +630,6 @@ public StarlarkSemantics toSkylarkSemantics() {
.experimentalCcSkylarkApiEnabledPackages(experimentalCcSkylarkApiEnabledPackages)
.experimentalEnableAndroidMigrationApis(experimentalEnableAndroidMigrationApis)
.experimentalGoogleLegacyApi(experimentalGoogleLegacyApi)
.experimentalNinjaActions(experimentalNinjaActions)
.experimentalPlatformsApi(experimentalPlatformsApi)
.experimentalStarlarkConfigTransitions(experimentalStarlarkConfigTransitions)
.experimentalStarlarkUnusedInputsList(experimentalStarlarkUnusedInputsList)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,17 +152,9 @@ public Iterator<K> iterator() {
// SkylarkInterfaceUtils.getSkylarkCallable. The two 'get' methods cause it to get
// confused as to which one has the annotation. Fix it and remove "2" suffix.
public Object get2(Object key, Object defaultValue, StarlarkThread thread) throws EvalException {
Object v = this.get(key);
if (v != null) {
return v;
if (containsKey(key, null, thread)) {
return this.get(key);
}

// This statement is executed for its effect, which is to throw "unhashable"
// if key is unhashable, instead of returning defaultValue.
// I think this is a bug: the correct behavior is simply 'return defaultValue'.
// See https://github.com/bazelbuild/starlark/issues/65.
containsKey(thread.getSemantics(), key);

return defaultValue;
}

Expand All @@ -186,7 +178,7 @@ public Object get2(Object key, Object defaultValue, StarlarkThread thread) throw
public Object pop(Object key, Object defaultValue, StarlarkThread thread) throws EvalException {
Object value = get(key);
if (value != null) {
remove(key, (Location) null);
remove(key, /*loc=*/ null);
return value;
}
if (defaultValue != Starlark.UNBOUND) {
Expand All @@ -211,7 +203,7 @@ public Tuple<Object> popitem(StarlarkThread thread) throws EvalException {
}
Object key = keySet().iterator().next();
Object value = get(key);
remove(key, (Location) null);
remove(key, /*loc=*/ null);
return Tuple.pair(key, value);
}

Expand Down Expand Up @@ -239,7 +231,7 @@ public Object setdefault(K key, Object defaultValue) throws EvalException {
if (value != null) {
return value;
}
put(key, (V) defaultValue, (Location) null);
put(key, (V) defaultValue, /*loc=*/ null);
return defaultValue;
}

Expand Down Expand Up @@ -278,7 +270,7 @@ public NoneType update(Object args, Dict<String, Object> kwargs, StarlarkThread
? (Dict<K, V>) args
: getDictFromArgs("update", args, thread.mutability());
dict = Dict.plus(dict, (Dict<K, V>) kwargs, thread.mutability());
putAll(dict, (Location) null);
putAll(dict, /*loc=*/ null);
return Starlark.NONE;
}

Expand Down Expand Up @@ -387,11 +379,11 @@ public void unsafeShallowFreeze() {
*
* @param key the key of the added entry
* @param value the value of the added entry
* @param unused a nonce value to select this overload, not Map.put
* @param loc the location to use for error reporting
* @throws EvalException if the key is invalid or the dict is frozen
*/
public void put(K key, V value, Location unused) throws EvalException {
checkMutable();
public void put(K key, V value, Location loc) throws EvalException {
checkMutable(loc);
EvalUtils.checkHashable(key);
contents.put(key, value);
}
Expand All @@ -400,12 +392,12 @@ public void put(K key, V value, Location unused) throws EvalException {
* Puts all the entries from a given map into the dict, after validating that mutation is allowed.
*
* @param map the map whose entries are added
* @param unused a nonce value to select this overload, not Map.put
* @param loc the location to use for error reporting
* @throws EvalException if some key is invalid or the dict is frozen
*/
public <KK extends K, VV extends V> void putAll(Map<KK, VV> map, Location unused)
public <KK extends K, VV extends V> void putAll(Map<KK, VV> map, Location loc)
throws EvalException {
checkMutable();
checkMutable(loc);
for (Map.Entry<KK, VV> e : map.entrySet()) {
KK k = e.getKey();
EvalUtils.checkHashable(k);
Expand All @@ -417,12 +409,12 @@ public <KK extends K, VV extends V> void putAll(Map<KK, VV> map, Location unused
* Deletes the entry associated with the given key.
*
* @param key the key to delete
* @param unused a nonce value to select this overload, not Map.put
* @param loc the location to use for error reporting
* @return the value associated to the key, or {@code null} if not present
* @throws EvalException if the dict is frozen
*/
V remove(Object key, Location unused) throws EvalException {
checkMutable();
V remove(Object key, Location loc) throws EvalException {
checkMutable(loc);
return contents.remove(key);
}

Expand All @@ -435,11 +427,11 @@ public NoneType clearDict() throws EvalException {
/**
* Clears the dict.
*
* @param unused a nonce value to select this overload, not Map.put
* @param loc the location to use for error reporting
* @throws EvalException if the dict is frozen
*/
private void clear(Location unused) throws EvalException {
checkMutable();
void clear(Location loc) throws EvalException {
checkMutable(loc);
contents.clear();
}

Expand Down Expand Up @@ -516,16 +508,21 @@ public <X, Y> Map<X, Y> getContents(
}

@Override
public Object getIndex(StarlarkSemantics semantics, Object key) throws EvalException {
Object v = get(key);
if (v == null) {
throw Starlark.errorf("key %s not found in dictionary", Starlark.repr(key));
public final Object getIndex(Object key, Location loc) throws EvalException {
if (!this.containsKey(key)) {
throw new EvalException(loc, Starlark.format("key %r not found in dictionary", key));
}
return v;
return this.get(key);
}

@Override
public final boolean containsKey(Object key, Location loc) throws EvalException {
return this.containsKey(key);
}

@Override
public boolean containsKey(StarlarkSemantics semantics, Object key) throws EvalException {
public final boolean containsKey(Object key, Location loc, StarlarkThread thread)
throws EvalException {
EvalUtils.checkHashable(key);
return this.containsKey(key);
}
Expand Down Expand Up @@ -620,12 +617,6 @@ public Collection<V> values() {

// disallowed java.util.Map update operations

// TODO(adonovan): make MutabilityException a subclass of (unchecked)
// UnsupportedOperationException, allowing the primary Dict operations
// to satisfy the Map operations below in the usual way (like ImmutableMap does).
// Add "ForStarlark" suffix to disambiguate SkylarkCallable-annotated methods.
// Same for StarlarkList.

@Deprecated
@Override
public void clear() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ public enum FlagIdentifier {
StarlarkSemantics::experimentalEnableAndroidMigrationApis),
EXPERIMENTAL_BUILD_SETTING_API(StarlarkSemantics::experimentalBuildSettingApi),
EXPERIMENTAL_GOOGLE_LEGACY_API(StarlarkSemantics::experimentalGoogleLegacyApi),
EXPERIMENTAL_NINJA_ACTIONS(StarlarkSemantics::experimentalNinjaActions),
EXPERIMENTAL_PLATFORM_API(StarlarkSemantics::experimentalPlatformsApi),
EXPERIMENTAL_STARLARK_CONFIG_TRANSITION(
StarlarkSemantics::experimentalStarlarkConfigTransitions),
Expand Down Expand Up @@ -148,8 +147,6 @@ public boolean flagValue(FlagIdentifier flagIdentifier) {

public abstract boolean experimentalGoogleLegacyApi();

public abstract boolean experimentalNinjaActions();

public abstract boolean experimentalPlatformsApi();

public abstract boolean experimentalStarlarkConfigTransitions();
Expand Down Expand Up @@ -258,7 +255,6 @@ public static Builder builderWithDefaults() {
.experimentalAllowIncrementalRepositoryUpdates(true)
.experimentalEnableAndroidMigrationApis(false)
.experimentalGoogleLegacyApi(false)
.experimentalNinjaActions(false)
.experimentalPlatformsApi(false)
.experimentalStarlarkConfigTransitions(true)
.experimentalStarlarkUnusedInputsList(true)
Expand Down Expand Up @@ -316,8 +312,6 @@ public abstract static class Builder {

public abstract Builder experimentalGoogleLegacyApi(boolean value);

public abstract Builder experimentalNinjaActions(boolean value);

public abstract Builder experimentalPlatformsApi(boolean value);

public abstract Builder experimentalStarlarkConfigTransitions(boolean value);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright 2019 The Bazel Authors. All rights reserved.
// Copyright 2017 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
Expand All @@ -11,7 +11,6 @@
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
//

package com.google.devtools.build.lib.packages;

Expand Down Expand Up @@ -130,7 +129,6 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E
+ rand.nextDouble(),
"--experimental_enable_android_migration_apis=" + rand.nextBoolean(),
"--experimental_google_legacy_api=" + rand.nextBoolean(),
"--experimental_ninja_actions=" + rand.nextBoolean(),
"--experimental_platforms_api=" + rand.nextBoolean(),
"--experimental_starlark_config_transitions=" + rand.nextBoolean(),
"--experimental_starlark_unused_inputs_list=" + rand.nextBoolean(),
Expand Down Expand Up @@ -182,7 +180,6 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) {
ImmutableList.of(String.valueOf(rand.nextDouble()), String.valueOf(rand.nextDouble())))
.experimentalEnableAndroidMigrationApis(rand.nextBoolean())
.experimentalGoogleLegacyApi(rand.nextBoolean())
.experimentalNinjaActions(rand.nextBoolean())
.experimentalPlatformsApi(rand.nextBoolean())
.experimentalStarlarkConfigTransitions(rand.nextBoolean())
.experimentalStarlarkUnusedInputsList(rand.nextBoolean())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,7 @@ public void sanityCheckUserDefinedTestRule() throws Exception {
"test/skylark/test_rule.bzl",
"def _impl(ctx):",
" output = ctx.outputs.out",
" ctx.actions.write(output = output, content = 'hello', is_executable=True)",
" return [DefaultInfo(executable = output)]",
" ctx.actions.write(output = output, content = 'hello')",
"",
"fake_test = rule(",
" implementation = _impl,",
Expand All @@ -203,7 +202,7 @@ public void sanityCheckUserDefinedTestRule() throws Exception {
"test/skylark/BUILD",
"load('//test/skylark:test_rule.bzl', 'fake_test')",
"fake_test(name = 'test_name')");
getConfiguredTarget("//test/skylark:test_name");
getConfiguredTarget("//test/skylark:fake_test");
}

@Test
Expand All @@ -230,10 +229,9 @@ public void testOutputGroups() throws Exception {
.getOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL);
ConfiguredTarget myTarget = getConfiguredTarget("//test/skylark:my");
Depset result = (Depset) getMyInfoFromTarget(myTarget).getValue("result");
assertThat(result.getSet(Artifact.class).toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group").toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(result.getSet(Artifact.class)).containsExactlyElementsIn(hiddenTopLevelArtifacts);
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group"))
.containsExactlyElementsIn(hiddenTopLevelArtifacts);
}

@Test
Expand All @@ -257,10 +255,9 @@ public void testOutputGroupsDeclaredProvider() throws Exception {
.getOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL);
ConfiguredTarget myTarget = getConfiguredTarget("//test/skylark:my");
Depset result = (Depset) getMyInfoFromTarget(myTarget).getValue("result");
assertThat(result.getSet(Artifact.class).toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group").toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(result.getSet(Artifact.class)).containsExactlyElementsIn(hiddenTopLevelArtifacts);
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group"))
.containsExactlyElementsIn(hiddenTopLevelArtifacts);
}


Expand Down Expand Up @@ -292,10 +289,9 @@ public void testOutputGroupsAsDictionary() throws Exception {
ConfiguredTarget myTarget = getConfiguredTarget("//test/skylark:my");
StructImpl myInfo = getMyInfoFromTarget(myTarget);
Depset result = (Depset) myInfo.getValue("result");
assertThat(result.getSet(Artifact.class).toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group").toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(result.getSet(Artifact.class)).containsExactlyElementsIn(hiddenTopLevelArtifacts);
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group"))
.containsExactlyElementsIn(hiddenTopLevelArtifacts);
assertThat(myInfo.getValue("has_key1")).isEqualTo(Boolean.TRUE);
assertThat(myInfo.getValue("has_key2")).isEqualTo(Boolean.FALSE);
assertThat((Sequence) myInfo.getValue("all_keys"))
Expand Down Expand Up @@ -327,10 +323,9 @@ public void testOutputGroupsAsDictionaryPipe() throws Exception {
.getOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL);
ConfiguredTarget myTarget = getConfiguredTarget("//test/skylark:my");
Depset result = (Depset) getMyInfoFromTarget(myTarget).getValue("result");
assertThat(result.getSet(Artifact.class).toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group").toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(result.getSet(Artifact.class)).containsExactlyElementsIn(hiddenTopLevelArtifacts);
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group"))
.containsExactlyElementsIn(hiddenTopLevelArtifacts);
}

@Test
Expand All @@ -357,11 +352,11 @@ public void testOutputGroupsWithList() throws Exception {
.getOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL);
ConfiguredTarget myTarget = getConfiguredTarget("//test/skylark:my");
Depset result = (Depset) getMyInfoFromTarget(myTarget).getValue("result");
assertThat(result.getSet(Artifact.class).toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group").toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_empty_group").toList()).isEmpty();
assertThat(result.getSet(Artifact.class)).containsExactlyElementsIn(hiddenTopLevelArtifacts);
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group"))
.containsExactlyElementsIn(hiddenTopLevelArtifacts);
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_empty_group"))
.isEmpty();
}

@Test
Expand All @@ -386,11 +381,11 @@ public void testOutputGroupsDeclaredProviderWithList() throws Exception {
.getOutputGroup(OutputGroupInfo.HIDDEN_TOP_LEVEL);
ConfiguredTarget myTarget = getConfiguredTarget("//test/skylark:my");
Depset result = (Depset) getMyInfoFromTarget(myTarget).getValue("result");
assertThat(result.getSet(Artifact.class).toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group").toList())
.containsExactlyElementsIn(hiddenTopLevelArtifacts.toList());
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_empty_group").toList()).isEmpty();
assertThat(result.getSet(Artifact.class)).containsExactlyElementsIn(hiddenTopLevelArtifacts);
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_group"))
.containsExactlyElementsIn(hiddenTopLevelArtifacts);
assertThat(OutputGroupInfo.get(myTarget).getOutputGroup("my_empty_group"))
.isEmpty();
}

@Test
Expand Down Expand Up @@ -3353,9 +3348,9 @@ public void testLegacyProvider_AddCanonicalLegacyKeyAndModernKey() throws Except
ObjcProvider providerFromFoo = (ObjcProvider) target.get("foo");

// The modern key and the canonical legacy key "objc" are set to the one available ObjcProvider.
assertThat(providerFromModernKey.define().toList()).containsExactly("foo");
assertThat(providerFromObjc.define().toList()).containsExactly("foo");
assertThat(providerFromFoo.define().toList()).containsExactly("foo");
assertThat(providerFromModernKey.define()).containsExactly("foo");
assertThat(providerFromObjc.define()).containsExactly("foo");
assertThat(providerFromFoo.define()).containsExactly("foo");
}

@Test
Expand All @@ -3382,9 +3377,9 @@ public void testLegacyProvider_DontAutomaticallyAddKeysAlreadyPresent() throws E
ObjcProvider providerFromObjc = (ObjcProvider) target.get("objc");
ObjcProvider providerFromBah = (ObjcProvider) target.get("bah");

assertThat(providerFromModernKey.define().toList()).containsExactly("prov");
assertThat(providerFromObjc.define().toList()).containsExactly("objc");
assertThat(providerFromBah.define().toList()).containsExactly("bah");
assertThat(providerFromModernKey.define()).containsExactly("prov");
assertThat(providerFromObjc.define()).containsExactly("objc");
assertThat(providerFromBah.define()).containsExactly("bah");
}

@Test
Expand Down Expand Up @@ -3412,10 +3407,10 @@ public void testLegacyProvider_FirstNoncanonicalKeyBecomesCanonical() throws Exc
ObjcProvider providerFromFoo = (ObjcProvider) target.get("foo");
ObjcProvider providerFromBar = (ObjcProvider) target.get("bar");

assertThat(providerFromModernKey.define().toList()).containsExactly("prov");
assertThat(providerFromModernKey.define()).containsExactly("prov");
// The first defined provider is set to the legacy "objc" key.
assertThat(providerFromObjc.define().toList()).containsExactly("foo");
assertThat(providerFromFoo.define().toList()).containsExactly("foo");
assertThat(providerFromBar.define().toList()).containsExactly("bar");
assertThat(providerFromObjc.define()).containsExactly("foo");
assertThat(providerFromFoo.define()).containsExactly("foo");
assertThat(providerFromBar.define()).containsExactly("bar");
}
}

0 comments on commit 3346072

Please sign in to comment.