More linting

[alexmojaki]

Some basic linting with pyflakes was introduced in #99. This needs to be extended with checks from other libraries:

• pylint
• pylint plugins. https://github.com/pyta-uoft/pyta has some custom checkers which might be useful, but the project itself isn't right for us, see Raw data API? pyta-uoft/pyta#670
• Plugins which extend pyflakes/flake8. For the latter I think plugins tend to be for flake8 so we may have to change our pyflakes code to use flake8 instead. An example of a nice looking flake8 plugin is https://pypi.org/project/flake8-bugbear/. Here are some warnings it has which interest me:
  • B002: Python does not support the unary prefix increment. Writing ++n is equivalent to +(+(n)), which equals n. You meant n += 1.
  • B006: Do not use mutable data structures for argument defaults. They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.
  • B007: Loop control variable not used within the loop body. If this is intended, start the name with an underscore.
  • B008: Do not perform function calls in argument defaults. The call is performed only once at function definition time. All calls to your function will reuse the result of that definition-time function call. If this is intended, assign the function call to a module-level variable and use that variable as a default value.
  • B009: Do not call getattr(x, 'attr'), instead use normal property access: x.attr. Missing a default to getattr will cause an AttributeError to be raised for non-existent properties. There is no additional safety in using getattr if you know the attribute name ahead of time.
  • B010: Do not call setattr(x, 'attr', val), instead use normal property access: x.attr = val. There is no additional safety in using setattr if you know the attribute name ahead of time.
  • B012: Use of break, continue or return inside finally blocks will silence exceptions or override return values from the try or except blocks. To silence an exception, do it explicitly in the except block. To properly use a break, continue or return refactor your code so these statements are not in the finally block.
  • B013: A length-one tuple literal is redundant. Write except SomeError: instead of except (SomeError,):.
  • B014: Redundant exception types in except (Exception, TypeError):. Write except Exception:, which catches exactly the same exceptions.
• Here are some other things I'd like to check. If we find them in libraries, great. I expect pylint has many of these. If not, maybe we can implement some ourselves, or open issues in other repos.
  • Unreachable code, e.g. after return or break.
  • Variables which might be undefined, e.g. if they were only defined in an if with no else.
  • Variables which shadow the same name in an outer scope. Local and global variables should have different names.
  • Variables which shadow builtins, e.g. list = [1, 2, 3].
  • Trailing commas creating a singleton tuple, e.g. x = 1,. This is a nasty thing for a beginner to not realise they've done. They can just add parentheses to be explicit.
  • Implicit concatenation of adjacent string literals, e.g. foo("a" "b"). Indicates they may have forgotten a comma or something else. Just add +.
  • Parentheses which are really pointless, e.g. x = (1). Probably meant (1,) to make a singleton tuple.
  • Redefined a variable before using it. pyflakes already does this in futurecoder but only for functions and classes. x = 1; x = 2; foo(x) is equally pointless.
  • Assigned to self in a method.

General guidelines for things to check:

• Red flags that they may have made a mistake or forgotten something.
• Signs that they misunderstand how Python works.

Things not to check:

• Code which might be legit and there is no easy workaround. For example sometimes you don't need a loop variable, but we can still check for it and require naming the variable with an underscore, e.g. for _ in range(10). But we don't want to make people put comments like # noqa to silence warnings.
• Stuff which is just a matter of appearance/style and has no bearing on how the program runs, e.g. most/all of PEP8.
• Errors which will lead to an obvious exception, e.g. completely undefined variables or syntax errors.

[aroberge]

I'm interested in adding something similar as an add-on to the friendly_traceback family -- possibly naming it friendly_lint. The focus would be when friendly/friendly_traceback is used in interactive mode (i.e. in a console, or Jupyter notebook, etc.). Due to this restrictions, "global issues" such as variable defined but not used or modules imported but not used, etc., would not be considered. [However, see note at the end.] Stylistic issues would also be completely ignored. In addition to some of the cases you mentioned, I would like to consider at least two more that I have seen in beginners' code:

if name = "Alex" or "André":
   ...

and

def some_test():
    ...

if some_test:  # not called, so always True
    ...

I suspect that pyflakes would not be able to deal with the last case, and that I would have to use pylint.

For using with friendly, I was thinking that raising some custom FriendlySyntaxWarning would be the way to go, as this would give the option to get more information using what(), where(), why() only when needed. However, I don't know if raising warnings would be problematic for futurecoder. I would also give the option for the end-user to disable these warnings; perhaps an option to printing some information instead of raising a warning might be suitable for futurecoder.

---

[Note] To avoid duplication of effort, it might make sense to have two different "modes" for friendly_lint, with a second one dealing with all issues that you mentioned for futurecoder, with a function that could be used to explicitly lint a module.

[alexmojaki]

Awesome, it'd be great to collaborate on this.

I suggest first creating a standalone library whose main feature is a function that accepts a string (or maybe an AST) and returns a list of objects/dicts containing lint messages and other metadata. friendly[_traceback] and futurecoder can then do whatever they want with that list, e.g. raise warnings or print them. Similar to how stack_data is mostly focused on exposing information to be used by other libraries, while friendly_traceback gives the user friendly interface. The main goals I'd want for the library are:

