PyCQA / flake8

flake8 is a python tool that glues together pycodestyle, pyflakes, mccabe, and third-party plugins to check the style and quality of some python code.
https://flake8.pycqa.org
Other
3.45k stars 308 forks source link

`logical_line` for call with f-string can result in invalid ast under Python 3.12 #1948

Closed stephenfin closed 3 months ago

stephenfin commented 3 months ago

how did you install flake8?

$ pip install flake8

unmodified output of flake8 --bug-report

{}

describe the problem

what I expected to happen

Some of the plugins in hacking use a combo of ast.parse and logical_line to generate an AST for an individual line that is then fed into an ast.NodeVisitor subclass. These work just fine on Python 3.11 and earlier, but I've noticed they fail under specific circumstances on Python 3.12. I've included a minimal reproducer below. There's a chance I am "holding it wrong" but I'd like to confirm this first.

sample code

test.py

import unittest

class TestFoo(unittest.TestCase):

    def test_foo(self):
        response_key = 'foo'
        response_val = 'bar'
        body_output = ''
        self.assertEqual(
            f'{{"{response_key}": "{response_val}"}}', body_output
        )

checks.py:

class NoneArgChecker(ast.NodeVisitor):
    def __init__(self, func_name, num_args=2):
        self.func_name = func_name
        self.num_args = num_args
        self.none_found = False

    def visit_Call(self, node):
        # snipped
        ...

def foo(logical_line, noqa):
    if noqa:
        return

    for func_name in (
        'assertEqual', 'assertIs', 'assertNotEqual', 'assertIsNot'
    ):
        try:
            start = logical_line.index('.%s(' % func_name) + 1
        except ValueError:
            continue
        checker = NoneArgChecker(func_name)
        # print(logical_line)
        checker.visit(ast.parse(logical_line))
        continue
        if checker.none_found:
            yield start, "H203: Use assertIs(Not)None to check for None"

tox.ini:

[flake8:local-plugins]
extension =
    X123 = checks:foo
paths = .

commands ran

$ flake8 test.py
wow.py:1:23: E999 SyntaxError: invalid syntax. Perhaps you forgot a comma?

If I comment out the print statement, I see different results for Python 3.12 compared Pythons 3.10 and 3.11. Under 3.12:

self.assertEqual(f'x{x{response_key}xxxx{response_val}xx}', body_output)

Under 3.10 and 3.11:

self.assertEqual(f'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx', body_output)

additional information

The E999 error is associated with line 1 of the file, despite the error actually coming from a plugin. This led me on a wild goose chase as I tried to figure out why flake8 thought my file was invalid Python but Python itself did not. I suspect this might also be user error and I should be handling the potential exception from ast.parse in my plugin but again, I just wanted to confirm that this was expected practice.

stephenfin commented 3 months ago

I think this may be a bug introduced in https://github.com/PyCQA/flake8/commit/43266a2e26ec49a55b866c78b295deaebb1debf7. Simplifying the above reproducer:

test.py

key = 'foo'
val = 'bar'
print(f'{{"{key}": "{val}"}}'

checks.py

def foo(logical_line):
    print(logical_line)

If we run this we get:

❯ flake8 test.py
key = 'xxx'
val = 'xxx'
print(f'x{x{key}xxxx{val}xx}')

I think we want to get:

❯ flake8 test.py
key = 'xxx'
val = 'xxx'
print(f'xxxxxxxxxxxxxxxxxxxx')

Right?

asottile commented 3 months ago

nope! the method is meant to redact string contents and not code

stephenfin commented 3 months ago

Then we should have:

print(f'xxx{key}xxxx{val}xxx')

?

asottile commented 3 months ago

ah yes I see the quirk. ugh. the FSTRING_MIDDLE token is misleading for curly braces -- should be an easy fix (pycodestyle will likely need the same fix)

want to try a patch?

stephenfin commented 3 months ago

want to try a patch?

Done. Apologies in advance for the fuzzy wording: I have no idea what the below code is intended to account for:

https://github.com/PyCQA/flake8/blob/2a811cc4d2aaed3e8eb5a9f04f08ccc8af7c0791/src/flake8/processor.py#L207-L218

(and that's after git blameing my way back as far as 23c9091b...)

sigmavirus24 commented 3 months ago

want to try a patch?

Done. Apologies in advance for the fuzzy wording: I have no idea what the below code is intended to account for:

https://github.com/PyCQA/flake8/blob/2a811cc4d2aaed3e8eb5a9f04f08ccc8af7c0791/src/flake8/processor.py#L207-L218

(and that's after git blameing my way back as far as 23c9091b...)

Not looking at where you blamed back to but a bunch of this was implemented to keep compat with pycodestyle. If you want reasoning it's likely in that project