Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit diagnostics for new syntax as per the target Python version #6591

Open
11 tasks
dhruvmanila opened this issue Aug 15, 2023 · 13 comments
Open
11 tasks

Emit diagnostics for new syntax as per the target Python version #6591

dhruvmanila opened this issue Aug 15, 2023 · 13 comments
Labels
help wanted Contributions especially welcome parser Related to the parser rule Implementing or modifying a lint rule

Comments

@dhruvmanila
Copy link
Member

dhruvmanila commented Aug 15, 2023

Our parser doesn't take into account the Python version aka target-version setting while parsing the source code. This means that we would allow having a match statement when the target Python version is 3.9 or lower. We want to signal this to the user.

One solution is to provide a diagnostic after the parsing is done. This diagnostic would indicate to the user that there's a syntax usage which isn't valid for the current target version. This is what's being done in Rome and Pyright1. We would still continue linting the file and emit other diagnostics as well.

Following is a non-exhaustive list of syntax changes with the minimum required Python version:

3.13

  • Type defaults for type parameters (PEP 696)

3.12

3.11

  • except* syntax
  • Star expressions are allowed in index operations via PEP 646
  • Star expressions are allowed in annotations for *args via PEP 646

3.10

3.9

  • Relaxing grammar restrictions on decorators (PEP 614)

3.8

Footnotes

  1. Pyright: Type alias statement requires Python 3.12 or newer

@dhruvmanila
Copy link
Member Author

The Match and Type alias statement can be implemented in the AST Statement analyzer within the match arms of the respective statement and checking the target-version setting.

I would suggest to add this as a new rule under the RUF category and all of the above would be in the same rule code.

@dhruvmanila dhruvmanila added good first issue Good for newcomers rule Implementing or modifying a lint rule and removed good first issue Good for newcomers labels Aug 15, 2023
@td-anne
Copy link

td-anne commented Dec 7, 2023

