Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 26 commits into from
Mar 17, 2024

Conversation

kmr-srbh
Copy link
Contributor

@kmr-srbh kmr-srbh commented Mar 9, 2024

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

  • Variables
integer_list: list[i32] = [1, 2, 3, 4]
print(type(integer_list))
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
<type 'list[i32]'>
  • Constants
print(type("Hello, LPython"))
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
<type 'str'>

Compare object types (requires #2591 to be merged)

integer: i32 = 1
integer_list: list[i32] = [1, 2, 3, 4, 5]
print(type(integer) == type(integer_list))
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
False

@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented Mar 9, 2024

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.

src/libasr/pass/intrinsic_function_registry.h Outdated Show resolved Hide resolved
src/libasr/pass/intrinsic_function_registry.h Outdated Show resolved Hide resolved
src/lpython/semantics/python_ast_to_asr.cpp Outdated Show resolved Hide resolved
src/libasr/pass/intrinsic_function_registry.h Outdated Show resolved Hide resolved
src/libasr/pass/intrinsic_function_registry.h Outdated Show resolved Hide resolved
kmr-srbh and others added 4 commits March 9, 2024 21:33
Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
@Thirumalai-Shaktivel
Copy link
Collaborator

Also, Let's add some tests.

kmr-srbh and others added 2 commits March 9, 2024 22:02
Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented Mar 9, 2024

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
Copy link
Collaborator

Minimum reproducible example (MRE), please?

@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented Mar 9, 2024

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()?

Copy link
Collaborator

@Thirumalai-Shaktivel Thirumalai-Shaktivel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)))

src/libasr/pass/intrinsic_function_registry.h Outdated Show resolved Hide resolved
src/libasr/pass/intrinsic_function_registry.h Outdated Show resolved Hide resolved
src/libasr/pass/intrinsic_function_registry.h Outdated Show resolved Hide resolved
kmr-srbh and others added 3 commits March 10, 2024 09:56
Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
@kmr-srbh
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

Thirumalai-Shaktivel commented Mar 10, 2024

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
Copy link
Contributor Author

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. 👍

@kmr-srbh
Copy link
Contributor Author

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
Copy link
Collaborator

Thirumalai-Shaktivel commented Mar 10, 2024

Which command did you use to build lpython?

@kmr-srbh
Copy link
Contributor Author

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
Copy link
Collaborator

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

Or you can just run ./build.sh

@kmr-srbh
Copy link
Contributor Author

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?

@kmr-srbh
Copy link
Contributor Author

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 kmr-srbh marked this pull request as ready for review March 10, 2024 06:18
@kmr-srbh
Copy link
Contributor Author

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

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'>"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
m_value = StringConstant("<type 'type'>", character(13)); // 13 is the length of the string "<type 'type'>"
m_value = StringConstant("<class 'type'>", character(13)); // 13 is the length of the string "<type 'type'>"
>>> type(type(1))
<class 'type'>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had intentionally not used the word 'class' as we do not currently support it fully. Should we write it here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, we have to make sure both CPython and LPython generates the same output.
So, I suggested the above changes and test this both in CPython and LPython(llvm).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am really sorry for the confusion. The return type of type() in CPython is of type - type, whereas here we are using a string for it. They would not be similar.

I request guidance on how do we go about this @Thirumalai-Shaktivel.

tests/tests.toml Outdated
Comment on lines 413 to 416
[[test]]
filename = "../integration_tests/test_builtin_type.py"
asr = true

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[[test]]
filename = "../integration_tests/test_builtin_type.py"
asr = true

Tests this using integration_tests/CMakeLists

test: llvm, cpython

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cpython test fails because the type() has a different return type there.

Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
@Thirumalai-Shaktivel
Copy link
Collaborator

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
Copy link
Collaborator

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

Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I shared some comments. Overall the implementation looks in the right direction.

integration_tests/test_builtin_type.py Outdated Show resolved Hide resolved
src/libasr/codegen/asr_to_c_cpp.h Show resolved Hide resolved
src/lpython/semantics/python_ast_to_asr.cpp Outdated Show resolved Hide resolved
@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as draft March 17, 2024 18:08
Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just need to fix #2592 (comment) and #2592 (comment). I think then this would be good to merge.

Copy link
Collaborator

@Shaikh-Ubaid Shaikh-Ubaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good! Thanks!

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as ready for review March 17, 2024 20:34
@Shaikh-Ubaid Shaikh-Ubaid merged commit 118d23e into lcompilers:main Mar 17, 2024
13 checks passed
@kmr-srbh kmr-srbh deleted the type-function branch March 19, 2024 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants