Perform token-based check even if AST build failed to allow Cython linting

[kmaehashi]

Please describe how you installed Flake8

$ pip install flake8

Please provide the exact, unmodified output of flake8 --bug-report

{
  "dependencies": [],
  "platform": {
    "python_implementation": "CPython",
    "python_version": "3.9.5",
    "system": "Linux"
  },
  "plugins": [
    {
      "is_local": false,
      "plugin": "mccabe",
      "version": "0.6.1"
    },
    {
      "is_local": false,
      "plugin": "pycodestyle",
      "version": "2.8.0"
    },
    {
      "is_local": false,
      "plugin": "pyflakes",
      "version": "2.4.0"
    }
  ],
  "version": "4.0.1"
}

Please describe the problem or feature

Hi, thank you very much for maintaining flake8!

I've updated to flake8 4.0.1 today and found that flake8 no longer emits warnings other than E999 when there is a syntax error in a file. I think this is totally OK for Python projects, but this made it impossible to apply flake8 against Cython code.

I understand maintainers here don't want to care about non-Python things, but flake8 is playing an important role for many Cython-based projects (including us) to keep the quality of code because there are no Cython linters available. Here are some examples:

• Pandas: https://github.com/pandas-dev/pandas/blob/master/flake8/cython.cfg
• CuPy: https://github.com/cupy/cupy/blob/master/.flake8.cython
• ... and many others: https://github.com/search?p=1&q=%22flake8%22+%22pyx%22&type=Code

Expected Output

% flake8 --version             
3.9.2 (mccabe: 0.6.1, pycodestyle: 2.7.0, pyflakes: 2.3.1) CPython 3.9.5 on Linux
% flake8 --ignore E999 test.pyx
test.pyx:3:1: E302 expected 2 blank lines, found 1
test.pyx:6:1: E305 expected 2 blank lines after class or function definition, found 1
test.pyx:6:80: E501 line too long (84 > 79 characters)
test.pyx:6:81: E231 missing whitespace after ','
test.pyx:10:1: W391 blank line at end of file

test.pyx:

from libc.stdint cimport intptr_t

def foo():
    pass

cdef intptr_t very_very_long_function_name_that_exceepts_eighty_chars_per_line(x,y):
    if x < 3:
        return True
    return x

Actual Output

% flake8 --version
4.0.1 (mccabe: 0.6.1, pycodestyle: 2.8.0, pyflakes: 2.4.0) CPython 3.9.5 on Linux
% flake8 --ignore E999 test.pyx

Proposal

I'm totally new to flake8 codebase, but it seems flake8 4.0 first tries to perform AST-based checks, and aborts when it fails (#1320).

• 4.0.1: https://github.com/pycqa/flake8/blob/4.0.1/src/flake8/checker.py#L596-L603
• 3.9.2: https://github.com/pycqa/flake8/blob/3.9.2/src/flake8/checker.py#L587-L596

I'd like to propose changing the logic to:

• Perform AST-based check first.
• Whether AST-based check fails or not, perform token-based check ignoring SyntaxError.

(Thanks to @toslunar who first discovered this case.)

[asottile]

sorry but flake8 is a linter for python source and is not intended to lint cython source -- that this was working at all before was merely an accident of implementation

[jakirkham]

Hey Anthony, appreciate the work you are doing here 🙂

Would it be possible for us to discuss this a bit further?

Just as another data point to Kenichi's list above, RAPIDS was also relying on this

[sigmavirus24]

To reiterate and rephrase Anthony - Cython support was never intended, supported, or wanted because Flake8 only care about python.

We're not going to extend it because that way leads madness. The coffee of Flake8 is well tested and reasonable well organized. A fork could easily be made that could support Cython and Cython only while supporting that community's needs

[kmaehashi]

Hi Anthony and Ian, thanks for the response.
I agree that flake8 is not for Cython, and I am not expecting flake8 to support it explicitly. The point is that many downstream projects have been relying on this behavior for years, and now they lose the way to lint their code.

Forking is something that I want to avoid... because I understand and respect all the effort maintainers have put into flake8. Maintaining two linters for language having almost the same syntax sounds like a waste of efforts in the Python community.

I think the proposed change can be applied without breaking anything. I'm happy to work on a pull-request if the idea is acceptable.

[asottile]

