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

Better help in diagnosing circular imports. #12338

Closed
mrolle45 opened this issue Mar 12, 2022 · 2 comments
Closed

Better help in diagnosing circular imports. #12338

mrolle45 opened this issue Mar 12, 2022 · 2 comments
Labels

Comments

@mrolle45
Copy link

Feature

When a reported error is due to some circular imports, the error report should have notes about the circularity.
The Common Issues and Solutions document should go into detail about how import statements affect the order of processing of modules, and how a cycle could cause a module to be checked before the module it is importing from.

Pitch

I spent a couple of days figuring out why an import was reported as an error, when the name being imported was indeed present. I wound up downloading the mypy code and debugging it. I saw that there were 20-odd modules that were all doing from x import ... from each other, but I had to add some code to find an import chain from the imported module back to the importing module. Finally, I found the link in the cycle that I could remove, and then the type check ran smoothly.

FYI, it was something like module A says from B import *, and so on from B to C to D to E. And E says from A import foo. I actually had this last statement guarded by an if TYPE_CHECKING: as was suggested in the docs. However I also had the same statement elsewhere in E without the guard; this was the statement I removed.

Details

I do not expect mypy to figure out every case of the consequences of circular imports. Some cases are correct at runtime and pass mypy as well.
However, when an imported name foo is not found in an imported module B, mypy should at least recognize that
B hasn't been processed yet. This might be accomplished with a flag to that effect (maybe there is one already, for all I know). Or else mypy can look for an import chain from B back to A.
When this condition is recognized, the import statement visitor for A should add notes to the error report. These would explain that because of a circular import, mypy could not determine the names for B. They would also show one import path from B back to A and state that there could be other paths as well.
The import chain will only include dependencies that are at least as strong as (the priority <=) the dependency from A to B. It might also have a link to more detailed documentation on readthedocs.

Documentation

The Common issues and solutions section need more detail about circular imports. In fact, it should have a section for Errors reported for obviously correct code, possibly right below the one for No errors reported for obviously wrong code. One item in that section would be for missing imported module attribute or an undefined name which was imported, when the imported module indeed defines that name.
A suggestion would be that a circular import may be involved (even if not suggested by mypy when reporting the error). Explain that there are several levels of strength of dependency, and enumerate them. They correspond to the PRI_* constants in build.py. The principle is that the order of processing of A and B is that if there is a dependency chain direction only from A to B, then Bwill always be checked first. If there are chains both ways, then the strength of the dependency is the weakest one in the chain, and the strongest of possibly multiple such chains. That is, strength is a transitive property. If both chains have the same strength, then the order of processing is arbitrary, and whichever one is processed first is out of luck. If both chains have different strengths, then the stronger dependency determines the order of processing and the weaker dependency is disregarded. An import guarded byif TYPE_CHECKING:` is a good way of breaking the circularity, but provided that (1) it is not used circularly, and (2) there is not some stronger path. (see my case above where I had a guarded import and the same import unguarded).

Implementation

One thing that would help implement these features would be to define a BuildGraph class which encapsulates the graph parameters passed around various methods and functions. Unfortunately, the import statement checker doesn't have access to this graph. It would be helpful to have this object stored in the BuildManager. It would also contain the methods for ordering the processing of the modules. It would no doubt make the code clearer.
I don't know if the graph would possibly be different between iterations, but I would guess that the basic structure of the import dependencies between modules is statically determined by the code in those modules. In that case, the manager would only have to create it once. If not, there be ways that the graph could be updated in place.

@97littleleaf11
Copy link
Collaborator

Thanks for your reporting. We have a related discussion here: #8584.

@mrolle45
Copy link
Author

Another case that bit me, and I needed to find the cycle of imports and break it...
Two modules A and B. A has from B import Foo. B has from A import * and a definition for class Foo.
The reported error says Name "Foo" already defined (possibly by an import). It happens that A is analyzed first (first pass), and so it has Foo in its names. Then B imports Foo from A, and the definition for class Foo is a duplicate.
In this situation, a description of how the duplicate name actually got there would save the poor programmer a lot of trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants