more than one target found for cross reference should prefer current file's method over other libraries methods

dvorapa commented 6 years ago

Subject: more than one target found for cross reference should prefer current file's method over other libraries methods

Problem

When building HTML of our project, Sphinx outputs cca 200 warnings like this. But half of them should be never thrown, because Sphinx is not sure, whether to use current file's one or imported library's one. For example, see:
```
/src/pywikibot/interwiki_graph.py:docstring of pywikibot.interwiki_graph.GraphDrawer.__init__:: WARNING: more than one target found for cross-reference 'Subject': scripts.interwiki.Subject, pywikibot.interwiki_graph.Subject
```
The expected one is clear: The one from current file interwiki_graph should be preferred over the imported, but Sphinx still throwns a warning.

Procedure to reproduce the problem

Build HTML of Pywikibot (last build by us: https://integration.wikimedia.org/ci/job/pywikibot-core-tox-publish/424/console) or try to simulate the situation

Error logs / results

/src/pywikibot/interwiki_graph.py:docstring of pywikibot.interwiki_graph.GraphDrawer.__init__:: WARNING: more than one target found for cross-reference 'Subject': scripts.interwiki.Subject, pywikibot.interwiki_graph.Subject

https://integration.wikimedia.org/ci/job/pywikibot-core-tox-publish/424/console

Expected results

It should prefer the current file over the imported one

Reproducible project / your project

https://www.mediawiki.org/wiki/Manual:Pywikibot

Environment info

OS: Debian Jessie
Python version: 3.4
Sphinx version: 1.7.4
Napoleon, Epytext

tk0miya commented 6 years ago

Where can I see the content ofinterwiki_graph? I'd like to see your source and try to reproduce this. I can't understand how to do it. Please let me know the way to reproduce (or investigate).

Reproducible project / your project https://www.mediawiki.org/wiki/Manual:Pywikibot

dvorapa commented 6 years ago

Reproducible project / your project https://www.mediawiki.org/wiki/Manual:Pywikibot

As you could find out in that link, the source code can be found for example here: https://phabricator.wikimedia.org/diffusion/PWBC/. Also you could look to https://pypi.org/project/pywikibot/.

Sphinx config is in the docs/ folder. Dependencies for Sphinx are written in docs/conf.py and tox.ini's [testenv:doc] (or I could collect them into one list and send them to you, if there will be any problem). The Sphinx build live can be found here: https://doc.wikimedia.org/pywikibot/

The file in the description is stored in pywikibot/ folder. But if you search through build log, you could see there is also a bunch of other similar ones. Today, I wanted to clean "more than one target found for cross reference" warnings, but I found out many of them have this issue.

If there will be any problem, I'll write more detailed step-by-step quide to build our docs.

tk0miya commented 6 years ago

Confirmed with this Dockerfile:

FROM tk0miya/sphinx-html

RUN apt update; apt install -y libmysqlclient-dev
RUN git clone https://gerrit.wikimedia.org/r/pywikibot/core.git
WORKDIR /docs/core
RUN git submodule update --init
RUN pip3 install tox
RUN sed -i -e 's/python3.4/python3.5/' tox.ini
RUN tox -e doc

I understand the issue.

But half of them should be never thrown, because Sphinx is not sure, whether to use current file's one or imported library's one.

At present, there are no way to tell the objects in the file to Sphinx. The autodoc extension only generates reST file. And Sphinx-core only converts it to output format.

I feel your proposal is reasonable. So it would be nice if we can implement it in future!

lwasser commented 3 years ago

hi there! i'm curious about this thread as i'm running into a similar issue with a pr for nbconvert . what is weird is that it's throwing warnings only in circleCI not in my local build and also on pages that are just text but have api content below such as this page. and it's not throwing warnings (treated as errors) each run, it throws ONE of them and quits. then i fix it and another pops up. Does anyone have a suggestion regarding how to treat these warnings by chance? @tk0miya it would be wonderful to have a clean CI build.

mezhaka commented 3 years ago

Is there any workaround at the moment? I have classes with the same name in different modules in my project and I get the error (removed acutal names):

Warning, treated as error:
/opt/.../client.py:docstring of BarBaz.function::more than one target found for cross-reference 'ClassA': module_x.ClassA, module_z.ClassA

Am I forced to rename the classes now or is there a way to ignore this particular situation?

ewjoachim commented 3 years ago

On my project, the doc builds correctly for Sphinx 3.x and fails (because warnings treated as errors) on 4.x. I'll need to stay on 3.x until it's resolved, but if you have an idea where to look for, I can try poking and see if I find a fix :)

abhinavsingh commented 2 years ago

This bug out of no where has started impacting us from yesterday https://github.com/abhinavsingh/proxy.py/runs/4836507299?check_suite_focus=true

