New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix daemon support for our built-in plugins #5828

Merged
merged 16 commits into from Nov 2, 2018

Conversation

Projects
None yet
4 participants
@msullivan
Copy link
Collaborator

msullivan commented Oct 24, 2018

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.

@msullivan msullivan requested a review from ilevkivskyi Oct 24, 2018

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Oct 25, 2018

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
Copy link
Collaborator

ilevkivskyi left a comment

Generally looks good, here are some high level comments.

@@ -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)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 25, 2018

Collaborator

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).

This comment has been minimized.

@msullivan

msullivan Oct 25, 2018

Collaborator

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 25, 2018

Collaborator

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

This comment has been minimized.

@msullivan

msullivan Oct 25, 2018

Collaborator

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

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

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.

Show resolved Hide resolved mypy/plugins/attrs.py Outdated
Show resolved Hide resolved test-data/unit/deps.test Outdated
Show resolved Hide resolved test-data/unit/fine-grained.test
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

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

@JukkaL
Copy link
Collaborator

JukkaL left a comment

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

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

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

Document this attribute in the docstring.

@@ -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)

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

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.

@@ -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]

This comment has been minimized.

@JukkaL

JukkaL Oct 31, 2018

Collaborator

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.

@msullivan msullivan force-pushed the plugin-daemon branch from 5151ba4 to f8d531e Nov 2, 2018

@msullivan msullivan merged commit dc6fcca into master Nov 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@msullivan msullivan deleted the plugin-daemon branch Nov 2, 2018

@gvanrossum

This comment has been minimized.

Copy link
Member

gvanrossum commented Nov 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment