[Mlir-commits] [mlir] 9f53354 - [mlir][python] Remove __str__ from bindings of StringAttr.

[Ingo Müller]

Author: Ingo Müller
Date: 2023-09-01T07:35:54Z
New Revision: 9f5335487ae559a5f7976f1c3cb92ded2b123016

URL: https://github.com/llvm/llvm-project/commit/9f5335487ae559a5f7976f1c3cb92ded2b123016
DIFF: https://github.com/llvm/llvm-project/commit/9f5335487ae559a5f7976f1c3cb92ded2b123016.diff

LOG: [mlir][python] Remove __str__ from bindings of StringAttr.

This reverts a feature introduced in commit
2a5d497494c24425e99655b85e2277dd3f15a400. The goal of that commit was to
allow `StringAttr`s to by used transparently wherever Python `str`s are
expected. But, as the tests in https://reviews.llvm.org/D159182 reveal,
pybind11 doesn't do this conversion based on `__str__` automatically,
unlike for the other types introduced in the commit above. At the same
time, changing `__str__` breaks the symmetry with other attributes of
`print(attr)` printing the assembly of the attribute, so the change
probably has more disadvantages than advantages.

Reviewed By: springerm, rkayaith

Differential Revision: https://reviews.llvm.org/D159255

Added: 
    

Modified: 
    mlir/lib/Bindings/Python/IRAttributes.cpp
    mlir/test/python/dialects/builtin.py
    mlir/test/python/ir/attributes.py

Removed: 
    


################################################################################
diff  --git a/mlir/lib/Bindings/Python/IRAttributes.cpp b/mlir/lib/Bindings/Python/IRAttributes.cpp
index 6531d6276ae67f..105d2cecf20a19 100644
--- a/mlir/lib/Bindings/Python/IRAttributes.cpp
+++ b/mlir/lib/Bindings/Python/IRAttributes.cpp
@@ -596,8 +596,13 @@ class PyStringAttribute : public PyConcreteAttribute<PyStringAttribute> {
         },
         py::arg("type"), py::arg("value"),
         "Gets a uniqued string attribute associated to a type");
-    c.def_property_readonly("value", toPyStr,
-                            "Returns the value of the string attribute");
+    c.def_property_readonly(
+        "value",
+        [](PyStringAttribute &self) {
+          MlirStringRef stringRef = mlirStringAttrGetValue(self);
+          return py::str(stringRef.data, stringRef.length);
+        },
+        "Returns the value of the string attribute");
     c.def_property_readonly(
         "value_bytes",
         [](PyStringAttribute &self) {
@@ -605,14 +610,6 @@ class PyStringAttribute : public PyConcreteAttribute<PyStringAttribute> {
           return py::bytes(stringRef.data, stringRef.length);
         },
         "Returns the value of the string attribute as `bytes`");
-    c.def("__str__", toPyStr,
-          "Converts the value of the string attribute to a Python str");
-  }
-
-private:
-  static py::str toPyStr(PyStringAttribute &self) {
-    MlirStringRef stringRef = mlirStringAttrGetValue(self);
-    return py::str(stringRef.data, stringRef.length);
   }
 };
 

diff  --git a/mlir/test/python/dialects/builtin.py b/mlir/test/python/dialects/builtin.py
index cb7d4c5629f7e7..18ebba61e7fea8 100644
--- a/mlir/test/python/dialects/builtin.py
+++ b/mlir/test/python/dialects/builtin.py
@@ -134,13 +134,13 @@ def testBuildFuncOp():
                 ),
                 visibility="nested",
             )
-            # CHECK: Name is: some_func
+            # CHECK: Name is: "some_func"
             print("Name is: ", f.name)
 
             # CHECK: Type is: (tensor<2x3x4xf32>, tensor<2x3x4xf32>) -> tensor<2x3x4xf32>
             print("Type is: ", f.type)
 
-            # CHECK: Visibility is: nested
+            # CHECK: Visibility is: "nested"
             print("Visibility is: ", f.visibility)
 
             try:

diff  --git a/mlir/test/python/ir/attributes.py b/mlir/test/python/ir/attributes.py
index 8c0c01d253baa7..dbd6bad05e01dc 100644
--- a/mlir/test/python/ir/attributes.py
+++ b/mlir/test/python/ir/attributes.py
@@ -21,7 +21,7 @@ def testParsePrint():
     assert t.context is ctx
     ctx = None
     gc.collect()
-    # CHECK: hello
+    # CHECK: "hello"
     print(str(t))
     # CHECK: StringAttr("hello")
     print(repr(t))
@@ -290,25 +290,14 @@ def testStringAttr():
         sattr = StringAttr(Attribute.parse('"stringattr"'))
         # CHECK: sattr value: stringattr
         print("sattr value:", sattr.value)
-        # CHECK: sattr value_bytes: b'stringattr'
-        print("sattr value_bytes:", sattr.value_bytes)
-        # CHECK: sattr str: stringattr
-        print("sattr str:", str(sattr))
-
-        typed_sattr = StringAttr(Attribute.parse('"stringattr" : i32'))
-        # CHECK: typed_sattr value: stringattr
-        print("typed_sattr value:", typed_sattr.value)
-        # CHECK: typed_sattr str: stringattr
-        print("typed_sattr str:", str(typed_sattr))
+        # CHECK: sattr value: b'stringattr'
+        print("sattr value:", sattr.value_bytes)
 
         # Test factory methods.
-        # CHECK: default_get: StringAttr("foobar")
-        print("default_get:", repr(StringAttr.get("foobar")))
-        # CHECK: typed_get: StringAttr("12345" : i32)
-        print(
-            "typed_get:",
-            repr(StringAttr.get_typed(IntegerType.get_signless(32), "12345")),
-        )
+        # CHECK: default_get: "foobar"
+        print("default_get:", StringAttr.get("foobar"))
+        # CHECK: typed_get: "12345" : i32
+        print("typed_get:", StringAttr.get_typed(IntegerType.get_signless(32), "12345"))
 
 
 # CHECK-LABEL: TEST: testNamedAttr
@@ -317,8 +306,8 @@ def testNamedAttr():
     with Context():
         a = Attribute.parse('"stringattr"')
         named = a.get_named("foobar")  # Note: under the small object threshold
-        # CHECK: attr: StringAttr("stringattr")
-        print("attr:", repr(named.attr))
+        # CHECK: attr: "stringattr"
+        print("attr:", named.attr)
         # CHECK: name: foobar
         print("name:", named.name)
         # CHECK: named: NamedAttribute(foobar="stringattr")