search

astral-sh / ruff

An extremely fast Python linter and code formatter, written in Rust.
https://docs.astral.sh/ruff
MIT License
32.41k stars 1.08k forks source link

Implement Pylint #970

Open charliermarsh opened 1 year ago

charliermarsh commented 1 year ago

This is the parent issue for tracking parity with Pylint. Below, we've enumerated all Pylint rules.

Rules that are checked off have been implemented in Ruff already (either as a Pylint rule, e.g., PLE0237, or by way of overlap with another linter like Pyflakes, as with missing-format-string-key).

Rules that are crossed out have been removed some consideration. (In most cases, crossed-out rules represent Pylint specific rules, e.g., rules related to Pylint configuration.)

At time of writing, many of the remaining rules require type inference and/or multi-file analysis, and aren't ready to be implemented in Ruff. (See: https://github.com/astral-sh/ruff/issues/970#issuecomment-1565594417 for an enumeration.) If you're looking to start work on a specific rule, I'd suggest commenting in the issue, to get some input on whether Ruff is capable of supporting it at present.

For guidance on getting started, see the Contributing documentation.

Note: Don't implement rules that are part of Pylint extension until #1774 is completed. Don't implement rules that mainly target Python 2.

Error

Warning

Convention

Refactor

charliermarsh commented 1 year ago

Need to figure out what the right "check code" approach is here. We could just start to put these under RUF. Or we could do PYE for "Pylint Error", and so on. Or we could use PYL for all of them. In either case, though, we have to allow four-digit codes, as opposed to our current three-digit codes.

JonathanPlasse commented 1 year ago

I vote for PYE as it would keep the same categories and the migration would be easier.

harupy commented 1 year ago

I'll work on unneeded-not this weekend.

relsunkaev commented 1 year ago

I would suggest PL as the pylint prefix (PLE -> PyLint Error). I think PYE might be a bit ambiguous with PYflakes, PYdocstyle, etc.

MartinBernstorff commented 1 year ago

Would absolutely love to see this happen. I know flake8 intentionally decided not to implement e.g. unexpected-keyword-arg because it required looking outside of the current file for imported functions.

I expect you've already considered this, and really hope this won't stop ruff from implementing it, but just wanted to raise it.

charliermarsh commented 1 year ago

@relsunkaev :+1: Went with PLE! The first code is checked in as PLE1142 (matching E1142 from Pylint).

charliermarsh commented 1 year ago

@MartinBernstorff - Definitely. The multi-file stuff will come, just not as quickly.

hmc-cs-mdrissi commented 1 year ago

Not sure if it should be separate issue, but looking at missing-function-docstring rule (C0116), pylint's version of it is different. It only requires that a function has docstring some in class hierarchy and does not require over-ridden methods to have a docstring. As an example,

class Foo:
  def validate(self):
    """Doc."""
    ...

class Bar(Foo):
  def validate(self):
    ...

gives no errors in pylint even though second validate is missing docstring being pylint allows inheriting one. This is very useful when implementing interface and the docstring would really be same for all things that subclass.

charliermarsh commented 1 year ago

I'd like to get pointless-statement, unnecessary-ellipsis, and unnecessary-pass in.

charliermarsh commented 1 year ago

Relevant source for defining a pointless-statement: https://github.com/PyCQA/pylint/blob/56121344f6c9c223d8e4477c0cdb58c46c840b4b/pylint/checkers/base/basic_checker.py#L447

thejcannon commented 1 year ago

A while ago I set out to find all the pylint codes make redundant by type checking.

Unfortunately that code got lost to the sands of time.

You might consider skipping those as type checking tools are very sophisticated, if you can do the effort of compiling that list

thejcannon commented 1 year ago

I've also thought very long and hard about the fact that pylint appears very sophisticated when it comes to types because it builds on astroid which works very hard to infer types based on code (pre-type-annotations)

Ideally ruff (or any modern linter) is type-aware to be as helpful as possible, but that requires either:

  1. A Rust-implemented type checkers for Python. Short of a large company writing and maintaining this I doubt it'll keep up with the other big dogs
  2. Having the type information be seeded to ruff. Aside from the overhead and protocol, this actually seems feasible.
dae commented 1 year ago

For what it's worth, the type awareness of pylint is the killer feature that keeps us using it in our project despite how painfully slow it is. Our first-party code is almost fully typed, and mypy is generally good at catching typing issues. We've found multiple instances where pylint caught an issue that mypy missed however - usually in third-party code with missing or incomplete type definitions, but occasionally also in our own code when mypy doesn't notice (eg due to the presence of __getattr__: https://github.com/python/mypy/issues/6251).

thejcannon commented 1 year ago

Out of curiosity, have you tried pyright? It claims to have inference support for codebases that aren't fully typed.

dae commented 1 year ago

Mypy has a check_untyped_defs which sounds equivalent, and we use that. We currently rely on mypy's no_strict_optional which pyright has no equivalent to, so pyright is not currently usable on our codebase.

alonme commented 1 year ago

Started to look at import-error / E0401,

From what i can tell - ruff doesn't currently use any "context" about the python environment, right? In order to add this rule - we need to use something like pkgutil - which can be slow, or implement import resolution in ruff.

@charliermarsh Is the project open to any of these options currently?

charliermarsh commented 1 year ago

We have some support for import resolution, but not enough to power import-error right now.

What we do support is (1) a src list, which is used for first-party resolution, similar to isort and reorder-python-imports (so it detects module-level first-party imports, but doesn't do "full" resolution), (2) a namespace-packages list, for namespace packages, and (3) we do crawl the filesystem to find all __init__.py files to power some checks.

Does import-error need to be able to lookup third-party packages?

martimlobao commented 1 year ago

Does import-error need to be able to lookup third-party packages?

FWIW this is one of the biggest reasons we use pylint at all, despite obviously being much slower to do than just first party code.

alonme commented 1 year ago

Any plans on when and how to implement more import resolution features?

WDYT about creating an issue to gather rules that will need these capabilities?

charliermarsh commented 1 year ago

I just went through and mapped all the checked-off rules to the relevant codes within Ruff. I'm guessing we implement more than the 86 that are checked off (at time of writing), but it's my best guess based on a single pass through the list.

justinchuby commented 1 year ago

It’d be really nice if ruff can understand the inline pylint ignores to make migration very easy.

spaceone commented 1 year ago

It’d be really nice if ruff can understand the inline pylint ignores to make migration very easy.

this is covered in https://github.com/charliermarsh/ruff/issues/1203

colin99d commented 1 year ago

We can probbably mark bad-configuration-section as complete, it has to do with a pylint refactor

Pierre-Sassoulas commented 1 year ago

useless-option-value too, it's a warning for an issue in the pylint configuration.

colin99d commented 1 year ago

bad-plugin-value is also for pylint plugins that cannot be loaded, which we do not need.

charliermarsh commented 1 year ago

Thank you! Going to strike them out.

tomecki commented 1 year ago

What's the policy with duplicate logic being checked by multiple linters? E.g. too-many-format-args seems functionally equivalent to F524 which is already implemented:

https://github.com/charliermarsh/ruff/blob/41900316186ae65b588ec0fda64c21bfee4b9956/src/rules/pyflakes/rules/strings.rs#L191

charliermarsh commented 1 year ago

We tend towards including the Flake8 variant over the Pylint variant. We're working on enabling rule aliasing, so that we'll be able to map multiple codes to the same underlying rule (details TBD).

colin99d commented 1 year ago

@charliermarsh do we have anyway of knowing the minor version of python we are on? broken-collections-callable should only be run on python 3.9.0 and 3.9.1, and I don't know of a way to detect only those two sub-versions.

charliermarsh commented 1 year ago

@colin99d - Hmm -- not really. I think we should just skip that rule.

DanielNoord commented 1 year ago

Just my two cents, but from working on pylint for some time we have seen the definite need to specify the expected python version, preferably with the config option py-version which is slowly becoming a standard within the python ecosystem. It might be nice to support such an option as well.

colin99d commented 1 year ago

@charliermarsh, do we have any way of knowing that the name token data is a dict, particularly if the item is defined in a different file.

data = {'Paris': 2_165_423, 'New York City': 8_804_190, 'Tokyo': 13_988_129}
for city, population in data:  # [dict-iter-missing-items]
    print(f"{city} has population {population}.")
charliermarsh commented 1 year ago

@colin99d - No, unlike Pylint we don't have that capability right now.

AvnerCohen commented 1 year ago

@charliermarsh Would love to understand the T-shirt size effort for E0611 (before I take the time to dive into the code base for contribution) and how likely it is to be part of ruff.

I guess the simplest example would be:

from requests import aaa

Would pass on ruff or flake8, on pylint, i'd get:

E0611: No name 'aaa' in module 'requests' (no-name-in-module)
charliermarsh commented 1 year ago

@AvnerCohen - I would call that a "Large". Ruff operates under a single-file model right now, so each file is analyzed in isolated (though we do some traversals upfront to infer the package hierarchy etc.). To power something like that, we need to move to a model in which we do some analysis of the available members in each module as a first pass, then enforce lint rules as a second pass. It's within scope, but it's a significant feature more-so than a single rule, if that makes sense :)

charliermarsh commented 1 year ago

Related to #2914 -- I've posted some more commentary in there.

sbrugman commented 1 year ago

The pylint rules implemented by PyCharm: https://github.com/charliermarsh/ruff/issues/2972

matthewlloyd commented 1 year ago

The pylint rules implemented by PyCharm: #2972

Opened a new issue for PyCharm inspections covered and not covered by PyLint here, since #2972 may soon be closed: https://github.com/charliermarsh/ruff/issues/3040

chanman3388 commented 1 year ago

I was looking at maybe implementing E0203 along with some others that are of the same ilk, but I noticed that, at least in my noddy testing and reading around, we don't really seem to keep track of these neither per scope. Would the sensible thing be to push a new Binding onto the bindings member of the Checker in checkers/ast.rs? If so, what sort of Binding? Binding::Binding?

r3m0t commented 1 year ago

I'll do invalid-character-*

sanmai-NL commented 1 year ago

@charliermarsh What is the suggested approach to configure a project so as to have Ruff subsume Pylint, where both implement the same checks? Right now, we have lot of duplicate warnings.

Kircheneer commented 1 year ago

W0123 is an overlap of PHG001, so I guess that could be ticked off? Same goes for W0122 with S102. 👍🏻

mdbernard commented 1 year ago

For convenience, this page in the Pylint documentation lists all of the Pylint rules, with links to descriptions and examples of each: https://pylint.pycqa.org/en/latest/user_guide/messages/messages_overview.html.

latonis commented 1 year ago

@charliermarsh , I've began work on implementing duplicate-argument-name / E0108, however, the parser throws an error when attempting to parse the test Python file. Does this mean it can be closed off as already implemented or is there something I can alter elsewhere? :) 

cargo run -p ruff_cli -- check crates/ruff/resources/test/fixtures/pylint/duplicate_argument_name.py --no-cache --select PLE0108 
   Compiling ruff v0.0.257 (/home/jacoblatonis/Projects/ruff/crates/ruff)
   Compiling ruff_cli v0.0.257 (/home/jacoblatonis/Projects/ruff/crates/ruff_cli)
warning: `ruff` (lib) generated 3 warnings
    Finished dev [unoptimized + debuginfo] target(s) in 21.66s
     Running `target/debug/ruff check crates/ruff/resources/test/fixtures/pylint/duplicate_argument_name.py --no-cache --select PLE0108`
error: Failed to parse crates/ruff/resources/test/fixtures/pylint/duplicate_argument_name.py: duplicate argument 'first_name' in function definition at line 1 column 39
charliermarsh commented 1 year ago

@latonis - Ah yeah, we can't support that one right now, since it's not valid Python. It's sort of debatable whether Ruff should support that.

thejcannon commented 1 year ago

Type checkers ought to flag that as invalid syntax. Or running the code under tests.

Maybe at the most ruff flags it can't operate because there's a syntax error at and well there's your failure

charliermarsh commented 1 year ago

:thumbsup: Yeah Ruff would flag that there's a syntax error in that file, and would probably tell you where / what it is but I haven't tested that specific case.

itamarst commented 1 year ago

The table above suggests ruff implements cell-var-from-loop, but it doesn't show up in the documentation or on the command-line with ruff check. Is it implemented under a different name, or was that a mistake and it's not actually implemented?

itamarst commented 1 year ago

Ah, cell-var-from-loop is equivalent to B023. For ease of switching for pylint users, then, might be handy to alias B023 as PLW0640, if aliasing is supported.

rjarry commented 1 year ago

Hey folks, Thanks a lot for this awesome project!

I have scrubbed over the list of pylint checks and it looks like (at least) no-name-in-module / E0611 would require leveraging the new ImportMap feature.

Also, producing a proper implementation of no-member / E1101 seems significantly more involved.

Are there any plans to work on these? I wish I could help but I am a bit clueless about rust...