It all started after we add a file named pool.py in a separate module. A file name pool.py already exists in another module. But Sphinx cannot figure this out leading into warnings and failed CI.

ewjoachim commented 2 years ago

There's a similar but different issue causing the same messages at #10088 and an ongoing PR in #10089 . Is there a chance it solves your issue ?

abhinavsingh commented 2 years ago

@ewjoachim Yep, that worked. When can we expect the next release (or pre-release). We'll need to remove the diff below at some point.

diff --git a/docs/requirements.in b/docs/requirements.in
index 24f2e03..263030d 100644
--- a/docs/requirements.in
+++ b/docs/requirements.in
@@ -1,6 +1,6 @@
 myst-parser[linkify] >= 0.15.2
 setuptools-scm >= 6.3.2
-Sphinx >= 4.3.0
+Sphinx @ git+https://github.com/ewjoachim/sphinx@py-domain-canonical-any-10088
 furo >= 2021.11.15
 sphinxcontrib-apidoc >= 0.3.0
 sphinxcontrib-towncrier >= 0.2.0a0

abhinavsingh commented 2 years ago

There's a similar but different issue causing the same messages at #10088 and an ongoing PR in #10089 . Is there a chance it solves your issue ?

Sorry bad news is that CI still failed , ref https://github.com/abhinavsingh/proxy.py/runs/4844619931?check_suite_focus=true

Unfortunately, I am not sure about our setup either. There is a .in and .txt requirements file. I have consulted the contributor who added this workflow and waiting upon them.

abhinavsingh commented 2 years ago

Update:

Looks like we cannot try the patch out until release. Other CI runs starts to throw unable to verify hashes from private repo. Also, I am unsure if the patch completely fixes the issue.

Latest run https://github.com/abhinavsingh/proxy.py/runs/4845237618?check_suite_focus=true

We are also seeing newer could be replaced by an extlink (try using ':issue:`642#issuecomment-960819271`' instead) errors. All in all, we have 97 new warnings :(

rmorshea commented 2 years ago

Before Sphinx had a better solution for resolving type annotations I created a package called sphinx-resolve-py-references that did some static analysis to disambiguate what was being referred to. It basically traces the imports back to the original definition. The core of the Sphinx related logic can be found here and the static analysis via the AST can be found here. Perhaps this is something that could be contributed to the core of sphinx?

Depending on how it's implemented it would also mean that you could reference classes imported in the same module without having specify the full path:

from some_package import MyClass

def my_function():
    '''Uses :class:`MyClass`'''
    # The above reference would normally fail because `MyClass`
    # is not defined in this module. However this extension
    # automatically resolves the reference

ssbarnea commented 2 years ago

Apparently Sphinx gets really confused about the new annotations in python and assumes that classes are defined in the files that in fact are only using them as types.

from __future__ import annotations
from foo import Bar

x: Bar  # <-- Guess what, now sphinx believes that Bar is defined in this file too

ewjoachim commented 2 years ago

Might be related or not, but https://github.com/sphinx-doc/sphinx/pull/10089 was recently merged, and maybe it's involved in the change of behaviour you observed. Let me know: if I as the one breaking things, I'd love to know and try to fix.

ssbarnea commented 2 years ago

@ewjoachim Unrelated as I was already using 5.1.1 which included that fix. I found as a workaround to avoid using annotations for these types, like:

from typing import TYPE_CHECKING
if TYPE_CHECKING:
  from foo import Bar

x: "Bar"

UPDATE: While this workaround worked in few cases, i reached others where it does not seem to work.

AA-Turner commented 2 years ago

@ssbarnea do you have a consistently failing reproducer?

A

ewjoachim commented 2 years ago

Unrelated as I was already using 5.1.1

It's the other way around. My question is whether or not the fix included in 5.1.1 introduced the issue.

Also, you may want to look at from __future__ import annotations :)

leycec commented 1 year ago

@rmorshea: Your sphinx-resolve-py-references fork is extremely impressive. Sadly, it's also unmaintained – which is no slight on your breathtaking hard work. I wouldn't want to maintain a non-trivial Sphinx monkey patch in perpetuity, either. :sweat_smile:

Sadly, I incorrectly assumed Sphinx resolved relative references against the current module out-of-the-box and then documented a several thousand-line runtime type-checker under that assumption. Several hundred sphinx-build warnings later, I now realize this is where the venerable saying about assumptions comes from. I briefly contemplated inlining the core logic of @rmorshea's sphinx-resolve-py-references fork into @beartype's conf.py configuration. Then I realized that meant I would need to maintain a non-trivial Sphinx monkey patch in perpetuity. The sweat pouring off my forehead is real, people.

I'm now furiously canonicalizing all relative references into absolute references. Since this is terrible, I find myself pounding the keyboard repeatedly. Is there really no sane solution for this four year-old issue in 2022? This should really just work by default:

