Add Class Decorator/Metaclass/Base Class plugin #4328

euresti · 2017-12-06T00:40:01Z

Work towards #2088

Also use the plugin to demo adding __init__ to attr.s Helps python#2088

euresti · 2017-12-06T03:53:59Z

mypy/plugin.py

+    has_default = {}  # type: Dict[str, Expression]
+    args = []
+
+    for name, table in info.names.items():


Just discovered this is overly broad, but you get the idea.

ilevkivskyi

Sorry for a long delay! This looks good for a start. You can start adding tests to show how it works with attrs. Also could you please fix lint failures and mypy self check? (see Travis logs)

ilevkivskyi · 2017-12-10T23:18:39Z

mypy/plugin.py

+    """Interface for accessing semantic analyzer functionality in plugins."""
+
+    @abstractmethod
+    def named_type(self, qualified_name: str, args: Optional[List[Type]] = None) -> Instance:


Is this the only method that would be potentially used by plugins? Do we need to add some more?

My current explorations also need parse_bool and anal_type

fail could be useful too. Should I add them as needed or add them now?

Should I add them as needed or add them now?

I am not sure why we actually need this class. Can't we just pass everything? Or am I missing something?

I'm following the pattern of the other plugins. Technically the object that's passed in is the full object. I believe this base class is here to limit what methods the plugins should be calling. i.e. if the method isn't listed here then the mypy checker will complain about it.

Then we probably need to think what to add here. My expectation is that many methods of SemanticAnalyzer may be useful. You can play more with your implementation for attrs and add here whatever you ever used.

I think we can keep only these methods, and then add more in your PR with the actual plugin (if necessary).

Here is why we are using an ABC instead exposing the entire semantic analyzer interface:

By only giving access to a subset of an interface, we declare the public part of the semantic analyzer interface. This has a few benefits related to modularity and loose coupling:

