Fix daemon support for our built-in plugins #5828

msullivan · 2018-10-24T02:46:43Z

This is an initial take on support for proper support for the daemon from plugins
(or, at least from our existing built-in plugins).

There are two main support components for this:

A new plugin_generated field on SymbolTableNode to indicate that a node was created by a plugin and should be stripped as part of aststrip. The _add_method support routine is modified to always set this field.
An add_plugin_dependency method in SemanticAnalyzerPluginInterface and a plugin_deps field in MypyFile for it to store dependencies.

The dataclass and attrs plugins are modified to use the add_plugin_dependency API. Unfortunately, the dependencies induced by dataclass and attrs are actually pretty subtle! The type of a dataclass's __init__ method (which is computed when the class is semantically analyzed) is based on the full set of attributes declared in the class and in all of its dataclass ancestors. This means that it doesn't suffice for a class to depend just on those attributes, it must also depend on future attributes that may be added. We do this by using wildcard triggers: each dataclass depends on the wildcard trigger of its dataclass ancestors, and the wildcard trigger for a dataclass depends on each of its attributes.
This means that when a new attribute is added to a dataclass, we will add a dependency from the wildcard trigger to the attribute, then immediately trigger it, which will trigger rechecks of all subclasses.

The plugin_generated scheme, unfortunately, only kind of works, since plugins can (and do) also modify nodes. The two approaches I can think of for fixing that are making copies of ASTs and using that to ditch aststrip entirely (leads to a ~20% memory usage increase, but also lets us get rid of aststrip) or to have a callback based system where plugins can register functions to rollback changes made by the plugin (this is clunky and would be painful for plugin writers).
It isn't really clear that it matters, though, in practice. It might be workable to (maybe with a little care) not need to strip those changes. It doesn't seem to for attrs and dataclasses (which modify Vars to make them as properties if the class is frozen.)

This doesn't add a dependency API to the typechecker plugins, since they weren't needed. Do we think that is true in general, or will some plugins probably want that?

Work towards #5153.

ilevkivskyi · 2018-10-25T16:30:28Z

The plugin_generated scheme, unfortunately, only kind of works, since plugins can (and do) also modify nodes. The two approaches I can think of for fixing that [...]

Note that SQLAlchemy plugin will do something similar, we will need update all variables with Column type to have InstrumentedAttribute type (which is a descriptor). I am however not sure how important it is to be careful with stripping this change. For __init__ we will do something very similar to dataclasses.

Re: possible solutions, I think providing a callback is not an option. Instead of storing full AST can we just store original symbol tables for classes?

ilevkivskyi

Generally looks good, here are some high level comments.

ilevkivskyi · 2018-10-25T16:36:21Z

mypy/plugins/common.py

