Repository: sphinx-doc/sphinx

Issue:
autosummary table gets confused by long / complex type hints
**Describe the bug**

While creating the table for `.. autosummary::`, the function

https://github.com/sphinx-doc/sphinx/blob/15bc5a32bb0b78b432803e55fffa10c801182c75/sphinx/ext/autosummary/__init__.py#L430

will not produce the expected results, some type hints remain, albeit broken up.  Here is the long function signature in question:

```py
from pathlib import Path
# ...
def filter_file(path: Union[Path, str], pattern: str,
                repl: Union[Callable[[Match], str], str], count: int = 0,
                flags: int = 0, backup_extension: str = ".orig",
                line_based: bool = False, demand_different: bool = True,
                encoding: Optional[str] = None) -> Path:
```

In the `.. autosummary::` table, the following is produced:

```
filter_file(path, str], pattern, repl, str], …)
```

The rendered docs can be found [online here](https://ci-exec.readthedocs.io/en/latest/api/patch.html#module-ci_exec.patch), but the root of the problem can be seen by the `path` and `repl` parameters (first and third parameter).

Stepping through, what actually happens is that everything just gets split on `', '`:

https://github.com/sphinx-doc/sphinx/blob/15bc5a32bb0b78b432803e55fffa10c801182c75/sphinx/ext/autosummary/__init__.py#L448-L454

So when we have something like `Union[Path, str]` or the (rather ugly) more complicated `Union[Callable[[Match], str], str]`, splitting on `', '` will give us something like this (splitting each argument on line to make clear):

```py
(Pdb) p args
[
    'path: Union[pathlib.Path',
    'str]',
    'pattern: str',
    'repl: Union[Callable[[Match[AnyStr]]',
    'str]',
    # ... remaining arguments ...
]
```

This is actually a pretty hard problem to solve with regular expressions, if it's even possible to do so (unless I'm missing something, it's kind of late xD).  I think we would have to start counting how many `[` we have seen and match them with `]` (after having seen a `:`).

So I was playing around with alternatives, found a nice SO answer that has some relevant stuff, here's a rough prototype:

```py
#!/usr/bin/env python3

import ast
import re
class HintDestroyer(ast.NodeTransformer):
    # See: https://stackoverflow.com/a/42734810/3814202
    # we only need to visit function since we only create function
    def visit_FunctionDef(self, node):
        node.returns = None
        if node.args.args:
            for arg in node.args.args:
                # just for a glimpse ...
                print("Removing annotation for {}: {}".format(arg.arg, arg.annotation.__dict__))
                arg.annotation = None
        return node

# sample input that breaks
sig = "(path: Union[pathlib.Path, str], pattern: str, repl: Union[Callable[[Match[AnyStr]], str], str], count: int = 0, flags: int = 0, backup_extension: str = '.orig', line_based: bool = False, demand_different: bool = True, encoding: Optional[str] = None) -> pathlib.Path"

# create a valid function signature to parse with ast module.
function = "def foo{sig}: pass\n".format(sig=sig)

# parse and transform
parsed_function = ast.parse(function)
transformed = HintDestroyer().visit(parsed_function)
# we only created a single "function", there should be a single module with a
# function as its body.
transformed_function = transformed.body[0]

# is this what we want?
args = ", ".join([item.arg for item in transformed_function.args.args])
print(args)
# output:
# path, pattern, repl, count, flags, backup_extension, line_based, demand_different, encoding
```

Right now I _believe_ what `args` and `opts` in the current autosummary code are doing is trying to shove `opts` at the end _if there is space_ (since there's a maximum length here).  I'm not sure why this happens, since all of the keyword arguments have to be last anyway right?

Basically we can use `ast` and just limited join on the transformed thing if we don't care about necessarily preserving existing behavior.  But I'm not sure about the implications of all this ... so thought I would open up the issue for discussion of other possible solutions or maybe downsides to use `ast` (it definitely doesn't come for free, but it's also a simple parse).

**To Reproduce**
Steps to reproduce the behavior:

```
$ git clone https://github.com/svenevs/ci_exec.git
$ cd ci_exec
$ tox -e docs
$ open .tox/docs/tmp/html/index.html

# ... or ...

$ pip install -r docs/requirements.txt
$ cd docs
$ make html 
$ open _build/html/index.html
```

**Expected behavior**
No stray `str]` end up as a "parameter" in the autosummary table.

**Your project**
https://github.com/svenevs/ci_exec

**Screenshots**

![over_parse](https://user-images.githubusercontent.com/5871461/56345759-5fedd200-6175-11e9-839f-5f5e8284b612.png)


**Environment info**
- OS: linux
- Python version: 3.7.2
- Sphinx version: 2.0.1
- Sphinx extensions:  sphinx.ext.autosummary
- Extra tools: n/a

Discussion:

Patch:
diff --git a/CHANGES b/CHANGES
index 9a1973cec..541cfff94 100644
--- a/CHANGES
+++ b/CHANGES
@@ -108,6 +108,7 @@ Bugs fixed
 * #6351: "Hyperlink target is not referenced" message is shown even if
   referenced
 * #6165: autodoc: ``tab_width`` setting of docutils has been ignored
+* #6311: autosummary: autosummary table gets confused by complex type hints
 * Generated Makefiles lack a final EOL (refs: #6232)
 
 Testing
diff --git a/sphinx/ext/autodoc/__init__.py b/sphinx/ext/autodoc/__init__.py
index 05cd2efc7..26936956e 100644
--- a/sphinx/ext/autodoc/__init__.py
+++ b/sphinx/ext/autodoc/__init__.py
@@ -365,8 +365,8 @@ def check_module(self):
             return False
         return True
 
-    def format_args(self):
-        # type: () -> str
+    def format_args(self, **kwargs):
+        # type: (Any) -> str
         """Format the argument signature of *self.object*.
 
         Should return None if the object does not have a signature.
@@ -385,8 +385,8 @@ def format_name(self):
         # directives of course)
         return '.'.join(self.objpath) or self.modname
 
-    def format_signature(self):
-        # type: () -> str
+    def format_signature(self, **kwargs):
+        # type: (Any) -> str
         """Format the signature (arguments and return annotation) of the object.
 
         Let the user process it via the ``autodoc-process-signature`` event.
@@ -397,7 +397,13 @@ def format_signature(self):
         else:
             # try to introspect the signature
             try:
-                args = self.format_args()
+                try:
+                    args = self.format_args(**kwargs)
+                except TypeError:
+                    warnings.warn("Documenter.format_args() takes keyword arguments "
+                                  "since Sphinx-2.1",
+                                  RemovedInSphinx40Warning)
+                    args = self.format_args()
             except Exception as err:
                 logger.warning(__('error while formatting arguments for %s: %s') %
                                (self.fullname, err), type='autodoc')
@@ -956,15 +962,15 @@ def get_doc(self, encoding=None, ignore=1):
             return lines
         return super().get_doc(None, ignore)  # type: ignore
 
-    def format_signature(self):
-        # type: () -> str
+    def format_signature(self, **kwargs):
+        # type: (Any) -> str
         if self.args is None and self.env.config.autodoc_docstring_signature:  # type: ignore
             # only act if a signature is not explicitly given already, and if
             # the feature is enabled
             result = self._find_signature()
             if result is not None:
                 self.args, self.retann = result
-        return super().format_signature()  # type: ignore
+        return super().format_signature(**kwargs)  # type: ignore
 
 
 class DocstringStripSignatureMixin(DocstringSignatureMixin):
@@ -972,8 +978,8 @@ class DocstringStripSignatureMixin(DocstringSignatureMixin):
     Mixin for AttributeDocumenter to provide the
     feature of stripping any function signature from the docstring.
     """
-    def format_signature(self):
-        # type: () -> str
+    def format_signature(self, **kwargs):
+        # type: (Any) -> str
         if self.args is None and self.env.config.autodoc_docstring_signature:  # type: ignore
             # only act if a signature is not explicitly given already, and if
             # the feature is enabled
@@ -983,7 +989,7 @@ def format_signature(self):
                 # DocstringSignatureMixin.format_signature.
                 # Documenter.format_signature use self.args value to format.
                 _args, self.retann = result
-        return super().format_signature()
+        return super().format_signature(**kwargs)
 
 
 class FunctionDocumenter(DocstringSignatureMixin, ModuleLevelDocumenter):  # type: ignore
@@ -1000,8 +1006,8 @@ def can_document_member(cls, member, membername, isattr, parent):
         return (inspect.isfunction(member) or inspect.isbuiltin(member) or
                 (inspect.isroutine(member) and isinstance(parent, ModuleDocumenter)))
 
-    def format_args(self):
-        # type: () -> str
+    def format_args(self, **kwargs):
+        # type: (Any) -> str
         if inspect.isbuiltin(self.object) or inspect.ismethoddescriptor(self.object):
             # cannot introspect arguments of a C function or method
             return None
@@ -1011,9 +1017,9 @@ def format_args(self):
                     not inspect.isbuiltin(self.object) and
                     not inspect.isclass(self.object) and
                     hasattr(self.object, '__call__')):
-                args = Signature(self.object.__call__).format_args()
+                args = Signature(self.object.__call__).format_args(**kwargs)
             else:
-                args = Signature(self.object).format_args()
+                args = Signature(self.object).format_args(**kwargs)
         except TypeError:
             if (inspect.is_builtin_class_method(self.object, '__new__') and
                inspect.is_builtin_class_method(self.object, '__init__')):
@@ -1024,10 +1030,10 @@ def format_args(self):
             # signature without the first argument.
             try:
                 sig = Signature(self.object.__new__, bound_method=True, has_retval=False)
-                args = sig.format_args()
+                args = sig.format_args(**kwargs)
             except TypeError:
                 sig = Signature(self.object.__init__, bound_method=True, has_retval=False)
-                args = sig.format_args()
+                args = sig.format_args(**kwargs)
 
         # escape backslashes for reST
         args = args.replace('\\', '\\\\')
@@ -1055,8 +1061,8 @@ class DecoratorDocumenter(FunctionDocumenter):
     # must be lower than FunctionDocumenter
     priority = -1
 
-    def format_args(self):
-        args = super().format_args()
+    def format_args(self, **kwargs):
+        args = super().format_args(**kwargs)
         if ',' in args:
             return args
         else:
@@ -1099,8 +1105,8 @@ def import_object(self):
                 self.doc_as_attr = True
         return ret
 
-    def format_args(self):
-        # type: () -> str
+    def format_args(self, **kwargs):
+        # type: (Any) -> str
         # for classes, the relevant signature is the __init__ method's
         initmeth = self.get_attr(self.object, '__init__', None)
         # classes without __init__ method, default __init__ or
@@ -1110,18 +1116,19 @@ def format_args(self):
                 not(inspect.ismethod(initmeth) or inspect.isfunction(initmeth)):
             return None
         try:
-            return Signature(initmeth, bound_method=True, has_retval=False).format_args()
+            sig = Signature(initmeth, bound_method=True, has_retval=False)
+            return sig.format_args(**kwargs)
         except TypeError:
             # still not possible: happens e.g. for old-style classes
             # with __init__ in C
             return None
 
-    def format_signature(self):
-        # type: () -> str
+    def format_signature(self, **kwargs):
+        # type: (Any) -> str
         if self.doc_as_attr:
             return ''
 
-        return super().format_signature()
+        return super().format_signature(**kwargs)
 
     def add_directive_header(self, sig):
         # type: (str) -> None
@@ -1312,15 +1319,15 @@ def import_object(self):
 
         return ret
 
-    def format_args(self):
-        # type: () -> str
+    def format_args(self, **kwargs):
+        # type: (Any) -> str
         if inspect.isbuiltin(self.object) or inspect.ismethoddescriptor(self.object):
             # can never get arguments of a C function or method
             return None
         if inspect.isstaticmethod(self.object, cls=self.parent, name=self.object_name):
-            args = Signature(self.object, bound_method=False).format_args()
+            args = Signature(self.object, bound_method=False).format_args(**kwargs)
         else:
-            args = Signature(self.object, bound_method=True).format_args()
+            args = Signature(self.object, bound_method=True).format_args(**kwargs)
         # escape backslashes for reST
         args = args.replace('\\', '\\\\')
         return args
diff --git a/sphinx/ext/autosummary/__init__.py b/sphinx/ext/autosummary/__init__.py
index 5840f0ccd..656e67bf1 100644
--- a/sphinx/ext/autosummary/__init__.py
+++ b/sphinx/ext/autosummary/__init__.py
@@ -329,7 +329,12 @@ def get_items(self, names):
 
             # -- Grab the signature
 
-            sig = documenter.format_signature()
+            try:
+                sig = documenter.format_signature(show_annotation=False)
+            except TypeError:
+                # the documenter does not support ``show_annotation`` option
+                sig = documenter.format_signature()
+
             if not sig:
                 sig = ''
             else:
@@ -445,7 +450,7 @@ def mangle_signature(sig, max_chars=30):
     args = []  # type: List[str]
     opts = []  # type: List[str]
 
-    opt_re = re.compile(r"^(.*, |)([a-zA-Z0-9_*]+)=")
+    opt_re = re.compile(r"^(.*, |)([a-zA-Z0-9_*]+)\s*=\s*")
     while s:
         m = opt_re.search(s)
         if not m:
diff --git a/sphinx/util/inspect.py b/sphinx/util/inspect.py
index ddc7ee5ef..58eca7ffb 100644
--- a/sphinx/util/inspect.py
+++ b/sphinx/util/inspect.py
@@ -413,8 +413,8 @@ def return_annotation(self):
         else:
             return None
 
-    def format_args(self):
-        # type: () -> str
+    def format_args(self, show_annotation=True):
+        # type: (bool) -> str
         args = []
         last_kind = None
         for i, param in enumerate(self.parameters.values()):
@@ -435,7 +435,7 @@ def format_args(self):
                               param.POSITIONAL_OR_KEYWORD,
                               param.KEYWORD_ONLY):
                 arg.write(param.name)
-                if param.annotation is not param.empty:
+                if show_annotation and param.annotation is not param.empty:
                     if isinstance(param.annotation, str) and param.name in self.annotations:
                         arg.write(': ')
                         arg.write(self.format_annotation(self.annotations[param.name]))


Test patch:
diff --git a/tests/roots/test-ext-autosummary/autosummary_dummy_module.py b/tests/roots/test-ext-autosummary/autosummary_dummy_module.py
index 5506d0bc9..02e6c0e7d 100644
--- a/tests/roots/test-ext-autosummary/autosummary_dummy_module.py
+++ b/tests/roots/test-ext-autosummary/autosummary_dummy_module.py
@@ -1,4 +1,5 @@
 from os import *  # NOQA
+from typing import Union
 
 
 class Foo:
@@ -11,3 +12,7 @@ def bar(self):
     @property
     def baz(self):
         pass
+
+
+def bar(x: Union[int, str], y: int = 1):
+    pass
diff --git a/tests/roots/test-ext-autosummary/index.rst b/tests/roots/test-ext-autosummary/index.rst
index fc84927bb..c52e96ed9 100644
--- a/tests/roots/test-ext-autosummary/index.rst
+++ b/tests/roots/test-ext-autosummary/index.rst
@@ -8,4 +8,5 @@
 
    autosummary_dummy_module
    autosummary_dummy_module.Foo
+   autosummary_dummy_module.bar
    autosummary_importfail
diff --git a/tests/test_ext_autosummary.py b/tests/test_ext_autosummary.py
index 2ccfd9342..929c3455f 100644
--- a/tests/test_ext_autosummary.py
+++ b/tests/test_ext_autosummary.py
@@ -13,9 +13,13 @@
 from unittest.mock import Mock
 
 import pytest
+from docutils import nodes
 
-from sphinx.ext.autosummary import mangle_signature, import_by_name, extract_summary
-from sphinx.testing.util import etree_parse
+from sphinx import addnodes
+from sphinx.ext.autosummary import (
+    autosummary_table, autosummary_toc, mangle_signature, import_by_name, extract_summary
+)
+from sphinx.testing.util import assert_node, etree_parse
 from sphinx.util.docutils import new_document
 
 html_warnfile = StringIO()
@@ -179,6 +183,24 @@ def test_escaping(app, status, warning):
 def test_autosummary_generate(app, status, warning):
     app.builder.build_all()
 
+    doctree = app.env.get_doctree('index')
+    assert_node(doctree, (nodes.paragraph,
+                          nodes.paragraph,
+                          addnodes.tabular_col_spec,
+                          autosummary_table,
+                          autosummary_toc))
+    assert_node(doctree[3],
+                [autosummary_table, nodes.table, nodes.tgroup, (nodes.colspec,
+                                                                nodes.colspec,
+                                                                [nodes.tbody, (nodes.row,
+                                                                               nodes.row,
+                                                                               nodes.row,
+                                                                               nodes.row)])])
+    assert doctree[3][0][0][2][0].astext() == 'autosummary_dummy_module\n\n'
+    assert doctree[3][0][0][2][1].astext() == 'autosummary_dummy_module.Foo()\n\n'
+    assert doctree[3][0][0][2][2].astext() == 'autosummary_dummy_module.bar(x[, y])\n\n'
+    assert doctree[3][0][0][2][3].astext() == 'autosummary_importfail\n\n'
+
     module = (app.srcdir / 'generated' / 'autosummary_dummy_module.rst').text()
     assert ('   .. autosummary::\n'
             '   \n'