Disallow module cycles in autosummary

[tbekolay]

Subject: Fix bug with module cycles in autsummary

Feature or Bugfix

• Bugfix

Purpose

• Consider the following piece of ReST:

  .. automodule:: sphinx.ext.autosummary
     :members:
  
     .. autosummary::
  
        sphinx.ext.autosummary.Autosummary
  

  This inserts an autosummary after the module docstring, but before the members of the module. Without the change in this commit, this would fail because import_by_name would attempt to import

  sphinx.ext.autosummary.sphinx.ext.autosummary.Autosumary
  

  because the prefix (from the parent) is sphinx.ext.autosummary, and the name is sphinx.ext.autosummary.Autosummary, which is able to be imported from sphinx.ext.autosummary with the above string, but is not the way that anyone would want to refer to it.

• I was using Sphinx master. I would make this PR on the 2.3 branch, but there doesn't seem to be one at the moment.

I would be happy to add a test to this PR, but it's not clear to me how the @pytest.mark.sphinx fixture/mark works ... is there any quick documentation on this? Basically I would like to be able to run Sphinx with the above piece of ReST to show that there is a bug before this commit and is fixed with this commit.

You can reproduce this locally by replacing the contents of doc/usage/extensions/autosummary.rst with

:mod:`sphinx.ext.autosummary` -- Generate autodoc summaries
===========================================================

.. automodule:: sphinx.ext.autosummary
   :members:

   .. autosummary::

      sphinx.ext.autosummary.Autosummary

Without this PR, the autosummary does not generate the correct link (but for some reason does not raise a warning, though it does for me in other projects). With this change it generates the correct link.

[tk0miya]

automodule directive changes the current module path to specified one during parsing its contents. Inside the directive, the autosummary directive is recognized as relative definition from sphinx.ext.autosummary. So you don't need to give sphinx.ext.autosummary namespace to autosummary directive.

It would be working fine with this code:

.. automodule:: sphinx.ext.autosummary
   :members:

   .. autosummary::

      Autosummary

IMO, this is an intended behavior of autodoc and autosummary.

[tbekolay]

Thanks for the quick response @tk0miya!

While your snippet does work properly, it renders differently:

Autosummary(name, arguments, options, …)

While mine renders as:

sphinx.ext.autosummary.Autosummary(name, …)

which, in our case, is what we want. Is there some option of autosummary that would allow printing the fully qualified name rather than what you put in the body of the autosummary? If not, would you be okay with a PR adding such an option, something like :use-qualname: (which has an analogy with Python 3's __qualname__ vs __name__)?

---

As an aside, another reason why I find this to be a bug is because it works the way that I want it to work unless you happen to import the top-level name in the file you're summarizing. For example, picking another random file, the following works the way that I was expecting:

.. automodule:: sphinx.addnodes
   :members:

   .. autosummary::

      sphinx.addnodes.translatable
      sphinx.addnodes.toctree

Even though this could instead be

.. automodule:: sphinx.addnodes
   :members:

   .. autosummary::

      translatable
      toctree

it works the way that I want solely because sphinx.addnodes does not import sphinx. Since sphinx.ext.autosummary does import sphinx, you get the issue that I'm fixing in this PR. It would make more sense to me if you forced the latter form and errored if you try to do what works here; you could achieve that by modifying my PR to do the following:

        if prefix is not None and name.startswith(prefix):
            # Disallow module cycles (e.g., sphinx.ext.sphinx.ext...)
            raise SphinxError("summarized item should not include the current module")

---

Another reason why I consider the current behavior a bug is that the _import_by_name function implies a type of importing behavior that is not actually possible in Python. According to _import_by_name, I would assume that it is possible to do import sphinx.ext.autosummary.sphinx.ext.autosummary, but it is not:

>>> import sphinx.ext.autosummary.sphinx.ext.autosummary
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ModuleNotFoundError: No module named 'sphinx.ext.autosummary.sphinx'

While it is possible to do from sphinx.ext.autosummary import sphinx, the implied

from sphinx.ext.autosummary import sphinx.ext.autosummary

is a SyntaxError because you can't have dotted notation on the right hand side of a from-import.

_import_by_name is able to find sphinx.ext.autosummary.sphinx.ext.autosummary because it does

import sphinx.ext.autosummary as parent
for name in ["sphinx", "ext", "autosummary"]:
    parent = getattr(parent, name)

which works, but implies that you can do from sphinx.ext.autosummary import sphinx.ext.autosummary.AutoSummary, which you can't do.

It seems like this block is specifically designed to result in what I'm considering a bug here. Do you remember the intended use case that would result in going through this code path?

[AA-Turner]

Thanks @tbekolay!

A

[mun1r0b0t]

This is causing existing builds to emit warnings, and fail when -W is used. I created an issue at #12589. Can someone take a look and provide a fix?