Skip to content

CPython allows uninitialized variable, LPython does not ? #1991

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

Open
rebcabin opened this issue Jun 21, 2023 · 5 comments
Open

CPython allows uninitialized variable, LPython does not ? #1991

rebcabin opened this issue Jun 21, 2023 · 5 comments
Assignees
Labels
question Further information is requested

Comments

@rebcabin
Copy link
Contributor

Not sure whether this is intentional.

repro: https://github.com/rebcabin/lpython/tree/brian-lasr/lasr/LP-pycharm/Issue1990

CPython:

(lp) ┌─(~/CLionProjects/lpython/lasr/LP-pycharm/Issue1990)───────────────────────────────────────────────────────────────────────────────────────────────(brian@Golf37:s014)─┐
└─(13:34:57 on brian-lasr ✖ ✭)──> PYTHONPATH='../../../src/runtime/lpython' python ./lasr_lexer.py                                                        ──(Wed,Jun21)─┘
STRINGIO TEST
READ-SEEK-TELL-TEST
LEXER TEST
REGEX TEST

LPython:

(lp) ┌─(~/CLionProjects/lpython/lasr/LP-pycharm/Issue1990)───────────────────────────────────────────────────────────────────────────────────────────────(brian@Golf37:s014)─┐
└─(13:36:44 on brian-lasr ✖ ✭)──> ../../../src/bin/lpython -I. ./lasr_lexer.py                                                                            ──(Wed,Jun21)─┘
semantic error: `rr` must be initialized with an instance of ReRange
   --> ./lasr_lexer.py:139:5
    |
139 |     rr : ReRange
    |     ^^^^^^^^^^^^ 
@rebcabin rebcabin added the question Further information is requested label Jun 21, 2023
@certik
Copy link
Contributor

certik commented Jun 21, 2023

Hm, I just discussed this precise issue yesterday with our team, I thought it's ok, but I can now see it's not.

Here is the example:

@dataclass
class ReRange:
    start_char_ord : i32
    end_char_ord   : i32

def check_re_range_alternatives(
        alts : list[ReRange],
        char : str) -> bool:
    result : bool = False
    rr : ReRange
    for rr in alts:
        if check_re_range(rr, char):
            result = True
            break
    return result

It gives:

semantic error: `rr` must be initialized with an instance of ReRange
  --> a.py:10:5
   |
10 |     rr : ReRange
   |     ^^^^^^^^^^^^ 

The issue is that rr is used as a loop iterator, so it makes no sense to initialize it ahead of time.

This was done in #1808. I think we should relax this requirement. The reason we did (I think) is that it's relatively easy to enforce the struct to be initialized. But as we can see now, there is a use case when this is not a good idea. @Shaikh-Ubaid if you have time to have a look at this, that would be great.

@Shaikh-Ubaid
Copy link
Collaborator

$ cat examples/expr2.py   
from lpython import dataclass, i32

@dataclass
class A:
    n: i32

def main0():
    a: A
    a.n = 5
    print(a)

main0()
$ python examples/expr2.py 
Traceback (most recent call last):
  File "/Users/ubaid/Desktop/OpenSource/lpython/examples/expr2.py", line 12, in <module>
    main0()
  File "/Users/ubaid/Desktop/OpenSource/lpython/examples/expr2.py", line 9, in main0
    a.n = 5
UnboundLocalError: local variable 'a' referenced before assignment
$ lpython examples/expr2.py
5

It seems CPython does not define a variable unless it is assigned. Since everything that compiles/runs with LPython should run with CPython, to tackle cases as above, we made it mandatory to initialize the variable if is a not a Simple Type. We need to see for a possible Robust fix/approach here.

@Shaikh-Ubaid
Copy link
Collaborator

If anyone have any ideas, please feel free to share.

For the moment, I am assigning this to me.

@Shaikh-Ubaid Shaikh-Ubaid self-assigned this Jun 22, 2023
@certik
Copy link
Contributor

certik commented Jun 22, 2023

I would fix this issue as follows. This should not be allowed:

def main0():
    a: A
    a.n = 5
    print(a)

Rather, either this:

def main0():
    a: A
    a = A(5) # Or "a = A()" if CPython allows it
    a.n = 5
    print(a)

or this must be required:

def main0():
    a: A = A(5) # Or "a: A = A()" if CPython allows it
    a.n = 5
    print(a)

The PR #1808 enforces the third option. We should also allow the second option, but not the first. The second option should also allow:

    rr : ReRange
    for rr in alts:
        pass

Note that for loop over a list of structs is not supported yet, but eventually it will be supported.

@rebcabin
Copy link
Contributor Author

rebcabin commented Jun 22, 2023

I'll give another use case for declaring variables with types without initializing them: imperative code where re-use of variables is intended.

This is better:

def lexer_test() -> None: # lexer : InOut[LasrLexer]):
    # Issue #1981
    print('LEXER TEST')
    # re-use these variables multiple times
    lexer : LasrLexer
    t : str

    # first test
    lexer = LasrLexer(StringIO('  foo  bar  '))
    __post_init__(lexer.fd)
    eat_white(lexer)
    t = peek_debug(lexer)
    # Issue 1971 print({"t": t})
    assert t == 'f', "t == 'f'"

    # second test
    lexer = LasrLexer(StringIO('(Integer 4 [])'))
    __post_init__(lexer.fd)
    t = get_non_white((lexer))
    assert t == '(', "t = '('"

because we can freely re-order the tests, than this:

def lexer_test() -> None: # lexer : InOut[LasrLexer]):
    # Issue #1981
    print('LEXER TEST')

    # first test
    lexer : LasrLexer = LasrLexer(StringIO('  foo  bar  '))
    __post_init__(lexer.fd)
    eat_white(lexer)
    t : str = peek_debug(lexer)
    # Issue 1971 print({"t": t})
    assert t == 'f', "t == 'f'"

    # second test
    lexer = LasrLexer(StringIO('(Integer 4 [])'))
    __post_init__(lexer.fd)
    t = get_non_white((lexer))
    assert t == '(', "t = '('"

The latter is what we must do now, due to LPython limitations. If we reorder the tests, we must edit them to ensure that the first one has the initialization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants