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 item access from Const data-structures #2579

Merged
merged 33 commits into from
May 1, 2024

Conversation

kmr-srbh
Copy link
Contributor

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

Fixes #2578
The following changes were made:

  • Handle subscripts for mutable types dict and list. Return the corresponding value.
  • Throw an error for immutable types stating that Const is not required.

Fix

Dictionary

d: Const[dict[str, i32]] = {"a": 1, "b": 2, "c": 3}
print(d["a"])
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
1

List

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

Tuple

t: Const[tuple[i32, i32, i32, i32, i32]] = (1, 2, 3, 4, 5)
print(t[0])
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
semantic error: 'Const' not required as tuples are already immutable
  --> ./examples/example.py:10:4
   |
10 | t: Const[tuple[i32, i32, i32, i32, i32]] = (1, 2, 3, 4, 5)
   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 


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

String

s: Const[str] = "Hello, LPython!"
print(s[0])
(base) saurabh-kumar@Awadh:~/Projects/System/lpython$ ./src/bin/lpython ./examples/example.py
semantic error: 'Const' not required as strings are already immutable
  --> ./examples/example.py:13:4
   |
13 | s: Const[str] = "Hello, LPython!"
   |    ^^^^^^^^^^ 


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

Copy link
Collaborator

@Thirumalai-Shaktivel Thirumalai-Shaktivel left a comment

Choose a reason for hiding this comment

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

Let's add test to intregration_tests and register in CMakeLists

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/lpython/semantics/python_ast_to_asr.cpp Outdated Show resolved Hide resolved
src/lpython/semantics/python_ast_to_asr.cpp Outdated Show resolved Hide resolved
@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 07:15
@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented Mar 10, 2024

@Shaikh-Ubaid @Thirumalai-Shaktivel there is a considerable degree of redundancy here, like for dictionary and lists. I have to implement slicing on Const list and the problem will arise there too. What is the fix? Functions?

@kmr-srbh kmr-srbh marked this pull request as ready for review March 12, 2024 11:24
@Thirumalai-Shaktivel
Copy link
Collaborator

What is the status of this PR?

The CI seems to fail, please mark this PR ready for review once it is ready.

@Thirumalai-Shaktivel Thirumalai-Shaktivel marked this pull request as draft March 23, 2024 05:55
@kmr-srbh
Copy link
Contributor Author

What is the status of this PR?

The CI seems to fail, please mark this PR ready for review once it is ready.

@Thirumalai-Shaktivel this PR is ready. The only blocker is the issue stated here - #2567 (comment). Could you please look into it?

@kmr-srbh
Copy link
Contributor Author

@Shaikh-Ubaid , type_get_past_const was removed in the new additions from LFortran. What do we do here now? Use get_contained_type()?

@Shaikh-Ubaid
Copy link
Collaborator

@Shaikh-Ubaid , type_get_past_const was removed in the new additions from LFortran. What do we do here now? Use get_contained_type()?

Simply remove the call to type_get_past_const(). Cont_t node no more exists. You need not use anything.

@kmr-srbh kmr-srbh changed the title Add support for accessing items from data-structures within Const Add support for item access from Const data-structures May 1, 2024
@kmr-srbh
Copy link
Contributor Author

kmr-srbh commented May 1, 2024

@Shaikh-Ubaid as the Const node was removed, this works out-of-the-box now. I have reverted all the previous unnecessary changes. The tests and semantic error for creating Const str and Const tuple have been retained. This is ready.

A related issue is the failing slicing on Const list and Const str. I will open an issue for it.

@kmr-srbh kmr-srbh requested a review from Shaikh-Ubaid May 1, 2024 05:42
integration_tests/CMakeLists.txt 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
src/lpython/semantics/python_ast_to_asr.cpp Outdated Show resolved Hide resolved
tests/tests.toml Outdated Show resolved Hide resolved
@kmr-srbh kmr-srbh requested a review from Shaikh-Ubaid May 1, 2024 14:04
@kmr-srbh kmr-srbh marked this pull request as ready for review May 1, 2024 14:04
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.

@kmr-srbh For this PR, you can simply add an integration test that verifies that const list and dict and operation on them are supported by lpython. And it will be good to merge.

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/lpython/semantics/python_ast_to_asr.cpp Outdated Show resolved Hide resolved
@kmr-srbh kmr-srbh requested a review from Shaikh-Ubaid May 1, 2024 16:15
@kmr-srbh kmr-srbh requested a review from Shaikh-Ubaid May 1, 2024 16:52
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.

Looks good. Minimal changes that just do the task. Thanks for this.

@Shaikh-Ubaid Shaikh-Ubaid enabled auto-merge (squash) May 1, 2024 16:53
@Shaikh-Ubaid Shaikh-Ubaid merged commit dfeacbc into lcompilers:main May 1, 2024
14 checks passed
@kmr-srbh kmr-srbh deleted the support-const-access 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: Trying to access items from data-structure contained within Const fails
3 participants