bazel syntax: disallow PathFragment as a Starlark value
All native functions that return a PathFragment have been updated (I think).
Also, delete SkylarkPrintable, which was implemented by Path and PathFragment,
which meant they could be used in the Skylark formatting mechanism even though
they were not true Skylark values. Now, we fail an assertion if they are used.
Deleting this interface breaks the vfs -> skylarkinterface dependency.
SkylarkPrinter can now be merged with BasePrinter.
This would leave the skylarkinterface package with one concern only: annotations.
Also, hide Appendable and Formattable from the public API.
PiperOrigin-RevId: 277970755
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkPrintable.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkPrintable.java
deleted file mode 100644
index 8edfbd5..0000000
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkPrintable.java
+++ /dev/null
@@ -1,58 +0,0 @@
-// 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.skylarkinterface;
-
-/**
- * Interface for types that aren't {@link SkylarkValue}s, but that we still want to support printing
- * of.
- */
-public interface SkylarkPrintable {
-
- /**
- * Prints an official representation of object x.
- *
- * <p>For regular data structures, the value should be parsable back into an equal data structure.
- *
- * @param printer a printer to be used for formatting nested values.
- */
- void repr(SkylarkPrinter printer);
-
- /**
- * Prints an informal, human-readable representation of the value.
- *
- * <p>By default dispatches to the {@code repr} method.
- *
- * @param printer a printer to be used for formatting nested values.
- */
- default void str(SkylarkPrinter printer) {
- repr(printer);
- }
-
- /**
- * Prints an informal debug representation of the value.
- *
- * <p>This debug representation is only ever printed to the terminal or to another out-of-band
- * channel, and is never accessible to Skylark code. Therefore, it is safe for the debug
- * representation to reveal properties of the value that are usually hidden for the sake of
- * performance, determinism, or forward-compatibility.
- *
- * <p>By default dispatches to the {@code str} method.
- *
- * @param printer a printer to be used for formatting nested values.
- */
- default void debugPrint(SkylarkPrinter printer) {
- str(printer);
- }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkPrinter.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkPrinter.java
index 0e950e4..47f6614 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkPrinter.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkPrinter.java
@@ -23,6 +23,9 @@
* <p>Allows passing Printer.BasePrinter instances to classes that can't import Printer directly
* because of circular dependencies.
*/
+// TODO(adonovan): eliminate, replacing all references by syntax.BasePrinter,
+// which should be renamed Printer. The printList methods can be expressed as static helpers,
+// but they are so trivial they should just be inlined.
public interface SkylarkPrinter {
/** Append a char to the printer's buffer */
SkylarkPrinter append(char c);
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkValue.java b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkValue.java
index 8907516..0b46c62 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkinterface/SkylarkValue.java
@@ -14,12 +14,46 @@
package com.google.devtools.build.lib.skylarkinterface;
-/**
- * Java objects that are also Skylark values.
- *
- * <p>This is used for extending the Skylark interpreter with domain-specific values.
- */
-public interface SkylarkValue extends SkylarkPrintable {
+/** Base interface for all Starlark values besides boxed Java primitives. */
+public interface SkylarkValue {
+
+ /**
+ * Prints an official representation of object x.
+ *
+ * <p>Convention is that the string should be parseable back to the value x. If this isn't
+ * feasible then it should be a short human-readable description enclosed in angled brackets, e.g.
+ * {@code "<foo object>"}.
+ *
+ * @param printer a printer to be used for formatting nested values.
+ */
+ void repr(SkylarkPrinter printer);
+
+ /**
+ * Prints an informal, human-readable representation of the value.
+ *
+ * <p>By default dispatches to the {@code repr} method.
+ *
+ * @param printer a printer to be used for formatting nested values.
+ */
+ default void str(SkylarkPrinter printer) {
+ repr(printer);
+ }
+
+ /**
+ * Prints an informal debug representation of the value.
+ *
+ * <p>This debug representation is only ever printed to the terminal or to another out-of-band
+ * channel, and is never accessible to Skylark code. Therefore, it is safe for the debug
+ * representation to reveal properties of the value that are usually hidden for the sake of
+ * performance, determinism, or forward-compatibility.
+ *
+ * <p>By default dispatches to the {@code str} method.
+ *
+ * @param printer a printer to be used for formatting nested values.
+ */
+ default void debugPrint(SkylarkPrinter printer) {
+ str(printer);
+ }
/**
* Returns if the value is immutable.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index 6ba50786..08c71f5 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -31,7 +31,6 @@
import com.google.devtools.build.lib.syntax.SkylarkList.MutableList;
import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
import com.google.devtools.build.lib.util.SpellChecker;
-import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Collection;
import java.util.IllegalFormatException;
import java.util.List;
@@ -187,8 +186,7 @@
|| c.equals(Boolean.class)
// there is a registered Skylark ancestor class (useful e.g. when using AutoValue)
|| SkylarkInterfaceUtils.getSkylarkModule(c) != null
- || ImmutableMap.class.isAssignableFrom(c) // will be converted to SkylarkDict
- || PathFragment.class.isAssignableFrom(c); // other known class
+ || ImmutableMap.class.isAssignableFrom(c); // will be converted to SkylarkDict
}
// TODO(bazel-team): move the following few type-related functions to SkylarkType
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Printer.java b/src/main/java/com/google/devtools/build/lib/syntax/Printer.java
index bf41cf7..ee7521e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Printer.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Printer.java
@@ -15,7 +15,6 @@
import com.google.common.base.Strings;
import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkPrintable;
import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
import com.google.devtools.build.lib.syntax.SkylarkList.Tuple;
@@ -126,7 +125,7 @@
* @param arguments positional arguments.
* @return the formatted string.
*/
- public static Formattable formattable(final String pattern, Object... arguments) {
+ static Formattable formattable(final String pattern, Object... arguments) {
final List<Object> args = Arrays.asList(arguments);
return new Formattable() {
@Override
@@ -147,7 +146,7 @@
*
* @return buffer
*/
- public static Appendable append(Appendable buffer, char c) {
+ private static Appendable append(Appendable buffer, char c) {
try {
return buffer.append(c);
} catch (IOException e) {
@@ -156,12 +155,12 @@
}
/**
- * Append a char sequence to a buffer. In case of {@link IOException} throw an
- * {@link AssertionError} instead
+ * Append a char sequence to a buffer. In case of {@link IOException} throw an {@link
+ * AssertionError} instead
*
* @return buffer
*/
- public static Appendable append(Appendable buffer, CharSequence s) {
+ private static Appendable append(Appendable buffer, CharSequence s) {
try {
return buffer.append(s);
} catch (IOException e) {
@@ -273,8 +272,8 @@
// values such as Locations or ASTs.
this.append("null");
- } else if (o instanceof SkylarkPrintable) {
- ((SkylarkPrintable) o).repr(this);
+ } else if (o instanceof SkylarkValue) {
+ ((SkylarkValue) o).repr(this);
} else if (o instanceof String) {
writeString((String) o);
@@ -310,6 +309,15 @@
this.append(o.toString());
} else {
+ // Assertion to catch latent uses of Path/PathFragment in format operations.
+ // But what is the harm of using Java toString for all non-Starlark values?
+ // We implicitly trust the native rules in so many other ways.
+ // TODO(adonovan): replace this entire case with this.append(o).
+ String classname = o.getClass().getName();
+ if (classname.endsWith("vfs.PathFragment") || classname.endsWith("Path")) {
+ throw new IllegalStateException("invalid argument: " + o + " (" + classname + ")");
+ }
+
// Other types of objects shouldn't be leaked to Skylark, but if happens, their
// .toString method shouldn't be used because their return values are likely to contain
// memory addresses or other nondeterministic information.
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/BUILD b/src/main/java/com/google/devtools/build/lib/vfs/BUILD
index 1e39005..cdd7a6c 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/vfs/BUILD
@@ -26,7 +26,6 @@
deps = [
"//src/main/java/com/google/devtools/build/lib:filetype",
"//src/main/java/com/google/devtools/build/lib:os_util",
- "//src/main/java/com/google/devtools/build/lib:skylarkinterface",
"//src/main/java/com/google/devtools/build/lib/actions:commandline_item",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization",
"//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
@@ -51,7 +50,6 @@
deps = [
":pathfragment",
"//src/main/java/com/google/devtools/build/lib:filetype",
- "//src/main/java/com/google/devtools/build/lib:skylarkinterface",
"//src/main/java/com/google/devtools/build/lib/clock",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/profiler",
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/Path.java b/src/main/java/com/google/devtools/build/lib/vfs/Path.java
index 55f2bb2..c12818b 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/Path.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/Path.java
@@ -18,8 +18,6 @@
import com.google.common.hash.Hasher;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkPrintable;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
import com.google.devtools.build.lib.util.FileType;
import java.io.File;
import java.io.FileNotFoundException;
@@ -58,8 +56,7 @@
*/
@ThreadSafe
@AutoCodec
-public class Path
- implements Comparable<Path>, Serializable, SkylarkPrintable, FileType.HasFileType {
+public class Path implements Comparable<Path>, Serializable, FileType.HasFileType {
private static FileSystem fileSystemForSerialization;
/**
@@ -343,16 +340,6 @@
return OS.compare(this.path, o.path);
}
- @Override
- public void repr(SkylarkPrinter printer) {
- printer.append(path);
- }
-
- @Override
- public void str(SkylarkPrinter printer) {
- repr(printer);
- }
-
/** Returns true iff this path denotes an existing file of any kind. Follows symbolic links. */
public boolean exists() {
return fileSystem.exists(this, true);
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java b/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java
index ea4b2fd..4e5a5e9 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/PathFragment.java
@@ -22,8 +22,6 @@
import com.google.devtools.build.lib.skyframe.serialization.SerializationException;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.skyframe.serialization.strings.StringCodecs;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkPrintable;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
import com.google.devtools.build.lib.util.FileType;
import com.google.protobuf.CodedInputStream;
import com.google.protobuf.CodedOutputStream;
@@ -56,7 +54,6 @@
implements Comparable<PathFragment>,
Serializable,
FileType.HasFileType,
- SkylarkPrintable,
CommandLineItem {
private static final OsPathPolicy OS = OsPathPolicy.getFilePathOs();
@@ -351,11 +348,6 @@
}
@Override
- public void repr(SkylarkPrinter printer) {
- printer.append(normalizedPath);
- }
-
- @Override
public boolean equals(Object o) {
if (this == o) {
return true;
@@ -760,6 +752,7 @@
return new PathFragmentSerializationProxy(normalizedPath);
}
+ @SuppressWarnings("unused") // found by CLASSPATH-scanning magic
private static class Codec implements ObjectCodec<PathFragment> {
private final ObjectCodec<String> stringCodec = StringCodecs.asciiOptimized();
