Remove E999 as a rule, disallow any disablement methods for syntax error

[dhruvmanila]

Summary

This PR updates the way syntax errors are handled throughout the linter.

The main change is that it's now not considered as a rule which involves the following changes:

• Update Message to be an enum with two variants - one for diagnostic message and the other for syntax error message
• Provide methods on the new message enum to query information required by downstream usages

This means that the syntax errors cannot be hidden / disabled via any disablement methods. These are:

1. Configuration via select, ignore, per-file-ignores, and their extend-* variants

   $ cargo run -- check ~/playground/ruff/src/lsp.py --extend-select=E999 --no-preview --no-cache 
       Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.10s
        Running `target/debug/ruff check /Users/dhruv/playground/ruff/src/lsp.py --extend-select=E999 --no-preview --no-cache`
   warning: Rule `E999` is deprecated and will be removed in a future release. Syntax errors will always be shown regardless of whether this rule is selected or not.
   /Users/dhruv/playground/ruff/src/lsp.py:1:8: F401 [*] `abc` imported but unused
     |
   1 | import abc
     |        ^^^ F401
   2 | from pathlib import Path
   3 | import os
     |
     = help: Remove unused import: `abc`

2. Command-line flags via --select, --ignore, --per-file-ignores, and their --extend-* variants

   $ cargo run -- check ~/playground/ruff/src/lsp.py --no-cache --config=~/playground/ruff/pyproject.toml 
       Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.11s
        Running `target/debug/ruff check /Users/dhruv/playground/ruff/src/lsp.py --no-cache --config=/Users/dhruv/playground/ruff/pyproject.toml`
   warning: Rule `E999` is deprecated and will be removed in a future release. Syntax errors will always be shown regardless of whether this rule is selected or not.
   /Users/dhruv/playground/ruff/src/lsp.py:1:8: F401 [*] `abc` imported but unused
     |
   1 | import abc
     |        ^^^ F401
   2 | from pathlib import Path
   3 | import os
     |
     = help: Remove unused import: `abc`

This also means that the output format needs to be updated:

1. The code, noqa_row, url fields in the JSON output is optional (null for syntax errors)
2. Other formats are changed accordingly
   For each format, a new test case specific to syntax errors have been added. Please refer to the snapshot output for the exact format for syntax error message.

The output of the --statistics flag will have a blank entry for syntax errors:

315     F821    [ ] undefined-name
119             [ ] syntax-error
103     F811    [ ] redefined-while-unused

The language server is updated to consider the syntax errors by convert them into LSP diagnostic format separately.

Preview

There are no quick fixes provided to disable syntax errors. This will automatically work for ruff-lsp because the noqa_row field will be null in that case.

Even with noqa comment, the syntax error is displayed:

Rule documentation page:

Test Plan

• Disablement methods via config shows a warning
  • select, extend-select
  • ignore doesn't show any message
  • per-file-ignores, extend-per-file-ignores doesn't show any message
• Disablement methods via command-line flag shows a warning
  • --select, --extend-select
  • --ignore doesn't show any message
  • --per-file-ignores, --extend-per-file-ignores doesn't show any message
• File with syntax errors should exit with code 1
• Language server
  • Should show diagnostics for syntax errors
  • Should not recommend a quick fix edit for adding noqa comment
  • Same for ruff-lsp

resolves: #8447

[github-actions]

ruff-ecosystem results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+0 -2 violations, +0 -0 fixes in 1 projects; 49 projects unchanged)

demisto/content (+0 -2 violations, +0 -0 fixes)

- Packs/ThreatQ/Integrations/ThreatQ/ThreatQ.py:106:12: E999 SyntaxError: Multiple exception types must be parenthesized
- Packs/ThreatQ/Integrations/ThreatQ/ThreatQ.py:325:8: E999 SyntaxError: Multiple exception types must be parenthesized

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
E999	2	0	2	0	0

Linter (preview)

ℹ️ ecosystem check encountered linter errors. (no lint changes; 1 project error)

demisto/content (error)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

warning: The top-level linter settings are deprecated in favour of their counterparts in the `lint` section. Please update the following options in `pyproject.toml`:
  - 'ignore' -> 'lint.ignore'
  - 'select' -> 'lint.select'
  - 'unfixable' -> 'lint.unfixable'
  - 'per-file-ignores' -> 'lint.per-file-ignores'
warning: `PGH001` has been remapped to `S307`.
warning: `PGH002` has been remapped to `G010`.
warning: `PLR1701` has been remapped to `SIM101`.
ruff failed
  Cause: Selection of deprecated rule `E999` is not allowed when preview is enabled.

[MichaReiser]

What happens if a user specifies E999 in select, ignore, exclude?

[MichaReiser]

