Always report parse errors back to the user

[charliermarsh]

Summary

We treat parse errors as lint errors -- they have their own code (E999). If we fail to parse a file, we add an E999 diagnostic, and move on. Parse errors are recoverable, as many checks don't rely on the AST (like E501, line-length enforcement) -- so even if the parse fails, we can still report some other diagnostics.

However, here's a common and confusing workflow: say a user introduces (e.g.) a match statement into their code, then runs ruff --select I /path/to/file.py. They'll then wonder why their imports aren't being sorted. We can't parse match statements, so under the hood, we're adding an E999 to the diagnostic list, but it's being suppressed (since E999 isn't in the select), so the user doesn't get any feedback.

This PR modifies the linter to propagate parse errors. They're still included as E999, and we still try to find other violations; however, we also log a warning to the console and avoid doing any caching to the file (so the warning appears on every invocation).

Here's an example.

Given these code changes:

We now yield:

So it's a little redundant, between the warning and the E999, but I think it's much clearer.

Closes #2473.

[charliermarsh]

Having access to the parse error here means that we can also catch errors that we introduced.

[gertvdijk]

Hi! I've just upgraded to 0.0.241 from 0.0.240 and seeing this as new error in --diff mode. I just wanted to make sure that the following behaviour is as expected.

• 0 exit status code (seems OK to me in --diff mode, but just looks odd in output with red error-prefixed lines.)
• There seems to be no way to silence this error - having code E999 separate from the syntax error. A little bit of a show-stopper to me.

[charliermarsh]

@gertvdijk - Hi! We could omit this warning if you run under -q (which already omits everything that isn't a lint error). Would that fix this for you?

Curious though - what's the context in which you're running over code consistently that fails to parse?

[gertvdijk]

@gertvdijk - Hi! We could omit this warning if you run under -q (which already omits everything that isn't a lint error). Would that fix this for you?

Yes, I think I would like that, but I'm afraid that it also hides more/other/future problems. It produced errors in regular mode, but it allowed me start using Ruff in --diff mode, until 0.0.241. Ideally, I would like to see a way to silence the "syntax error" error. I am sure the Python interpreter with proper test coverage is more accurate than Ruff on that anyway.

Curious though - what's the context in which you're running over code consistently that fails to parse?

The infamous #282. 😐

^{I just released a project last weekend and this started to show suddenly when running the run-all-linters script also in diff-mode which used to be OK in 0.0.240.}

[ashb]

Interestingly this seemed to remove the ability for us to do #noqa: E999 on match lines which we had as a working pattern before.

[charliermarsh]

Interestingly this seemed to remove the ability for us to do #noqa: E999 on match lines which we had as a working pattern before.

The intent is that the behavior is totally unchanged, except that we also show you an error message in the console. If it's behaving at all differently, then it's a bug and I should fix it 😂

[charliermarsh]

I could add an --ignore-parse-errors flag to suppress these. I don't mind doing so, I just worry that it can lead to other confusing situations for users.

[charliermarsh]

The other option is that we could avoid omitting these if you add # noqa to the top of the file.

[charliermarsh]

To summarize a few options -- and I'd love feedback:

1. Add --ignore-parse-errors to pyproject.toml and the CLI to always suppress the extra warning message. (The downside here is that you're opting in to the confusing possibility of having a parse error hidden from you and thereby suppressing all other lint errors.)
2. Avoid these messages when # noqa is present at the top of the file. (But then you can't get any lint enforcement on those files. Maybe that's fine though.)
3. Only show this message if E999 is disabled. That way, you can continue to suppress it with a # noqa: E999 if you want, but we also avoid silently failing.

[gertvdijk]

Thanks for the thoughts!

I would vote for option 1, if it's not possible to silence it on a block-level (whole match/case block). If you see the chance of making it fail on the use of that flag if there are no parse errors, that would be great. It will then act as a notification that a future Ruff will understand the syntax. (Just like how mypy will complain if a typing-ignore is not needed (anymore).)

Against option 2 for the given reason - it would silence half my project.

Option 3 sounds confusing to me. I think I get both E999 and the syntax error on the match/case block. I would need a # noqa E999 on that and then I'm back at the error message I came here to complain about. 😅

[charliermarsh]

(Sorry, for Option 3, I meant that we'd only show the error if the E999 code wasn't selected at all. So if you ran ruff /path/to/file.py --select B, and hit a syntax error, we'd show the warning. If you ran ruff /path/to/file.py --select B --select E, we wouldn't show the warning, even if gets suppressed by a noqa.)

[ashb]

Oh yes, I was just confused by the error output when I'd already had the # noqa: E999 on the line, but didn't look closely at the exit code.

I think I'd vote for a 4th option: don't show the parse errors if the line is decorated with E999 ignore, even if that error isn't selected.

For clarity, my code is:

  62   │     if not settings:
  63   │         settings = get_settings()
  64   │     match settings.app_kind:  # noqa: E999
  65   │         case AppKind.hypervisor:

And the output I get is

❯ poetry run ruff src; echo $?
error: Failed to parse src/laminar/app.py: invalid syntax. Got unexpected token 'settings' at line 64 column 11
0

[charliermarsh]

That's a nice idea. I went ahead and added it. So if you have this:

x = 1

y = 2


match x:  # noqa: E999
    1

We'll always suppress that parse error, regardless of whether E999 is active for the current ruff invocation.