Clean up string representations for unknown objects

Don't call the `toString` method on unknown objects as it potentially breaks hermeticity and determinism. Use a generic string like "<unknown object package.ClassName>" instead. PiperOrigin-RevId: 161970449
acckcc · Jul 17, 2017 · d4cc4b6 · d4cc4b6
1 parent 9f18a51
commit d4cc4b6
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 15 deletions.
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
@@ -387,8 +387,10 @@ public BasePrinter repr(Object o) {
         this.append(o.toString());
 
       } else {
-        // TODO(bazel-team): change to a special representation for unknown objects
-        this.append(o.toString());
+        // 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.
+        this.append("<unknown object " + o.getClass().getName() + ">");
       }
 
       return this;

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
@@ -388,6 +388,17 @@ public void testStringRepresentations_Targets() throws Exception {
     }
   }
 
+  @Test
+  public void testStringRepresentationsOfUnknownObjects() throws Exception {
+    update("mock", new Object());
+
+    assertThat(eval("str(mock)")).isEqualTo("<unknown object java.lang.Object>");
+    assertThat(eval("repr(mock)")).isEqualTo("<unknown object java.lang.Object>");
+    assertThat(eval("'{}'.format(mock)")).isEqualTo("<unknown object java.lang.Object>");
+    assertThat(eval("'%s' % mock")).isEqualTo("<unknown object java.lang.Object>");
+    assertThat(eval("'%r' % mock")).isEqualTo("<unknown object java.lang.Object>");
+  }
+
   @Test
   public void testLegacyStringRepresentations_Labels() throws Exception {
     setSkylarkSemanticsOptions("--incompatible_descriptive_string_representations=false");

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
@@ -618,15 +618,6 @@ public void reprLegacy(SkylarkPrinter printer) {
     };
   }
 
-  private Object createUnknownObj() {
-    return new Object() {
-      @Override
-      public String toString() {
-        return "<unknown object>";
-      }
-    };
-  }
-
   @Test
   public void testPercOnObject() throws Exception {
     newTest("--incompatible_descriptive_string_representations=true")
@@ -636,8 +627,8 @@ public void testPercOnObject() throws Exception {
         .update("obj", createObjWithStr())
         .testStatement("'%s' % obj", "<str legacy marker>");
     newTest()
-        .update("unknown", createUnknownObj())
-        .testStatement("'%s' % unknown", "<unknown object>");
+        .update("unknown", new Object())
+        .testStatement("'%s' % unknown", "<unknown object java.lang.Object>");
   }
 
   @Test
@@ -649,8 +640,10 @@ public void testPercOnObjectList() throws Exception {
         .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>");
+        .update("unknown", new Object())
+        .testStatement(
+            "'%s %s' % (unknown, unknown)",
+            "<unknown object java.lang.Object> <unknown object java.lang.Object>");
   }
 
   @Test