I wonder if it still makes sense to model SyntaxError as a rule, now that it no longer behaves like a rule. Is it just Message where we need to pass E999 to? Could we use an enum in Message?

For example, I suspect that E999 is still valid according to the schema?

[dhruvmanila]

What happens if a user specifies E999 in select, ignore, exclude?

It shouldn't affect anything, the E999 diagnostics (if any) will still be displayed.

I wonder if it still makes sense to model SyntaxError as a rule, now that it no longer behaves like a rule. Is it just Message where we need to pass E999 to? Could we use an enum in Message?

Good point. Let me check.

[MichaReiser]

It shouldn't affect anything, the E999 diagnostics (if any) will still be displayed.

But we don't show a warning. I'm worried that this is confusing for users that would expect E999 not to show up

[dhruvmanila]

For example, I suspect that E999 is still valid according to the schema?

Yeah, that's correct. I guess that doesn't make sense since it, now, doesn't behave as a rule (as you mentioned). Although I wonder how would one go about configuring the fixable part if and when we introduce them.

For some more context, Pyright doesn't seem to consider it as a rule either, at least that's what I'm inferring from the fact that there's no "rule" field in the JSON output:

       {
            "file": "/Users/dhruv/playground/ruff/parser/_.py",
            "severity": "error",
            "message": "Expected expression to the right of \"=\"",
            "range": {
                "start": {
                    "line": 3,
                    "character": 17
                },
                "end": {
                    "line": 4,
                    "character": 0
                }
            }
        }

Vs the one with the "rule" field:

       {
            "file": "/Users/dhruv/playground/ruff/src/lsp.py",
            "severity": "error",
            "message": "\"name\" is not defined",
            "range": {
                "start": {
                    "line": 9,
                    "character": 6
                },
                "end": {
                    "line": 9,
                    "character": 10
                }
            },
            "rule": "reportUndefinedVariable"
        }

I'm not sure how does Biome does it but they have a separate ParseDiagnostic and it's in a separate category ("parse") not present in the documentation.

[MichaReiser]

I'm not sure how does Biome does it but they have a separate ParseDiagnostic and it's in a separate category ("parse") not present in the documentation.

Biome supports different Diagnostics and only Formatter and Linter diagnostics are suppressable.

[dhruvmanila]

For example, I suspect that E999 is still valid according to the schema?

I think we can just remove this code from the rule table so that it's not considered valid in config. But, that opens up another set of problems.

[charliermarsh]

What's the motivation for making it unsuppressable?

[MichaReiser]

There's some reasoning in the internal discord thread https://discord.com/channels/1039017663004942429/1082324250112823306/1250836504738267157

[charliermarsh]

I don't see the motivation described in there, just more context on why it was added initially.

[dhruvmanila]

Historically, I think the main motivation was because the parser didn't support new syntax which isn't the case with the new parser. We guarantee that the Ruff parser will always match the latest Python grammar. This means there's no need to suppress the syntax errors although there can be use cases in a test fixture for which the file should be exluded instead. This also means that we don't need to show the syntax errors as log messages in addition to the diagnostics.

Other syntax errors: In the future, Ruff will also start providing syntax error diagnostics for unsupported syntax as per the target version or the ones raised by the compiler. Allowing to disable syntax errors means that Ruff won't be able to provide diagnostics for these kinds of syntax errors in case it's disabled. But, the syntax errors indicate that Python won't be able to run the code itself.

Looking at the ecosystem, the following tools support ignoring syntax errors:

• flake8 via noqa: E999
• pylint
  We can't disable syntax errors in Pylint via comment:

   x =  # pylint: disable=syntax-error

  But, we can via the config file:

   [tool.pylint."MESSAGES CONTROL"]
   disable = ["syntax-error"]

While, the following doesn't:

• Pyright: https://github.com/microsoft/pyright/blob/main/docs/configuration.md#main-configuration-options (typeCheckingMode)
  • Also, Pyright doesn't have any rule id corresponding to a syntax error
• mypy: https://mypy.readthedocs.io/en/stable/error_code_list.html#report-syntax-errors-syntax
• biome, under the "parse" category

   // biome-ignore parse: reason
   import fs from 'fs;

• eslint (no rule id corresponding to syntax errors)
• oxc

Searching for E999 on https://grep.app gives the following relevant results:

• Ignoring syntax errors from Pylint because Ruff will raise them
  • home-assistant/core: https://github.com/home-assistant/core/blob/dev/pyproject.toml#L209
  • ansible/molecule: https://github.com/ansible/molecule/blob/fd3ed55b11bf896aa1892ee75a6f32761eed3032/pyproject.toml#L138C34-L138C35