sorry the proposed change will not be accepted -- the fix here was intentional and fixed a long standing bug where erroneous lint errors would appear on files with syntax errors in them (pycodestyle is a bit garbage-in garbage-out when it comes to files with syntax errors)

I believe most of what you're seeking is to utilize pycodestyle's checks -- which you still can. but note that pycodestyle also doesn't support cython syntax and will not give you accurate results so you're still continuing in unsupported territory:

$ ~/opt/venv/bin/pycodestyle t.pyx 
t.pyx:3:1: E302 expected 2 blank lines, found 1
t.pyx:6:1: E305 expected 2 blank lines after class or function definition, found 1
t.pyx:6:80: E501 line too long (84 > 79 characters)
t.pyx:6:81: E231 missing whitespace after ','

[kmaehashi]

FYI, I've tried my proposal here: kmaehashi@6a38d22
tox pass with this branch, and still reports the correct locations for syntax errors.

the fix here was intentional and fixed a long standing bug where erroneous lint errors would appear on files with syntax errors in them (pycodestyle is a bit garbage-in garbage-out when it comes to files with syntax errors)

Thanks for the explanation, understood the motivation of the change in #1320. In that case, do you think it is acceptable to introduce a config option (defaults to False) to force running checks even if there is a syntax error?

[asottile]

sorry I do not, as said above flake8 is a linter for python files and anything else is unsupported

[tacaswell]

A fork could easily be made that could support Cython and Cython only while supporting that community's needs

I think this is misunderstand how Cython is used in practice. I do not know of any major project that is only Cython they are all a mix of Python and Cython sources so thinking of there being a wholly separate "Cython community" is not correct. Practically, the "garbage-in-garbage-out" for linting on pyx files has been 'good enough'.

Even leaving Cython aside, as a user I do not always want invalid syntax to effectively suppress all other errors / warnings! For example if I'm using flake8 to back highlighting in an editor where flake8 is auto-run on on save (or at the editors discretion independent of when the user save) and then highlighting lines by parsing the output of flake8. I would rather have the other issues remain flagged (best effort) as problems rather than flashing good/bad based on if there was some other syntax error in the file when flake8 got run. While I see the case for this not being the default behavior, I also see a strong case for giving users the ability to push the syntax error back down to being "at the same level" as any other error/warning and accept the risk of spurious warnings.

Although I think that the maintainers position here is very much purity over practicality, I suspect this can be worked around without having to fork flake8.

[asottile]

I would rather have the other issues remain flagged (best effort) as problems rather than flashing good/bad based on if there was some other syntax error in the file when flake8 got run.

this was previously the case, though it was not better as you suggest but much much worse -- every time you would type a ( the editor would highlight half of the rest of the file thinking everything was a function argument (because that's how pycodestyle interprets everything after an open paren)

[tacaswell]

Event in that case, the errors above would still be flagged correctly so not all is lost. There are many more ways to make a file syntactically invalid than an un-closed ( which do lead to useful results.

I definitely take the point that linting on syntactically invalid Python is undefined, sometimes it is useful sometimes it is complete trash (depending on exactly what way it is invalid). One solution is YOLO (old behavior) and press on and the other solution is to refuse to continue (new behavior). Both have their pros and cons, but neither seems obviously, unassailablely, "right" to me. I think accepting a flag to let the user choose which they prefer (--syntax-error-[not]-fatal and defaulting to the new behavior or tie it into the existing error suppression system and make it non fatal only if the user ignores E999?) is the best option as it lets users pick which mode is best for them.

---

I am very aware of the phenomena of user find creative and "off-label" uses for tools you maintain (I am the Matplotlib project lead and a core developer on h5py) and am normally on the other side of this discussion ;)

Obligatory XKCD https://xkcd.com/1172/ reference

[asottile]

an option is the worst of both worlds -- we have to maintain complexity and behaviour that we don't agree with and isn't the default and is likely to be broken

this will not be implemented, as stated above, feel free to fork if you want to make an unsupported extension

[tacaswell]

I respectfully disagree, the status-quo is the worst of all worlds. A bunch of your user are broken, a bunch of people are angry/disappointed, and a lot of extra work will be done by the wider community over a subjective judgement call that has a simple (already implemented!) fix.

---

Everyone who has shown up in this thread is a core dev of at least one major project, please have faith that if one of us gave you a PR to add the flag it would come with sufficient tests and given that the change was noticed quickly that the flag would be used regularly.

---

I promise this is my last post to this issue.