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 support for dict methods with Const #2567

Merged
merged 40 commits into from
May 1, 2024

Conversation

kmr-srbh
Copy link
Contributor

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

Fixes #2585

Support for following attributes was implemented:

  • dict.get - fetches the required value
  • dict.pop - throws a SemanticError stating that changes cannot be made to a const dict
  • dict.keys - fetches the list of keys
  • dict.values - fetches the list of values

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]

@kmr-srbh kmr-srbh changed the title Implement attributes for Const dict Add support for dict methods with Const Mar 3, 2024
@Shaikh-Ubaid
Copy link
Collaborator

@kmr-srbh What issue does this PR fix?

@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented Mar 7, 2024

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

Shaikh-Ubaid commented Mar 7, 2024

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

Shaikh-Ubaid commented Mar 7, 2024

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

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

@Thirumalai-Shaktivel Thirumalai-Shaktivel marked this pull request as draft March 7, 2024 06:57
@kmr-srbh kmr-srbh marked this pull request as ready for review March 7, 2024 13:29
@kmr-srbh
Copy link
Contributor Author

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

src/lpython/semantics/python_attribute_eval.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
src/libasr/codegen/asr_to_llvm.cpp Outdated Show resolved Hide resolved
tests/errors/test_dict_const.py Outdated Show resolved Hide resolved
kmr-srbh and others added 8 commits March 10, 2024 10:04
Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
Co-authored-by: Thirumalai Shaktivel <74826228+Thirumalai-Shaktivel@users.noreply.github.com>
@Shaikh-Ubaid
Copy link
Collaborator

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

kmr-srbh commented May 1, 2024

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

kmr-srbh commented May 1, 2024

@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 kmr-srbh marked this pull request as ready for review May 1, 2024 05:11
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.

Thanks for this! I shared some comments. I will review again after these are resolved.

src/libasr/codegen/asr_to_llvm.cpp Outdated Show resolved Hide resolved
src/libasr/codegen/asr_to_llvm.cpp Outdated Show resolved Hide resolved
src/libasr/codegen/asr_to_llvm.cpp Outdated Show resolved Hide resolved
integration_tests/test_const_dict.py Show resolved Hide resolved
tests/tests.toml Outdated Show resolved Hide resolved
@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented May 1, 2024

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

src/libasr/codegen/asr_to_llvm.cpp Outdated Show resolved Hide resolved
src/libasr/codegen/asr_to_llvm.cpp Outdated Show resolved Hide resolved
src/libasr/codegen/asr_to_llvm.cpp Outdated Show resolved Hide resolved
@kmr-srbh kmr-srbh requested a review from Shaikh-Ubaid May 1, 2024 16:17
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 enabled auto-merge (squash) May 1, 2024 16:37
@Shaikh-Ubaid Shaikh-Ubaid merged commit a74d529 into lcompilers:main May 1, 2024
14 checks passed
@kmr-srbh kmr-srbh deleted the const-dict-methods branch May 2, 2024 13:58
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.

Bug: Dict methods do not work on a Const dict
3 participants