lcompilers / lpython

Python compiler
https://lpython.org/
Other
1.37k stars 157 forks source link

Add an intrinsic implementation of `type(object)` function #2592

Closed kmr-srbh closed 3 months ago

kmr-srbh commented 4 months ago

Currently the logic is simple - get the type of the passed argument using ASRUtils::type_to_str_python(*ttype_t) and return it as a StringConstant. I request guidance to improve this PR.

Get type of the object

kmr-srbh commented 4 months ago

The reason which causes the above 2 tests to fail is also the one I need help with. The argument Vec<ASR::expr_t*> args remains unused as I get the type directly through ASR::ttype_t* t1. @Thirumalai-Shaktivel please have a look into the PR.

Thirumalai-Shaktivel commented 4 months ago

Also, Let's add some tests.

kmr-srbh commented 4 months ago

Also, Let's add some tests.

There is a small issue. The return type of type() is currently the same as the passed argument. How do we go about fixing it?

Thirumalai-Shaktivel commented 4 months ago

Minimum reproducible example (MRE), please?

kmr-srbh commented 4 months ago

Minimum reproducible example (MRE), please?

l: list[i32] = [1, 2, 3, 4, 5]
t: str = type(l)

Another one

l: list[i32] = [1, 2, 3, 4, 5]
print(type(type(l))

In CPython, a statement like the above returns <class 'type'>. Should we also not have a separate type for type()?

kmr-srbh commented 4 months ago

I think the following code output would differ with CPython, we have to fix it before merging this PR. Rest looks good.

print(type(type(1)))

Going the print() way?

Thirumalai-Shaktivel commented 4 months ago

Nope, I meant the case: type(type(1)) Here CPython prints as: <class 'type'>, But LPython using this PR changes, prints as <type 'str'>. We need to fix this.

kmr-srbh commented 4 months ago

Nope, I meant the case: type(type(1)) Here CPython prints as: <class 'type'>, But LPython using this PR changes, prints as <type 'str'>. We need to fix this.

But this will always be the case until we create a specific type for type(). Imagine type(type(type(1))).

Thirumalai-Shaktivel commented 4 months ago

Check if the args[0] is IntrinsicScalarFunciton and down_cast it and check if the m_intrinsic_id is static_cast<IntrinsicScalarFunctions::ObjectType>, if so then store the output as <class 'type'>. Otherwise do eval_ObjectType, it would work for any number of nested type(..).

Later in the future, if we find any other different output or cases, we will modify it accordingly.

kmr-srbh commented 4 months ago

Check if the args[0] is IntrinsicScalarFunciton and down_cast it and check if the m_intrinsic_id is static_cast<IntrinsicScalarFunctions::ObjectType>, if so then store the output as <class 'type'>. Otherwise do eval_ObjectType, it would work for any number of nested type(..).

Later in the future, if we find any other different output or cases, we will modify it accordingly.

Hmm, this seems a good idea. Let's test it out. :+1:

kmr-srbh commented 4 months ago

I am getting this error now. I think it is because of the clean build we did for the isspace() branch.

CMake Error at CMakeLists.txt:10 (file):
  file STRINGS file "/home/saurabh-kumar/Projects/System/lpython/version"
  cannot be read.
.
.
.
-- Configuring incomplete, errors occurred!
Thirumalai-Shaktivel commented 4 months ago

Which command did you use to build lpython?

kmr-srbh commented 4 months ago

Which command did you use to build lpython?

When this error occurred now, I followed the complete process which we do on a fresh build. As it turns out, only ./build1.sh threw the error. Other commands did not do anything.

Thirumalai-Shaktivel commented 4 months ago

Before ./build1.sh, we have to run ./build0.sh to generate all the required files.

Or you can just run ./build.sh

kmr-srbh commented 4 months ago

Thanks @Thirumalai-Shaktivel it works now. It was an issue with the tag which had previously caused ./build0.sh to fail. What is the correct way of using tags here at LPython? Always using the default one?

Thirumalai-Shaktivel commented 4 months ago

Yes, I think git automatically picks the correct tag, see ci/version.sh

kmr-srbh commented 4 months ago

I am pushing the tests.

kmr-srbh commented 4 months ago

Thanks a lot for your support and guidance @Thirumalai-Shaktivel! Without you I could not have made it this far. Thank you very much!

kmr-srbh commented 3 months ago

@Thirumalai-Shaktivel do you think this PR is ready?

Thirumalai-Shaktivel commented 3 months ago

Apply the following diff: Let's test only the data types for now; later, we can handle it based on the requirements.

diff --git a/integration_tests/test_builtin_type.py b/integration_tests/test_builtin_type.py
index dea42bc74..961ed245c 100644
--- a/integration_tests/test_builtin_type.py
+++ b/integration_tests/test_builtin_type.py
@@ -1,19 +1,27 @@
-from lpython import i32, f64, list, str, dict, Const
+from lpython import i32, f64, Const

 def test_builtin_type():
     i: i32 = 42
+    f: f64 = 64.
     s: str = "Hello, LPython!"
     l: list[i32] = [1, 2, 3, 4, 5]
     d: dict[str, i32] = {"a": 1, "b": 2, "c": 3}
-    CONST_LIST: Const[list[f64]] = [12.22, 14.63, 33.82, 19.18]
+    res: str = ""

-    assert type(i) == "<type 'i32'>"
-    assert type(s) == "<type 'str'>"
-    assert type(l) == "<type 'list[i32]'>"
-    assert type(d) == "<type 'dict[str, i32]'>"
-    assert type(CONST_LIST) == "<type 'Const[list[f64]]'>"
-
-    assert type(type(i)) == "<type 'type'>"
-    assert type(type(CONST_LIST)) == "<type 'type'>"
+    res = str(type(i))
+    print(res)
+    assert res == "<class 'int'>"
+    res = str(type(f))
+    print(res)
+    assert res == "<class 'float'>"
+    res = str(type(s))
+    print(res)
+    assert res == "<class 'str'>"
+    res = str(type(l))
+    print(res)
+    assert res == "<class 'list'>"
+    res = str(type(d))
+    print(res)
+    assert res == "<class 'dict'>"

 test_builtin_type()
diff --git a/src/libasr/pass/intrinsic_function_registry.h b/src/libasr/pass/intrinsic_function_registry.h
index e70524b62..19d219f53 100644
--- a/src/libasr/pass/intrinsic_function_registry.h
+++ b/src/libasr/pass/intrinsic_function_registry.h
@@ -1173,7 +1173,24 @@ namespace ObjectType {

     static ASR::expr_t *eval_ObjectType(Allocator &al, const Location &loc,
             ASR::ttype_t* t1, Vec<ASR::expr_t*>& /*args*/) {
-        std::string object_type = "<type '" + ASRUtils::type_to_str_python(t1) + "'>";
+        std::string object_type = "<class '";
+        switch (t1->type) {
+            case ASR::ttypeType::Integer : {
+                object_type += "int"; break;
+            } case ASR::ttypeType::Real : {
+                object_type += "float"; break;
+            } case ASR::ttypeType::Character : {
+                object_type += "str"; break;
+            } case ASR::ttypeType::List : {
+                object_type += "list"; break;
+            } case ASR::ttypeType::Dict : {
+                object_type += "dict"; break;
+            } default: {
+                LCOMPILERS_ASSERT_MSG(false, "Unsupported type");
+                break;
+            }
+        }
+        object_type += "'>";
         return StringConstant(object_type, character(object_type.length()));
     }

@@ -1185,17 +1202,8 @@ namespace ObjectType {
         }
         ASR::expr_t *m_value = nullptr;
         Vec<ASR::expr_t *> arg_values;
-
-        if (ASR::is_a<ASR::IntrinsicScalarFunction_t>(*args[0])) {
-            ASR::IntrinsicScalarFunction_t *object = ASR::down_cast<ASR::IntrinsicScalarFunction_t>(args[0]);
-            if (static_cast<IntrinsicScalarFunctions>(object->m_intrinsic_id) == IntrinsicScalarFunctions::ObjectType) {
-                m_value = StringConstant("<type 'type'>", character(13)); // 13 is the length of the string "<type 'type'>"
-            }
-        }
-        else {
-            m_value = eval_ObjectType(al, loc, expr_type(args[0]), arg_values);
-        }
-
+        m_value = eval_ObjectType(al, loc, expr_type(args[0]), arg_values);
+
         return ASR::make_IntrinsicScalarFunction_t(al, loc,
             static_cast<int64_t>(IntrinsicScalarFunctions::ObjectType),
             args.p, args.n, 0, ASRUtils::expr_type(m_value), m_value);
Thirumalai-Shaktivel commented 3 months ago

@Shaikh-Ubaid, do you have any design suggestions here?