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

Support for overloaded functions in stubgenc generated by pybind11 #5975

Merged
merged 19 commits into from Jan 29, 2019

Conversation

Projects
None yet
4 participants
@wiktorn
Copy link
Contributor

commented Nov 29, 2018

For overloaded methods pybind generates following docstring:

__init__(*args, **kwargs)
Overloaded function.

1. __init__(self: TestClass, arg0: str) -> None

2. __init__(self: TestClass, arg0: str, arg1: str) -> None

For such docstrings stubgenc currently produces following stub:

def __init__(self, *args, **kwargs) -> Any: ...

With this change stubgenc will generate following stub:

@overload
def __init__(self, arg0: str) -> None: ...
@overload
def __init__(self, arg0: str, arg1: str) -> None: ...

This pull request introduces:

  • class - FunctionSig - representing signature of function/method (function name, arguments, return value)
  • class - ArgSig - representing information about function argument (argument name, type, default value)

infer_sig_from_docstring now returns list of FunctionSig . List contains one object when function is not overloaded, more objects - when it's overloaded. Each object represents signature of function.

infer_arg_sig_from_docstring is a new function that parses argument list for function in docstring. As the type information may contain commas, the code checks, if the comma is outside brackets. I tried to approach that with regex and failed, also tried to use ast module, but its not straightforward to move from ast form back to source code form that's why I settled with naive approach, as this is not performance critical path. Tests for edge cases included in test_infer_sig_from_docstring

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Dec 1, 2018

For such docstrings stubgenc will produce following stub:

I presume you mean that stubgenc should produce such a stub, and this PR implements that behavior? (We prefer our commit messages to describe the change rather than the new state of affairs.)

I will try to review soon, but I think this has missed the train for the upcoming 0.650 release (see #5960).

@wiktorn

This comment has been minimized.

Copy link
Contributor Author

commented Dec 1, 2018

Ok, updated description.

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

Hm... Having looked at this a bit more, it really feels like string partitioning is a pretty poor way of parsing something of this complexity. I realize that all of stubgen looks like a hack, but since you are doing your best to make it better, perhaps you can improve the approach to parsing a bit more? Maybe write a tiny recursive-descent parser, using the tokenize module for tokenization?

@wiktorn

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

Thank you for pointing me to tokenize module. Looks like this is something I was looking for. Looks like it is time for me to refresh my knowledge and re-read library reference :-)

I'll start with replacing parsing docstring function declarations. Though quick scan of the code suggests, that some regex usage will remain in the stubgenc/stubutil.

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Dec 14, 2018

Hey @wiktorn, just to let you know, we're planning fairly extensive refactorings of part of stubgen, however, we don't expect the docstring parsing to be affected, so don't worry!

@wiktorn

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2018

Thank you for the heads up @gvanrossum . I'll finally have more time next week to polish this PR.

@wiktorn

This comment has been minimized.

Copy link
Contributor Author

commented Jan 12, 2019

@gvanrossum Can you trigger another AppVeyor build? My guess that this failure was totally unrelated to my changes (some exceptions in IPC and ast3 module that are not mentioned in this changes)

@ethanhs

This comment has been minimized.

Copy link
Collaborator

commented Jan 12, 2019

@wiktorn if you rebase on master those errors should go away.

@wiktorn wiktorn force-pushed the wiktorn:stubgenc_pybind_arg_type_object branch from 1530871 to ae08bd3 Jan 12, 2019

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Jan 25, 2019

Sorry I haven't got to review this yet! I missed that you had updated it because you used git push -f. In the future please leave the old revisions alone and just push new changes, it's easier to review.

Note that when #6256 lands this will become one big merge conflict, but according to Ivan it's easy to resolve -- he just moved all the docstring parsing logic to a new file, stubdoc.c.

@gvanrossum
Copy link
Member

left a comment

Here are a few review comments anyway. Sorry again!

Show resolved Hide resolved mypy/stubgenc.py Outdated
Show resolved Hide resolved mypy/stubgenc.py Outdated
Show resolved Hide resolved mypy/stubutil.py Outdated
Show resolved Hide resolved mypy/stubutil.py Outdated
Show resolved Hide resolved mypy/stubutil.py Outdated
Show resolved Hide resolved mypy/stubutil.py Outdated
Show resolved Hide resolved mypy/stubutil.py Outdated
@wiktorn

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2019

I hope that's ok right now. Just let me know how you want to move forward. Shall I till #6256 lands in master and merge master back to this branch?

Sorry for the force push, that's how I understood @ethanhs and since there was no comments on code yet, I've decided to rebase instead of merging master

@gvanrossum
Copy link
Member

left a comment

LG! This looks ready to merge.

I'm leaving it up to @ilevkivskyi which one to merge first: this one (then he has to merge your code back into #6256) or the latter (leaving the merge up to you).

@ilevkivskyi
Copy link
Collaborator

left a comment

I would be glad to merge this before my PR, but I have a bunch of additional comments.

Show resolved Hide resolved mypy/stubutil.py Outdated
Show resolved Hide resolved mypy/stubutil.py Outdated
Show resolved Hide resolved mypy/stubutil.py Outdated
Show resolved Hide resolved mypy/stubutil.py Outdated
Show resolved Hide resolved mypy/stubutil.py Outdated
Show resolved Hide resolved mypy/test/teststubgen.py Outdated
Show resolved Hide resolved mypy/test/teststubgen.py
Show resolved Hide resolved mypy/test/teststubgen.py Outdated
Show resolved Hide resolved mypy/test/teststubgen.py
Show resolved Hide resolved mypy/test/teststubgen.py

wiktorn added some commits Jan 28, 2019

@ilevkivskyi
Copy link
Collaborator

left a comment

It looks like there are bunch of comments from my previous review that are not implemented. I tried to explain some of them more, in case you didn't understand what to do. Are you going to implement them?

If no, I would probably merge this anyway and fix them myself, but it would be better if you can do this.

if name == 'getitem':
return '(index)'
return [TypedArgSig(name='index', type=None, default=False)]

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jan 29, 2019

Collaborator

If you would use normal classes instead of named tuples you could define __init__ as:

    def __init__(self, name: str, type: Optional[str] = None, default: bool = False) -> None: ...

and then this an d a dozen others will be just TypedArgSig('index').

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jan 29, 2019

Collaborator

Also with normal classes you can define __repr__(), to simplify test cases significantly. You can just check that str(<generated signature>) matches an expected string.

return [
TypedArgSig(name='name', type=None, default=False),
TypedArgSig(name='value', type=None, default=False)
]

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jan 29, 2019

Collaborator

For multiline we use this style:

return [TypedArgSig(name='name', type=None, default=False),
        TypedArgSig(name='value', type=None, default=False)]

(also many of these will not need to be multiline if you implement the suggestion above).

Show resolved Hide resolved mypy/test/teststubgen.py
Review fixes
TypedArgSig -> ArgSig
TypedFunctionSig -> FunctionSig
@wiktorn

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

I've missed your comments on conversation tab on GitHub. As I reviewed changes I found your comments and started to address them.

wiktorn added some commits Jan 29, 2019

Review fixes
Shorten multiline lists.
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Jan 29, 2019

@wiktorn Currently there is one more big change that I proposed: switch from named tuples to normal classes, see #5975 (comment) and #5975 (comment)

After that you could also remove redundant type=None etc. For the tests just use:

class ArgSig:
    ...
    def __repr__(self) -> str:
        r = self.name
        if self.type:
            r += ': ' + self.type
        if self.default:
            r += ' = ...' if self.type else '=...'
        return r

class FunctionSig:
    ...
    def __repr__(self) -> str:
        args = ','.join([str(arg) for arg in self.args])
        return '{}({}) -> {}: ...'

Then many tests can be simplified to just check the string representation of the result. You can still define __eq__() if you want to use direct equality checks in some tests.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Jan 29, 2019

(also there is a lint failure now)

@wiktorn

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

And what do you think about this approach:

ArgSig = NamedTuple('ArgSig', [
    ('name', str),
    ('type', Optional[str]),
    ('default', bool)
])

ArgSig.__new__.__defaults__ = (None, False)

As far as I see, it works in Python3.4, and once oldest supported version will be 3.7, we can move this declaration to NamedTuple call itself.

I'm reluctant to use repr strings in tests though.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Jan 29, 2019

ArgSig.__new__.__defaults__ = (None, False)

With this call sites will not type-check by mypy.

I'm reluctant to use repr strings in tests though.

Why? If you just want to be sure it is clear from the string form it is a custom class, use something like "FunctionSig('method(self, arg: int) -> Any')" it is still quicker to grasp that the current form. I however don't have strong opinion about this, unlike about the default arguments.

@wiktorn

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

ArgSig.__new__.__defaults__ = (None, False)

With this call sites will not type-check by mypy.

As I'm testing it right now, it rather doesn't pass self-check:
error: "Callable[[Type[NT], str, Optional[str], bool], NT]" has no attribute "__defaults__"

Though it properly detects problems in call sites

error: Argument "type" to "ArgSig" has incompatible type "int"; expected "Optional[str]"
error: Argument "default" to "ArgSig" has incompatible type "str"; expected "bool"

So I'll convert them to normal classes.

I'm reluctant to use repr strings in tests though.

Why? If you just want to be sure it is clear from the string form it is a custom class, use something like "FunctionSig('method(self, arg: int) -> Any')" it is still quicker to grasp that the current form. I however don't have strong opinion about this, unlike about the default arguments.

We could actually have __str__ method which would provide stub signature. The only problem I see is that to cover whole API contract, we would need still to check if types in ArgSig's are properly set, as they are used to add necessary imports

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

commented Jan 29, 2019

Though it properly detects problems in call sites

I meant that mypy will flag as errors the calls with less arguments like ArgSig('index') if you would use the __defaults__ pattern.

So I'll convert them to normal classes.

OK

The only problem I see is that to cover whole API contract, we would need still to check if types in ArgSig's are properly set, as they are used to add necessary imports

Yes, this could be useful only for testing. Anyway, I don't insist on this. You can just define __eq__() instead and keep your current tests.

Review fixes.
Create ArgList class instead of NamedTuple and provide defaults for type and
default
@wiktorn

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

I did not add defaults to FunctionSig as it doesn't shorten that much and forces to give a thought, if arguments are properly set.

Show resolved Hide resolved mypy/stubgenc.py Outdated
@ilevkivskyi
Copy link
Collaborator

left a comment

OK, I am going to merge this now. There are some minor formatting changes, but I will take care of those.

@ilevkivskyi ilevkivskyi merged commit d6aef70 into python:master Jan 29, 2019

2 checks passed

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

This comment has been minimized.

Copy link
Collaborator

commented Jan 29, 2019

@wiktorn Thanks for contributing! I have successfully merged my PR on top for yours.

@wiktorn

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2019

Thank you for your patience @ilevkivskyi and help with getting this merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.