Official Lint Rules & Tool

Ideas
21

PEP8 are the Cpython specific guidelines iiuc, the fact that the community at large has adopted them is not reason to revise PEP8 unless the core developers want to revise it.

3 Likes
22

I just tried ruff’s default rules on the SymPy codebase and I got:

$ ruff check sympy --statistics
987	E712	[*] true-false-comparison
917	E741	[ ] ambiguous-variable-name
358	E731	[*] lambda-assignment
173	E402	[ ] module-import-not-at-top-of-file
122	E721	[ ] type-comparison
 70	E711	[*] none-comparison
 57	E701	[ ] multiple-statements-on-one-line-colon
 26	E702	[ ] multiple-statements-on-one-line-semicolon
 25	E713	[*] not-in-test
 11	E714	[*] not-is-test
  2	E401	[*] multiple-imports-on-one-line
  1	E743	[ ] ambiguous-function-name
[*] fixable with `ruff check --fix`

Top of the list are 987 cases of E712 but these are actually incorrect:

sympy/assumptions/handlers/calculus.py:193:8: E712 Avoid equality comparisons to `True`; use `if abs(expr.base) <= 1:` for truth checks
    |
193 |     if (abs(expr.base) <= 1) == True and ask(Q.extended_positive(expr.exp), assumptions):
    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ E712
    |
    = help: Replace with `abs(expr.base) <= 1`

Here what ruff has not understood is that the expression abs(expr.base) <= 1 does not necessarily evaluate to a bool because the query involves symbolic expressions and may be unresolvable:

In [1]: x = Symbol('x')

In [2]: abs(x) < 1
Out[2]: │x│ < 1

In [3]: if abs(x) < 1:
   ...:     print('smaller')
   ...: 
---------------------------------------------------------------------------
TypeError: cannot determine truth value of Relational: Abs(x) < 1

Here ruff claims that it can fix these but it must not be allowed to do so because the code is correct and changing it as suggested would introduce a bug in each place. More than once people have opened pull requests attempting to apply this fix.

The next rule down E741 is just complaining about certain variable names. The next is E731 which asks for lambda functions to be rewritten using def. Then E402 complains about imports not being at the top of a file but that is needed to avoid circular imports.

None of these rules belong in what I would consider to be the basic set of uncontroversial rules that is only concerned with checking for correctness while avoiding false positives, not complaining about style etc. Each of them suggests changes that are either incorrect in context or that are just not very important and don’t really concern correctness.

On the other hand there are many other rules that could go into the basic set but that are not enabled by default. It would be a lot of work to figure out which those rules are. It would be great if someone could figure that out though and identify the rules that at least seem likely to be universally applicable and then make it possible to select exactly those lint rules.

5 Likes
23

Agreed! But perhaps we could have a repo of curated configs that folks could adopt easily, e.g. by including them via git URL in their local configs.

2 Likes
24

Just as a note, we (Astral) totally agree the default rule set is not ideal right now. We’re very interested in re-categorizing everything and establishing an uncontroversial and universally applicable default rule set — it’s just a big project.

10 Likes
25

It is a big project and it doesn’t have to be Astral that does that work: anyone can go study the lint rules and figure out a better way to organise them, study their effect on different codebases, talk to maintainers etc like Irit did for PEP 765.

@sirosen proposed at the outset of this thread that core Python could do this work in order to make an official set of lint rules. I would expect that if some people actually did the work to identify useful rule sets and wrote it up then popular linting tools would take advantage of that without anything needing to be “official”. I’m sure that regardless of whether PEP 765 is accepted its reasoning is enough to convince people that the relevant lint rules are worthy of being enabled by default.

1 Like
26

Isn’t that exactly what linting tools already do?

27

Yes and no. As far as I can tell the ruff rules come from the flake8 ecosystem and there the rule categories reflect who wrote the rules rather than being grouped in a way that makes sense when considering which rules are useful to apply in a project.

The relevant ruff rule for PEP 765 is B012 which is in the B category that is a somewhat random collection of rules some people implemented in an extension to flake8. There isn’t really any coherent organisation to these rules so you either have to enable them one by one or enable a category and then go through and then disable them one by one. I just tried enabling the B category in SymPy and immediately I find loads of false errors such as:

sympy/core/tests/test_expr.py:347:44: B023 Function definition does not bind loop variable `na`
    |
345 |         for na in [NonArithmetic(), object()]:
346 | 
347 |             raises(TypeError, lambda : e + na)
    |                                            ^^ B023
348 |             raises(TypeError, lambda : na + e)
349 |             raises(TypeError, lambda : e - na)

Any use of pytest’s raises in a loop will hit this particular (false) error.

Most rules in the B category look good but you can’t just enable the whole suite because they are not all unambiguously good. To use the rules you would need to spend time evaluating and choosing amongst them or alternatively (as SymPy does) you can just not use any of the rules in this category even though some of them are good.

I think that the linters have allowed many problematic rules to creep in by falling back on downstream configurability rather then careful upstream curation. The system sort of works because you can disable the rules if you want but it is a waste of everyone’s time to sift through the rules and decide which to use.