seddonym / import-linter

Import Linter allows you to define and enforce rules for the internal and external imports within your Python project.
https://import-linter.readthedocs.io/
BSD 2-Clause "Simplified" License
664 stars 45 forks source link

Add implicit import of a module's parent #203

Open neilmacintyre opened 10 months ago

neilmacintyre commented 10 months ago

If __init__.py imports a forbidden model (or imports a modules that imports a forbidden model) when I import another module from that package, the forbidden model will be imported but import-linter does not detect it.

For example if I have the following contract:

[importlinter]
root_package=src
include_external_packages=True

[importlinter:contract:1]
name=No math
type=forbidden
source_modules=src.module2
forbidden_modules=
   math

And a project with the following structure:

└── src
    ├── __init__.py
    ├── module1
    │   ├── __init__.py
    │   └── test.py
    └── module2
        └── __init__.py

Where src/__init__.py and src/module1/test.py are empty and src/module2/__init__.py is:

from src.module1 import test

and src/module1/__init__.py is:

import math

Running python -X importtime -c "import src.module2", we see that src.module2 imports math but when I run lint-imports it does not show any contracts being broken.

import time: self [us] | cumulative | imported package
import time:       147 |        147 |   src
import time:       330 |        330 |     math
import time:       217 |        547 |   src.module1
import time:       171 |        171 |   src.module1.test
import time:      1512 |       2375 | src.module2
seddonym commented 10 months ago

Interesting issue!

As I understand it, what you're expecting to happen is that because Python executes the __init__.py of parents when importing a module, Import Linter should treat those as imports too. Or to put it another way, any module implicitly imports its parent - e.g. src.module1.test implicitly imports src.module1.

This is - at the moment at least - by design. Import Linter (and, under the hood, Grimp) models the import graph based on explicit imports. So src.module1 is not a dependency of src.module1.test.

We could change this, but it's a fairly fundamental change of what imports mean, so right now I'm reluctant to. For me, there's a simplicity in just considering explicitly defined imports. Import Linter is a static analysis library and so it isn't trying to model what happens at runtime anyway.

If we were to do it, the easiest option would be to provide an additional top-level argument when building the graph, along these lines:

[importlinter]
root_package = mypackage
add_parent_imports = True

I'm on the fence as to whether that would be a good change though. If you really need this functionality you could create your own custom contract type based on the forbidden contract, which manually adds an import from every child to its parent using graph.add_import.

neilmacintyre commented 10 months ago

Thanks again. That makes sense and the suggestion to make a custom contract type sounds like a good idea.