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 common plugin API available for all hooks #6044

Merged
merged 11 commits into from Dec 13, 2018

Conversation

Projects
None yet
4 participants
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi commented Dec 9, 2018

There is a problem with get_base_class_hook(): only the full name of the base class is passed to it. This causes problems when this hook needs to interact with other hooks. In normal mode this can be solved by using a global plugin state, but this doesn't work in incremental mode, since this plugin state is not stored in cache. There is however already a field that is stored in cache between incremental runs and is free to use by plugins: TypeInfo.metadata.

The problem however is that plugin hooks get only full names and can't access the corresponding TypeInfo. This is a common enough problem that we should add an API to make the nodes referred by full names accessible to the plugin hooks. This is done by adding a common lookup method to the Plugin class.

@ilevkivskyi ilevkivskyi requested review from msullivan , JukkaL and gvanrossum Dec 9, 2018

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Dec 10, 2018

Btw, @mkurnikov you may be interested in this. The pattern with global state (like manager_subclasses) that you use in the Django plugin will not work in incremental mode.

@JukkaL
Copy link
Collaborator

JukkaL left a comment

Is the reason this is required that the generated base class is not stored in the symbol table? Otherwise, you could look up the SymbolNode for the base class using the full name, right?

Have you considered the alternative of making sure that all generated classes are included in the symbol table? I wonder if we there could be other places where we'd want to look up the TypeInfo using fullname. This wouldn't help with those.

@@ -5586,3 +5586,75 @@ python_version=3.6
[out]
[out2]
tmp/a.py:2: error: Incompatible types in assignment (expression has type "int", variable has type "str")

[case testBaseClassPluginHookWorksIncremental]

This comment has been minimized.

@JukkaL

JukkaL Dec 10, 2018

Collaborator

I'd move this to check-custom-plugin.test and move the plugin code to a separate file.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 10, 2018

Author Collaborator

OK, makes sense.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Dec 10, 2018

Is the reason this is required that the generated base class is not stored in the symbol table?

No, the problem is that some user defined classes are special for some plugins, so the plugin can't know the set of names on which it should trigger. Previously, I tried work around this by storing all class names found to be "magic" in a plugin global state, but this doesn't work in incremental mode

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Dec 10, 2018

I wonder if we there could be other places where we'd want to look up the TypeInfo using fullname. This wouldn't help with those.

Hm, indeed, get_method_hook, get_method_signature_hook, get_attribute_hook, and even get_function_hook (because of generated __init__s that may be imprecise/incomplete after semantic analysis) will benefit from this (potentially also get_type_analyze_hook). But the problem with get_function_hook and get_type_analyze_hook is not clear what TypeInfo to pass there. A more global solution is to update all plugin hooks with a second argument: lookup_fully_qualified. This is however a (horribly) backwards incompatible change.

@JukkaL

This comment has been minimized.

Copy link
Collaborator

JukkaL commented Dec 10, 2018

A more global solution is to update all plugin hooks with a second argument: lookup_fully_qualified.

Sorry, I don't understand this. What's the problem with using the api attribute? Semantic analyzer already has this, and we could add lookup_typeinfo or similar to the checker API.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Dec 10, 2018

Sorry, I don't understand this. What's the problem with using the api attribute? Semantic analyzer already has this, and we could add lookup_typeinfo or similar to the checker API.

Initially we pass only full name to various get_xxx methods, and the plugin needs to decide whether to act on this name or not. A plugin can of course accept all full names, and then decide later using the full context, but then the whole idea of this first call loses all sense.

@gvanrossum
Copy link
Member

gvanrossum left a comment

Lg apart from Jukka’s suggestion.

Ivan Levkivskyi added some commits Dec 11, 2018

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Dec 11, 2018

@JukkaL I updated the PR as we discussed. Also as you proposed, I will add the docs in a separate PR.

@msullivan Could you please check that I didn't break anything mypyc-related?

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Dec 11, 2018

Also @msullivan now that default plugin implementation moved out of plugin.py do we still need to compile this module? Maybe we can just leave it interpreted and remove all the related mypyc hacks?

@ilevkivskyi ilevkivskyi changed the title Add base class info hook Add common plugin API available for all hooks Dec 11, 2018

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Dec 11, 2018

(Updated the title and PR description.)