from some_package import MyClass

def my_function():
    '''Uses :class:`MyClass`'''

EDIT: We gave up on canonicalizing all relative references into absolute references. Instead, we just ruthlessly silenced all several hundred warnings with a trivial workaround shamelessly pilfered from @RDFLib:

# List of zero or more Sphinx-specific warning categories to be squelched (i.e.,
# suppressed, ignored).
suppress_warnings = [
    #FIXME: *THIS IS TERRIBLE.* Generally speaking, we do want Sphinx to inform
    #us about cross-referencing failures. Remove this hack entirely after Sphinx
    #resolves this open issue:
    #    https://github.com/sphinx-doc/sphinx/issues/4961
    # Squelch mostly ignorable warnings resembling:
    #     WARNING: more than one target found for cross-reference 'TypeHint':
    #     beartype.door._doorcls.TypeHint, beartype.door.TypeHint
    #
    # Sphinx currently emits *HUNDREDS* of these warnings against our
    # documentation. All of these warnings appear to be ignorable. Although we
    # could explicitly squelch *SOME* of these warnings by canonicalizing
    # relative to absolute references in docstrings, Sphinx emits still others
    # of these warnings when parsing PEP-compliant type hints via static
    # analysis. Since those hints are actual hints that *CANNOT* by definition
    # by canonicalized, our only recourse is to squelch warnings altogether.
    'ref.python',
]

That'll do, Sphinx. That'll do.

rmorshea commented 1 year ago

@leycec if there's interest, I'd be willing to update the package. I just merged some minor updates (unreleased) but it's hard to know if I've covered all the edge cases that people may have in their own projects.

AA-Turner commented 1 year ago

@rmorshea would you be interested in merging it to the Sphinx core?

A

rmorshea commented 1 year ago

That would be great. Though, it would be useful if someone more familiar with Sphinx took a look at it to see if all the heuristics I came up with make sense. It's also not the cleanest code I've ever written, so it might require some work on my end to clean things up a bit before that review.

martinpakosch commented 1 year ago

Hi @rmorshea,

I came across this issue as I am affected myself and across your extension. I gave it a try, simply pulled the source into my venv as I wanted to try the latest updates. ;-)

However, as soon as I activate this extension I get additional warnings on my builds while the cross-reference warnings still persist. My new warnings look like this:

WARNING: No referent for name 'mylib.core.base.BaseDataAPI' in 'C:\\<...>\\src\\mylib\\resources\\something\\mymodule.py:docstring of mylib.resources.something.mymodule.MySpecificAPI' via module 'mylib.resources.something'

The question is, what does this mean? My structure:

mylib.resources.backbone.mymodule.MySpecificAPI

This is a class in mymodule.py.
The docstring contains no references at all.
The class is a subclass of BaseDataAPI, and that's the only reference to BaseDataAPI.
BaseDataAPI is imported in mymodule.py with from ...core.base import BaseDataAPI.

mylib.core.base.BaseDataAPI

This is a class in base.py.
Regarding MySpecificAPI there is no more to say - its simply a subclass and its unknown/unrelated from BaseDataAPI class's perspective.

My Env:

Python 3.7
Sphinx 5.2.3

@AA-Turner Regarding the cross-reference warnings (to share some experience): My usecase is fairly simple. Example with one warning regarding my custom exception InvalidError:

Primary access: mylib.exceptions.errors.InvalidError
Secondary access: mylib.exceptions.InvalidError, provided via import to simplify public API structure.
Referenced in docstring (example): mylib.resources.something.mymodule.MySpecificAPI as relative reference: :raises: InvalidError. It is imported via from ...exceptions import InvalidError.

So this is fairly simple in terms of complexity. My expectation would be, that Sphinx sees this docstring reference, tries to find InvalidError locally in module.py where it is referenced, then looks at the imports and traces it back to its secondary access point. But without warnings. :D Hopefully this will work at least with rmorshea's extension after it's included to the Sphinx core. Right now it's not working in my case.

Kind regards, Martin

rmorshea commented 1 year ago

Hey @martinpakosch, I made an issue in my repository where I'll answer this question. For others who have questions specifically about my extension it seems like it would be best to post an issue there rather than in this thread.

martinpakosch commented 1 year ago

Cool, thx @rmorshea. Was not able to create it directly due to the previous "archive" state of your repo. :D

ShaheedHaque commented 1 year ago

@AA-Turner @rmorshea Any thoughts on progressing a merge of this code?

sigma67 commented 1 year ago

What solved this issue for me is setting the following autodoc option in conf.py:

autodoc_default_options = {
    'ignore-module-all': True
}

https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html#directive-option-automodule-members

sphinx-doc / sphinx