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

Support additional plugin hooks #3534

Merged
merged 11 commits into from Jun 21, 2017

Conversation

Projects
None yet
3 participants
@JukkaL
Collaborator

JukkaL commented Jun 13, 2017

Includes these features:

  • Add hook that overrides the inferred type of an instance attribute.
    In particular, this can be used to override the type of __call__.
  • Add hook for custom semantic analysis of types. This makes it possible
    to have some limited custom syntax for generic types. (We can extend
    the syntactic possibilities in the future.)
  • Use the name of a callable to decide which hook to apply. This makes
    it possible to use custom hooks for callables returned by functions.

These hooks are needed for some internal Dropbox use cases.

Support additional plugin hooks
Includes these features:

* Add hook that overrides the inferred type of an instance attribute.
  In particular, this can be used to override the type of `__call__`.
* Add hook for custom semantic analysis of types. This makes it possible
  to have some limited custom syntax for generic types. (We can extend
  the syntactic possibilities in the future.)
* Use the name of a callable to decide which hook to apply. This makes
  it possible to use custom hooks for callables returned by functions.
Show outdated Hide outdated mypy/build.py
source_set = BuildSourceSet(sources)
errors = Errors(options.show_error_context, options.show_column_numbers)
plugin = load_custom_plugins(DefaultPlugin(options.python_version), options, errors)

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Hm, I wonder if load_custom_plugins() shouldn't also be responsible for constructing the DefaultPlugin instance. And maybe the latter would also want to have access to the options and errors? Makes for a more universal plugin API.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Hm, I wonder if load_custom_plugins() shouldn't also be responsible for constructing the DefaultPlugin instance. And maybe the latter would also want to have access to the options and errors? Makes for a more universal plugin API.

This comment has been minimized.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Good ideas -- updated.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Good ideas -- updated.

@@ -430,10 +430,11 @@ def __init__(self, data_dir: str,
reports: Reports,
options: Options,
version_id: str,
plugin: Plugin) -> None:
plugin: Plugin,
errors: Errors) -> None:

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Add this to the docstring too.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Add this to the docstring too.

This comment has been minimized.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

It was mentioned already, but moved to another place where it might be easier to find.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

It was mentioned already, but moved to another place where it might be easier to find.

This comment has been minimized.

@gvanrossum

gvanrossum Jun 20, 2017

Member

In some future PR maybe we can remove the attributes from the docstring and instead use class-level annotations + comments for them.

@gvanrossum

gvanrossum Jun 20, 2017

Member

In some future PR maybe we can remove the attributes from the docstring and instead use class-level annotations + comments for them.

