lcompilers / lpython

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

Add compile-time support for `dict.keys` #2660

Closed kmr-srbh closed 2 months ago

kmr-srbh commented 2 months ago

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 commented 2 months ago

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

Shaikh-Ubaid commented 2 months ago

@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 commented 2 months ago

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.

Shaikh-Ubaid commented 2 months ago

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

kmr-srbh commented 2 months ago

@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. :+1:

Shaikh-Ubaid commented 2 months ago

Overall, it seems fine to me. Let's wait on this until https://github.com/lcompilers/lpython/pull/2661#issuecomment-2080395795 is clear.