mypy icon indicating copy to clipboard operation
mypy copied to clipboard

stubgenc: Introduce an object-oriented system for extracting function signatures

Open chadrik opened this issue 3 years ago • 6 comments
trafficstars

Description

Note: This change is part of a series of upcoming MRs to improve stubgenc.

stubgenc tries 3 approaches to infer signatures for a function. There are two problems with the current design:

  1. the logic for how these approaches are applied is somewhat convoluted, to the point that it's not even clear at first that there are 3 distinct approaches (from rst files, from docstrings, fallback guess).
  2. there's not a clear path for how a developer would create a new approach.

Here's the heart of the current logic (comments added for clarity):

    if (
        name in ("__new__", "__init__")
        and name not in sigs
        and class_name
        and class_name in class_sigs
    ): 
        # get from rst (only works classes)
        inferred: Optional[List[FunctionSig]] = [
            FunctionSig(
                name=name,
                args=infer_arg_sig_from_anon_docstring(class_sigs[class_name]),
                ret_type=ret_type,
            )
        ]
    else:
        docstr = getattr(obj, '__doc__', None)
        # get from docstring
        inferred = infer_sig_from_docstring(docstr, name)
        if inferred:
            assert docstr is not None
            if is_pybind11_overloaded_function_docstring(docstr, name):
                del inferred[-1]
        if not inferred:
            if class_name and name not in sigs:
                # fall back to using a list of known special methods
                inferred = [FunctionSig(name, args=infer_method_sig(name, self_var),
                                        ret_type=ret_type)]
            else:
                # fall back to a catchall signature
                inferred = [FunctionSig(name=name,
                                        args=infer_arg_sig_from_anon_docstring(
                                            sigs.get(name, '(*args, **kwargs)')),
                                        ret_type=ret_type)]
        elif class_name and self_var:
            # why do we only fix self var for docstring signatures and not rst signatures?
            args = inferred[0].args
            if not args or args[0].name != self_var:
                args.insert(0, ArgSig(name=self_var))

And after my changes:

    inferred: Optional[List[FunctionSig]] = None
    if class_name:
        # method:
        assert self_var is not None, "self_var should be provided for methods"
        for sig_gen in sig_generators:
            inferred = sig_gen.get_method_sig(obj, module.__name__, class_name, name, self_var)
            if inferred:
                args = inferred[0].args
                if not args or args[0].name != self_var:
                    args.insert(0, ArgSig(name=self_var))
                break
    else:
        # function:
        for sig_gen in sig_generators:
            inferred = sig_gen.get_function_sig(obj, module.__name__, name)
            if inferred:
                break

This clarifies the logic and opens up the possibility for future expansion and customization.

Test Plan

This MR has two commits:

  1. implement the object-oriented inference system: this change is designed to preserve the current behavior so required no changes to test behavior
  2. fix a bug where @classmethod and self-var fixes were not applied to every overload. tests have been updated and a new test added to reflect the change.

chadrik avatar Aug 21 '22 17:08 chadrik

Hi, any thoughts on this?

chadrik avatar Aug 26 '22 15:08 chadrik

Hey everyone, it would be great to get some feedback on this PR.

chadrik avatar Sep 08 '22 21:09 chadrik

@ilevkivskyi Sorry to flag you directly here, but I'm not sure how to get someone to look at my PR. Can you help me out? This is the first of several PRs that I'd like to make to improve stubgen and it's been 3 weeks without any response, so at this rate it's going to take awhile!

chadrik avatar Sep 16 '22 17:09 chadrik

Sorry, this is volunteer run and people may just not have time to look at this. As for reviewers, I guess @hauntsaninja may be a good person to look at this.

ilevkivskyi avatar Sep 19 '22 09:09 ilevkivskyi

Sorry, this is volunteer run and people may just not have time to look at this.

I completely understand, and I appreciate all of your efforts! Just looking for anyway that I can get a reviewer to help me get this through. I've got what I think are some great contributions, that start with this MR. I've used the complete set of changes to make a really nice set of stubs for PySide and I've got more to come.

chadrik avatar Sep 22 '22 20:09 chadrik

Thanks, this looks good! Your PR description made this easy to review. Left one nit, tag me once you've made the change (assuming you agree).

Thanks for helping out. I've responded to your suggestion.

(I'd say I'm mostly casually acquainted with stubgen and don't feel any great sense of ownership over it, so I'm not going to make promises... but if you ever have another PR that gets stalled more than a month, let me know)

Thanks for the offer!

chadrik avatar Sep 22 '22 23:09 chadrik

Thanks! Please don't force push to PRs after they've been reviewed. It makes reviewing the delta much harder, especially so when the force push includes a rebase. We squash commits on merge. If you want to test against latest master, hit the "Update branch" button in the UI.

hauntsaninja avatar Sep 24 '22 00:09 hauntsaninja