Show outdated Hide outdated mypy/checkmember.py
@@ -76,8 +77,9 @@ def analyze_member_access(name: str,
if method.is_property:
assert isinstance(method, OverloadedFuncDef)
first_item = cast(Decorator, method.items[0])
plugin = chk.plugin if chk is not None else None

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

I don't think chk can ever be None. The type should probably be adjusted.

@gvanrossum

gvanrossum Jun 13, 2017

Member

I don't think chk can ever be None. The type should probably be adjusted.

This comment has been minimized.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Updated

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Updated

Show outdated Hide outdated mypy/checkmember.py
else:
result = t
else:
result = t

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Might be simpler to pre-initialize result = t before entering the if matching the else on the previous line?

@gvanrossum

gvanrossum Jun 13, 2017

Member

Might be simpler to pre-initialize result = t before entering the if matching the else on the previous line?

This comment has been minimized.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Done

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Done

Show outdated Hide outdated mypy/plugin.py
[
('named_instance', NamedInstanceCallback),
('msg', MessageBuilder),
('context', Context)

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Honestly I'm no fan of this spread-out way of formatting a NamedTuple, but if you do this, at least add a trailing comma.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Honestly I'm no fan of this spread-out way of formatting a NamedTuple, but if you do this, at least add a trailing comma.

This comment has been minimized.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Tightened named tuple formatting a bit.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Tightened named tuple formatting a bit.

Show outdated Hide outdated mypy/plugin.py
@@ -81,6 +117,12 @@ def get_method_signature_hook(self, fullname: str) -> Optional[MethodSignatureHo
def get_method_hook(self, fullname: str) -> Optional[MethodHook]:
return None
def get_attribute_hook(self, fullname: str) -> Optional[AttributeHook]:

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

I'm not sure I like the proliferation of different callback types. Even though these can be statically checked, it seems a pain for users to keep them apart. (Plus, how easy is it really for users to type-check their plugins? There are no stubs for mypy.)

@gvanrossum

gvanrossum Jun 13, 2017

Member

I'm not sure I like the proliferation of different callback types. Even though these can be statically checked, it seems a pain for users to keep them apart. (Plus, how easy is it really for users to type-check their plugins? There are no stubs for mypy.)

This comment has been minimized.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Redesigned the plugin system to not rely on complex callback types. Now each callback just takes a single argument. The type of the argument depends on the kind of the callback, but I think that it's easy enough to keep track of a single argument type per callback. Callback types are now simple enough to not require type aliases. This also makes it easier to pass more stuff to callbacks in the future, as existing callbacks will likely continue to work, whereas if we'd add new arguments, existing callbacks would break.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Redesigned the plugin system to not rely on complex callback types. Now each callback just takes a single argument. The type of the argument depends on the kind of the callback, but I think that it's easy enough to keep track of a single argument type per callback. Callback types are now simple enough to not require type aliases. This also makes it easier to pass more stuff to callbacks in the future, as existing callbacks will likely continue to work, whereas if we'd add new arguments, existing callbacks would break.

Show outdated Hide outdated mypy/semanal.py
@@ -1534,7 +1539,8 @@ def visit_assignment_stmt(self, s: AssignmentStmt) -> None:
self.lookup_qualified,
self.lookup_fully_qualified,
self.tvar_scope,
self.fail, allow_unnormalized=True)
self.fail,
self.plugin, allow_unnormalized=True)

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Move the keyword option to a new line. (Also below.)

@gvanrossum

gvanrossum Jun 13, 2017

Member

Move the keyword option to a new line. (Also below.)

This comment has been minimized.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Done

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Done

Show outdated Hide outdated mypy/typeanal.py
@@ -453,6 +433,44 @@ def analyze_callable_type(self, t: UnboundType) -> Type:
assert isinstance(ret, CallableType)
return ret.accept(self)
def analyze_callable_args(self, t: TypeList) -> Optional[Tuple[List[Type],

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

Using t for the arg name is a little fishy (especially since t in the code you refactored out was just a Type, and t here corresponds to t.args there). Maybe tl?

@gvanrossum

gvanrossum Jun 13, 2017

Member

Using t for the arg name is a little fishy (especially since t in the code you refactored out was just a Type, and t here corresponds to t.args there). Maybe tl?

This comment has been minimized.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Renamed to arglist.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

Renamed to arglist.

Show outdated Hide outdated mypy/typeanal.py
args.append(arg)
kinds.append(ARG_POS)
names.append(None)
check_arg_names(names, [t] * len(args), self.fail, "Callable")

This comment has been minimized.

@gvanrossum

gvanrossum Jun 13, 2017

Member

The meaning of t here seems different from what it was before the refactor??? What am I missing?

@gvanrossum

gvanrossum Jun 13, 2017

Member

The meaning of t here seems different from what it was before the refactor??? What am I missing?

This comment has been minimized.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

It's valid since it's only used for error context, and TypeList is a valid error context. Each argument gets the same error context which is not optimal, though.

@JukkaL

JukkaL Jun 20, 2017

Collaborator

It's valid since it's only used for error context, and TypeList is a valid error context. Each argument gets the same error context which is not optimal, though.

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 19, 2017

Member

@rkr-at-dbx Can you review?

Member

gvanrossum commented Jun 19, 2017

@rkr-at-dbx Can you review?

JukkaL added some commits Jun 20, 2017

Major redesign of the plugin system
Instead of passing several arguments to hook function, always
pass just a single object. This simplifies the signatures of
hooks.

Instead of passing callback functions to hooks, pass an object
that implements a specific interface.

These changes are intended to make it easier to write plugins,
and to make it easier to evolve the plugin system. Adding
extra attributes to context or extra methods to the internal
interfaces doesn't require changes to existing plugins.
@gvanrossum

I left some comments that are just suggestions or comment/docstring nits -- feel free to address some or none of them and then merge yourself.

Show outdated Hide outdated mypy/build.py
except Exception:
print('Error constructing plugin instance of {}\n'.format(plugin_type.__name__))
raise # Propagate to display traceback
if not custom_plugins:
return default_plugin
if len(plugins) == 1:

This comment has been minimized.

@gvanrossum

gvanrossum Jun 20, 2017

Member

I wonder if the logic to optimize the no-custom-plugins case (by avoiding ChainedPlugin) is needed. Did you find an actual speed difference?

@gvanrossum

gvanrossum Jun 20, 2017

Member

I wonder if the logic to optimize the no-custom-plugins case (by avoiding ChainedPlugin) is needed. Did you find an actual speed difference?

This comment has been minimized.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

I didn't benchmark the code. Removed the special case.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

I didn't benchmark the code. Removed the special case.

Show outdated Hide outdated mypy/build.py
Return a plugin that chains all custom plugins (if any) and falls
back to default_plugin.
Return a plugin that encapsulates all plugins chained together. Always
at least include the default plugin.

This comment has been minimized.

@gvanrossum

gvanrossum Jun 20, 2017

Member

Might want to add that the default plugin comes first in the search order (this is significant because the previous version of the code put it last IIRC).

@gvanrossum

gvanrossum Jun 20, 2017

Member

Might want to add that the default plugin comes first in the search order (this is significant because the previous version of the code put it last IIRC).

This comment has been minimized.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Oops, that wasn't my intent. Changed so that the default plugin comes last again.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Oops, that wasn't my intent. Changed so that the default plugin comes last again.

This comment has been minimized.

@gvanrossum

gvanrossum Jun 21, 2017

Member

So a user plugin can override the default plugin. Should a hook returned by a user plugin be able to fall back dynamically on the default plugin's hook for the same function? E.g. suppose to add a special case for open() I would have to copy the default open() handler and modify it. It would be nice if there was some kind of "super call" mechanism for this.

@gvanrossum

gvanrossum Jun 21, 2017

Member

So a user plugin can override the default plugin. Should a hook returned by a user plugin be able to fall back dynamically on the default plugin's hook for the same function? E.g. suppose to add a special case for open() I would have to copy the default open() handler and modify it. It would be nice if there was some kind of "super call" mechanism for this.

This comment has been minimized.

@JukkaL

JukkaL Jun 22, 2017

Collaborator

Hmm that might be useful, but probably not worth implementing until we have concrete use cases. Created #3591 to track this.

@JukkaL

JukkaL Jun 22, 2017

Collaborator

Hmm that might be useful, but probably not worth implementing until we have concrete use cases. Created #3591 to track this.

@@ -430,10 +430,11 @@ def __init__(self, data_dir: str,
reports: Reports,
options: Options,
version_id: str,
plugin: Plugin) -> None:
plugin: Plugin,
errors: Errors) -> None:

This comment has been minimized.

@gvanrossum

gvanrossum Jun 20, 2017

Member

In some future PR maybe we can remove the attributes from the docstring and instead use class-level annotations + comments for them.

@gvanrossum

gvanrossum Jun 20, 2017

Member

In some future PR maybe we can remove the attributes from the docstring and instead use class-level annotations + comments for them.

@@ -392,17 +392,21 @@ def apply_function_plugin(self,
# Apply function plugin
callback = self.plugin.get_function_hook(fullname)
assert callback is not None # Assume that caller ensures this

This comment has been minimized.

@gvanrossum

gvanrossum Jun 20, 2017

Member

This still feels a bit awkward to me, but I agree that other ways to factor this out aren't much better. Maybe the docstring should just call out that there are two calling cases and that the caller should ensure the relevant callback isn't None?

@gvanrossum

gvanrossum Jun 20, 2017

Member

This still feels a bit awkward to me, but I agree that other ways to factor this out aren't much better. Maybe the docstring should just call out that there are two calling cases and that the caller should ensure the relevant callback isn't None?

This comment has been minimized.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Updated docstring

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Updated docstring

from typing import TypeVar, Generic, Callable
T = TypeVar('T', bound=Callable[..., None])
class Signal(Generic[T]):
__call__: Callable[..., None]

This comment has been minimized.

@gvanrossum

gvanrossum Jun 20, 2017

Member

Add a comment here saying this type is replaced by the plugin?

@gvanrossum

gvanrossum Jun 20, 2017

Member

Add a comment here saying this type is replaced by the plugin?

This comment has been minimized.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Added comment

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Added comment

def signal_call_callback(ctx: AttributeContext) -> Type:
if isinstance(ctx.type, Instance) and ctx.type.type.fullname() == 'm.Signal':
return ctx.type.args[0]
return ctx.inferred_attr_type

This comment has been minimized.

@gvanrossum

gvanrossum Jun 20, 2017

Member

Would there be value in changing the protocol so that if a callback/hook returns None this means it didn't come up with anything and the plugin framework will return the originally inferred type?

@gvanrossum

gvanrossum Jun 20, 2017

Member

Would there be value in changing the protocol so that if a callback/hook returns None this means it didn't come up with anything and the plugin framework will return the originally inferred type?

This comment has been minimized.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

I prefer the current way for these reasons:

  • Signatures are simpler (no Optional[...] return type).
  • The behavior of the hooks is more explicit this way.

However, I renamed inferred_* to default_* in hopes of making this a little friendlier.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

I prefer the current way for these reasons:

  • Signatures are simpler (no Optional[...] return type).
  • The behavior of the hooks is more explicit this way.

However, I renamed inferred_* to default_* in hopes of making this a little friendlier.

from m import Signal
s: Signal[[int, DefaultArg(str, 'x')]] = Signal()
reveal_type(s) # E: Revealed type is 'm.Signal[def (builtins.int, x: builtins.str =)]'
s.x # E: Signal[Callable[[int, str], None]] has no attribute "x"

This comment has been minimized.

@gvanrossum

gvanrossum Jun 20, 2017

Member

IIUC this error message is suboptimal, right? It loses some details for the second arg.

@gvanrossum

gvanrossum Jun 20, 2017

Member

IIUC this error message is suboptimal, right? It loses some details for the second arg.

This comment has been minimized.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Yeah, that's a bug. Created #3581 to track it since it's mostly unrelated to this PR.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Yeah, that's a bug. Created #3581 to track it since it's mostly unrelated to this PR.

Show outdated Hide outdated test-data/unit/plugins/named_callable.py
def get_function_hook(self, fullname):
if fullname == 'm.decorator1':
return decorator_call_hook
if fullname == 'm._decorated':

This comment has been minimized.

@gvanrossum

gvanrossum Jun 20, 2017

Member

Aarg. It took me on a wild goose chase figuring out where this name was defined, until I finally realized it's done in decorator_call_hook() below. :-)

@gvanrossum

gvanrossum Jun 20, 2017

Member

Aarg. It took me on a wild goose chase figuring out where this name was defined, until I finally realized it's done in decorator_call_hook() below. :-)

This comment has been minimized.

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Added comment

@JukkaL

JukkaL Jun 21, 2017

Collaborator

Added comment

@gvanrossum

This comment has been minimized.

Show comment
Hide comment
@gvanrossum

gvanrossum Jun 21, 2017

Member

@rkr-at-dbx: you can just focus on the test(s) involving Signal.

Member

gvanrossum commented Jun 21, 2017

@rkr-at-dbx: you can just focus on the test(s) involving Signal.

@JukkaL JukkaL merged commit 81ec44b into master Jun 21, 2017

3 checks passed

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

@gvanrossum gvanrossum deleted the user-plugins-more-hooks branch Jun 21, 2017

@@ -474,8 +473,9 @@ def __init__(self, data_dir: str,
self.version_id = version_id
self.modules = {} # type: Dict[str, MypyFile]
self.missing_modules = set() # type: Set[str]
self.plugin = plugin

This comment has been minimized.

@ilinum

ilinum Oct 8, 2017

Collaborator

@JukkaL you have self.plugin = plugin twice in the init (on 476 and 485 at the end of the init). Is that on purpose?

@ilinum

ilinum Oct 8, 2017

Collaborator

@JukkaL you have self.plugin = plugin twice in the init (on 476 and 485 at the end of the init). Is that on purpose?

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