@ilevkivskyi ilevkivskyi referenced this pull request Dec 11, 2018

Merged

Add plugin docs #6057

@msullivan

This comment has been minimized.

Copy link
Collaborator

msullivan commented Dec 12, 2018

@msullivan Could you please check that I didn't break anything mypyc-related?

Seems good.

@msullivan

This comment has been minimized.

Copy link
Collaborator

msullivan commented Dec 12, 2018

Also @msullivan now that default plugin implementation moved out of plugin.py do we still need to compile this module? Maybe we can just leave it interpreted and remove all the related mypyc hacks?

IIRC, the main rationale for these hacks was performance. It allows the default modules to be called more efficiently than if the module interface was interpreted.

@JukkaL
Copy link
Collaborator

JukkaL left a comment

I think that this adds additional tech debt which we should be able to avoid pretty easily. See my comment below.

@@ -3568,6 +3568,20 @@ def lookup_qualified(self, name: str) -> SymbolTableNode:
msg = "Failed qualified lookup: '{}' (fullname = '{}')."
raise KeyError(msg.format(last, name))

def lookup_fully_qualified_or_none(self, fullname: str) -> Optional[SymbolTableNode]:

This comment has been minimized.

@JukkaL

JukkaL Dec 12, 2018

Collaborator

I'd rather not add yet another lookup function, even if we are planning to refactor this soon. Can you move this to a new module, such as mypy.lookup? This can be a top-level function that accepts a modules dictionary and fullname. You can then pass the modules dictionary to the plugin, and the plugin can call this function (also the semantic analyzer can call the new function).

Passing the modules dictionary to plugins would have a few additional benefits: it only needs to be done once per build, and the plugins would be given less access to semantic analyzer and type checker internals.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 12, 2018

Author Collaborator

Yes, this makes sense. I incidentally pushed before fixing some things, I will one more commit soon.

Ivan Levkivskyi added some commits Dec 12, 2018

Ivan Levkivskyi
Ivan Levkivskyi
Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Dec 12, 2018

@JukkaL OK, I updated the PR as you suggested. I took the implementation of lookup_fully_qualified() from fixup.py (it works well with both packages and nested classes), and added some comments and a docstring. Now this same function is used in fixup.py, plugin.py, and semanal.py. I didn't touch the one in checker.py because it is subtly different. Also didn't touch other lookup methods, we can do this separately.

I will update the docs PR later today.

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Dec 12, 2018

@msullivan

IIRC, the main rationale for these hacks was performance. It allows the default modules to be called more efficiently than if the module interface was interpreted.

Can this somehow be automated in mypyc? Updating both WrappedPlugin and InterpretedPlugin in addition to Plugin and ChainedPlugin is easy to forget.

@@ -47,6 +50,21 @@ def analyze_callable_args(self, arglist: TypeList) -> Optional[Tuple[List[Type],
('api', TypeAnalyzerPluginInterface)])


@trait
class CommonPluginApi:

This comment has been minimized.

@JukkaL

JukkaL Dec 13, 2018

Collaborator

Is this class needed any more?

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 13, 2018

Author Collaborator

I like to have it for documentation purpose, to separate thing that should be overridden (like get_xxx_hook() methods) from things that can be used (like options and lookup_fully_qualified()). We can however remove it if you don't like it.

# TODO: gradually move existing lookup functions to this module.


def lookup_fully_qualified(name: str, modules: Dict[str, MypyFile],

This comment has been minimized.

@JukkaL

JukkaL Dec 13, 2018

Collaborator

Can you verify that this new lookup method doesn't cause a significant slowdown using compiled mypy? The original method was simpler and if there is a performance impact, it may make sense to keep the original method as an alternative/optimization (but carefully document it as such).

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Dec 13, 2018

Author Collaborator

I decided to keep the old look-up function in semanal.py anyway. It doesn't really belong to this PR. We can make a separate PR that cleans up all lookup related things.

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Dec 13, 2018

@JukkaL @gvanrossum are you happy with the PR now?

@gvanrossum
Copy link
Member

gvanrossum left a comment

LG now.

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

ilevkivskyi commented Dec 13, 2018

AppVeyor build succeeded but somehow still shown as running here, I am going to merge now.

@ilevkivskyi ilevkivskyi merged commit a26b4df into python:master Dec 13, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:add-new-hook branch Dec 13, 2018

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