lcompilers / lpython

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

Fix bug in printing a list #2654

Closed advikkabra closed 5 months ago

advikkabra commented 5 months ago

Fixes #2639 . I am not sure how/where to test this, but this fixes the issue by making an auxiliary variable in the ASR.

Shaikh-Ubaid commented 5 months ago

We should add an integration test. We can have an assert on the length of the list, couple initial elements and couple ending elements.

Shaikh-Ubaid commented 5 months ago

but this fixes the issue by making an auxiliary variable in the ASR.

Could you describe what the issue was and how creating an auxiliary variable fixes it?

advikkabra commented 5 months ago

but this fixes the issue by making an auxiliary variable in the ASR.

Could you describe what the issue was and how creating an auxiliary variable fixes it?

While calling a function which modifies the list in a print function, it repeatedly called the function. This was because in the loop made to print the list, the function was called everytime the length of the list was accessed, so it kept adding elements. By making an auxiliary variable, it keeps the length constant.

Shaikh-Ubaid commented 5 months ago

I checked the solution locally. It seems to fail for me.

% cat examples/expr2.py 
from lpython import i32

def test_issue_2639():
    l: list[i32] = [1, 2]

    def add_item(i: i32) -> list[i32]:
        l.append(i)
        return l

    print(add_item(3))
    print(l)
    assert len(l) == 3
    assert l[0] == 1
    assert l[1] == 2
    assert l[2] == 3

test_issue_2639()
% lpython examples/expr2.py
[1, 2, 3]
[1, 2, 3, 3, 3, 3]
AssertionError
Shaikh-Ubaid commented 5 months ago

The reason it does not fail at the CI is because in the test added we need to make a call to test_issue_2639().

advikkabra commented 5 months ago

It should be fixed now. I had not been using the auxiliary variable in one place. I'm not sure why the tests are failing, because the tests pass on my machine.

Shaikh-Ubaid commented 5 months ago

The tests fail for the C backend (at the CI and on my system locally).

% ./integration_tests/run_tests.py -b c
...
99% tests passed, 1 tests failed out of 290

Label Time Summary:
c           =   2.46 sec*proc (290 tests)
cpython     =   1.78 sec*proc (275 tests)
llvm        =   2.39 sec*proc (288 tests)
wasm        =   0.83 sec*proc (38 tests)
wasm_x64    =   0.08 sec*proc (23 tests)
wasm_x86    =   0.04 sec*proc (12 tests)
x86         =   0.00 sec*proc (1 test)

Total Test time (real) =   0.77 sec

The following tests FAILED:
        109 - test_list_11 (Failed)
Errors while running CTest
Command failed: ctest -j8 --output-on-failure

@advikkabra Can you look into it?

advikkabra commented 5 months ago

I think the issue is with nested functions. When the function is not nested it works. I think nested functions might need to be reworked.