lcompilers / lpython

Python compiler
https://lpython.org/
Other
1.5k stars 158 forks source link

Add support for `dict` methods with `Const` #2567

Closed kmr-srbh closed 5 months ago

kmr-srbh commented 7 months ago

Fixes #2585

Support for following attributes was implemented:

get()

d: Const[dict[str, i32]] = {"a": 1, "b": 2, "c": 3}
print(d.get("a"))
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
1

pop()

d: Const[dict[str, i32]] = {"a": 1, "b": 2, "c": 3}
print(d.pop("a"))
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
semantic error: cannot pop elements from a const dict
 --> ./examples/example.py:8:7
  |
8 | print(d.pop("a"))
  |       ^^^^^^^^^^ 

Note: Please report unclear or confusing messages as bugs at
https://github.com/lcompilers/lpython/issues.

keys()

d: Const[dict[str, i32]] = {"a": 1, "b": 2, "c": 3}
print(d.keys())
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
['c', 'a', 'b']

values()

d: Const[dict[str, i32]] = {"a": 1, "b": 2, "c": 3}
print(d.values())
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
[3, 1, 2]
Shaikh-Ubaid commented 7 months ago

@kmr-srbh What issue does this PR fix?

kmr-srbh commented 7 months ago

@kmr-srbh What issue does this PR fix?

I did not create a separate issue for this as I had worked out a fix. The main problem was that accessing values inside Const was not being handled when handling subscript indices. The same was true for Const list. I had worked out #2560 for that. Screenshot from 2024-03-07 08-17-54

In the testing process I found that the dictionary methods were still failing. This PR adds support for using dictionary methods on a Const dict. #2570 does the same for Const list. Screenshot from 2024-03-07 08-19-00

Shaikh-Ubaid commented 7 months ago

@kmr-srbh What issue does this PR fix?

I did not create a separate issue for this as I had worked out a fix.

Why don't you create an issue first and list all the bugs related to Dict and Dict operations that you discovered? Share an example for each failure. Then in this PR specify exactly which bugs you have fixed (or are targeting to fix).

That would help highlight the contributions made and also make it simpler to review. Thanks!

Shaikh-Ubaid commented 7 months ago

Also, you do not need to share the output as an image. Instead, you can simply share it in markdown as:

(lp) ubaid@ubaids-MacBook-Pro lpython % cat examples/expr2.py  
from lpython import i32

def main0():
    x: i32
    x = (2+3)*5
    print(x)

if __name__ == "__main__":
   main0()
(lp) ubaid@ubaids-MacBook-Pro lpython % lpython examples/expr2.py 
25

Sharing in text format as above is helpful as you can also search for some keyword in your code later.

The markdown code for the above code snippet is:

```python
(lp) ubaid@ubaids-MacBook-Pro lpython % cat examples/expr2.py  
from lpython import i32

def main0():
    x: i32
    x = (2+3)*5
    print(x)

if __name__ == "__main__":
   main0()
```
```console
(lp) ubaid@ubaids-MacBook-Pro lpython % lpython examples/expr2.py 
25
```
Thirumalai-Shaktivel commented 7 months ago

Please mark this PR ready for review once it is ready.

kmr-srbh commented 6 months ago

@Shaikh-Ubaid @Thirumalai-Shaktivel This PR is now ready.

kmr-srbh commented 6 months ago

@Shaikh-Ubaid The same error here too:

code generation error: asr_to_llvm: module failed verification. Error:
Stored value type does not match pointer operand type!
  store %dict* %const_dict, %dict* %CONST_DICTIONARY_FLOAT, align 8
 %dict = type { i32, i32, i32, %key_value*, i8*, i1 }Stored value type does not match pointer operand type!
  store %dict.1* %const_dict298, %dict.1* %CONST_DICTIONARY_INTEGR, align 8
 %dict.1 = type { i32, i32, i32, %key_value.0*, i8*, i1 }
Shaikh-Ubaid commented 6 months ago

@Shaikh-Ubaid The same error here too:

I believe the fix could/would be similar to the one we did for https://github.com/lcompilers/lpython/pull/2570. I will look into it tomorrow. Untill then check https://github.com/lcompilers/lpython/pull/2570/commits/0b9b94fd9dffad08adaec9f13a3bf7bc47f9e769 and see if you can figure out a fix along similar lines.

kmr-srbh commented 6 months ago

Untill then check https://github.com/lcompilers/lpython/commit/0b9b94fd9dffad08adaec9f13a3bf7bc47f9e769 and see if you can figure out a fix along similar lines.

I am trying @Shaikh-Ubaid.

kmr-srbh commented 6 months ago

@Shaikh-Ubaid could you please look into https://github.com/lcompilers/lpython/pull/2567#issuecomment-1987333638. I could not devote much time to work a fix for it. This is the main blocker in this PR and #2579 .

Thirumalai-Shaktivel commented 6 months ago

Marking this PR as draft for now.

Shaikh-Ubaid commented 5 months ago

Const ASR node was recently removed. @kmr-srbh please, can you check if the examples shared in the PR description and the examples added in the integration tests in this PR work with the latest main? Can you share each example that you check and share its output here?

kmr-srbh commented 5 months ago

Const ASR node was recently removed. @kmr-srbh please, can you check if the examples shared in the PR description and the examples added in the integration tests in this PR work with the latest main? Can you share each example that you check and share its output here?

I will the share the output. I think this should get back to working with a few small changes. The changes we had previously made to handle const list (merged) works. I think we will have to replace the ASR::is_a<ASR::Const_t> with ASRUtils::is_const(). The call to ASRUtils::type_get_past_const() must be removed too. I will look into this PR and make the necessary changes to get this working.

Thanks a lot for looking into this @Shaikh-Ubaid!

kmr-srbh commented 5 months ago

@Shaikh-Ubaid the PR is ready. Here is an example that works:

CONST_DICT: Const[dict[str, i32]] = {"a": 1, "b": 2, "c": 3}
print(CONST_DICT.get("a"))
print(CONST_DICT.keys())
print(CONST_DICT.values())

# print(CONST_DICT.pop("a"))
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
1
['a', 'b', 'c']
[1, 2, 3]

Uncommenting the last line throws the required error:

(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
semantic error: cannot pop elements from a const dict
 --> ./examples/example.py:6:7
  |
6 | print(CONST_DICT.pop("a"))
  |       ^^^^^^^^^^^^^^^^^^^ 

Note: Please report unclear or confusing messages as bugs at
https://github.com/lcompilers/lpython/issues.
kmr-srbh commented 5 months ago

@Shaikh-Ubaid I have made the necessary changes. We tests keys() and values() by checking the output length. The tests fail with an LLVM codegen error in local scope. I have used a global scope to test this.

Here is an example that fails:

def f():
    CONST_DICT: Const[dict[str, i32]] = {"a": 1, "b": 2, "c": 3}
    print(CONST_DICT.get("a"))
    print(CONST_DICT.keys())
    print(CONST_DICT.values())

f()