From 52c15aad3051e454f3a6ef08aa5c720d4885c223 Mon Sep 17 00:00:00 2001 From: Luca Di Grazia Date: Sun, 4 Sep 2022 18:50:39 +0200 Subject: [PATCH] Remove the flag --incompatible_depset_is_not_iterable https://github.com/bazelbuild/bazel/issues/5816 RELNOTES: None. PiperOrigin-RevId: 280688754 --- .../packages/StarlarkSemanticsOptions.java | 16 ++++++ .../devtools/build/lib/syntax/EvalUtils.java | 8 ++- .../build/lib/syntax/MethodLibrary.java | 57 +++++++++++-------- .../build/lib/syntax/SkylarkNestedSet.java | 2 +- .../build/lib/syntax/StarlarkSemantics.java | 5 ++ .../SkylarkSemanticsConsistencyTest.java | 2 + .../lib/syntax/SkylarkEvaluationTest.java | 32 ++++------- .../lib/syntax/SkylarkNestedSetTest.java | 17 ++++-- 8 files changed, 84 insertions(+), 55 deletions(-) diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java index f75c9dce74e..dc4d68b8479 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/packages/StarlarkSemanticsOptions.java @@ -329,6 +329,21 @@ public class StarlarkSemanticsOptions extends OptionsBase implements Serializabl // TODO(elenairina): Move option to graveyard after the flag is removed from the global blazerc. public boolean incompatibleDisallowLegacyJavaInfo; + @Option( + name = "incompatible_string_join_requires_strings", + defaultValue = "true", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, + metadataTags = { + OptionMetadataTag.INCOMPATIBLE_CHANGE, + OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES + }, + help = + "If set to true, the argument of `string.join` must be an iterable whose elements are " + + "strings. If set to false, elements are first converted to string. " + + "See https://github.com/bazelbuild/bazel/issues/7802") + public boolean incompatibleStringJoinRequiresStrings; + @Option( name = "incompatible_disallow_struct_provider_syntax", defaultValue = "false", @@ -631,6 +646,7 @@ public StarlarkSemantics toSkylarkSemantics() { .incompatibleRemoveNativeMavenJar(incompatibleRemoveNativeMavenJar) .incompatibleRestrictNamedParams(incompatibleRestrictNamedParams) .incompatibleRunShellCommandString(incompatibleRunShellCommandString) + .incompatibleStringJoinRequiresStrings(incompatibleStringJoinRequiresStrings) .incompatibleVisibilityPrivateAttributesAtDefinition( incompatibleVisibilityPrivateAttributesAtDefinition) .internalSkylarkFlagTestCanary(internalSkylarkFlagTestCanary) diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java index a428f0b7b75..c3b8a94545f 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java @@ -301,7 +301,8 @@ public static Object checkNotNull(Expression expr, Object obj) throws EvalExcept return obj; } - public static Collection toCollection(Object o, Location loc) throws EvalException { + public static Collection toCollection(Object o, Location loc, @Nullable StarlarkThread thread) + throws EvalException { if (o instanceof Collection) { return (Collection) o; } else if (o instanceof Sequence) { @@ -352,11 +353,12 @@ private static Collection nestedSetToCollection( } } - public static Iterable toIterable(Object o, Location loc) throws EvalException { + public static Iterable toIterable(Object o, Location loc, @Nullable StarlarkThread thread) + throws EvalException { if (o instanceof Iterable) { return (Iterable) o; } else if (o instanceof Map) { - return toCollection(o, loc); + return toCollection(o, loc, thread); } else { throw new EvalException(loc, "type '" + getDataTypeName(o) + "' is not iterable"); diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java index e0b267d49cd..9b8cbe9b611 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java @@ -62,10 +62,11 @@ class MethodLibrary { + "min([5, 6, 3]) == 3", extraPositionals = @Param(name = "args", type = Sequence.class, doc = "The elements to be checked."), - useLocation = true) - public Object min(Sequence args, Location loc) throws EvalException { + useLocation = true, + useStarlarkThread = true) + public Object min(Sequence args, Location loc, StarlarkThread thread) throws EvalException { try { - return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR.reverse(), loc); + return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR.reverse(), loc, thread); } catch (ComparisonException e) { throw new EvalException(loc, e); } @@ -81,22 +82,25 @@ public Object min(Sequence args, Location loc) throws EvalException { + "max([5, 6, 3]) == 6", extraPositionals = @Param(name = "args", type = Sequence.class, doc = "The elements to be checked."), - useLocation = true) - public Object max(Sequence args, Location loc) throws EvalException { + useLocation = true, + useStarlarkThread = true) + public Object max(Sequence args, Location loc, StarlarkThread thread) throws EvalException { try { - return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR, loc); + return findExtreme(args, EvalUtils.SKYLARK_COMPARATOR, loc, thread); } catch (ComparisonException e) { throw new EvalException(loc, e); } } /** Returns the maximum element from this list, as determined by maxOrdering. */ - private static Object findExtreme(Sequence args, Ordering maxOrdering, Location loc) + private static Object findExtreme( + Sequence args, Ordering maxOrdering, Location loc, StarlarkThread thread) throws EvalException { // Args can either be a list of items to compare, or a singleton list whose element is an // iterable of items to compare. In either case, there must be at least one item to compare. try { - Iterable items = (args.size() == 1) ? EvalUtils.toIterable(args.get(0), loc) : args; + Iterable items = + (args.size() == 1) ? EvalUtils.toIterable(args.get(0), loc, thread) : args; return maxOrdering.max(items); } catch (NoSuchElementException ex) { throw new EvalException(loc, "expected at least one item", ex); @@ -119,9 +123,10 @@ private static Object findExtreme(Sequence args, Ordering maxOrdering // TODO(cparsons): This parameter should be positional-only. legacyNamed = true) }, - useLocation = true) - public Boolean all(Object collection, Location loc) throws EvalException { - return !hasElementWithBooleanValue(collection, false, loc); + useLocation = true, + useStarlarkThread = true) + public Boolean all(Object collection, Location loc, StarlarkThread thread) throws EvalException { + return !hasElementWithBooleanValue(collection, false, loc, thread); } @SkylarkCallable( @@ -140,14 +145,15 @@ public Boolean all(Object collection, Location loc) throws EvalException { // TODO(cparsons): This parameter should be positional-only. legacyNamed = true) }, - useLocation = true) - public Boolean any(Object collection, Location loc) throws EvalException { - return hasElementWithBooleanValue(collection, true, loc); + useLocation = true, + useStarlarkThread = true) + public Boolean any(Object collection, Location loc, StarlarkThread thread) throws EvalException { + return hasElementWithBooleanValue(collection, true, loc, thread); } - private static boolean hasElementWithBooleanValue(Object collection, boolean value, Location loc) - throws EvalException { - Iterable iterable = EvalUtils.toIterable(collection, loc); + private static boolean hasElementWithBooleanValue( + Object collection, boolean value, Location loc, StarlarkThread thread) throws EvalException { + Iterable iterable = EvalUtils.toIterable(collection, loc, thread); for (Object obj : iterable) { if (Starlark.truth(obj) == value) { return true; @@ -195,7 +201,7 @@ public StarlarkList sorted( final StarlarkThread thread) throws EvalException, InterruptedException { - Object[] array = EvalUtils.toCollection(iterable, loc).toArray(); + Object[] array = EvalUtils.toCollection(iterable, loc, thread).toArray(); if (key == Starlark.NONE) { try { Arrays.sort(array, EvalUtils.SKYLARK_COMPARATOR); @@ -275,7 +281,7 @@ public StarlarkList reversed(Object sequence, Location loc, StarlarkThread th throw new EvalException(loc, "Argument to reversed() must be a sequence, not a dictionary."); } ArrayDeque tmpList = new ArrayDeque<>(); - for (Object element : EvalUtils.toIterable(sequence, loc)) { + for (Object element : EvalUtils.toIterable(sequence, loc, thread)) { tmpList.addFirst(element); } return StarlarkList.copyOf(thread.mutability(), tmpList); @@ -296,9 +302,10 @@ public StarlarkList reversed(Object sequence, Location loc, StarlarkThread th // TODO(cparsons): This parameter should be positional-only. legacyNamed = true) }, - useLocation = true) - public Tuple tuple(Object x, Location loc) throws EvalException { - return Tuple.copyOf(EvalUtils.toCollection(x, loc)); + useLocation = true, + useStarlarkThread = true) + public Tuple tuple(Object x, Location loc, StarlarkThread thread) throws EvalException { + return Tuple.copyOf(EvalUtils.toCollection(x, loc, thread)); } @SkylarkCallable( @@ -319,7 +326,7 @@ public Tuple tuple(Object x, Location loc) throws EvalException { useLocation = true, useStarlarkThread = true) public StarlarkList list(Object x, Location loc, StarlarkThread thread) throws EvalException { - return StarlarkList.copyOf(thread.mutability(), EvalUtils.toCollection(x, loc)); + return StarlarkList.copyOf(thread.mutability(), EvalUtils.toCollection(x, loc, thread)); } @SkylarkCallable( @@ -622,7 +629,7 @@ private String getIntegerPrefix(String value) { useLocation = true) public StarlarkList enumerate(Object input, Integer start, Location loc, StarlarkThread thread) throws EvalException { - Collection src = EvalUtils.toCollection(input, loc); + Collection src = EvalUtils.toCollection(input, loc, thread); Object[] array = new Object[src.size()]; int i = 0; for (Object x : src) { @@ -1176,7 +1183,7 @@ public StarlarkList zip(Sequence args, Location loc, StarlarkThread thread throws EvalException { Iterator[] iterators = new Iterator[args.size()]; for (int i = 0; i < args.size(); i++) { - iterators[i] = EvalUtils.toIterable(args.get(i), loc).iterator(); + iterators[i] = EvalUtils.toIterable(args.get(i), loc, thread).iterator(); } ArrayList> result = new ArrayList<>(); boolean allHasNext; diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java index d2765817019..755a328e5de 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/SkylarkNestedSet.java @@ -181,7 +181,7 @@ static SkylarkNestedSet of(SkylarkNestedSet left, Object right, Location loc) // TODO(adonovan): enforce that we never construct a SkylarkNestedSet with a StarlarkType // that represents a non-Skylark type (e.g. NestedSet). // One way to do that is to disallow constructing StarlarkTypes for classes - // that would fail Starlark.valid; however remains the problem that + // that would fail isSkylarkAcceptable; however remains the problem that // Object.class means "any Starlark value" but in fact allows any Java value. public static SkylarkNestedSet of(SkylarkType contentType, NestedSet set) { return new SkylarkNestedSet(contentType, set, null, null); diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java index 8ffd56d46ec..5dfed56e478 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/syntax/StarlarkSemantics.java @@ -190,6 +190,8 @@ public boolean flagValue(FlagIdentifier flagIdentifier) { public abstract boolean incompatibleRunShellCommandString(); + public abstract boolean incompatibleStringJoinRequiresStrings(); + public abstract boolean incompatibleVisibilityPrivateAttributesAtDefinition(); public abstract boolean internalSkylarkFlagTestCanary(); @@ -271,6 +273,7 @@ public static Builder builderWithDefaults() { .incompatibleRemoveNativeMavenJar(true) .incompatibleRunShellCommandString(false) .incompatibleRestrictNamedParams(true) + .incompatibleStringJoinRequiresStrings(true) .incompatibleVisibilityPrivateAttributesAtDefinition(false) .internalSkylarkFlagTestCanary(false) .incompatibleDoNotSplitLinkingCmdline(true) @@ -350,6 +353,8 @@ public abstract static class Builder { public abstract Builder incompatibleRunShellCommandString(boolean value); + public abstract Builder incompatibleStringJoinRequiresStrings(boolean value); + public abstract Builder incompatibleVisibilityPrivateAttributesAtDefinition(boolean value); public abstract Builder internalSkylarkFlagTestCanary(boolean value); diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java index 8dc374343c0..7a31a810730 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/packages/SkylarkSemanticsConsistencyTest.java @@ -155,6 +155,7 @@ private static StarlarkSemanticsOptions buildRandomOptions(Random rand) throws E "--incompatible_remove_native_maven_jar=" + rand.nextBoolean(), "--incompatible_restrict_named_params=" + rand.nextBoolean(), "--incompatible_run_shell_command_string=" + rand.nextBoolean(), + "--incompatible_string_join_requires_strings=" + rand.nextBoolean(), "--incompatible_visibility_private_attributes_at_definition=" + rand.nextBoolean(), "--incompatible_restrict_string_escapes=" + rand.nextBoolean(), "--incompatible_disallow_dict_lookup_unhashable_keys=" + rand.nextBoolean(), @@ -204,6 +205,7 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .incompatibleRemoveNativeMavenJar(rand.nextBoolean()) .incompatibleRestrictNamedParams(rand.nextBoolean()) .incompatibleRunShellCommandString(rand.nextBoolean()) + .incompatibleStringJoinRequiresStrings(rand.nextBoolean()) .incompatibleVisibilityPrivateAttributesAtDefinition(rand.nextBoolean()) .incompatibleRestrictStringEscapes(rand.nextBoolean()) .incompatibleDisallowDictLookupUnhashableKeys(rand.nextBoolean()) diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java index 6b8bb043d5d..902e9647211 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java @@ -146,7 +146,7 @@ public Boolean isEmpty(String str) { public void value() {} @SkylarkCallable(name = "return_bad", documented = false) public Bad returnBad() { - return new Bad(); // not a legal Starlark value + return new Bad(); } @SkylarkCallable(name = "struct_field", documented = false, structField = true) public String structField() { @@ -520,8 +520,7 @@ static final class MockClassObject implements ClassObject, SkylarkValue { public Object getValue(String name) { switch (name) { case "field": return "a"; - case "nset": - return NestedSetBuilder.stableOrder().build(); // not a legal Starlark value + case "nset": return NestedSetBuilder.stableOrder().build(); default: return null; } } @@ -1458,15 +1457,11 @@ public void testStructAccessType_nonClassObject() throws Exception { } @Test - public void testJavaFunctionReturnsIllegalValue() throws Exception { - update("mock", new Mock()); - IllegalArgumentException e = - assertThrows(IllegalArgumentException.class, () -> eval("mock.return_bad()")); - assertThat(e) - .hasMessageThat() - .contains( - "cannot expose internal type to Starlark: class" - + " com.google.devtools.build.lib.syntax.SkylarkEvaluationTest$Bad"); + public void testJavaFunctionReturnsMutableObject() throws Exception { + new SkylarkTest() + .update("mock", new Mock()) + .testIfExactError( + "method 'return_bad' returns an object of invalid type Bad", "mock.return_bad()"); } @Test @@ -1506,15 +1501,10 @@ public void testSetIsNotIterable() throws Exception { } @Test - public void testFieldReturnsNestedSet() throws Exception { - update("mock", new MockClassObject()); - IllegalArgumentException e = - assertThrows(IllegalArgumentException.class, () -> eval("mock.nset")); - assertThat(e) - .hasMessageThat() - .contains( - "cannot expose internal type to Starlark: class" - + " com.google.devtools.build.lib.collect.nestedset.NestedSet"); + public void testClassObjectCannotAccessNestedSet() throws Exception { + new SkylarkTest() + .update("mock", new MockClassObject()) + .testIfErrorContains("internal error: type 'NestedSet' is not allowed", "v = mock.nset"); } @Test diff --git a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java index ea49524a56f..f4abf4d2d53 100644 --- a/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java +++ b/dataset/GitHub_Java/bazelbuild.bazel/src/test/java/com/google/devtools/build/lib/syntax/SkylarkNestedSetTest.java @@ -41,19 +41,26 @@ public void testConstructor() throws Exception { } @Test - public void testTuples() throws Exception { + public void testTuplePairs() throws Exception { exec( + // Depsets with tuple-pairs "s_one = depset([('1', '2'), ('3', '4')])", "s_two = depset(direct = [('1', '2'), ('3', '4'), ('5', '6')])", "s_three = depset(transitive = [s_one, s_two])", "s_four = depset(direct = [('1', '3')], transitive = [s_one, s_two])", + // Depsets with tuple-pairs and non-pair tuples are considered just tuple depsets. "s_five = depset(direct = [('1', '3', '5')], transitive = [s_one, s_two])", "s_six = depset(transitive = [s_one, s_five])", "s_seven = depset(direct = [('1', '3')], transitive = [s_one, s_five])", - "s_eight = depset(direct = [(1, 3)], transitive = [s_one, s_two])"); // note, tuple of int - assertThat(get("s_one").getContentType()).isEqualTo(SkylarkType.TUPLE); - assertThat(get("s_two").getContentType()).isEqualTo(SkylarkType.TUPLE); - assertThat(get("s_three").getContentType()).isEqualTo(SkylarkType.TUPLE); + "s_eight = depset(direct = [(1, 3)], transitive = [s_one, s_two])"); + assertThat(get("s_one").getContentType()).isEqualTo(SkylarkType.STRING_PAIR); + assertThat(get("s_two").getContentType()).isEqualTo(SkylarkType.STRING_PAIR); + assertThat(get("s_three").getContentType()).isEqualTo(SkylarkType.STRING_PAIR); + assertThat(get("s_four").getContentType()).isEqualTo(SkylarkType.STRING_PAIR); + + assertThat(get("s_five").getContentType()).isEqualTo(SkylarkType.TUPLE); + assertThat(get("s_six").getContentType()).isEqualTo(SkylarkType.TUPLE); + assertThat(get("s_seven").getContentType()).isEqualTo(SkylarkType.TUPLE); assertThat(get("s_eight").getContentType()).isEqualTo(SkylarkType.TUPLE); assertThat(get("s_four").getSet(Tuple.class))