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

chadrik · 2017-04-23T00:52:38Z

Hi,
I'm picking up where I left off on #2240.

Here's a bit of a review:

This PR would allow third-party tools to extend mypy with callbacks to parse PEP484 type annotations within function docstrings. To do so, this PR introduces a basic "hook" system, as a pre-cursor to a complete plugin system (issue #1240).

A hook can be registered, along with options specific to that hook, using the mypy configuration file:

[mypy]
fast_parser = true
hooks.docstring_parser = mypydoc.parse_docstring

[docstring_parser]
default_return_type = 'None'

Hook options specified in the config file are passed to the hook as a dictionary.

One big new change from where I left off with #2240 is that the docstring_parser hook is now passed the active mypy.errors.Errors instance so that it can use the full range of error reporting, especially specification of blocker and severity level.

I have concerns about exposing the mypy.errors.Errors class as part of a public API so I was considering writing a simple wrapper that exposes only the bare minimum interface, perhaps something like this:

class Errors:
    _errors = None  # type: errors.Errors

    def __init__(self, errors: errors.Errors):
        self._errors = errors

    def report(self, line: int, column: int, message: str, blocker: bool = False,
               severity: str = 'error', file: str = None, only_once: bool = False,
               origin_line: int = None) -> None:
        return self._errors.report(line, column, message, blocker, severity, file, only_once, origin_line)

What do you think?

Sorry about getting stalled out before (I blamed it on the birth of my second daughter!). Looking forward to getting this wrapped up.

chadrik · 2017-06-05T18:08:20Z

Added tests and improved the structure a bit.

Hooks are now defined using a Hook generic class, for holding the hook function and its options.
A Hooks class holds all known hook types as Hook instances
A Hooks instance is stored on Options

Thus accessing a docstring_parser hook function from an Options instance is done like so: options.hooks.docstring_parser.func

Questions:

Do you like the hooks namespace used on Options and in the mypy.ini file, or should I ditch it?
I wrote a simplified version of pydoc.locate to import and locate an object given a dotted string path. I'd like to use the more complete version in pydoc, but there are no stubs for pydoc in the typeshed. Additionally locate does not appear in pydoc.__all__ so I'm unclear if it would even be appropriate to add it to the stubs if I were to author them. As an aside, I've found pydoc.locate to be incredibly useful (as have quite a few people on stack overflow) and I would love for it to find a more prominent place in the stdlib.
tests are failing in python 3.3 due to use of importlib.spec_from_file_location. Should I use the deprecated functions or is 3.3 support going away soon?
is it ok to use mypy.errors.Error as a public API or would you prefer a simple wrapper to limit the public interface?

Thanks!

ilevkivskyi · 2017-06-05T18:40:47Z

mypy/hooks.py

+
+from typing import Callable, Dict, Generic, Optional, Tuple, TypeVar, TYPE_CHECKING
+
+if TYPE_CHECKING:


You should use MYPY = False and then if MYPY: .... TYPE_CHECKING is not present in older versions of Python.

ilevkivskyi · 2017-06-05T18:41:33Z

@gvanrossum You reviewed the previous attempt, maybe you could review this PR too?

chadrik · 2017-06-06T21:59:05Z

I see that @JukkaL just pushed up a general-plugins branch for #1240. There's a lot of overlap with what I've done so it seems like a good starting place for this PR. The mechanism for registering and loading custom plugins looks like it has not been added yet, so I'm interested to see how that will work: via mypy.ini or something else?

JukkaL · 2017-06-07T14:45:25Z

@chadrik Plugins will be registered through mypy.ini. Once the general plugin framework lands, hopefully it isn't hard to extend it to support plugins that parse docstrings.

gvanrossum · 2017-06-07T15:35:45Z

I'm not sure how to proceed. After #3501 lands maybe we can look at how to add hooks for parsing docstrings using the new Plugin class, but a bunch more machinery needs to be invented first. But I expect that a thorough review of the current PR would be a waste of time, since the plan is to let you define a plugin for this purpose outside the mypy source code.

JukkaL · 2017-06-07T15:47:08Z

Maybe wait until #3501 has landed and there is support for user-defined plugins (in a follow-up PR)? Then we can look at adding docstring parse hooks to the Plugin class. The latter would hopefully be pretty straightforward once the rest of the plugin system is in place.

gvanrossum · 2017-06-07T15:49:25Z

Maybe Chad can help design/write the code for user-defined plugins?

chadrik · 2017-06-07T17:00:23Z

I'd be glad to help out with the user-defined plugins. I have prior experience designing a handful of similar systems. I'll drop some thoughts over in the other PR.

gvanrossum · 2017-06-09T23:46:04Z

In the meantime I hope you don't mind if I close this one (again). Thanks for your suggestions on the dsign of a plugin system, your insights are useful!

chadrik · 2017-06-10T01:14:37Z

@gvanrossum If you don't mind I'd like to keep this open specifically for adding the docstring support once we wrap up the user-plugin feature (#3517). I don't want to complicate that PR by mixing it up with docstrings. Based on the progress in #3517 this won't be lingering for long.

gvanrossum · 2017-06-10T01:27:37Z

OK, np! Though I was under the impression that with a powerful enough plugin system we could have plugins installed from PyPI, so the docstring plugin wouldn't need to be a mypy PR?

chadrik · 2017-06-10T15:58:41Z

I was under the impression that with a powerful enough plugin system we could have plugins installed from PyPI, so the docstring plugin wouldn't need to be a mypy PR?

The closest existing hook to what I would need is the MethodSignatureHook which is given a CallableType and is expected to return a new CallableType with an improved signature. I looked into adding a docstring attribute to CallableType, but here's the problem: a CallableType is not created if no type annotations are found, and the docstrings are intended to be the source of those type annotations, so we have a catch22 / order-of-operations problem.

Docstrings are an alternative to type comments, so it makes sense for a docstring hook to run earlier, at the time that type annotations are extracted from code and turned into Types. That's the approach that I've taken in this PR. When I port it to the plugin system, I plan to add a new hook type and call it in fastparse. That is, unless you see a way to make it work using MethodSignatureHook in checkexpr.

gvanrossum · 2017-06-10T20:04:29Z

Agreed, I had always understood that you need a hook in the parsing stage for this purpose. It's just that the code of the hook (i.e. the actual docstring parsing) doesn't have to be part of mypy, right?

chadrik · 2017-06-10T20:51:12Z

Correct. This PR will be to add the new hook type to the Plugin class and call it at the appropriate place in fastparse.

gvanrossum · 2017-06-10T23:55:15Z

Honestly that sounds like a new PR, but you are free to reuse this one if it's easier for you.

gvanrossum

Waiting for you.

chadrik · 2017-06-14T17:22:56Z

here you go!

gvanrossum · 2017-06-20T20:25:27Z

Heads up: once #3534 lands (specifically 3283cbf) I believe this will require more work, since Jukka is changing the signatures for hooks. Instead of having a separate signature for each hook type, the hooks get passed a specific object which has various useful attributes -- it's easier to add new attributes that way, without having to update all hooks of that type.

gvanrossum · 2017-06-14T17:35:27Z

mypy/fastparse.py



 def parse(source: Union[str, bytes], fnam: str = None, errors: Errors = None,
-          options: Options = Options()) -> MypyFile:
+          options: Options = Options(), plugin: Plugin = Plugin()) -> MypyFile:


There ought to be no reason to provide a default. And the default definitely should not create a Plugin instance that's usually not used.

gvanrossum · 2017-06-14T22:47:50Z

mypy/fastparse.py

@@ -58,10 +59,12 @@

 TYPE_COMMENT_SYNTAX_ERROR = 'syntax error in type comment'
 TYPE_COMMENT_AST_ERROR = 'invalid type comment or annotation'
+TYPE_COMMENT_DOCSTRING_ERROR = ('Arguments parsed from docstring are not '


Maybe 'One or more arguments specified in docstring are not '.

gvanrossum · 2017-06-14T22:49:28Z

mypy/fastparse.py

+        return_type = type_map.pop('return', AnyType())
+        if type_map:
+            errors.report(line, 0,
+                          TYPE_COMMENT_DOCSTRING_ERROR.format(list(type_map), arg_names))


Maybe there also ought to be an error for arguments in the arg list but not in the docstring (given that there's a type map at all).

Also, I think the error message here needn't list the expected arg names (those are easily recovered from the source) but only the list of args in the docstring but not in the arg list.

Maybe there also ought to be an error for arguments in the arg list but not in the docstring (given that there's a type map at all).

I think it's important that docstring annotations can be sparse, as with multi-line python 2 function annotations.

gvanrossum · 2017-06-20T21:05:42Z

mypy/fastparse.py

@@ -345,6 +369,14 @@ def do_func_def(self, n: Union[ast3.FunctionDef, ast3.AsyncFunctionDef],
            return_type = TypeConverter(self.errors, line=n.returns.lineno
                                        if n.returns else n.lineno).visit(n.returns)

+            docstring_hook = self.plugin.get_docstring_parser_hook()
+            if docstring_hook is not None and not any(arg_types) and return_type is None:


I wonder if there'd be value in parsing the docstring (if present) even if there are types in the signature, and flagging discrepancies (only when both are in direct conflict) and maybe even merging the info (if some args have a type only in one of the two sources)? (Admittedly we don't do that for PEP-3107-style annotations and signature type comments either, but there we at least reject the presence of both. Here you seem to completely disregard the docstring if any types are present in the signature.)

Rejecting the presence of both seems like the right choice.

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

mypy/plugin.py

+# The function's return type, if specified, is stored in the mapping with the special
+# key 'return'.  Other than 'return', each key of the mapping must be one of the
+# arguments of the documented function; otherwise, an error will be raised.
+DocstringParserHook = Callable[


This is the part that will need updating after #3534 lands.

chadrik

Thanks for the feedback. I'll get these fixed up and pushed, then do an optimistic rebase against #3534 so that I'm ready when that lands.

chadrik · 2017-06-22T01:46:53Z

@gvanrossum rebased against master and addressed review notes

chadrik · 2017-07-05T19:38:31Z

Hi @gvanrossum & @ilevkivskyi, I'm ready for the next round of notes. thanks!

chadrik · 2017-07-23T22:10:14Z

Hey guys, I know that this issue isn't very high on your priority list, but I feel it's very close, so it would be great to get it completed!

gvanrossum · 2017-08-15T17:02:54Z

@JukkaL Maybe you can look at this?

JukkaL · 2017-08-15T17:09:04Z

I'm going to be on vacation soon for a little over a week and I unfortunately may not have time for this until after my vacation. I can get back to this in 2 weeks or so.

chadrik · 2017-08-16T00:51:12Z

No worries. Enjoy your vacation and I'll talk to you in 2 weeks.

…

On Tue, Aug 15, 2017 at 10:09 AM Jukka Lehtosalo ***@***.***> wrote: I'm going to be on vacation soon for a little over a week and I unfortunately may not have time for this until after my vacation. I can get back to this in 2 weeks or so. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3225 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAD3E72dyu7Lf9ui8yc8OKa3tlnR8jiNks5sYdCzgaJpZM4NFQ6Y> .

ethanhs · 2017-12-16T02:10:34Z

It appears this has hit merge conflicts, @chadrik can you rebase?
@JukkaL were you going to review this?

gvanrossum · 2017-12-16T02:56:34Z

The merges don't look very hard. I used to be very skeptical of this approach but I've mellowed a bit and I think this is fine to go in. I might even find time to review it over my holiday break (which starts now).

chadrik · 2017-12-16T17:40:47Z

I merged and pushed. There are a few remaining issues, but that's all I have time for at this moment.

gvanrossum · 2017-12-16T18:09:33Z

Strangely, the conflicts are still there. Perhaps you didn't fetch before merging?

chadrik · 2017-12-16T22:34:58Z

Yeah, I noticed that, but thought there might be a delay. I squashed the branch and force pushed it, which cleared up the issue.

smessmer · 2018-03-09T03:53:21Z

What's the status of this? Is this abandoned or got it replaced by a different PR or project? I'd really like to have mypy checking for pybind11 C extensions (which do add type info to the doc strings).

gvanrossum · 2018-03-09T04:44:43Z

It's a complicated patch, and we haven't found the time to review it. I don't know what pybind11 is but if it's a code generator, I would suggest that a more effective approach would be to generate separate .pyi files -- we're seeing this approach adopted in other code generators (e.g. for protocol buffers, see https://github.com/dropbox/mypy-protobuf/).

smessmer · 2018-03-09T05:13:51Z

Thanks. In short, pybind11 is a python extension that allows C++ functions to be called from python. When accessing it from python, they have .__doc__ strings with PEP484 type hints. Would be cool if mypy could use these. https://pybind11.readthedocs.io/en/stable/

chadrik · 2018-03-11T05:55:36Z

@smessmer While it would still be cool to get this merged, I don't think it's going to work for your case. This plugin runs within mypy and so does not import any code and thus only has access to statically defined docstrings. Unless pybind11 is generating .py files containing entrypoint/wrapper functions and classes with statically inspectable docstrings -- which I'm guessing it's not -- this plugin isn't going to "see" the doc attributes generated by the bindings. And if it is then it should be simple to make those into .pyi files.

smessmer · 2018-03-11T06:46:40Z

I see, makes sense. Thanks for the explanation. I had misunderstood the PR description to have plugins import python modules into a python runtime and read the doc strings there.

gvanrossum · 2018-08-01T21:45:24Z

I propose to (once again) abandon this idea.

chadrik · 2018-08-03T18:56:36Z

I've finally released my solution to this: https://pypi.org/project/doc484/

It's command-line tool that inserts type comments into your code based on docstrings. It has the advantage of working with PyCharm and other PEP484-compatible tools and not just mypy.

Feel free to close this.

edit: closed it myself, because it's my pull request :)

gvanrossum · 2018-08-03T19:06:47Z

That's a cool tool, and in retrospect a much better solution! Thanks for your persistence.

chadrik force-pushed the docstrings branch 3 times, most recently from 7f655a2 to 254f3d2 Compare April 23, 2017 16:12

chadrik force-pushed the docstrings branch 2 times, most recently from 376cd58 to 724c484 Compare June 5, 2017 17:55

ilevkivskyi reviewed Jun 5, 2017

View reviewed changes

gvanrossum self-requested a review June 5, 2017 21:42

gvanrossum self-assigned this Jun 5, 2017

gvanrossum closed this Jun 9, 2017

gvanrossum reopened this Jun 10, 2017

gvanrossum closed this Jun 10, 2017

gvanrossum reopened this Jun 10, 2017

gvanrossum mentioned this pull request Jun 12, 2017

Using sphinx documentation annotations for type checking in py2 mode #1015

Closed

gvanrossum added the topic-plugins The plugin API and ideas for new plugins label Jun 13, 2017

JukkaL mentioned this pull request Jun 14, 2017

Add docstring type annotation parse hook to plugin system #3542

Closed

gvanrossum reviewed Jun 14, 2017

View reviewed changes

chadrik force-pushed the docstrings branch from 724c484 to da31d85 Compare June 14, 2017 17:18

gvanrossum removed the blocked label Jun 14, 2017

gvanrossum reviewed Jun 20, 2017

View reviewed changes

chadrik commented Jun 20, 2017

View reviewed changes

chadrik force-pushed the docstrings branch from da31d85 to ee58984 Compare June 22, 2017 01:33

Add plugin hook for extracting types from docstrings

d97c4c1

chadrik force-pushed the docstrings branch from 2a920c5 to d97c4c1 Compare December 16, 2017 22:33

chadrik closed this Aug 3, 2018


		from typing import Callable, Dict, Generic, Optional, Tuple, TypeVar, TYPE_CHECKING

		if TYPE_CHECKING:

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

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

Conversation

chadrik commented Apr 23, 2017

chadrik commented Jun 5, 2017

Choose a reason for hiding this comment

ilevkivskyi commented Jun 5, 2017

chadrik commented Jun 6, 2017

JukkaL commented Jun 7, 2017

gvanrossum commented Jun 7, 2017

JukkaL commented Jun 7, 2017

gvanrossum commented Jun 7, 2017 via email

chadrik commented Jun 7, 2017

gvanrossum commented Jun 9, 2017

chadrik commented Jun 10, 2017

gvanrossum commented Jun 10, 2017

chadrik commented Jun 10, 2017

gvanrossum commented Jun 10, 2017

chadrik commented Jun 10, 2017

gvanrossum commented Jun 10, 2017

gvanrossum left a comment

Choose a reason for hiding this comment

chadrik commented Jun 14, 2017

gvanrossum commented Jun 20, 2017

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 left a comment

Choose a reason for hiding this comment

chadrik commented Jun 22, 2017

chadrik commented Jul 5, 2017

chadrik commented Jul 23, 2017

gvanrossum commented Aug 15, 2017

JukkaL commented Aug 15, 2017

chadrik commented Aug 16, 2017 via email

ethanhs commented Dec 16, 2017

gvanrossum commented Dec 16, 2017

chadrik commented Dec 16, 2017

gvanrossum commented Dec 16, 2017

chadrik commented Dec 16, 2017

smessmer commented Mar 9, 2018

gvanrossum commented Mar 9, 2018 via email

smessmer commented Mar 9, 2018

chadrik commented Mar 11, 2018

smessmer commented Mar 11, 2018

gvanrossum commented Aug 1, 2018

chadrik commented Aug 3, 2018 • edited Loading

gvanrossum commented Aug 3, 2018 via email

chadrik commented Aug 3, 2018 •

edited

Loading