There might be other new syntax worth addressing:

  • 3.11 except *
  • 3.11 Variadic generics
  • 3.8 Walrus operator (if there's interest in supporting pre-3.8)
  • 3.10 Type unions with | (?)
  • 3.10 Parenthesized with statements (with (a_gen() as a, b_gen() as b):
  • 3.12 type parameter syntax (def max[T](list[T]) -> T:)
  • 3.12-only f-strings (?)
  • 3.9 dict unions with | (?)
  • 3.9 lower-case list[]/dict[] in type annotations
  • 3.9 PEP 617 extended generator syntax

Some of these might be closer to functional changes than syntax and hard to detect. But perhaps the easy ones could be folded in? There are advantages to python 3.12 that might make people want to develop with it even though their code is meant to run on python 3.9 (say).

@s-cork
Copy link

s-cork commented Apr 11, 2024

  • 3.8 Walrus operator (if there's interest in supporting pre-3.8)

Just to say we have a use case for targeting 3.7. Thanks for the great work on this!

@achimnol
Copy link

achimnol commented Apr 22, 2024

So far, most new syntaxes were not "auto-applied" during formatting, but PEP-701 f-string placeholder updates are auto-applied to all codes. This prevents backporting those codes to older Python versions seamlessly as Ruff does not report syntax errors in older Python versions. Looking forward to see this issue to be resolved. 👀

@zanieb
Copy link
Member

zanieb commented Jun 4, 2024

We'd appreciate contributions here if someone is interested in working on the project. We can do this relatively incrementally, but there is some initial complexity to determine how diagnostics are propagated.

@dhruvmanila / @charliermarsh will provide some additional details.

@dhruvmanila
Copy link
Member Author

There might be other new syntax worth addressing:

@td-anne For visibility, I've updated the PR description based on your list but this issue is mainly focused on the syntax itself and not the semantic meaning. For example, dictionary union ({'a': 1} | {'b': 2}) is valid syntactically but raises a TypeError at runtime. So, the PR description is updated with the following:

  • 3.11 except *
  • 3.8 Walrus operator (if there's interest in supporting pre-3.8)
  • 3.10 Parenthesized with statements (with (a_gen() as a, b_gen() as b):
  • 3.12 type parameter syntax (def max[T](list[T]) -> T:)
  • 3.12-only f-strings (?)

But, the following can possibly be done as a separate rule and is not part of what we're proposing here.

  • 3.11 Variadic generics
  • 3.10 Type unions with | (?)
  • 3.9 dict unions with | (?)
  • 3.9 lower-case list[]/dict[] in type annotations

Can you expand on the following? I don't see any new syntax related to PEP 617.

  • 3.9 PEP 617 extended generator syntax

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 5, 2024

[ ] 3.11 Variadic generics

These did involve the introduction of some new syntax: unpacking using * at the outermost level of an expression in a type annotation was invalid syntax on earlier versions of Python. See https://peps.python.org/pep-0646/#grammar-changes

@dhruvmanila
Copy link
Member Author

I see. Thanks for pointing that out. I'll update the PR description.

@dhruvmanila
Copy link
Member Author

The goal here is to update Ruff to detect syntax which is invalid as per the target-version. There are two ways to achieve this:

  1. Update the parser to detect this, add new parse error types
  2. Add a new single rule

For (1),

  • There needs to be additional changes to update the linter to show all parse errors as diagnostics. Currently, only the first parse error is shown to the user. This will be updated by me soon.
  • Pyright also uses this approach.
  • The parser has all the context. For example, parentheses context for parenthesized with-items

For (2),

  • It can utilize the infrastructure provided by the linter including ignoring it via noqa comment
  • It's separate from the parser itself and can plug into the diagnostic system directly
    • This has an added benefit that the formatter will still run even if an unsupported syntax is being used.
    • On the other hand, f-string formatting might become simpler because the formatter doesn't need to consider the target version

I'm more leaning towards (2). An open question here is to whether implement this as a new rule or add it to E999. The new rule can be added under the ruff group with the name as unsupported-syntax.

@AlexWaygood
Copy link
Member

I support detecting these syntax errors via the linter (Option (2)) wherever possible. I think it'll be great to have these errors be # noqa-able and to be able to plug into the diagnostic system directly. However, I don't necessarily see a need to be dogmatic about it -- if there are specific syntax errors (such as parenthesized with items) that would be much easier to detect in the parser, then possibly we could detect some in the parser.

I'd much prefer to have these syntax errors be detected as part of E999 rather than adding a new lint rule. Although we'll be using a different part of ruff to detect them, from the perspective of users a syntax error is a syntax error. I think it will be pretty confusing for them if some syntax errors are detected by one rule and other syntax errors are detected by another rule. The effect of a syntax error for a user is always the same -- your code is never going to be compiled by Python, let alone run!

@MichaReiser
Copy link
Member

I'd much prefer to have these syntax errors be detected as part of E999 rather than adding a new lint rule. Although we'll be using a different part of ruff to detect them, from the perspective of users a syntax error is a syntax error. I think it will be pretty confusing for them if some syntax errors are detected by one rule and other syntax errors are detected by another rule. The effect of a syntax error for a user is always the same -- your code is never going to be compiled by Python, let alone run!

I think one key difference between the two is that you should never suppress a syntax error that is an error regardless of the python version (I don't even know if we support suppressing them). I don't know if there are good reasons for suppressing python-version specific syntax errors but I could see an argument for that. I don't think these should be suppressed by inline noqa comments but a user might decide to suppress them with per-file-ignores (although a new pyrpoject.toml with the right requires-python version would be better)

@AlexWaygood
Copy link
Member

The only reasons I can see for wanting to suppress a syntax error diagnostic are:

  • Deliberate syntax errors (e.g. in test files or data files)
  • A bug in Ruff, where it incorrectly reports a syntax error even though there isn't one

If you have the right target version in your config file, I can't personally see any reason why you'd want to be able to suppress Python-version-specific syntax error diagnostics, or treat them any differently from syntax that's invalid on all Python versions.

@achimnol
Copy link

achimnol commented Aug 3, 2024

Any updates on this? We are still holding back the ruff version due to the f-string backporting issue.

yaniv-aknin added a commit to yaniv-aknin/filmfarm that referenced this issue Aug 18, 2024
Once astral-sh/ruff#6591 is fixed, ruff will do this.
(and I should also have CI, but meh)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions especially welcome parser Related to the parser rule Implementing or modifying a lint rule
Projects
None yet
Development

No branches or pull requests

8 participants