Everything else in the semantic analyzer can be more freely modified without breaking plugins. If we change the public interface, we may break 3rd party plugins. (Currently the plugin interface is still not officially supported so we can change things without worries, but I'd like to make the interface more stable in the future.)

It's less likely that plugins will rely on internal implementation details that are likely to change, or on poorly thought-out internal APIs that are hard to use correctly.

It's easier to reason about plugins in general, as the plugins are expected to stick to the exposed public interface instead of freely accessing mypy internals.

This can avoid a cyclic import dependency (maybe not right now, but it will help untangle cyclic dependencies). They slow down incremental checking.

ilevkivskyi · 2017-12-10T23:28:31Z

mypy/plugin.py

+
+    for name, table in info.names.items():
+        if table.type:
+            var = Var(name.lstrip("_"), table.type)


Variable name table is a bit misleading, I would call this symbol or similar.

ilevkivskyi · 2017-12-10T23:31:31Z

mypy/plugin.py

+    args = []
+
+    for name, table in info.names.items():
+        if table.type:


I think this should be treated differently. You should look at the right hand sides for assignments in the class and take those that have attr.ib (note however, that I am not an expert in attrs so I would like to see many tests for this reviewed by an attrs maintainer).

If possible I'd like to remove all this code from the PR. The actual plugin will require possibly more thought. Is it weird to just have a commit that creates a plugin but doesn't actually use it?

Also it turns out I can't actually use a plugin unless attr.pyi exists. So I'd have to figure out how to bring in python-attrs/attrs#238

If possible I'd like to remove all this code from the PR. The actual plugin will require possibly more thought. Is it weird to just have a commit that creates a plugin but doesn't actually use it?

It is totally OK. I would say it is even preferable to split this PR in two parts:

new plugin API

actual attrs plugin (with tests)

ilevkivskyi · 2017-12-10T23:52:08Z

mypy/semanal.py

+                    hook = self.plugin.get_class_base_hook(type_info.type.fullname())
+                    if hook:
+                        hook(ClassDefContext(defn, self))
+


I would factor out this whole block into a separate method, and call it here like self.apply_plugin_hooks(defn) or similar.

Sounds good. Will do.

euresti · 2017-12-11T21:14:07Z

All right. I cleaned up the code and everything passes. I removed the actual plugin to implement in a different PR.

ilevkivskyi · 2017-12-11T22:28:32Z

mypy/semanal.py

+        """Apply a plugin hook that may infer a more precise definition for a class."""
+        def get_fullname(expr: Expression) -> Optional[str]:
+            # We support @foo.bar(...) @foo.bar and @bar
+            # TODO: Support IndexExpressions?


IndexExpression is prohibited in decorators by the Python parser.

But not base classes which also uses this.

For base classes yes, this is important, since they can be generic.

euresti · 2017-12-12T19:36:04Z

Added correct base class support ... broke the world. Sigh.

ilevkivskyi · 2017-12-12T19:44:49Z

mypy/semanal.py

+            sym = self.lookup_qualified(base.name, base)
+            if sym is None or sym.node is None:
+                continue
+            base_name = sym.node.fullname()


Why this is more complex than in other cases? I would expect you can just extract the name using get_full_name (if you would add one more case for IndexExpr).

Because of this:

# Base class expressions (not semantically analyzed -- can be arbitrary expressions) base_type_exprs = None # type: List[Expression]

If I ask for the expr.base.fullname of class Foo(Mapping[str, str]) I get Mapping instead of typing.Mapping

ilevkivskyi · 2017-12-12T19:46:25Z

mypy/semanal.py

+            # We support foo.bar(...), foo.bar, and bar.
+            if isinstance(expr, CallExpr):
+                if isinstance(expr.callee, RefExpr):
+                    return expr.callee.fullname


RefExpr can be both NameExpr and MemberExpr. Why do you have the same code for both here, while below you need special casing?

I should probably just recurse.

euresti · 2017-12-13T15:09:20Z

Ok. Tests pass and IndexExpr is parsed. Let me know if this needs anything else.

ilevkivskyi

Thanks! This is almost ready, only some style comments.

ilevkivskyi · 2017-12-13T21:53:42Z

mypy/semanal.py

+        def get_fullname(expr: Expression) -> Optional[str]:
+            if isinstance(expr, CallExpr):
+                return get_fullname(expr.callee)
+            elif isinstance(expr, (MemberExpr, NameExpr)):


Replace (MemberExpr, NameExpr) with RefExpr.

ilevkivskyi · 2017-12-13T22:01:41Z

mypy/semanal.py

+                    return expr.fullname
+
+                # If we don't have a fullname look it up.
+                sym = self.lookup_type_node(expr)


Add a longer comment here explaining that currently base classes are analysed in different manner than other expressions (analysed by exprtotype.py etc.) and therefore some AST nodes doesn't have names set. Maybe also add a TODO item to unify this (this is however low priority since this will require major rewriting with minor benefits).

ilevkivskyi · 2017-12-13T22:03:28Z

mypy/semanal.py

+                sym = self.lookup_type_node(expr)
+                if sym:
+                    return sym.fullname
+            elif isinstance(expr, IndexExpr):


Move this branch after CallExpr so the order will look more logical.

ilevkivskyi · 2017-12-13T22:05:35Z

mypy/plugin.py

+    """Interface for accessing semantic analyzer functionality in plugins."""
+
+    @abstractmethod
+    def named_type(self, qualified_name: str, args: Optional[List[Type]] = None) -> Instance:


I think we can keep only these methods, and then add more in your PR with the actual plugin (if necessary).

ilevkivskyi · 2017-12-13T22:06:35Z

mypy/semanal.py

+            elif isinstance(expr, (MemberExpr, NameExpr)):
+                if expr.fullname:
+                    return expr.fullname
+


I think this empty line is not needed here.

ilevkivskyi · 2017-12-13T22:25:04Z

mypy/plugin.py

+                                 ) -> Optional[Callable[[ClassDefContext], None]]:
+        return None
+
+    def get_class_metaclass_hook(self, fullname: str


I think you can call this just get_metaclass_hook.

euresti · 2017-12-13T23:02:24Z

mypy/plugin.py

+                           ) -> Optional[Callable[[ClassDefContext], None]]:
+        return None
+
+    def get_class_base_hook(self, fullname: str


get_base_class_hook maybe?

get_base_class_hook maybe?

As you prefer, both are fine with me.

ilevkivskyi

I am going to merge this soon, unless there are some objections.

gvanrossum · 2017-12-14T00:04:16Z

None from me, Jukka will let you know when he's up. :-)

JukkaL

Since the plugins are run in the second pass of semantic analysis, I wonder how they will interact with things that happen later during semantic analysis? This might affect forward references at least. It's worth testing this at some point (and maybe writing test cases -- see below). This can happen in a later PR, but it's worth creating an issue to track this.

It's possible to write tests focused on the plugin architecture. It would be nice to have at least a few tests not specific to any real plugin. The file test-data/unit/check-custom-plugin.test has the existing test cases. This can happen in a separate PR.

Maybe rename AnalyzerPluginInterface to TypeAnalyzerPluginInterface to make it clear how it's different from the new ABC?

And sorry for the late minute review!

ilevkivskyi · 2017-12-14T11:46:35Z

@JukkaL

And sorry for the late minute review!

No problem, thanks! I have opened #4363 to track the tests.

I wonder how they will interact with things that happen later during semantic analysis? This might affect forward references at least.

Yes, it makes to have tests specifically for forward references. I created a separate issue for this #4364

@euresti
Could you please make this renaming?

Maybe rename AnalyzerPluginInterface to TypeAnalyzerPluginInterface to make it clear how it's different from the new ABC?

euresti · 2017-12-14T17:24:19Z

Renamed!

euresti added 3 commits December 5, 2017 16:37

RFC: Add Class Decorator/Metaclass/Base Class plugin

b84d891

Also use the plugin to demo adding __init__ to attr.s Helps python#2088

Fix types

c64cd48

Fix old pythons

f9e049d

euresti commented Dec 6, 2017

View reviewed changes

ilevkivskyi reviewed Dec 10, 2017

View reviewed changes

euresti added 2 commits December 11, 2017 11:04

Allow MemberExpr

757efc8

CR, also remove attrs code

c01352e

euresti changed the title ~~RFC: Add Class Decorator/Metaclass/Base Class plugin~~ Add Class Decorator/Metaclass/Base Class plugin Dec 11, 2017

ilevkivskyi reviewed Dec 11, 2017

View reviewed changes

euresti added 2 commits December 12, 2017 11:00

Support base expressions correctly

819b5ba

Comment tweak

1e5ff80

ilevkivskyi reviewed Dec 12, 2017

View reviewed changes

euresti added 2 commits December 12, 2017 22:10

Simplify IndexExpr lookup

da9224c

Remove comment

a91559a

ilevkivskyi mentioned this pull request Dec 13, 2017

Type for heterogeneous dictionaries with string keys python/typing#28

Closed

Fix tests

9981756

ilevkivskyi reviewed Dec 13, 2017

View reviewed changes

CR

1a82e2d

euresti commented Dec 13, 2017

View reviewed changes

get_base_class_hook

8ba4932

ilevkivskyi approved these changes Dec 13, 2017

View reviewed changes

JukkaL reviewed Dec 14, 2017

View reviewed changes

ilevkivskyi mentioned this pull request Dec 14, 2017

Add more test for plugin infrastructure #4363

Open

AnalyzerPluginInterface->TypeAnalyzerPluginInterface

32e6e9c

ilevkivskyi merged commit 29ba885 into python:master Dec 14, 2017

euresti deleted the class_decorator_plugin branch December 27, 2017 04:16

Add Class Decorator/Metaclass/Base Class plugin #4328

Add Class Decorator/Metaclass/Base Class plugin #4328

Conversation

euresti commented Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

ilevkivskyi left a 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

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

euresti commented Dec 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euresti commented Dec 12, 2017

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

euresti commented Dec 13, 2017

ilevkivskyi left a 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

Choose a reason for hiding this comment

ilevkivskyi left a comment

Choose a reason for hiding this comment

gvanrossum commented Dec 14, 2017 via email

JukkaL left a comment

Choose a reason for hiding this comment

ilevkivskyi commented Dec 14, 2017

euresti commented Dec 14, 2017

euresti commented Dec 6, 2017 •

edited

Loading