Skip to content
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 type checking plugin support for functions #3299

Merged
merged 3 commits into from
May 25, 2017
Merged

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented May 2, 2017

The plugins allow implementing special-case logic for
inferring the return type of certain functions with
tricky signatures such as open in Python 3.

Include plugins for open and contextlib.contextmanager.

Some design considerations:

  • The plugins have direct access to mypy internals. The
    idea is that most plugins will be included with mypy
    so mypy maintainers can update the plugins as needed.

  • User-maintained plugins are currently not supported but
    could be added in the future. However, the intention is
    to not have a stable plugin API, at least initially.
    User-maintained plugins would have to track mypy internal
    API changes. Later on, we may decide to provide a more
    stable API if there seems to be a significant need. The
    preferred way would still be to keep plugins in the
    mypy repo.

Work towards #2337.

The plugins allow implementing special-case logic for
inferring the return type of certain functions with
tricky signatures such as `open` in Python 3.

Include plugins for `open` and `contextlib.contextmanager`.

Some design considerations:

- The plugins have direct access to mypy internals. The
  idea is that most plugins will be included with mypy
  so mypy maintainers can update the plugins as needed.

- User-maintained plugins are currently not supported but
  could be added in the future. However, the intention is
  to not have a stable plugin API, at least initially.
  User-maintained plugins would have to track mypy internal
  API changes. Later on, we may decide to provide a more
  stable API if there seems to be a significant need. The
  preferred way would still be to keep plugins in the
  mypy repo.
@JukkaL
Copy link
Collaborator Author

JukkaL commented May 2, 2017

Note that this should not be merged yet, since this exposes some new errors in internal Dropbox codebases.

@gvanrossum
Copy link
Member

Should we use a different term than "plugins" given that these are more an internal mechanism for special-casing than an way for users to special-case their own functions? I guess "extensions" is already taken by another mypy-specific dimension...

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like too much of a hack. I also wonder what would happen with various error cases.

formal_arg_types = [None] * num_formals # type: List[Optional[Type]]
for formal, actuals in enumerate(formal_to_actual):
for actual in actuals:
formal_arg_types[formal] = arg_types[actual]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't there some edge cases where map_actuals_to_formals() returns overlapping mappings? (IIRC related to *args and worse.)

for actual in actuals:
formal_arg_types[formal] = arg_types[actual]
return self.function_plugins[fullname](
formal_arg_types, inferred_ret_type, args, self.chk.named_generic_type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the plugins have access to the Errors object too?

args: List[Expression],
named_generic_type: Callable[[str, List[Type]], Type]) -> Type:
"""Infer a better return type for 'contextlib.contextmanager'."""
arg_type = arg_types[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would fail if there were no arguments, right? (Which would give some other error but might still get here?)

with f('') as s:
reveal_type(s)
[out]
_program.py:13: error: Revealed type is 'def (x: builtins.int) -> contextlib.GeneratorContextManager[builtins.str*]'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that that class is misnamed in typeshed, it should be _GeneratorContextManager (to match what it's called at runtime). I also don't understand what its __call__ method is for (contextlib doesn't seem to have reference docs, and the source has few clues).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The __call__ is so that @contextmanager-decorated functions can also be used as decorators themselves (executing the decorated function within the context). Nick Coghlan has said that he considers this feature a design mistake in contextlib.

@sixolet
Copy link
Collaborator

sixolet commented May 3, 2017

The API for a plugin seems to take the formal argument types (subject to @gvanrossum's commentary), and also the actual arguments, but not the formal-to-actual mapping? That seems like it'd make using the actual arguments a little rough, if people were doing any kind of passing arguments you don't expect by name in an order you don't expect. I feel like it should get all the necessary information, in some kind of normalized form.

(Outside this diff, we should refactor to have an easier-to-understand package that represents a mapping of actual to formal args)

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well.

@ilevkivskyi ilevkivskyi merged commit 53879ef into master May 25, 2017
@ilevkivskyi ilevkivskyi deleted the function-plugins branch May 25, 2017 07:15
@gvanrossum
Copy link
Member

gvanrossum commented May 26, 2017

We forgot to check this against our internal codebases. There are a few errors. I haven't found a pattern yet but the error message is something like Argument N to "method" of "Class" has incompatible type "Foo"; expected "Bar" -- and this appears in a with statement so I suspect there's something wrong with contextmanager_callback()...

@ddfisher
Copy link
Collaborator

I took a closer look, and it seems like the internal errors are all legitimate (except for a couple cases of Cannot infer type of lambda which are ambiguous). Looks like this is catching some real stuff!

carljm added a commit to carljm/mypy that referenced this pull request May 30, 2017
* master: (23 commits)
  Make return type of open() more precise (python#3477)
  Add test cases that delete a file during incremental checking (python#3461)
  Parse each format-string component separately (python#3390)
  Don't warn about returning Any if it is a proper subtype of the return type (python#3473)
  Add __setattr__ support (python#3451)
  Remove bundled lib-typing (python#3337)
  Move version of extensions to post-release (python#3348)
  Fix None slice bounds with strict-optional (python#3445)
  Allow NewType subclassing NewType. (python#3465)
  Add console scripts (python#3074)
  Fix 'variance' label.
  Change label for variance section to just 'variance' (python#3429)
  Better error message for invalid package names passed to mypy (python#3447)
  Fix last character cut in html-report if file does not end with newline (python#3466)
  Print pytest output as it happens (python#3463)
  Add mypy roadmap (python#3460)
  Add flag to avoid interpreting arguments with a default of None as Optional (python#3248)
  Add type checking plugin support for functions (python#3299)
  Mismatch of inferred type and return type note (python#3428)
  Sync typeshed (python#3449)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants