[pyupgrade]: Remove outdated sys.version_info blocks

[colin99d]

A part of #827. Opening this up as a draft for visibility.

[colin99d]

@charliermarsh I am hitting a blocker and was wondering if you could offer some assistance. My issue is linting the code below:

if sys.version_info < (2,0):
    print("This script requires Python 2.0 or greater.")
elif sys.version_info < (2,6):
    print("This script requires Python 2.6 or greater.")
elif sys.version_info < (3,):
    print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,0):
    print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,12):
    print("Python 3.0 or 3.1 or 3.2")
else:
    print("Python 3")

Currently, my program lints every line, and removes it if the condition meets the requirements, this means we end up with the following:

elif sys.version_info < (3,12):
    print("Python 3.0 or 3.1 or 3.2")
else:
    print("Python 3")

As you can see, this is not a valid python statement. I tried creating checkers that either look at the parent, or see if the statement is an if to remove the "el" from the "elif" in the next line, however; since ruff goes through all elif and if statements before running again, the code is already broken before the next loop through. So essentially I need to know whether to convert an elif statement to an if statement.

Pyupgrade currently solves this issue by only removing one if or elif at a time, so to fix the statement above you would have to call pyupgrade four times.

Only running on if statements is not a viable solution here because sometimes the issue might only be on an elif statement. So far my best ideas would be to:

1. Somehow "skip" the rest of the statement until the next run through,
2. Save the state somehow, so it can know whether to skip (this seems like a pretty bad idea)
3. Allow the statement to be converted to the bad format, and then run a separate linter at the end that can fix it. This seems like a REALLY bad idea because if the linter breaks anywhere else, we will wreck people's code

[colin99d]

It looks like I have a second issue that could be easily fixed if I can skip fixing the rest of the statement until the next iteration. Is there any functionality like that?

[charliermarsh]

We avoid applying fixes that "overlap" in the text. So one thing you could do is... when you go to fix one of these, generate a fix that applies to the entire if chain, even if you're only changing one of the statements within it.

So if you had this:

if sys.version_info < (2,0):
    print("This script requires Python 2.0 or greater.")
elif sys.version_info < (2,6):
    print("This script requires Python 2.6 or greater.")
elif sys.version_info < (3,):
    print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,0):
    print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,12):
    print("Python 3.0 or 3.1 or 3.2")
else:
    print("Python 3")

Then to fix the first one, you'd generate a Fix::replace with this text:

if sys.version_info < (2,6):
    print("This script requires Python 2.6 or greater.")
elif sys.version_info < (3,):
    print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,0):
    print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,12):
    print("Python 3.0 or 3.1 or 3.2")
else:
    print("Python 3")

All the fixes will overlap, and so it'll only apply one per iteration.

[colin99d]

We avoid applying fixes that "overlap" in the text. So one thing you could do is... when you go to fix one of these, generate a fix that applies to the entire if chain, even if you're only changing one of the statements within it.

So if you had this:

if sys.version_info < (2,0):
    print("This script requires Python 2.0 or greater.")
elif sys.version_info < (2,6):
    print("This script requires Python 2.6 or greater.")
elif sys.version_info < (3,):
    print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,0):
    print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,12):
    print("Python 3.0 or 3.1 or 3.2")
else:
    print("Python 3")

Then to fix the first one, you'd generate a Fix::replace with this text:

if sys.version_info < (2,6):
    print("This script requires Python 2.6 or greater.")
elif sys.version_info < (3,):
    print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,0):
    print("This script requires Python 3.0 or greater.")
elif sys.version_info < (3,12):
    print("Python 3.0 or 3.1 or 3.2")
else:
    print("Python 3")

All the fixes will overlap, and so it'll only apply one per iteration.

Charlie this is absolutely perfect! Thank you so much!!!

[charliermarsh]

No worries, sorry it took me a while to respond. It might take some work to find the outermost if parent, but hopefully it's at least possible.

[colin99d]

@charliermarsh, I believe I found an issue with the rustpython_parser::lexer. When I run the following program in python, it runs fine:

