Refactor plugin system and special case TypedDict get and int.pow #3501

JukkaL · 2017-06-06T14:58:26Z

Implement a general-purpose way of extending type inference of
methods. Also special case TypedDict get and int.__pow__.

This an alternative to #2620 by @rowillia. I borrowed some test
cases from that PR. This PR has a few major differences:

Use the plugin system instead of full special casing.
Don't support d.get('x', {}) as it's not type safe. Once we
have Typed dicts with missing keys #2632 we can add support for this idiom safely.
Code like f = foo.get loses the special casing for get.

Fixes #2612. Work towards #1240.

Prepare for supporting more general plugins, for things other than just functions. The new design also makes it easier to add support for user-defined plugins.

Implement a general-purpose way of extending type inference of methods.

@rowillia

Some of the tests are adapted from #2620 by @rowillia.

gvanrossum

I looked all of this over once, and it looks pretty good! I have a few questions but I think it's nearly done.

gvanrossum · 2017-06-06T21:20:35Z

mypy/checker.py

    def __init__(self, errors: Errors, modules: Dict[str, MypyFile], options: Options,
-                 tree: MypyFile, path: str) -> None:
+                 tree: MypyFile, path: str, plugin: Optional[Plugin] = None) -> None:


Is there a use case for leaving the plugin unspecified?

Hmm probably not. It's trivial to provide an instance of the empty Plugin as the argument in case no plugin functionality is needed. I'll make this non-optional if it doesn't break anything.

gvanrossum · 2017-06-06T21:29:39Z

mypy/checkexpr.py

@@ -362,8 +363,10 @@ def apply_function_plugin(self,
            for actual in actuals:
                formal_arg_types[formal].append(arg_types[actual])
                formal_arg_exprs[formal].append(args[actual])
-        return self.function_plugins[fullname](
-            formal_arg_types, formal_arg_exprs, inferred_ret_type, self.chk.named_generic_type)
+        callback = self.plugin.get_function_hook(fullname)


But it seems odd since the caller also calls get_function_hook(). Why not pass in the callback?

gvanrossum · 2017-06-06T22:11:44Z

mypy/plugin.py

+    return inferred_return_type
+
+
+def int_pow_callback(


The pow() function is also candidate. And float powers are also interesting, since (-3)**0.5 or (-3.0)**0.5 will return a complex (there's a bug in typeshed, it claims that float**float -> float; but presumably in x**y, x is usually not a literal, so it's not easy do do much about this except very conservatively declare that float**float -> complex, which I expect would cause other problems than it solves; maybe float**float -> Any makes sense just like int**int -> Any?).

gvanrossum · 2017-06-06T22:16:06Z

test-data/unit/check-expressions.test

+reveal_type(a**2) # E: Revealed type is 'builtins.int'
+reveal_type(a**(-0)) # E: Revealed type is 'builtins.int'
+reveal_type(a**(-1)) # E: Revealed type is 'builtins.float'
+reveal_type(a**(-2)) # E: Revealed type is 'builtins.float'


The parentheses around the negative number are redundant; you can write a**-2. (Though it's nice to know that you handle parenthesized exponent too, since I presume users might not know the parentheses are redundant.)

I'll remove most of the parentheses but keep one pair to ensure that they are accepted.

gvanrossum · 2017-06-06T22:17:06Z

test-data/unit/check-expressions.test

+reveal_type(a**(-0)) # E: Revealed type is 'builtins.int'
+reveal_type(a**(-1)) # E: Revealed type is 'builtins.float'
+reveal_type(a**(-2)) # E: Revealed type is 'builtins.float'
+reveal_type(a**b) # E: Revealed type is 'Any'


At some point in the future I'd like mypy to do constant propagation too, so this would become int as well. :-)

Yeah, constant propagation for at least simple things like ints, floats and strings shouldn't be hard. Then we'd have to update this test case.

gvanrossum · 2017-06-07T00:08:22Z

test-data/unit/check-typeddict.test

+D = TypedDict('D', {'x': List[int], 'y': int})
+d: D
+reveal_type(d.get('x', [])) # E: Revealed type is 'builtins.list[builtins.int]'
+d.get('x', ['x']) # E: List item 0 has incompatible type "str"


Why is this an error while two lines below is not?

Here mypy first infers type List[int] for [...] based on type context and then checks that all items have compatible types (which they don't). Below the type of a has been inferred previously so the type context is ignored -- but this is actually fine, since the formal argument type is a union that accepts anything. This is perhaps a little unintuitive, but changing this would be hard and it's not really specific to this PR. I'm testing both cases to catch regressions.

OK, is there a issue for the general problem then? I can repro this like this:

from typing import * T = TypeVar('T') def f(a: Union[List[int], T]) -> T: pass f([1]) # OK f(['']) # E: List item 0 has incompatible type "str" a = [''] f(a) # OK

Created #3506 to track this issue.

gvanrossum · 2017-06-07T00:09:25Z

test-data/unit/check-typeddict.test

+d: D
+d.get() # E: No overload variant of "get" of "Mapping" matches argument types []
+d.get('x', 1, 2) # E: No overload variant of "get" of "Mapping" matches argument types [builtins.str, builtins.int, builtins.int]
+reveal_type(d.get('z')) # E: Revealed type is 'builtins.object*'


Could this be None instead, since we know it's not a valid key? Or is there the possibility that extra items are present in a TypedDict? (I can't recall where we ended up with that.)

gvanrossum · 2017-06-07T00:14:06Z

mypy/checkexpr.py

@@ -508,7 +512,7 @@ def check_call(self, callee: Type, args: List[Expression],
                    or (object_type is not None and self.plugin.get_method_hook(callable_name))):
                ret_type = self.apply_function_plugin(
                    arg_types, callee.ret_type, arg_kinds, formal_to_actual,
-                    args, len(callee.arg_types), callable_name, object_type)
+                    args, len(callee.arg_types), callable_name, object_type, context)


Could you add context to the list of arguments in the docstring?

gvanrossum · 2017-06-07T00:24:17Z

test-data/unit/check-expressions.test

@@ -1698,4 +1698,5 @@ reveal_type(a**(-0)) # E: Revealed type is 'builtins.int'
 reveal_type(a**(-1)) # E: Revealed type is 'builtins.float'
 reveal_type(a**(-2)) # E: Revealed type is 'builtins.float'
 reveal_type(a**b) # E: Revealed type is 'Any'
+reveal_type(a.__pow__(2)) # E: Revealed type is 'builtins.int'


Try a.__pow__(-2) too?

gvanrossum · 2017-06-07T00:30:14Z

test-data/unit/check-typeddict.test

+E = TypedDict('E', {'d': D})
+p = E(d=D(x=0, y=''))
+reveal_type(p.get('d', {'x': 1, 'y': ''})) # E: Revealed type is 'TypedDict(x=builtins.int, y=builtins.str, _fallback=__main__.D)'
+p.get('d', {}) # E: Expected items ['x', 'y'] but found [].


Wasn't there a use case where people write p.get('d', {}).get('x') and expect int or None?

Yes, but as mentioned in the PR description, handling it safely requires a way of specifying that some TypedDict items may be missing, and we don't have that feature yet. The return type of p.get('d', {}) should be a TypedDict that is like D but where both 'x' and 'y' may be missing. I'll add support for this in a separate PR once we can agree on the syntax (or I can add support for this as an mypy internal feature without public syntax so that missing keys are only generated through type inference).

chadrik · 2017-06-07T17:16:35Z

Here are some thoughts on the user-plugin aspect of this PR. Should we open a new PR for user-facing aspect of the plugin system?

Plugin discovery options

A. by file path. e.g. /path/to/myplugin.py. could also extend this with a MYPY_PLUGIN_PATH
- pro: easier to write test cases (I discovered that placing a file on the PYTHONPATH within the tests was difficult, likely by design)
- con: can't use pip to install plugins
B. by dotted path: e.g. package.module
- pro: easy for users to create pip-installable plugins
- con: adding plugin modules and their requirements to the PYTHONPATH could interfere with type checking?

C: setuptools entry points. e.g.:

setup(
    entry_points={
        'mypy.userplugin': ['my_plugin = my_module.plugin:register_plugin']
    }
)

Plugin chainability options

A. aggregate all user plugins into a single uber-plugin instance.
- each method on this aggregate plugin would cycle through its children in order until one returns a non-None result. we could then cache the mapping from feature to user-plugin instance to speed up future lookups.
- this is compatible with the current design which passes a single Plugin instance around.
B. register a plugin per feature (.e.g 'typing.Mapping.get'). this allows you to replace the search with a fast dictionary lookup, as well as detect up-front at registration time when two plugins contend for the same feature.

gvanrossum · 2017-06-07T17:57:55Z

@JukkaL if you're happy you can merge this now.

gvanrossum · 2017-06-07T18:00:15Z

@chadrik Good thoughts. Could you open a new issue or add this to the general issue about plugins? This PR will soon be merged and I don't want discussion unrelated to the PR itself happening here.

JukkaL · 2017-06-07T18:08:09Z

@chadrik Thanks for the write-up, it's very useful.

JukkaL added 10 commits June 6, 2017 15:37

Refactor plugin system

0c03d3f

Prepare for supporting more general plugins, for things other than just functions. The new design also makes it easier to add support for user-defined plugins.

Special case type checking of TypedDict get and int.__pow__

d82aa69

Implement a general-purpose way of extending type inference of methods.

Fix argument type context for TypedDict get

5661339

Report invalid TypedDict get key argument

753d52c

Add test case that uses typeshed stubs

166a911

Generalize method hook to work with Instances

b025991

TypedDict get tweaks

2cf5cd7

Some of the tests are adapted from #2620 by @rowillia.

Remove commented-out code

abe29a9

Various tweaks

8a240dc

Fix test case

e43d94a

gvanrossum requested changes Jun 7, 2017

View reviewed changes

gvanrossum mentioned this pull request Jun 7, 2017

Pluggable system for generating types from docstrings (revisited) #3225

Closed

Address review feedback

6db7d20

gvanrossum approved these changes Jun 7, 2017

View reviewed changes

JukkaL mentioned this pull request Jun 7, 2017

Inconsistent type inference with Union[List[int], T] context #3506

Open

JukkaL merged commit bcf89b1 into master Jun 7, 2017

ilevkivskyi mentioned this pull request Jun 7, 2017

Implement type-aware get for TypedDict #2620

Closed

ilevkivskyi deleted the general-plugins branch June 13, 2017 07:53

ilevkivskyi mentioned this pull request Jun 13, 2017

Assignment of Any does not get rid of Optional (add AnyUnion type?) #3526

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor plugin system and special case TypedDict get and int.pow #3501

Refactor plugin system and special case TypedDict get and int.pow #3501

JukkaL commented Jun 6, 2017

gvanrossum left a comment

gvanrossum Jun 6, 2017

JukkaL Jun 7, 2017

gvanrossum Jun 6, 2017

gvanrossum Jun 6, 2017

gvanrossum Jun 6, 2017

JukkaL Jun 7, 2017

gvanrossum Jun 6, 2017

JukkaL Jun 7, 2017

gvanrossum Jun 7, 2017

JukkaL Jun 7, 2017

gvanrossum Jun 7, 2017 •

edited

Loading

JukkaL Jun 7, 2017

gvanrossum Jun 7, 2017

gvanrossum Jun 7, 2017

gvanrossum Jun 7, 2017

gvanrossum Jun 7, 2017

JukkaL Jun 7, 2017

chadrik commented Jun 7, 2017

gvanrossum commented Jun 7, 2017

gvanrossum commented Jun 7, 2017

JukkaL commented Jun 7, 2017

Refactor plugin system and special case TypedDict get and int.__pow__ #3501

Refactor plugin system and special case TypedDict get and int.__pow__ #3501

Conversation

JukkaL commented Jun 6, 2017

gvanrossum 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

gvanrossum Jun 7, 2017 • edited Loading

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

chadrik commented Jun 7, 2017

Plugin discovery options

Other questions

Plugin chainability options

gvanrossum commented Jun 7, 2017

gvanrossum commented Jun 7, 2017

JukkaL commented Jun 7, 2017

Refactor plugin system and special case TypedDict get and int.pow #3501

Refactor plugin system and special case TypedDict get and int.pow #3501

gvanrossum Jun 7, 2017 •

edited

Loading