• codium-ai is ignoring E999 from Ruff; https://github.com/Codium-ai/pr-agent/blob/74bb07e9c40e735b47eed0d13b621e27a0f76bef/pyproject.toml#L75 although I cloned the repository and there are no syntax errors that Ruff is reporting when I removed it from the config
• hatch includes E999 in the selection: https://github.com/pypa/hatch/blob/72e09428a2e6846962413b227534c0eb365a86ed/ruff_defaults.toml#L133
• sphinx-gallery has a usage within per-file-ignores (https://github.com/sphinx-gallery/sphinx-gallery/blob/0d7823073f9307cfeaad370ad41207354dfe9713/pyproject.toml#L113-L115) for which I think the file should be excluded completely
• Used to ignore a file containing match statements which we already support now: https://github.com/alice-biometrics/meiga/blob/4856023988835dc8c1cac6d3bdc72ca842ecb59c/pyproject.toml#L134-L136

tl;dr for the above search is that most usages are to include E999 and to exclude syntax errors from Pylint.

[AlexWaygood]

Other syntax errors: In the future, Ruff will also start providing syntax error diagnostics for unsupported syntax as per the target version or the ones raised by the compiler. Allowing to disable syntax errors means that Ruff won't be able to provide diagnostics for these kinds of syntax errors in case it's disabled. But, the syntax errors indicate that Python won't be able to run the code itself.

Some of these syntax errors emitted by the Python compiler are very subtle -- some have been the subject of several revisions at CPython (e.g. python/cpython#100581).

From the perspective of Python users, these are all just SyntaxErrors, so I think they should all be bucketed under E999; Ruff users shouldn't have to care about the distinction of where the SyntaxErrors originate from in the CPython internals. But if we get the logic wrong somehow for one of these rules that we plan to implement outside of the parser, making E999 unsuppressable might mean that users who encounter E999 false positives on their code are unable to upgrade their Ruff version until there's a release with a bugfix. That doesn't sound ideal to me.

[MichaReiser]

I'll review the PR in detail in a minute, but I think we should wait to remove the E999 rule. Instead, we should mark it as deprecated and, ideally, emit a warning when it is used in any of the selectors (maybe that's already done automatically when it is marked as deprecated)?

The main reason is that we try to first deprecate a feature before completely removing it (gives users a nicer upgrade experience).

[dhruvmanila]

I'll review the PR in detail in a minute, but I think we should wait to remove the E999 rule. Instead, we should mark it as deprecated and, ideally, emit a warning when it is used in any of the selectors (maybe that's already done automatically when it is marked as deprecated)?

Yeah, I just thought of that, I'm already on it.

[MichaReiser]

This is great! I'm also like that Message is now an enum. I expect that we want to incorporate formatter errors in the future as well and it being an enum already moves in the right direction.

[MichaReiser]

Nit: I'm inclined to add a code and url method to Message that return Option<String> and Option<Url> to avoid the repetition in the different emitters.

[MichaReiser]

Uh, our parse errors only have a start offset and not a range. That's funny.

[dhruvmanila]

I'm not sure I follow here. This is mainly a copy-paste from the existing logic :)

[MichaReiser]

The only functionality that we "loose" when merging this PR is that it is no longer possible to disable E999 in the editor to avoid that both pyright and ruff show syntax errors. Is this something you plan to add to ruff server (and ruff lsp?)

Huh, RustRover's review feature isnt' there yet.... Submitting my review once would have been enough.

[dhruvmanila]

The only functionality that we "loose" when merging this PR is that it is no longer possible to disable E999 in the editor to avoid that both pyright and ruff show syntax errors. Is this something you plan to add to ruff server (and ruff lsp?)

It should be low effort if we want to add it to the new server. I don't think we should add any new "features" to ruff-lsp.

Any suggestions for the config name?

1. lint.hide_syntax_errors
2. lint.exclude_syntax_errors
3. hide_syntax_errors (global)
4. exclude_syntax_errors (global)

[AlexWaygood]

Any suggestions for the config name?

lint.ignore_syntax_errors?

[MichaReiser]

I'm leaning towards a global option because we may want to always show (or not show) syntax error in the future even if linting is disabled.

I'm leaning towards using enable because we already support a few enable options (and it's about enabling/disabling a LSP feature). I would not use the ignore semantics because it's too close to how rules are enabled/disabled.

syntax_errors.enable?

[AlexWaygood]

Makes sense. show_syntax_errors = true / show_syntax_errors = false?

[dhruvmanila]

syntax_errors.enable?

This would be useful if we want to provide more options apart from enabling / disabling. I'm not sure what they would be right now and don't know if there's any other functionality we could provide related to syntax errors (maybe "fix" enabled or not?). I'm leaning towards what Alex has suggested.

[dhruvmanila]

(I'm going to make the server changes in a follow-up PR.)