Clean up string representations for labels
If --incompatible_descriptive_string_representations is passed, labels are converted
to strings using `repr` differently: `Label("//package:name")` instead of
`"//package:name"`
This CL doesn't affect representations of other object types but provides the
necessary infrastructure for it.
PiperOrigin-RevId: 160955284
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index 4f4126e..833cd9c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -168,7 +168,7 @@
protected ConfigurationFactory configurationFactory;
protected BuildView view;
- private SequencedSkyframeExecutor skyframeExecutor;
+ protected SequencedSkyframeExecutor skyframeExecutor;
protected TimestampGranularityMonitor tsgm;
protected BlazeDirectories directories;
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index 5dde93b..860f1d7 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -509,7 +509,7 @@
"macro()",
"cc_library(name = 'a', srcs = [])");
getConfiguredTarget("//test:a");
- assertContainsEvent("selector({\"//foo:foo\": [\"//bar:bar\"]})");
+ assertContainsEvent("selector({Label(\"//foo:foo\"): [Label(\"//bar:bar\")]})");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkStringRepresentationsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkStringRepresentationsTest.java
new file mode 100644
index 0000000..7c777e2
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkStringRepresentationsTest.java
@@ -0,0 +1,303 @@
+// 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.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// 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.skylark;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.skylark.util.SkylarkTestCase;
+import com.google.devtools.build.lib.util.Pair;
+import com.google.devtools.build.lib.vfs.ModifiedFileSet;
+import com.google.devtools.build.lib.vfs.PathFragment;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for string representations of Skylark objects. */
+@RunWith(JUnit4.class)
+public class SkylarkStringRepresentationsTest extends SkylarkTestCase {
+
+ // Different ways to format objects, these suffixes are used in the `prepare_params` function
+ private static final ImmutableList<String> SUFFIXES =
+ ImmutableList.of("_str", "_repr", "_format", "_str_perc", "_repr_perc");
+
+ private Object skylarkLoadingEval(String code) throws Exception {
+ return skylarkLoadingEval(code, "");
+ }
+
+ /**
+ * Evaluates {@code code} in the loading phase in a .bzl file
+ *
+ * @param code The code to execute
+ * @param definition Additional code to define necessary variables
+ */
+ private Object skylarkLoadingEval(String code, String definition) throws Exception {
+ scratch.overwriteFile("eval/BUILD", "load(':eval.bzl', 'eval')", "eval(name='eval')");
+ scratch.overwriteFile(
+ "eval/eval.bzl",
+ definition,
+ String.format("x = %s", code), // Should be placed here to execute during the loading phase
+ "def _impl(ctx):",
+ " return struct(result = x)",
+ "eval = rule(implementation = _impl)");
+ skyframeExecutor.invalidateFilesUnderPathForTesting(
+ reporter,
+ new ModifiedFileSet.Builder()
+ .modify(PathFragment.create("eval/BUILD"))
+ .modify(PathFragment.create("eval/eval.bzl"))
+ .build(),
+ rootDirectory);
+
+ ConfiguredTarget target = getConfiguredTarget("//eval");
+ return target.get("result");
+ }
+
+ /**
+ * Asserts that all 5 different ways to convert an object to a string of {@code expression}
+ * ({@code str}, {@code repr}, {@code '%s'}, {@code '%r'}, {@code '{}'.format} return the correct
+ * {@code representation}. Not applicable for objects that have different {@code str} and {@code
+ * repr} representations.
+ *
+ * @param definition optional definition required to evaluate the {@code expression}
+ * @param expression the expression to evaluate a string representation of
+ * @param representation desired string representation
+ */
+ private void assertStringRepresentation(
+ String definition, String expression, String representation) throws Exception {
+ assertThat(skylarkLoadingEval(String.format("str(%s)", expression), definition))
+ .isEqualTo(representation);
+ assertThat(skylarkLoadingEval(String.format("repr(%s)", expression), definition))
+ .isEqualTo(representation);
+ assertThat(skylarkLoadingEval(String.format("'%%s' %% (%s,)", expression), definition))
+ .isEqualTo(representation);
+ assertThat(skylarkLoadingEval(String.format("'%%r' %% (%s,)", expression), definition))
+ .isEqualTo(representation);
+ assertThat(skylarkLoadingEval(String.format("'{}'.format(%s)", expression), definition))
+ .isEqualTo(representation);
+ }
+
+ private void assertStringRepresentation(String expression, String representation)
+ throws Exception {
+ assertStringRepresentation("", expression, representation);
+ }
+
+ /**
+ * Creates a set of BUILD and .bzl files that gathers objects of many different types available in
+ * Skylark and creates their string representations by calling `str` and `repr` on them. The
+ * strings are available in the configured target for //test/skylark:check
+ */
+ private void generateFilesToTestStrings() throws Exception {
+ // Generate string representations of different Skylark types. Objects are generated in
+ // test/skylark/rules.bzl: the top-level objects dict contains objects
+ // available during the loading phase, and _check_impl(ctx) returns objects that are available
+ // during the analysis phase. prepare_params(objects) converts a list of objects to a list of
+ // their string representations.
+
+ scratch.file(
+ "test/skylark/rules.bzl",
+ "aspect_ctx_provider = provider()",
+ "def prepare_params(objects):",
+ " params = {}",
+ " for k, v in objects.items():",
+ " params[k + '_str'] = str(v)",
+ " params[k + '_repr'] = repr(v)",
+ " params[k + '_format'] = '{}'.format(v)",
+ " params[k + '_str_perc'] = '%s' % (v,)",
+ " params[k + '_repr_perc'] = '%r' % (v,)",
+ " return params",
+ "",
+ "def _impl_aspect(target, ctx):",
+ " return [aspect_ctx_provider(ctx = ctx)]",
+ "my_aspect = aspect(implementation = _impl_aspect)",
+ "",
+ "def _impl(ctx): pass",
+ "dep = rule(implementation = _impl)",
+ "",
+ "def _genfile_impl(ctx):",
+ " ctx.file_action(output = ctx.outputs.my_output, content = 'foo')",
+ "genfile = rule(",
+ " implementation = _genfile_impl,",
+ " outputs = {",
+ " 'my_output': '%{name}.txt',",
+ " },",
+ ")",
+ "",
+ "def _check_impl(ctx):",
+ " objects = {",
+ " 'target': ctx.attr.deps[0],",
+ " 'alias_target': ctx.attr.deps[1],",
+ " 'aspect_target': ctx.attr.asp_deps[0],",
+ " 'input_target': ctx.attr.srcs[0],",
+ " 'output_target': ctx.attr.srcs[1],",
+ " 'rule_ctx': ctx,",
+ " 'aspect_ctx': ctx.attr.asp_deps[0][aspect_ctx_provider].ctx,",
+ " }",
+ " return struct(**prepare_params(objects))",
+ "check = rule(",
+ " implementation = _check_impl,",
+ " attrs = {",
+ " 'deps': attr.label_list(),",
+ " 'asp_deps': attr.label_list(aspects = [my_aspect]),",
+ " 'srcs': attr.label_list(allow_files = True),",
+ " },",
+ ")");
+
+ scratch.file(
+ "test/skylark/BUILD",
+ "load(':rules.bzl', 'check', 'dep', 'genfile')",
+ "",
+ "dep(name = 'foo')",
+ "dep(name = 'bar')",
+ "alias(name = 'foobar', actual = ':foo')",
+ "genfile(name = 'output')",
+ "check(",
+ " name = 'check',",
+ " deps = [':foo', ':foobar'],",
+ " asp_deps = [':bar'],",
+ " srcs = ['input.txt', 'output.txt'],",
+ ")");
+ }
+
+ @Test
+ public void testStringRepresentations_Strings() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=true");
+
+ assertThat(skylarkLoadingEval("str('foo')")).isEqualTo("foo");
+ assertThat(skylarkLoadingEval("'%s' % 'foo'")).isEqualTo("foo");
+ assertThat(skylarkLoadingEval("'{}'.format('foo')")).isEqualTo("foo");
+ assertThat(skylarkLoadingEval("repr('foo')")).isEqualTo("\"foo\"");
+ assertThat(skylarkLoadingEval("'%r' % 'foo'")).isEqualTo("\"foo\"");
+ }
+
+ @Test
+ public void testStringRepresentations_Labels() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=true");
+
+ assertThat(skylarkLoadingEval("str(Label('//foo:bar'))")).isEqualTo("//foo:bar");
+ assertThat(skylarkLoadingEval("'%s' % Label('//foo:bar')")).isEqualTo("//foo:bar");
+ assertThat(skylarkLoadingEval("'{}'.format(Label('//foo:bar'))")).isEqualTo("//foo:bar");
+ assertThat(skylarkLoadingEval("repr(Label('//foo:bar'))")).isEqualTo("Label(\"//foo:bar\")");
+ assertThat(skylarkLoadingEval("'%r' % Label('//foo:bar')")).isEqualTo("Label(\"//foo:bar\")");
+
+ assertThat(skylarkLoadingEval("'{}'.format([Label('//foo:bar')])")).isEqualTo("[Label(\"//foo:bar\")]");
+ }
+
+ @Test
+ public void testStringRepresentations_Primitives() throws Exception {
+ // Strings are tested in a separate test case as they have different str and repr values.
+ setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=true");
+
+ assertStringRepresentation("1543", "1543");
+ assertStringRepresentation("True", "True");
+ assertStringRepresentation("False", "False");
+ }
+
+ @Test
+ public void testStringRepresentations_Containers() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=true");
+
+ assertStringRepresentation("['a', 'b']", "[\"a\", \"b\"]");
+ assertStringRepresentation("('a', 'b')", "(\"a\", \"b\")");
+ assertStringRepresentation("{'a': 'b', 'c': 'd'}", "{\"a\": \"b\", \"c\": \"d\"}");
+ assertStringRepresentation("struct(d = 4, c = 3)", "struct(c = 3, d = 4)");
+ }
+
+ @Test
+ public void testLegacyStringRepresentations_Labels() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false");
+
+ assertThat(skylarkLoadingEval("str(Label('//foo:bar'))")).isEqualTo("//foo:bar");
+ assertThat(skylarkLoadingEval("'%s' % Label('//foo:bar')")).isEqualTo("//foo:bar");
+ assertThat(skylarkLoadingEval("'{}'.format(Label('//foo:bar'))")).isEqualTo("//foo:bar");
+ assertThat(skylarkLoadingEval("repr(Label('//foo:bar'))")).isEqualTo("\"//foo:bar\"");
+ assertThat(skylarkLoadingEval("'%r' % Label('//foo:bar')")).isEqualTo("\"//foo:bar\"");
+
+ // Also test that str representations (as opposed to repr) also use legacy formatting
+ // They are equivalent for labels, but not equivalent for lists of labels, because
+ // containers always render their items with repr
+ assertThat(skylarkLoadingEval("str([Label('//foo:bar')])")).isEqualTo("[\"//foo:bar\"]");
+ assertThat(skylarkLoadingEval("'{}'.format([Label('//foo:bar')])")).isEqualTo("[\"//foo:bar\"]");
+ assertThat(skylarkLoadingEval("'%s' % [Label('//foo:bar')]")).isEqualTo("[\"//foo:bar\"]");
+ }
+
+ @Test
+ public void testLegacyStringRepresentations_Functions() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false");
+
+ assertStringRepresentation("all", "<function all>");
+ assertStringRepresentation("def f(): pass", "f", "<function f>");
+ }
+
+ @Test
+ public void testLegacyStringRepresentations_Rules() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false");
+
+ assertStringRepresentation("native.cc_library", "<function cc_library>");
+ assertStringRepresentation("rule(implementation=str)", "<function rule>");
+ assertStringRepresentation("aspect(implementation=str)", "Aspect:<function str>");
+ }
+
+ @Test
+ public void testLegacyStringRepresentations_Providers() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false");
+
+ assertStringRepresentation("provider()", "<function <no name>>");
+ assertStringRepresentation("p = provider()", "p(b = 2, a = 1)", "p(a = 1, b = 2)");
+ }
+
+ @Test
+ public void testLegacyStringRepresentations_Select() throws Exception {
+ setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false");
+
+ assertStringRepresentation(
+ "select({'//foo': ['//bar']}) + select({'//foo2': ['//bar2']})",
+ "selector({\"//foo\": [\"//bar\"]}) + selector({\"//foo2\": [\"//bar2\"]})");
+ }
+
+ @Test
+ public void testLegacyStringRepresentations_Targets() throws Exception {
+ // alias targets in skylark used to leak their memory addresses in string representations,
+ // we don't try to preserve this behaviour as it's harmful.
+ // An example of their legacy representation:
+ // "<com.google.devtools.build.lib.rules.AliasConfiguredTarget@12da9140>"
+
+ setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false");
+
+ generateFilesToTestStrings();
+ ConfiguredTarget target = getConfiguredTarget("//test/skylark:check");
+
+
+ ImmutableList<Pair<String, String>> parameters = ImmutableList.of(
+ new Pair<>("rule_ctx", "//test/skylark:check"),
+ new Pair<>("aspect_ctx", "//test/skylark:bar"),
+ new Pair<>("input_target", "InputFileConfiguredTarget(//test/skylark:input.txt)"));
+ for (String suffix : SUFFIXES) {
+ for (Pair<String, String > pair : parameters) {
+ assertThat(target.get(pair.getFirst() + suffix)).isEqualTo(pair.getSecond());
+ }
+ }
+
+ // Legacy representation of several types of objects may contain nondeterministic chunks
+ parameters = ImmutableList.of(
+ new Pair<>("target", "ConfiguredTarget\\(//test/skylark:foo, [0-9a-f]+\\)"),
+ new Pair<>("aspect_target", "ConfiguredTarget\\(//test/skylark:bar, [0-9a-f]+\\)"),
+ new Pair<>("output_target", "ConfiguredTarget\\(//test/skylark:output.txt, [0-9a-f]+\\)"));
+ for (String suffix : SUFFIXES) {
+ for (Pair<String, String > pair : parameters) {
+ assertThat((String) target.get(pair.getFirst() + suffix)).matches(pair.getSecond());
+ }
+ }
+ }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
index 5e9391f..0e1bdab 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -47,8 +47,8 @@
* <p>If a test uses this method, it allows potential subclasses to run the very same test in a
* different mode in subclasses
*/
- protected ModalTestCase newTest() {
- return new BuildTest();
+ protected ModalTestCase newTest(String... skylarkOptions) {
+ return new BuildTest(skylarkOptions);
}
@Test
@@ -601,7 +601,12 @@
return new SkylarkValue() {
@Override
public void repr(SkylarkPrinter printer) {
- printer.append("str marker");
+ printer.append("<str marker>");
+ }
+
+ @Override
+ public void reprLegacy(SkylarkPrinter printer) {
+ printer.append("<str legacy marker>");
}
@Override
@@ -615,36 +620,45 @@
return new Object() {
@Override
public String toString() {
- return "unknown object";
+ return "<unknown object>";
}
};
}
@Test
public void testPercOnObject() throws Exception {
- newTest()
+ newTest("--incompatible_descriptive_string_representations=true")
.update("obj", createObjWithStr())
- .testStatement("'%s' % obj", "str marker");
+ .testStatement("'%s' % obj", "<str marker>");
+ newTest("--incompatible_descriptive_string_representations=false")
+ .update("obj", createObjWithStr())
+ .testStatement("'%s' % obj", "<str legacy marker>");
newTest()
.update("unknown", createUnknownObj())
- .testStatement("'%s' % unknown", "unknown object");
+ .testStatement("'%s' % unknown", "<unknown object>");
}
@Test
public void testPercOnObjectList() throws Exception {
- newTest()
+ newTest("--incompatible_descriptive_string_representations=true")
.update("obj", createObjWithStr())
- .testStatement("'%s %s' % (obj, obj)", "str marker str marker");
+ .testStatement("'%s %s' % (obj, obj)", "<str marker> <str marker>");
+ newTest("--incompatible_descriptive_string_representations=false")
+ .update("obj", createObjWithStr())
+ .testStatement("'%s %s' % (obj, obj)", "<str legacy marker> <str legacy marker>");
newTest()
.update("unknown", createUnknownObj())
- .testStatement("'%s %s' % (unknown, unknown)", "unknown object unknown object");
+ .testStatement("'%s %s' % (unknown, unknown)", "<unknown object> <unknown object>");
}
@Test
public void testPercOnObjectInvalidFormat() throws Exception {
- newTest()
+ newTest("--incompatible_descriptive_string_representations=true")
.update("obj", createObjWithStr())
- .testIfExactError("invalid argument str marker for format pattern %d", "'%d' % obj");
+ .testIfExactError("invalid argument <str marker> for format pattern %d", "'%d' % obj");
+ newTest("--incompatible_descriptive_string_representations=false")
+ .update("obj", createObjWithStr())
+ .testIfExactError("invalid argument <str marker> for format pattern %d", "'%d' % obj");
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java b/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java
index fdf4327..f1e5691 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java
@@ -21,6 +21,8 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
import java.util.Arrays;
@@ -55,7 +57,7 @@
assertThat(Printer.repr(Runtime.NONE)).isEqualTo("None");
assertThat(Printer.str(Label.parseAbsolute("//x"))).isEqualTo("//x:x");
- assertThat(Printer.repr(Label.parseAbsolute("//x"))).isEqualTo("\"//x:x\"");
+ assertThat(Printer.repr(Label.parseAbsolute("//x"))).isEqualTo("Label(\"//x:x\")");
List<?> list = MutableList.of(null, "foo", "bar");
List<?> tuple = Tuple.of("foo", "bar");
@@ -215,6 +217,14 @@
assertThat(Printer.str(list)).isEqualTo(String.format("[%s]", Joiner.on(", ").join(list)));
}
+ @Test
+ public void testLegacyPrinter() throws Exception {
+ assertThat(new Printer.LegacyPrinter().str(createObjWithStr()).toString())
+ .isEqualTo("<str legacy marker>");
+ assertThat(new Printer.LegacyPrinter().repr(createObjWithStr()).toString())
+ .isEqualTo("<repr legacy marker>");
+ }
+
private String printListWithLimit(List<?> list) {
return printList(list, Printer.SUGGESTED_CRITICAL_LIST_ELEMENTS_COUNT,
Printer.SUGGESTED_CRITICAL_LIST_ELEMENTS_STRING_LENGTH);
@@ -224,4 +234,33 @@
return Printer.printAbbreviatedList(
list, "[", ", ", "]", "", criticalElementsCount, criticalStringLength);
}
+
+ private SkylarkValue createObjWithStr() {
+ return new SkylarkValue() {
+ @Override
+ public void repr(SkylarkPrinter printer) {
+ printer.append("<repr marker>");
+ }
+
+ @Override
+ public void reprLegacy(SkylarkPrinter printer) {
+ printer.append("<repr legacy marker>");
+ }
+
+ @Override
+ public void str(SkylarkPrinter printer) {
+ printer.append("<str marker>");
+ }
+
+ @Override
+ public void strLegacy(SkylarkPrinter printer) {
+ printer.append("<str legacy marker>");
+ }
+
+ @Override
+ public boolean isImmutable() {
+ return false;
+ }
+ };
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index 256985c..b5c6a70 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -55,8 +55,8 @@
* Skylark context
*/
@Override
- protected ModalTestCase newTest() {
- return new SkylarkTest();
+ protected ModalTestCase newTest(String... skylarkOptions) {
+ return new SkylarkTest(skylarkOptions);
}
@Immutable
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
index 19665c6..70604c6 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
@@ -579,11 +579,15 @@
* A class that runs all tests in Build mode
*/
protected class BuildTest extends ModalTestCase {
- public BuildTest() {}
+ private final String[] skylarkOptions;
+
+ public BuildTest(String... skylarkOptions) {
+ this.skylarkOptions = skylarkOptions;
+ }
@Override
protected void run(Testable testable) throws Exception {
- enableBuildMode();
+ enableBuildMode(skylarkOptions);
testable.run();
}
}