import six

if True:
    if six.PY2:
        print("PY2")
    else:
        print("PY3")

However, when I attempt to use the lexer to get the tokens in the statement I get the following issue:
value: LexicalError { error: IndentationError, location: Location { row: 3, column: 4 } }. When printing the tokens out I get the following before the error occurs:

If
Name { name: "six" }
Dot
Name { name: "PY2" }
Colon
Newline
Indent
Name { name: "print" }
Lpar
String { value: "PY2", kind: String, triple_quoted: false }
Rpar
Newline

Based on this, and the position of the location where the error occurs, it looks like the parser is failing one the else token. Do you have any idea what could be going on here?

[charliermarsh]

I think you need to dedent the entire block before lexing.

Right now, you're feeding it:

if six.PY2:
        print("PY2")
    else:
        print("PY3")

...since you're starting at the if. So it looks like invalid indentation.

(This is a guess, I admittedly didn't look at the code.)

[colin99d]

I think you need to dedent the entire block before lexing.

Right now, you're feeding it:

if six.PY2:
        print("PY2")
    else:
        print("PY3")

...since you're starting at the if. So it looks like invalid indentation.

(This is a guess, I admittedly didn't look at the code.)

I think you are exactly right, is there an example somewhere in the code of this being done already? I noticed dedent does not work if the first line has no indent.

[charliermarsh]

@sbrugman - Do you have any idea why these are failing on Windows? I tried with both to_string_lossy() and display() but no luck.

[charliermarsh]

@colin99d - Did a pass over this, I was able to simplify a few things a little bit, but I also made it slightly more timid in some cases... Do you have any interest in looking through or even testing the code prior to merging?

[colin99d]

@colin99d - Did a pass over this, I was able to simplify a few things a little bit, but I also made it slightly more timid in some cases... Do you have any interest in looking through or even testing the code prior to merging?

Would love to! I can do this tonight.

[colin99d]

What is the reason for not refactoring single line if statements anymore? I am assuming there were some edge cases it was not handling well.

[charliermarsh]

I thought it would just be simpler to not worry about them given that they're pretty rare (e.g. Black doesn't preserve them). It's possible that the implementation was totally sound... I just get worried about cases like multi-statement lines and continuations:

if True: a = 1; b = 2

if True: a = 1; \
  b = 2

But this isn't a rigorous answer at all. Maybe we should revisit.

[colin99d]

I thought it would just be simpler to not worry about them given that they're pretty rare (e.g. Black doesn't preserve them). It's possible that the implementation was totally sound... I just get worried about cases like multi-statement lines and continuations:

if True: a = 1; b = 2

if True: a = 1; \
  b = 2

But this isn't a rigorous answer at all. Maybe we should revisit.

I have not seen this in practice before EVER, so maybe we ignore it, and if someone leaves an issue, and it gains some traction, we can invest into it.

[charliermarsh]

There is one failing case right now, which I think we can only solve with LibCST -- something like this:

if True:
    if sys.version_info >= (3, 9):
        expected_error = [
"<stdin>:1:5: Generator expression must be parenthesized",
"max(1 for i in range(10), key=lambda x: x+1)",
"    ^",
        ]
    elif PYPY:
        expected_error = []
    else:
        expected_error = []

Or even harder:

if True:
    if sys.version_info >= (3, 9):
        """this
        is valid"""

        """the indentation on
        this line is significant"""

        "this is" \
"allowed too"

        ("so is"
"this for some reason")

dedent isn't safe for these reasons.

[charliermarsh]

(Fixed.)

[charliermarsh]

Ok, I think this is good to go.

[charliermarsh]

@colin99d - LMK if you wanna give this a read tonight or if I should go ahead and merge. It's not urgent -- I probably won't release tonight anyway.

[charliermarsh]

Merging for now but LMK if you have any follow-up feedback.

[colin99d]

Sorry, had something come up last night. Looks good to me!!!

[charliermarsh]

All good, no prob at all!