cEP-0025.md: Integrate pyflakes AST #144

ankitxjoshi · 2018-05-11T04:10:39Z

Closes #143

ankitxjoshi · 2018-05-11T04:12:27Z

pseudocode for plugin implementation yet to be added.

jayvdb · 2018-05-11T05:49:48Z

cEP-0025.md

+                                              result.deadScopes)
+        }
+
+        yield HiddenResult(self, content)


create a HiddenResult subclass, and use OO. a dict is ugly, and brittle. backwards compatibility and deprecation with a dict is limited, so a dict will mean we have problems when the pyflakes internals change.

Also please see all the issues related to HiddenResults. Fixing a few of them in your project will make everything much neater.

ankitxjoshi · 2018-05-12T04:09:14Z

I believe that the flake8 wrapper (another part of the project) should be included in a different cEP.

jayvdb · 2018-05-12T04:43:56Z

cEP-0025.md

+
+## Introduction
+
+The current approach of plugin development for flake8 does not utilises


This jumps straight to flake8, without helping the reader understand how flake8 is related to this coala project.

You might want to start with something like flake8 is the most commonly used linter framework in the python world. (see https://pythonwheels.com/ for a list of most downloaded pypi packages, and there are other pypi stats pages which might be more appropriate to use.) flake8 has two builtin enabled linters, pyflakes and pycodestyle.
Additional linters are supported using the flake8 plugin system, and may be Python AST based or built using pycodestyle's internal logical-line-based checker API. There has not been an option to use the pyflakes internal AST based checker API to create custom linters.

(might be worth mentioning https://github.com/flintwork/flint for history sake..)

Then you can explain why using the pyflakes internal AST based checker API to create custom linters is a fantastic idea .. ;-)

To cover the pycodestyle's internal logical-line-based checker API, the best example is https://github.com/openstack-dev/hacking , which has lots of little rules.

It would also be useful to investigate the https://github.com/PyCQA/bandit plugin system (recently added to PyCQA), as that is AST based iirc.

It wouldnt hurt to also mention pylint, which has plugin support and the best Python ast IMO, astroid. And it would definitely be a good idea to document why we are using pyflakes instead of astroid.

One of the reasons is along the lines of what you've already said - pyflakes provides a lot of re-usable analysis, making plugins easy, while astroid is more detailed and powerful, it is much more complicated. And there is a much larger ecosystem of flake8 plugins.

A project objective not relevant for this cEP is that we may be able to get flake8 to add a new 'pyflakes ast' plugin type, which has the same API as our own, or a similar enough API that the wrapper is very thin.

Actually the last sentence isnt quite correct. The original project has a goal of "invoke existing flake8 plugins" (which would be the pycodestyle API and raw python ast plugin types which flake8 supports) which is beyond the scope of this pyflakes cEP

However the project also mentions "This allows the coala and flake8 communities to cooperate on plugin development using a common plugin framework.", so it is within the scope of this project to ensure that the new pyflakes ast plugins created are also usable by flake8. We might supply a patch to flake8 to support the new pyflakes ast plugin type, or this repo/package might provide a flake8-plugin which invokes all of the new pyflakes-ast plugins in this repo.

coala already has PyFlakesBear and PycodestyleBear, so we dont need a Flake8 bear for its default plugins. But there are lovely flake8 existing plugins which we would like to have as separate bears.

See coala/coala-bears#1388 for the start of a rabbit hole about why we dont want to depend on flake8 directly. And this problem would also occur if we depended on any flake8 plugins as they will give flake8 as a dependency, and then we are stuck with flake8 dictating our versions. And this problem would occur if this package of flake8-pyflakes-ast-plugins added a dependency on flake8. How to solve this dependency problem is an implementation detail which can be omitted from this cEP.

jayvdb · 2018-05-12T05:10:16Z

cEP-0025.md

+
+1. There will be a separate repository named as coala-pyflakes which will
+   be installable via `pip install coala-pyflakes`.
+2. The repository will contain a new package `pyflakes-plugins` which


If this package contains primarily bears, it needs to include either bears or coala in the name.

But it might be worth creating a non-coala API for these linter plugins - an API which is similar to other frameworks like flake8, so they could also run the same linters.

Then the package contains a bunch of generic pyflakes plugins, a coala meta bear (the pyflakes ast provider), and a coala bear meta wrapper (which creates a wrapper bear for each of the plugins).

Or we could have two packages, one for the generic pyflakes plugins, and a different one for the coala glue.

jayvdb · 2018-05-12T06:12:27Z

cEP-0025.md

+class PyflakesResult(HiddenResult):
+
+    @enforce_signature
+    def __init__(self, origin, deadScopes):


enforce signature depends on having type annotations here.

jayvdb · 2018-05-12T06:14:15Z

cEP-0025.md

+        self.generator_scope = self.get_scope(GeneratorScope, deadScopes)
+        self.doctest_scope = self.get_scope(DoctestScope, deadScopes)
+
+    def get_scope(self, scope_level, scopes):


get_scope -> get_scopes ; always use plural to indicate a list is returned.

and then update the instance attributes.

jayvdb · 2018-05-12T06:17:30Z

cEP-0025.md

+                    if isinstance(node, FutureImportation):
+                        yield Result.from_values(
+                            origin=self,
+                            message=('Future import %s found' % node_name),


this example assumes creates an error for any future imports.

pls investigate the new py37 future import, to see if it is desirable.

If yes, the logic needs to be improved.

If not, then this is a NoFutureImportBear.

I was planning to make this bear work 2 ways:

To make sure that a particular import is added to every module. For eg: If I want that print_function should be imported into every module, I would make it compulsory and use this to find instances where I have failed to add them.

If I don't want them, or don't want some (create a blacklist). The NoFutureImportBear you are discussing.

As an example NoFutureImportBear is better.
It is a simple bear that coala can use, as there are no py3 future imports which are supported on py34. Just add a comment which explains why this bear is simple.
An objective of this cEP and project is to show that it will allow simple rules based on the pyflakes ast.
Making it a complicated bear detracts from that goal.

jayvdb · 2018-05-12T06:27:24Z

cEP-0025.md

+Pyflakes provides with a basic API that does the traversing.
+So, if a developer uses enhanced AST he just needs to work on
+the implementation of the new logic that his/her plugin provides
+and not about the fidelity of the basic node handlers.


and important problem not yet discussed is that pyflakes internals show the state with erroneous objects as-is in the scopes.

That could lead to plugins doing the 'wrong' thing. Usually the wrong thing has been done in the code, and needs to be fixed in the source, and then coala restarted, so

maybe no pyflakes plugins are run unless pyflakes found no errors.

erroneous objects are detected (does pyflakes keep enough metadata to do this?), and then either removed (creating more problems?) or flagged (meaning plugin authors need to check a flag..ugh).

ankitxjoshi · 2018-05-13T10:10:49Z

Need pointers for node_visitor and scope_visitor that you discussed.

jayvdb · 2018-05-13T11:18:12Z

cEP-0025.md

+checker API (checkout [Hacking](https://github.com/openstack-dev/hacking)).
+
+There has not been an option to use the pyflakes internal AST based checker
+API to create custom linters. Thus, the full potential of enhanced AST isn't


enhanced AST here vs pyflakes internal AST on the previous line.

The reader may not get the leap from pyflakes internal AST to enhanced AST being the same thing.

jayvdb · 2018-05-13T11:22:57Z

cEP-0025.md

+Here is a brief overview of the architecture:
+
+1. There will be a separate repository named as coala-pyflakes which will
+   be installable via `pip install coala-pyflakes`.


It seems like we have three packages in one repo.

pyflakes-generic-plugins is easy -- it contains the generic plugins, and other generic stuff

pyflakes-plugin-bears is pyflakes enhanced AST coala Bears.

If pyflakes-plugin-bears contains metabear PyFlakesASTBear ...

What is in coala-pyflakes ?

jayvdb · 2018-05-13T11:23:29Z

cEP-0025.md

+   function, module, generator ,and doctest level. Thus, providing
+   the developer multiple views of a file allowing him to retrieve
+   results as per his demand.
+3. Apart from scope information, the metabear even returns a list of


even -> also

jayvdb · 2018-05-13T11:25:07Z

cEP-0025.md

+
+        Result.__init__(self, origin, message='')
+
+        self.class_scope = self.get_scopes(ClassScope, deadScopes)


all of these, except module_scope, should be plural: foo_scopes.

jayvdb · 2018-05-13T11:28:43Z

cEP-0025.md

+        self.doctest_scope = self.get_scopes(DoctestScope, deadScopes)
+        self.pyflakes_messages = pyflakes_messages
+
+    def get_scopes(self, scope_level, scopes):


scope_level -> scope_type.

many of these scopes can be nested in various ways, such as functions in a doctest.

We'll probably want to add some helper which makes it easier for plugins to skip anything in a doctest, as they are usually more complicated, and need different logic.

And this is why we probably need to support a visitor pattern, as plugins think about scopes from outer to the inner, and want to exclude large branches that dont interest them. But the visitor pattern can be thought about later after we've built some plugins and find the pain points.

Actually this is something that is worth mentioning in more detail above. I am pretty sure Python ast doesnt provide doctests, and pretty sure that pycodestyle also doesnt have any reasonable way to create plugins for doctest. The pyflakes ability to provide doctest is a serious feature for projects like coala who use them extensively. Currently errors in them are caught by pyflakes, but the style of them is mostly ignored.

Maybe this is another good example for the cEP, as you could easily show a plugin which is probably very difficult in any other framework.

jayvdb · 2018-05-13T11:38:44Z

cEP-0025.md

+        yield PyFlakesResult(self, result.deadScopes, result.messages)
+```
+
+## Demonstrating use of PyFlakesASTBear by creating a plugin


plugin -> simple bear.

jayvdb · 2018-05-19T10:31:23Z

cEP-0025.md

+
+## Implementation of PyFlakesASTBear
+
+1. PyFlakesASTBear will be LocalBear returning result of the type


use backticks everywhere that a class name or literal string is mentioned

jayvdb · 2018-05-19T10:31:38Z

cEP-0025.md

+   for the dependent bear. The result structure is described by
+   PyFlakesResult class.
+2. PyFlakesResult consists of a snapshot of the module at class,
+   function, module, generator ,and doctest level. Thus, providing


misplaced ,

jayvdb · 2018-05-19T10:33:01Z

cEP-0025.md

+        self.class_scopes = self.get_scopes(ClassScope, deadScopes)
+        self.function_scopes = self.get_scopes(FunctionScope, deadScopes)
+        self.generator_scopes = self.get_scopes(GeneratorScope, deadScopes)
+        self.doctest_scopes = self.get_scopes(DoctestScope, deadScopes)


I suspect there may only be one doctest scope.
can you check this. if so, make it singular instead of plural, and list it under the module_scope variable

No. There exists multiple doctest_scope. 1 scope per doctest. That's how it works.

jayvdb · 2018-05-19T10:34:04Z

cEP-0025.md

+
+        Result.__init__(self, origin, message='')
+
+        self.module_scope = self.get_scopes(ModuleScope, deadScopes)


I think this needs a [0] at the end to get only the first result.

jayvdb · 2018-05-19T10:35:01Z

cEP-0025.md

+                corrected = self.remove_import(file, lineno, node.name)
+                yield Result.from_values(
+                    origin=self,
+                    message=('Future import %s found' % node.name),


round brackets not needed.

jayvdb · 2018-05-19T10:36:16Z

cEP-0025.md

+        # Get the maximum column offset of FutureImportation for a line
+        max_column_for_a_line = dict()
+        for result in dependency_results.get(PyFlakesASTBear.name, []):
+            for node in result.get_nodes(result.module_scope[0],


yup; [0] needed in the Result subclass, not here.

jayvdb · 2018-05-19T10:37:33Z

cEP-0025.md

+            for node in result.get_nodes(result.module_scope[0],
+                                         FutureImportation):
+                lineno = result.get_node_location(node)[0]
+                col_offset = result.get_node_location(node)[2]


this [2] here isnt self-explanatory.

maybe get_node_location should return something which is easier to use.

We could even opt to remove the wrapper function.
node.source.lineno or node.source.col__offset will be good.

jayvdb · 2018-05-19T10:38:11Z

cEP-0025.md

+                        )
+            yield Result.from_values(
+                origin=self,
+                message=('Future import/imports found'),


Simplify to be 'Future import(s) found'

jayvdb · 2018-05-19T10:40:12Z

cEP-0025.md

+## A generic implementation for detecting `__future__` imports
+
+This implementation doesn't requires coala to be installed and is
+designed so that it can be used in other tools like flake8. This


clarify that coala will also be able to use these plugins, but these are also usable by flake8.

Also this example should be above the 'fixes' bear.

The fixes bear is the finale -- it shows why we would use coala bears -- there are features of coala which bears can use that generic plugins can not use.

jayvdb · 2018-05-19T13:11:54Z

cEP-0025.md

+
+    def get_node_location(self, node):
+        return {'lineno': node.source.lineno,
+                'depth': node.source.depth - 1,


depth isnt used in your examples.

and this method is just a thin wrapper around node.source ... maybe it should return node.source, which allows access to lineno and col_offset, and if pyflakes internals ever change then get_node_locationcould return its ownNamedTuple` which provides those attributes.

But maybe we dont need get_node_location. If the pyflakes internals ever removed or changed node.source, we could wrap all nodes with a backwards compatible class which provides a node and node.source which has the attributes which plugins expect.

jayvdb · 2018-05-19T13:13:20Z

cEP-0025.md

+        for result in dependency_results.get(PyFlakesASTBear.name, []):
+            for node in result.get_nodes(result.module_scope,
+                                         FutureImportation):
+                lineno = result.get_node_location(node)['lineno']


or this could be just lineno = node.source.lineno. I am not seeing the benefit of get_node_location.

jayvdb · 2018-05-19T13:13:38Z

cEP-0025.md

+                    message='Future import %s found' % node.name,
+                    file=filename,
+                    diffs={filename: corrected},
+                    line=result.get_node_location(node)['lineno'])


this could be line=lineno

jayvdb · 2018-05-19T13:14:29Z

cEP-0025.md

+                message = '{code}: Future import {name} found'
+                           .format(name=node.name,
+                                   code=CODE)
+                yield (node.source.lineno, node.source.col_offset,


I see you are using node.source.foo here, and it looks nice.

jayvdb · 2018-05-19T13:15:58Z

cEP-0025.md

+        for result in dependency_results.get(PyFlakesASTBear.name, []):
+            for node in result.get_nodes(result.module_scope,
+                                         FutureImportation):
+                lineno = result.get_node_location(node)['lineno']


get_node_location() in this method also looks to be more complexity than is needed.

jayvdb · 2018-05-19T13:16:27Z

cEP-0025.md

+                col_offset = result.get_node_location(node)['col_offset']
+                if lineno in max_column_for_a_line:
+                    max_column_for_a_line[lineno] = max(
+                                max_column_for_a_line[lineno],


very strange indentation here.

jayvdb · 2018-05-19T13:23:14Z

cEP-0025.md

+        line = file[lineno - 1]
+        # If a line contains \ in the end
+        if line.rstrip()[-1] == '\\':
+            next_line = file[lineno]


and what happens if the next_line also contains \ .. :P (now you may understand why I hate \ so passionately.

Some ways to handle this neatly is to create a concept of a 'logical line', which provides information about the entire line, and also have metadata about each distinct statement which gives (start_line,start_col),(end_line,end_col).

Given the complexity of this, I suggest you add a code comment that the algorithm hasnt been adjusted to support \ . That will make the logic a lot simpler and then you can solve the \ problem later, when you have real code and unit tests to verify all silly corner cases have been checked.

Closes coala#143

jayvdb · 2018-05-20T11:50:50Z

ack 95bc075

jayvdb · 2018-05-20T11:51:01Z

@gitmate-bot ff

gitmate-bot · 2018-05-20T11:51:03Z

Hey! I'm GitMate.io! This pull request is being fastforwarded automatically. Please DO NOT push while fastforward is in progress or your changes would be lost permanently ⚠️

gitmate-bot · 2018-05-20T11:51:43Z

Automated fastforward with GitMate.io was successful! 🎉

gitmate-bot added process/pending review size/XS labels May 11, 2018

jayvdb requested changes May 11, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels May 11, 2018

ankitxjoshi force-pushed the cEP-pyflakes branch from 6d18279 to 7f36145 Compare May 12, 2018 03:49

gitmate-bot added process/pending review size/S difficulty/newcomer and removed process/wip size/XS labels May 12, 2018

gitmate-bot added New cEP and removed difficulty/newcomer labels May 12, 2018

jayvdb requested changes May 12, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels May 12, 2018

jayvdb reviewed May 12, 2018

View reviewed changes

ankitxjoshi force-pushed the cEP-pyflakes branch from 7f36145 to aaabc8b Compare May 13, 2018 09:55

gitmate-bot added process/pending review and removed process/wip labels May 13, 2018

ankitxjoshi force-pushed the cEP-pyflakes branch from aaabc8b to 6651e38 Compare May 13, 2018 10:04

jayvdb requested changes May 13, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels May 13, 2018

jayvdb reviewed May 19, 2018

View reviewed changes

jayvdb added process/wip and removed process/pending review labels May 19, 2018

ankitxjoshi force-pushed the cEP-pyflakes branch from 978e384 to 01a8f6b Compare May 19, 2018 12:02

gitmate-bot added process/pending review and removed process/wip labels May 19, 2018

jayvdb reviewed May 19, 2018

View reviewed changes

jayvdb requested changes May 19, 2018

View reviewed changes

ankitxjoshi force-pushed the cEP-pyflakes branch from 01a8f6b to 69ff9b2 Compare May 19, 2018 14:00

cEP-0025.md: Integrate pyflakes AST

95bc075

Closes coala#143

ankitxjoshi force-pushed the cEP-pyflakes branch from 69ff9b2 to 95bc075 Compare May 19, 2018 14:02

jayvdb approved these changes May 19, 2018

View reviewed changes

jayvdb requested review from Makman2, Udayan12167 and adtac May 19, 2018 14:59

gitmate-bot added process/approved and removed process/pending review labels May 20, 2018

gitmate-bot merged commit 95bc075 into coala:master May 20, 2018


		## Introduction

		The current approach of plugin development for flake8 does not utilises


		Result.__init__(self, origin, message='')

		self.class_scope = self.get_scopes(ClassScope, deadScopes)


		## Implementation of PyFlakesASTBear

		1. PyFlakesASTBear will be LocalBear returning result of the type


		Result.__init__(self, origin, message='')

		self.module_scope = self.get_scopes(ModuleScope, deadScopes)

cEP-0025.md: Integrate pyflakes AST #144

cEP-0025.md: Integrate pyflakes AST #144

Conversation

ankitxjoshi commented May 11, 2018

ankitxjoshi commented May 11, 2018

Choose a reason for hiding this comment

ankitxjoshi commented May 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitxjoshi commented May 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb May 19, 2018 • edited Loading

Choose a reason for hiding this comment

ankitxjoshi May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb May 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayvdb commented May 20, 2018

jayvdb commented May 20, 2018

gitmate-bot commented May 20, 2018

gitmate-bot commented May 20, 2018

jayvdb May 19, 2018 •

edited

Loading

ankitxjoshi May 19, 2018 •

edited

Loading

jayvdb May 19, 2018 •

edited

Loading