@@ -110,5 +110,5 @@ def _add_method(
    func._fullname = info.fullname() + '.' + name
    func.line = info.line

-    info.names[name] = SymbolTableNode(MDEF, func)
+    info.names[name] = SymbolTableNode(MDEF, func, plugin_generated=True)


How about making _add_method a public and recommended way to add methods? I would at least remove the leading underscore. Another idea is to modify this function signature so that it accepts a list of strings (dependencies) and adds them instead of providing ctx.api.add_plugin_dependency (I would hide as more details from plugin writers as possible).

Happy to drop the underscore from _add_method. I just realized that attrs doesn't use it. It should be easy to make it use add_method but I realize now that I don't understand why attrs works without setting plugin_generated!

I'm not sure if avoiding direct calls to add_plugin_dependency would really help here. The plugin writer still would need to collect all the wildcard dependencies from the MRO, which is pretty grungy. Having to plumb that out to the add_method call seems like it complicates rather than hiding complexity.

I am OK with keeping add_plugin_dependency (unless @JukkaL has other ideas).

(It didn't work, I was just missing an attrs test.)

I think it's better to keep add_plugin_dependency. The plugin writer will need to add the necessary dependencies in either way, and in this case it's better to have two API functions that do one thing rather than having a single function that does a bunch of things. It's possible that we'll add a new method, such as add_attribute, that would also need to take a list of dependencies, and now we'd have duplicate functionality.

mypy/plugins/attrs.py

test-data/unit/deps.test

test-data/unit/fine-grained.test

ilevkivskyi

LGTM! But let us wait what @JukkaL says about possibly merging add_plugin_dependency() into add_method().

JukkaL

Looks good to me, just a few small things. Feel free to merge after you've addressed them.

JukkaL · 2018-10-31T13:00:16Z

mypy/nodes.py

                 no_serialize: bool = False) -> None:
        self.kind = kind
        self.node = node
        self.module_public = module_public
        self.implicit = implicit
        self.module_hidden = module_hidden
        self.cross_ref = None  # type: Optional[str]
+        self.plugin_generated = plugin_generated


Document this attribute in the docstring.

JukkaL · 2018-10-31T13:13:44Z

mypy/plugins/common.py

@@ -110,5 +110,5 @@ def _add_method(
    func._fullname = info.fullname() + '.' + name
    func.line = info.line

-    info.names[name] = SymbolTableNode(MDEF, func)
+    info.names[name] = SymbolTableNode(MDEF, func, plugin_generated=True)


I think it's better to keep add_plugin_dependency. The plugin writer will need to add the necessary dependencies in either way, and in this case it's better to have two API functions that do one thing rather than having a single function that does a bunch of things. It's possible that we'll add a new method, such as add_attribute, that would also need to take a list of dependencies, and now we'd have duplicate functionality.

JukkaL · 2018-10-31T13:15:32Z

test-data/unit/fine-grained.test

@@ -629,6 +629,304 @@ class M(type):
 ==
 a.py:4: error: Incompatible types in assignment (expression has type "str", variable has type "int")

+[case testDataclassUpdate1]


It would be good to have at least one test case with a deeper inheritance hierarchy. For example, have A <- B <- C and test that a change in A will get propagated to C.

gvanrossum · 2018-11-04T23:39:16Z

Do we have to make changes to the plugins outside this repo?

On Fri, Nov 2, 2018 at 12:47 PM Michael J. Sullivan < ***@***.***> wrote: Merged #5828 <#5828> into master. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5828 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACwrMvkAkOB3QeyqVSrJuy1X4yE8vHzQks5urKFNgaJpZM4X3BXr> .

-- --Guido (mobile)

msullivan requested a review from ilevkivskyi October 24, 2018 02:46

ilevkivskyi reviewed Oct 25, 2018

View reviewed changes

ilevkivskyi approved these changes Oct 25, 2018

View reviewed changes

JukkaL reviewed Oct 31, 2018

View reviewed changes

msullivan added 16 commits November 1, 2018 19:25

an initial attempt

66a0b84

Try #2, with less FuncDef

d394829

Another test

851073e

Add plugin dependency scheme

9981a61

Produce less drastic rechecks

7f3d389

Get rid of the promote_generated unfortunateness

c21c4f0

Support attrs also

488b6e8

Clear plugin_deps in strip

78eb4f6

remove some unused imports

0cc5cc0

fix tests and lint

34f0153

Eliminate the manual wildcard triggering

64634a3

Make add_method "public"

adfdf6b

Make attrs use add_method

9f74d4c

Make some of the tests have another update

4c47f49

lint.

5e6c15b

doc tweak, tests

f8d531e

msullivan force-pushed the plugin-daemon branch from 5151ba4 to f8d531e Compare November 2, 2018 02:34

msullivan merged commit dc6fcca into master Nov 2, 2018

msullivan deleted the plugin-daemon branch November 2, 2018 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix daemon support for our built-in plugins #5828

Fix daemon support for our built-in plugins #5828

Uh oh!

msullivan commented Oct 24, 2018

Uh oh!

ilevkivskyi commented Oct 25, 2018

Uh oh!

ilevkivskyi left a comment

Uh oh!

ilevkivskyi Oct 25, 2018

Uh oh!

msullivan Oct 25, 2018

Uh oh!

ilevkivskyi Oct 25, 2018

Uh oh!

msullivan Oct 25, 2018

Uh oh!

JukkaL Oct 31, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilevkivskyi left a comment

Uh oh!

JukkaL left a comment

Uh oh!

JukkaL Oct 31, 2018

Uh oh!

JukkaL Oct 31, 2018

Uh oh!

JukkaL Oct 31, 2018

Uh oh!

gvanrossum commented Nov 4, 2018 via email

Uh oh!

Uh oh!

Uh oh!

Fix daemon support for our built-in plugins #5828

Fix daemon support for our built-in plugins #5828

Uh oh!

Conversation

msullivan commented Oct 24, 2018

Uh oh!

ilevkivskyi commented Oct 25, 2018

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

JukkaL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Nov 4, 2018 via email

Uh oh!

Uh oh!