lcompilers / lpython

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

Add support for `list` methods with `Const` #2570

Closed kmr-srbh closed 6 months ago

kmr-srbh commented 7 months ago

Fixes #2584

The following changes were made:

count()

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

append()

l: Const[list[i32]] = [1, 2, 3, 4, 5, 1]
print(l.append(6))
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
semantic error: cannot append element to a const list
 --> ./examples/example.py:8:7
  |
8 | print(l.append(6))
  |       ^^^^^^^^^^^ 

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

Other functions work accordingly.

kmr-srbh commented 7 months ago

I am not sure why the tests fail. It passes when manually executing the file.

anutosh491 commented 7 months ago

I guess you need to update the references.

kmr-srbh commented 7 months ago

I guess you need to update the references.

I have not written any new tests @anutosh491. These are the old ones which are failing.

I made changes to the count method. It says the test related to the method failed, but when I execute it manually on my PC, it passes .i.e prints True for all the asserts.

Shaikh-Ubaid commented 7 months ago

@kmr-srbh what specific test-case or example that fails with LPython (of current main branch) does this PR fix/support?

PS: Can you point to the issue that this PR fixes?

kmr-srbh commented 7 months ago

@kmr-srbh what specific test-case or example that fails with LPython (of current main branch) does this PR fix/support?

PS: Can you point to the issue that this PR fixes?

Please see https://github.com/lcompilers/lpython/pull/2567#issuecomment-1982248424

Shaikh-Ubaid commented 7 months ago

Why don't you create an issue first and list all the bugs related to List and List 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!

kmr-srbh commented 7 months ago

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

I am opening an issue. I will keep it in mind for future bugs. Thank you!

Thirumalai-Shaktivel commented 7 months ago

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

kmr-srbh commented 7 months ago

I am adding the tests @Thirumalai-Shaktivel and @Shaikh-Ubaid. In the meantime, I request you to review this. In accordance to the simplification advised by @Shaikh-Ubaid , I have simplified all such expressions. I have tested the changes locally.

I am doing the same for the dictionary PR too.

kmr-srbh commented 7 months ago

For each SemanticError() newly added in this PR, please add an error reference test.

I am adding the tests in a while.

kmr-srbh commented 6 months ago

@Shaikh-Ubaid This PR is now ready.

Shaikh-Ubaid commented 6 months ago

Please mark it as "Ready for review" when ready.

kmr-srbh commented 6 months ago

I get this error when running the tests from ./integration_tests

from lpython import i32, list, str, Const

def test_const_list():
    CONST_INTEGER_LIST: Const[list[i32]] = [1, 2, 3, 4, 5, 1]

    assert CONST_INTEGER_LIST.count(1) == 2
    assert CONST_INTEGER_LIST.index(1) == 0

    CONST_STRING_LIST: Const[list[str]] = ["ALPHA", "BETA", "RELEASE"]
    assert CONST_STRING_LIST.count("ALPHA") == 1
    assert CONST_STRING_LIST.index("RELEASE") == 2

test_const_list()
code generation error: asr_to_llvm: module failed verification. Error:
Stored value type does not match pointer operand type!
  store %list* %const_list, %list* %CONST_INTEGER_LIST, align 8
 %list = type { i32, i32, i32* }Stored value type does not match pointer operand type!
  store %list.0* %const_list1, %list.0* %CONST_STRING_LIST, align 8
 %list.0 = type { i32, i32, i8** }
Shaikh-Ubaid commented 6 months ago

I get this error when running the tests from ./integration_tests

I attempted fixing it. Let's see if the CI passes.

Shaikh-Ubaid commented 6 months ago

I rebased this PR on main branch. Hence, to push to this branch further, you would need to first pull the changes from this branch with --rebase flag. For example,

git pull git@github.com:kmr-srbh/lpython.git const-list-attributes --rebase

or simply checkout a new copy of the remote branch and work on it if you are comfortable that way.

Shaikh-Ubaid commented 6 months ago

Please mark it as "Ready for review" when ready. I will do a final review then.