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 list methods with Const #2570

Merged
merged 18 commits into from
Mar 10, 2024

Conversation

kmr-srbh
Copy link
Contributor

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

Fixes #2584

The following changes were made:

  • list.append - throw a SemanticError stating changes not allowed
  • list.remove - throw a SemanticError stating changes not allowed
  • list.count - fetch the element count
  • list.index - fetch the element index
  • list.reverse - throw a SemanticError stating changes not allowed
  • list.pop - throw a SemanticError stating changes not allowed
  • list.clear - throw a SemanticError stating changes not allowed
  • list.insert - throw a SemanticError stating changes not allowed

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

kmr-srbh commented Mar 3, 2024

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

@anutosh491
Copy link
Collaborator

I guess you need to update the references.

@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented Mar 3, 2024

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

Shaikh-Ubaid commented Mar 6, 2024

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

kmr-srbh commented Mar 7, 2024

@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 #2567 (comment)

@Shaikh-Ubaid
Copy link
Collaborator

Shaikh-Ubaid commented Mar 7, 2024

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

kmr-srbh commented Mar 7, 2024

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
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:56
@kmr-srbh kmr-srbh marked this pull request as ready for review March 7, 2024 09:54
@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented Mar 7, 2024

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.

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.

I shared two comments above. I think the PR is getting in shape. Thanks for working on this @kmr-srbh!

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

@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented Mar 7, 2024

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

I am adding the tests in a while.

@kmr-srbh
Copy link
Contributor Author

@Shaikh-Ubaid This PR is now ready.

tests/tests.toml Outdated Show resolved Hide resolved
tests/tests.toml Outdated Show resolved Hide resolved
tests/tests.toml Outdated Show resolved Hide resolved
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.

I think the PR is in good direction. Great work @kmr-srbh!

@Shaikh-Ubaid Shaikh-Ubaid marked this pull request as draft March 10, 2024 08:22
@Shaikh-Ubaid
Copy link
Collaborator

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

@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented Mar 10, 2024

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

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

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

@Shaikh-Ubaid
Copy link
Collaborator

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.

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.

@Shaikh-Ubaid
Copy link
Collaborator

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

@kmr-srbh kmr-srbh marked this pull request as ready for review March 10, 2024 13: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 great! Thanks for this! Amazing work!

@Shaikh-Ubaid Shaikh-Ubaid merged commit db6ed14 into lcompilers:main Mar 10, 2024
13 checks passed
@kmr-srbh kmr-srbh deleted the const-list-attributes branch March 19, 2024 05:34
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: List methods do not work on a Const list
4 participants