Skip to content

Add list.pop #1845

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 13 commits into from
Jun 12, 2023
Merged

Add list.pop #1845

merged 13 commits into from
Jun 12, 2023

Conversation

kabra1110
Copy link
Collaborator

@kabra1110 kabra1110 commented May 21, 2023

Closes #1821.


Issues

  1. Currently does not work with structures like list[tuple[i32, i32]]. Looks like the function is called twice (?)
from lpython import i32

def list_pop():
    l: list[tuple[i32, i32]]
    l = [(1,2)]
    print(l.pop())
    print(l)

list_pop()
(1, IndexError: pop from empty list

This IndexError is output when trying to pop empty lists (as in CPython).

  1. For now, supported only as expression or statement. The first commit supports statement x.pop() by wrapping IntrinsicFunction into Expr, while the second supports expression (so print(x.pop()) works, for example).

Copy link
Collaborator

@czgdp1807 czgdp1807 left a comment

Choose a reason for hiding this comment

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

We will investigate the bug you are facing on Saturday.

@@ -935,6 +936,39 @@ static inline ASR::asr_t* create_ListReverse(Allocator& al, const Location& loc,

} // namespace ListReverse

namespace ListPop {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also add a verify_args and register it in the appropriate place. Try to see the pattern of other verify_args.

@czgdp1807
Copy link
Collaborator

The problem is with print_list_tuple ASR pass. It transforms print(l.pop()) into print(l.pop()[0], l.pop()[1]) which calls l.pop twice leading to the error because in second call to l.pop() the list is empty. The correct transformation would be t = l.pop(); print(t).

@czgdp1807
Copy link
Collaborator

Use the following test case,

from lpython import i32

def list_pop():
    l: list[tuple[i32, i32]]
    t: tuple[i32, i32]
    l = [(-4, 2)]
    t = l.pop()
    print(t)
    print(l)

list_pop()

@czgdp1807
Copy link
Collaborator

For making the following work, just create the node for list.pop and then wrap it up inside Expr if its created inside AST::Expr.

from lpython import i32

def list_pop():
    l: list[tuple[i32, i32]
    l = [(-4, 2)]
    l.pop()
    print(l)

list_pop()

Comment on lines 253 to 285
// static ASR::asr_t* eval_list_pop(ASR::expr_t *s, Allocator &al, const Location &loc,
// Vec<ASR::expr_t*> &args, diag::Diagnostics &diag) {
// if (args.size() > 1) {
// throw SemanticError("pop() takes atmost one argument",
// loc);
// }
// ASR::expr_t *idx = nullptr;
// ASR::ttype_t *int_type = ASRUtils::TYPE(ASR::make_Integer_t(al, loc,
// 4, nullptr, 0));
// ASR::ttype_t *type = ASRUtils::expr_type(s);
// ASR::ttype_t *list_type = ASR::down_cast<ASR::List_t>(type)->m_type;
// if (args.size() == 1) {
// ASR::ttype_t *pos_type = ASRUtils::expr_type(args[0]);
// if (!ASRUtils::check_equal_type(pos_type, int_type)) {
// std::string fnd = ASRUtils::type_to_str_python(pos_type);
// std::string org = ASRUtils::type_to_str_python(int_type);
// diag.add(diag::Diagnostic(
// "Type mismatch in 'pop', List index should be of integer type",
// diag::Level::Error, diag::Stage::Semantic, {
// diag::Label("type mismatch (found: '" + fnd + "', expected: '" + org + "')",
// {args[0]->base.loc})
// })
// );
// throw SemanticAbort();
// }
// idx = args[0];
// } else {
// // default is last index
// idx = (ASR::expr_t*)ASR::make_IntegerConstant_t(al, loc, -1, int_type);
// }

// return make_ListPop_t(al, loc, s, idx, list_type, nullptr);
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// static ASR::asr_t* eval_list_pop(ASR::expr_t *s, Allocator &al, const Location &loc,
// Vec<ASR::expr_t*> &args, diag::Diagnostics &diag) {
// if (args.size() > 1) {
// throw SemanticError("pop() takes atmost one argument",
// loc);
// }
// ASR::expr_t *idx = nullptr;
// ASR::ttype_t *int_type = ASRUtils::TYPE(ASR::make_Integer_t(al, loc,
// 4, nullptr, 0));
// ASR::ttype_t *type = ASRUtils::expr_type(s);
// ASR::ttype_t *list_type = ASR::down_cast<ASR::List_t>(type)->m_type;
// if (args.size() == 1) {
// ASR::ttype_t *pos_type = ASRUtils::expr_type(args[0]);
// if (!ASRUtils::check_equal_type(pos_type, int_type)) {
// std::string fnd = ASRUtils::type_to_str_python(pos_type);
// std::string org = ASRUtils::type_to_str_python(int_type);
// diag.add(diag::Diagnostic(
// "Type mismatch in 'pop', List index should be of integer type",
// diag::Level::Error, diag::Stage::Semantic, {
// diag::Label("type mismatch (found: '" + fnd + "', expected: '" + org + "')",
// {args[0]->base.loc})
// })
// );
// throw SemanticAbort();
// }
// idx = args[0];
// } else {
// // default is last index
// idx = (ASR::expr_t*)ASR::make_IntegerConstant_t(al, loc, -1, int_type);
// }
// return make_ListPop_t(al, loc, s, idx, list_type, nullptr);
// }

@@ -1,5 +1,5 @@
semantic error: Type mismatch in 'pop', List index should be of integer type
--> tests/errors/test_list3.py:5:15
semantic error: For now pop() takes no arguments
Copy link
Collaborator

Choose a reason for hiding this comment

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

These errors weren't there before. I would like to go back to the original errors. pop should take arguments and work with them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a commit to allow pop with position argument. Does not seem to work correctly with nested iterables.

from lpython import i32, f64

def test_list_pop():
    l1: list[tuple[i32, f64]]
    x: tuple[i32, f64]

    l1 = [(1, 1.0), (2, 4.0)]
    x = l1.pop(0)
    print(x)        # incorrect
    print(l1)       # correct

test_list_pop()
(2, 4.00000000000000000e+00)
[(2, 4.00000000000000000e+00)]

Also, errors correctly caught in verify_args lead to an LCompilersException. It does not seem related to this specific PR, though.

@czgdp1807 czgdp1807 marked this pull request as draft May 29, 2023 05:43
Copy link
Collaborator

@czgdp1807 czgdp1807 left a comment

Choose a reason for hiding this comment

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

Insert the following as a test case,

from lpython import i32, f64, InOut

def pop_wrapper(l: InOut[list[tuple[i32, f64]]], idx: i32) -> tuple[i32, f64]:
    return l.pop(idx)

def test_list_pop():
    l1: list[tuple[i32, f64]]
    x: tuple[i32, f64]
    i: i32

    l1 = [(1, 1.0), (2, 4.0), (3, 6.0)]

    x = pop_wrapper(l1, 0)
    print(x)
    assert x == (1, 1.0)

    l1.append((4, 8.0))
    assert x == (1, 1.0)

    print(l1)
    for i in range(len(l1)):
        assert l1[i] == (i + 2, 2.0*f64(i + 2))

test_list_pop()

Copy link
Collaborator

@czgdp1807 czgdp1807 left a comment

Choose a reason for hiding this comment

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

Rest LGTM.

@czgdp1807 czgdp1807 marked this pull request as ready for review June 12, 2023 05:40
@czgdp1807 czgdp1807 enabled auto-merge (squash) June 12, 2023 05:41
@czgdp1807 czgdp1807 merged commit 6c917f0 into lcompilers:main Jun 12, 2023
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.

Issues with List.pop()
2 participants