1. Combine linting libraries, primarily pylint and pyflakes/flake8. I searched some more and found a new project that seems to be doing this: https://flakeheaven.readthedocs.io/en/latest/index.html
2. Explain the warnings clearly for beginners.
3. Include translations for those explanations.
4. Clearly separate actual red flags from stylistic issues, likely not caring about the latter at all.
5. Allow for easy and flexible programmatic use as described above, since the main linting libraries are sometimes more focused on their CLI.

(1) is the biggest thing and it's possible that flakeheaven solves the problem for us. (2) and (3) might not actually need a separate library, maybe it's doable with plugins or something, that should be checked. (4) might be doable purely with the right configuration. And (5) might not really be an issue at all, or if it is then maybe contributing API improvements upstream would be better. So maybe there isn't that much to do.

As for specific checks, including your suggestions which I think are good, if there are no plugins which already cover what we want then again it probably makes more sense to contribute what we can upstream or create new plugins, so they can be used by normal pylint/flake8/etc users.

[aroberge]

I pretty much agree with all your points.

First, just a quick thought: I just installed flakeheaven in a clean virtual environment. Here's what was installed:

> python -m pip list
Package     Version
----------- -------
colorama    0.4.6
entrypoints 0.4
flake8      4.0.1
flakeheaven 3.2.0
mccabe      0.6.1
pip         22.2.1
pycodestyle 2.8.0
pyflakes    2.4.0
Pygments    2.13.0
setuptools  63.2.0
toml        0.10.2
urllib3     1.26.12

I don't see pylint installed (or astroid). I also wonder how useful it is to have all of those extra dependencies (colorama, entrypoints, mccabe, pycodestyle, pygments, and urllib3) for projects such as futurecoder and friendly_traceback. friendly already uses pygments and colorama, so this would not be relevant. So, while the stated goal of flakeheaven appear to be consistent with what we are looking for, I wonder if it is not excessive for what we are looking for. Perhaps a much simpler version might be preferable.

---

I'd like to start first by focusing on one or perhaps just a few test cases to explore what kind of info we'd need to collect and the API for external "users" - such as friendly and futurecoder. My sense is that tools like pylint, flake8 and their various plugins, give some standardized messages which are too generic and often cryptic. Thinking of the approach I used in friendly_traceback, I'd like to be able provide data that is specific to an actual code being used. So, given an example I mentioned before:

if name == "Alex" or "André":
   ...

I could see some error message from a typical plugin, somethin like "E4242 string constant in a binary or is always True"
(but better worded) ... which might be fine for most advanced users, but not something I'd like to display for this project; at the very least, a less cryptic description would have to be provided.

I'd also want to be able to provide more information, such as the location of the problematic code which could be used by some formatter, e.g.

if name == "Alex" or "André":
                     ^^^^^^^
   ...

(when using friendly, this would be the output of where()) and then being able to make a suggestion like (why() for friendly):

---

Your code is equivalent to the following:

if (name == "Alex") or ("André"):
                        ^^^^^^^

A non-empty string is equivalent to True in Python, which means that the above is equivalent to

if (name == "Alex") or True:
                       ^^^^

You likely meant to write:

if name == "Alex" or name == "André":
                     ^^^^^^^^^^^^^^^

---

We could perhaps start with an existing error message/warning from pyflakes (or one of its plugins) and see how we could transform it into something along the above lines - if you agree with this idea.

[alexmojaki]

I don't see pylint installed (or astroid).

flakeheaven clearly mentions pylint integration but I also wasn't able to understand what this actually means from the docs. Maybe it automatically uses pylint when it finds it already installed? I think opening an issue asking them to clarify in documentation is worth it.

I also wonder how useful it is to have all of those extra dependencies (colorama, entrypoints, mccabe, pycodestyle, pygments, and urllib3) for projects such as futurecoder and friendly_traceback.

That's definitely a problem. futurecoder has pygments already but not the others. Extra dependencies mean more to download before being able to run any code so they need to be avoided. So maybe it would make sense to create a separate package.

flake8 itself depends on mccabe and pycodestyle. Hopefully those aren't too big or there's a way to avoid requiring them or just use pyflakes instead but still be able to use flake8 plugins/extensions.

My sense is that tools like pylint, flake8 and their various plugins, give some standardized messages which are too generic and often cryptic.

Right, which is why one of the main goals is to add clearer, beginner-friendly explanations. That's what I've already done for pyflakes in https://github.com/alexmojaki/futurecoder/blob/master/core/linting.py . You should be able to see it in action by running code in the futurecoder editor (not the shell) that triggers a warning, e.g. an unused variable. The point of this issue was to expand the checks.

Thinking of the approach I used in friendly_traceback, I'd like to be able provide data that is specific to an actual code being used.

futurecoder currently only cares about a string describing the problem. It doesn't even use the line number. But maybe it should, and a more generic library should definitely provide that kind of stuff.

Looking at this code in pyflakes, I think loc is always an AST node and they just extract the lineno and col_offset. With a bit of tweaking (e.g. monkeypatching Message.__init__) it should be easy to hold on to the actual node and thus get a precise range of positions to do what you want.

[rootsmusic]

@alexmojaki Edulint is a linter that integrates both pylint and flake8, "with some tweaks to their default configuration". Its purpose is "aimed at helping novice programmers improve their coding style". So it's targeting me, but I'm unable to answer whether its capabilities match this issue's checklist.