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

False positive circular dependency detection #9175

Open
guyarad opened this issue Oct 22, 2023 · 4 comments
Open

False positive circular dependency detection #9175

guyarad opened this issue Oct 22, 2023 · 4 comments
Labels
Bug 🪲 Needs investigation 🔬 A bug or crash where it's not immediately obvious what is happenning

Comments

@guyarad
Copy link

guyarad commented Oct 22, 2023

Bug description

The code is given in this repository.
The detection root cause is the Step class definition. However, since we import the class first in the package's __init__.py file, the import is not circular by Python's rules.

We found 2 ways to solve it.

  1. Reasonable solution - instead of import the base class from the package level, import directly from the containing module.
  2. Unclear solution - adding __init__.py at the repository root level. Not sure why this makes the issue disappear.

To summarize - there are 2 related issues here:

  1. The circular dependency detection is not accurate. Demonstrated by make test.
  2. One of the mitigations to the issue is unexpected (if the issue is real, I wouldn't expect adding that file will resolve it).

To reproduce the issue and show the 2 resolutions work, simply run make verify_resolutions in the repository.

Related issue

Configuration

No response

Command used

pylint --recursive=y --disable=W,C0114,C0115,C0116,R0903 -s=n .

Pylint output

************* Module module1.derived
module1/derived.py:1:0: R0401: Cyclic import (module1 -> module1.derived) (cyclic-import)

Expected behavior

  1. Probably shouldn't identify as circular
  2. If identified as circular, unclear why adding __init__.py at the repository root make the issue disappear.

Pylint version

pylint 3.0.1
astroid 3.0.1
Python 3.11.6 (main, Oct  3 2023, 03:39:03) [GCC 10.2.1 20210110]

OS / Environment

Mac and Linux (both in a docker and in the host)

Additional dependencies

No response

@guyarad guyarad added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Oct 22, 2023
@Pierre-Sassoulas
Copy link
Member

Thank you for the reproducer, it's hard to do anything with circular import otherwise :) my intuition would be something linked to import order (maybe similar to #6535?), but this definitely need investigations.

@Pierre-Sassoulas Pierre-Sassoulas added Bug 🪲 Needs investigation 🔬 A bug or crash where it's not immediately obvious what is happenning and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Oct 22, 2023
kernicPanel added a commit to openfun/marsha that referenced this issue Oct 23, 2023
With pylint update to version 3, some false positives related to cyclic
imports have appeared.
This is probaly related to pylint-dev/pylint#9175
We choose to ignore them in __init__.py files.
@guyarad
Copy link
Author

guyarad commented Oct 23, 2023

@Pierre-Sassoulas I'm not sure it's directly related. pylint is correct in the sense derived module is importing the base class from the __init__ file. And the init file also imports the derived class. But it works in runtime, because the base class import in the init file is before the derived import. The issue you linked seems to be misidentification of packages.

Just to clarify, I don't especially mind that pylint identifies this as a circular, because as I mentioned, it technically is. The fact something works fine in runtime doesn't mean we have to produce code like that :)
The point here is that adding an empty __init__.py file in the repository root solves - and that is really odd, and inconsistent. And that bothers me.

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Oct 23, 2023

Ha, my bad, it's working as intended then. (Similar to #9124). Still need to fix the inconsistencies though.

@guyarad
Copy link
Author

guyarad commented Oct 30, 2023

@Pierre-Sassoulas from Python's perspective it's legal. So it should be ok. Anyway, I'm more bothered with the inconsistency of the detection and its resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 Needs investigation 🔬 A bug or crash where it's not immediately obvious what is happenning
Projects
None yet
Development

No branches or pull requests

2 participants