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

Add Class Decorator/Metaclass/Base Class plugin #4328

Merged
merged 13 commits into from Dec 14, 2017

Conversation

Projects
None yet
4 participants
@euresti
Contributor

euresti commented Dec 6, 2017

Work towards #2088

euresti added some commits Dec 4, 2017

RFC: Add Class Decorator/Metaclass/Base Class plugin
Also use the plugin to demo adding __init__ to attr.s

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

This comment has been minimized.

@euresti

euresti Dec 6, 2017

Contributor

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)

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 10, 2017

Collaborator

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

This comment has been minimized.

@euresti

euresti Dec 11, 2017

Contributor

My current explorations also need parse_bool and anal_type

This comment has been minimized.

@euresti

euresti Dec 11, 2017

Contributor

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 11, 2017

Collaborator

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?

This comment has been minimized.

@euresti

euresti Dec 11, 2017

Contributor

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 11, 2017

Collaborator

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 13, 2017

Collaborator

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

This comment has been minimized.

@JukkaL

JukkaL Dec 14, 2017

Collaborator

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.
for name, table in info.names.items():
if table.type:
var = Var(name.lstrip("_"), table.type)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 10, 2017

Collaborator

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

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 10, 2017

Collaborator

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

This comment has been minimized.

@euresti

euresti Dec 11, 2017

Contributor

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 11, 2017

Collaborator

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)
hook = self.plugin.get_class_base_hook(type_info.type.fullname())
if hook:
hook(ClassDefContext(defn, self))

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 10, 2017

Collaborator

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

This comment has been minimized.

@euresti

euresti Dec 11, 2017

Contributor

Sounds good. Will do.

@euresti

This comment has been minimized.

Contributor

euresti commented Dec 11, 2017

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

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

"""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?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 11, 2017

Collaborator

IndexExpression is prohibited in decorators by the Python parser.

This comment has been minimized.

@euresti

euresti Dec 11, 2017

Contributor

But not base classes which also uses this.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 11, 2017

Collaborator

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

@euresti

This comment has been minimized.

Contributor

euresti commented Dec 12, 2017

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

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 12, 2017

Collaborator

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

This comment has been minimized.

@euresti

euresti Dec 12, 2017

Contributor

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

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 12, 2017

Collaborator

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

This comment has been minimized.

@euresti

euresti Dec 12, 2017

Contributor

I should probably just recurse.

euresti added some commits Dec 13, 2017

@euresti

This comment has been minimized.

Contributor

euresti commented Dec 13, 2017

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.

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 13, 2017

Collaborator

Replace (MemberExpr, NameExpr) with RefExpr.

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 13, 2017

Collaborator

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

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 13, 2017

Collaborator

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

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 13, 2017

Collaborator

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

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 13, 2017

Collaborator

I think this empty line is not needed here.

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 13, 2017

Collaborator

I think you can call this just get_metaclass_hook.

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

This comment has been minimized.

@euresti

euresti Dec 13, 2017

Contributor

get_base_class_hook maybe?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 13, 2017

Collaborator

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

This comment has been minimized.

Member

gvanrossum commented Dec 14, 2017

@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

This comment has been minimized.

Collaborator

ilevkivskyi commented Dec 14, 2017

@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

This comment has been minimized.

Contributor

euresti commented Dec 14, 2017

Renamed!

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

2 checks passed

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

@euresti euresti deleted the euresti:class_decorator_plugin branch Dec 27, 2017

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