ruff exits with code 0 if there's a SyntaxError but rule E999 is not selected

[gavriil-deshaw]

As per the title, if the rule with code E999 is not included in the configuration, then ruff will terminate successfully even if there's a SyntaxError. So, for example, if one wanted to run ruff against only the Pylfakes rules, the logical configuration for the rules would be --select F, but this doesn't work as expected.

Specifically, running ruff --select F <filename.py> exits with code 0 even if there's a SyntaxError - it does print error: Failed to parse but it doesn't raise an error. I believe this shouldn't be the default behavior as it would allow ruff to succeed in automated pipelines even with a SyntaxError (and possibly other F code errors). As a workaround for the mentioned use-case, one can also include E999 in the selected rules. However, this isn't very intuitive since having a SyntaxError means that an AST can't be generated and so, even if E999 isn't really a Pyflakes error, the python code can't be run.

FWIW, ruff rules list E999 as a pycodestyle error code, while in reality it's a code returned by flake8 when it can't generate an AST for a given file. The equivalent pycodestyle code is E901 SyntaxError or IndentationError as listed here.

[zanieb]

Seems reasonable to change the exit code if we fail to parse any files — I'm unsure of the difficulty to implement this.

[gavriil-deshaw]

Thanks for your prompt response!

Would it be unreasonable to exit with code 1 after a ParseError? I'm not Rust expert, so I might be missing something, but I seem to have that working with a 2-line change. It works as intended and all the tests pass (cargo test | rg '; [1-9]?* failed' | wc -l outputs 0). If this isn't the desired behavior, an alternative would be to add cli option that would trigger this.

What do you think?

[zanieb]

What's the two line change? :)

I think we want to still lint the remaining files, so only exit at the end.

[charliermarsh]

I need to think on this... There's been a lot of back and forth on this behavior:

• Syntax error doesn't cause ruff to exit with non-zero code #196
• Raise error when failing to parse #199
• Always log syntax errors to the console #2473
• Always report parse errors back to the user #2505
• Suppress parse errors with explicit # noqa: E999 directives #2697
• Respect all rule-exemption sources when suppressing parser errors #3665

[gavriil-deshaw]

@zanieb I did it by just exiting with code 1 after displaying the ParseError, but that indeed means that the remaining files aren't linted. IMHO, this is a more intuitive behavior, but I imagine that it might break things for people who accommodate for the current behavior.

@charliermarsh what if you just add a cli option to exit if there's a ParseError? I think that's reasonable, if it's easily doable.

In any case, you obviously know the codebase (and other related issues logged) better than I do, so I'll leave this up to you :))

Thanks!

[zanieb]

I don't think exiting on the first error is an acceptable solution, unfortunately. We have people that rely on the current behavior and we can perform some lints despite syntax errors in a file.

[tusharsadhwani]

This affects writing rules as well, you write a new rule E123, and you run:

$ cargo run -p ruff -- check foo.py --no-cache --preview --select E123
    Finished dev [unoptimized + debuginfo] target(s) in 0.13s
     Running `target/debug/ruff check foo.py --no-cache --preview --select E123`
error: Failed to parse foo.py:1:5: Simple statements must be separated by newlines or semicolons
All checks passed!

The last line gives a false sense of everything working.

---

I think putting a seen_parse_error boolean flag in LinterResult and propagating it up to Diagnostics would be an okay strategy to implement this?

[zanieb]

I believe @dhruvmanila might eventually be looking into how we can surface parse errors as diagnostics?

[dhruvmanila]

Yes, ideally the syntax errors should be surfaced using the same diagnostic system Ruff uses to display lint errors. There are still some outstanding questions with regards to the exit code and how should E999 be used here but it should get clear when I start working on it.

[dhruvmanila]

I'll look into this today.

[dhruvmanila]

I think this will depend on #11901 which will remove E999 from being considered for any disablement methods which means that if there's any syntax error in the file, Ruff will always provide diagnostics for the same and will exit with a non-zero code.

[dhruvmanila]

Resolved in #11901 and will be released in v0.5.0.

[gavriil-deshaw]

Perfect, thanks for handling this!