Skip to content

Add compile-time support for dict.keys #2660

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

Merged
merged 9 commits into from
Apr 27, 2024

Conversation

kmr-srbh
Copy link
Contributor

@kmr-srbh kmr-srbh commented Apr 24, 2024

Working

from lpython import i32


print({1: "a"}.keys())
print({"a": 1, "b": 2, "c": 3}.keys())
print({(1, 2): "a", (3, 4): "b", (5, 6): "c"}.keys())

k_1: list[i32] = {
    1: [1, 2, 3],
    2: [4, 5, 6],
    3: [7, 8, 9],
}.keys()
print(k_1)

k_2: list[tuple[i32, i32]] = {
    (1, 2): "a",
    (3, 4): "b",
    (5, 6): "c",
}.keys()
print(k_2)

CPython

(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ python ./examples/example.py
dict_keys([1])
dict_keys(['a', 'b', 'c'])
dict_keys([(1, 2), (3, 4), (5, 6)])
dict_keys([1, 2, 3])
dict_keys([(1, 2), (3, 4), (5, 6)])

LPython

(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
[1]
['a', 'b', 'c']
[(1, 2), (3, 4), (5, 6)]
[1, 2, 3]
[(1, 2), (3, 4), (5, 6)]

@kmr-srbh
Copy link
Contributor Author

@Shaikh-Ubaid , all the tests pass on my local branch. Why does it fail here?

@ubaidsk
Copy link
Collaborator

ubaidsk commented Apr 24, 2024

@Shaikh-Ubaid , all the tests pass on my local branch. Why does it fail here?

It fails because the test updates that you made fail with CPython:

% cat examples/expr2.py 
from lpython import i32, f64
def test_dict_keys_values():
    d1: dict[i32, i32] = {}
    k1: list[i32]
    k1_copy: list[i32] = []
    v1: list[i32]
    v1_copy: list[i32] = []
    i: i32
    j: i32
    s: str
    key_count: i32
    for i in range(105, 115):
        d1[i] = i + 1
    k1 = d1.keys()
    for i in k1:
        k1_copy.append(i)
    v1 = d1.values()
    for i in v1:
        v1_copy.append(i)
    assert len(k1) == 10
    for i in range(105, 115):
        key_count = 0
        for j in range(len(k1)):
            if k1_copy[j] == i:
                key_count += 1
                assert v1_copy[j] == d1[i]
        assert key_count == 1
    d2: dict[str, str] = {}
    k2: list[str]
    k2_copy: list[str] = []
    v2: list[str]
    v2_copy: list[str] = []
    for i in range(105, 115):
        d2[str(i)] = str(i + 1)
    k2 = d2.keys()
    for s in k2:
        k2_copy.append(s)
    v2 = d2.values()
    for s in v2:
        v2_copy.append(s)
    assert len(k2) == 10
    for i in range(105, 115):
        key_count = 0
        for j in range(len(k2)):
            if k2_copy[j] == str(i):
                key_count += 1
                assert v2_copy[j] == d2[str(i)]
        assert key_count == 1


    # dict.keys on dict constant
    assert {1: "a"}.keys() == [1]
    print({1: "a"}.keys())

    assert {"a": 1, "b": 2, "c": 3}.keys() == ["a", "b", "c"]
    print({"a": 1, "b": 2, "c": 3}.keys())

    assert {1: [1, 2, 3], 2: [4, 5, 6], 3: [7, 8, 9]}.keys() == [1, 2, 3]
    print({1: [1, 2, 3], 2: [4, 5, 6], 3: [7, 8, 9]}.keys())

    assert {(1, 2): "a", (3, 4): "b", (5, 6): "c"}.keys() == [(1, 2), (3, 4), (5, 6)]
    print({(1, 2): "a", (3, 4): "b", (5, 6): "c"}.keys())

    k_1: list[i32] = {1: [1, 2, 3], 2: [4, 5, 6], 3: [7, 8, 9]}.keys()
    assert k_1 == [1, 2, 3]
    print(k_1)

    k_2: list[tuple[i32, i32]] = {(1, 2): "a", (3, 4): "b", (5, 6): "c"}.keys()
    assert k_2 == [(1, 2), (3, 4), (5, 6)]
    print(k_2)


test_dict_keys_values()
% python examples/expr2.py 
Traceback (most recent call last):
  File "/Users/ubaid/Desktop/OpenSource/lpython/examples/expr2.py", line 73, in <module>
    test_dict_keys_values()
  File "/Users/ubaid/Desktop/OpenSource/lpython/examples/expr2.py", line 52, in test_dict_keys_values
    assert {1: "a"}.keys() == [1]
AssertionError

Every test we add needs to work with CPython. After that we support it for LPython. If a test works with LPython, but does not work with CPython, then it means that we are working on some featured that is not required.

@kmr-srbh
Copy link
Contributor Author

Got the issue @Shaikh-Ubaid! You are absolutely right about the interoperability. The issue is related to the way I am testing the method. The output of dict.keys is an object view in CPython, but we do not support that yet. I am pushing new tests that check the length instead.

@ubaidsk
Copy link
Collaborator

ubaidsk commented Apr 25, 2024

@kmr-srbh do you mind posting the output for cpython for the example shared in the PR description?

@ubaidsk ubaidsk marked this pull request as draft April 25, 2024 19:35
@kmr-srbh
Copy link
Contributor Author

@kmr-srbh do you mind posting the output for cpython for the example shared in the PR description?

I am posting the output @Shaikh-Ubaid. 👍

Copy link
Collaborator

@ubaidsk ubaidsk left a comment

Choose a reason for hiding this comment

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

Shared two comments. Rest looks good to me. Thanks for this! Great work!

@@ -51,4 +51,27 @@ def test_dict_keys_values():
assert v2_copy[j] == d2[str(i)]
assert key_count == 1


# dict.keys on dict constant
assert len({1: "a"}.keys()) == 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you update the tests according to #2661 (comment)?

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 initially tried this @Shaikh-Ubaid , but it throws the error as stated by you in #2667. I am looking into it.

@ubaidsk
Copy link
Collaborator

ubaidsk commented Apr 27, 2024

Overall, it seems fine to me. Let's wait on this until #2661 (comment) is clear.

@kmr-srbh kmr-srbh marked this pull request as ready for review April 27, 2024 11:42
@kmr-srbh kmr-srbh requested a review from ubaidsk April 27, 2024 11:42
Copy link
Collaborator

@ubaidsk ubaidsk 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 to me. Thanks for this!

@ubaidsk ubaidsk force-pushed the dict-constant-keys branch from 317ddb6 to a5a3ebf Compare April 27, 2024 12:52
@ubaidsk ubaidsk enabled auto-merge (squash) April 27, 2024 12:52
@ubaidsk ubaidsk merged commit 6036111 into lcompilers:main Apr 27, 2024
14 checks passed
assem2002 pushed a commit to assem2002/lpython that referenced this pull request Apr 28, 2024
* Implement `dict.keys` for `DictConstant`

* Tests: Add tests and update references

* Tests: Update test

* Uncomment check for modifying attributes

* Tests: Update test references

* Delete tests/reference/asr-func_04-eef2656.stdout

* Style changes

Co-authored-by: Shaikh Ubaid <[email protected]>

* Style changes

Co-authored-by: Shaikh Ubaid <[email protected]>

* Tests: Move `print` before `assert`

---------

Co-authored-by: Shaikh Ubaid <[email protected]>
assem2002 pushed a commit to assem2002/lpython that referenced this pull request Apr 28, 2024
* Implement `dict.keys` for `DictConstant`

* Tests: Add tests and update references

* Tests: Update test

* Uncomment check for modifying attributes

* Tests: Update test references

* Delete tests/reference/asr-func_04-eef2656.stdout

* Style changes

Co-authored-by: Shaikh Ubaid <[email protected]>

* Style changes

Co-authored-by: Shaikh Ubaid <[email protected]>

* Tests: Move `print` before `assert`

---------

Co-authored-by: Shaikh Ubaid <[email protected]>
@kmr-srbh kmr-srbh deleted the dict-constant-keys branch May 2, 2024 13:57
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.

2 participants