Pyreverse: Aggregation/compostion doesn't work across modules.

[bMorgan01]

Bug description

I have a project with this structure:

pyreverse_test
     __init__.py
     a.py
     b.py

a.py contains:

class A:
    def __init__(self) -> None:
        self.var = 2

b.py contains:

from a import A

class B:
    def __init__(self) -> None:
        self.a_obj: A = A()

Running pyreverse yields this result:

Configuration

No response

Command used

Working directory: pyreverse_test

pyreverse . -f ALL -o png

Pylint output

Format png is not supported natively. Pyreverse will try to generate it using Graphviz...
Analysed 3 modules with a total of 1 imports

Expected behavior

I expect the result to match this, un-separated, code.

class A:
    def __init__(self) -> None:
        self.var = 2

class B:
    def __init__(self) -> None:
        self.a_obj: A = A()

Which is this:

Pylint version

pylint 3.0.2
astroid 3.0.1
Python 3.10.11 (tags/v3.10.11:7d4cc5a, Apr  5 2023, 00:38:17) [MSC v.1929 64 bit (AMD64)]

OS / Environment

Windows 10 / powershell

Additional dependencies

No response

[nickdrozd]

This works in some cases but not others. I have no idea why.

[Julfried]

This issue is interesting - I found that PyReverse correctly detects the aggregation relationship with relative imports (using dot notation: from .a import A), but may fail with non-relative imports (from a import A). I think this is expected and I am not sure if this is really a bug. What do you think?

[Pierre-Sassoulas]

In my opinion we should "normalize" the import and treat both way of importing the same in pyreverse if the final result is equivalent in python. Let's hear other opinions before taking a decision.

[Julfried]

Agree, I think we should treat the imports "normalized"

[Julfried]

Actually thinking about it more , we should not normalize the paths. Looking at the provided example this is actually invalid python code as far as I understand the import system. When running this test script:

from pyreverse_test import b

test = b.B()

I get an import error:

Traceback (most recent call last):
  File "c:\WORKSPACE\pylint\test.py", line 1, in <module>
    from pyreverse_test import b
  File "c:\WORKSPACE\pylint\pyreverse_test\b.py", line 1, in <module>
    from a import A
ModuleNotFoundError: No module named 'a'

And this actually makes sense because you are trying to absolut import the class from within the package context. Using relative imports (from .a import A) makes the script run without any errors and pyreverse also produces the expected output

Output from pyreverse .\pyreverse_test\ -o mmd -f ALL:

classDiagram
  class A {
    var : int
    \_\_init\_\_() None
  }
  class B {
    a_obj
    \_\_init\_\_() None
  }
  A --* B : a_obj

In my opinion pyreverse should focus on clear error messages when imports can't be resolved, instead of silently normalizing invalid imports. @Pierre-Sassoulas What do you think?

[Pierre-Sassoulas]

Yes, 'from pyreverse_test.a import A' should work the same as a relative import 'from .a import A', but wrong imports shouldn't work.

[Julfried]

Can confirm. Both from .a import A and from pyreverse_test.a import A produce the correct output.

The invalid import from a import A produces this output as expected:

classDiagram
  class A {
    var : int
    \_\_init\_\_() None
  }
  class B {
    a_obj
    \_\_init\_\_() None
  }

[bMorgan01]

The invalid import from a import A produces this output as expected:

Why is from a import A invalid?

Edit: saw your error traceback from earlier. I dont know if this is a "works on my machine" situation, but that's how I've always done local project imports

I think if you put the main function in b.py instead, the import would work as expected

[Pierre-Sassoulas]

@Julfried if you send the message before the image is loaded by github it doesn't wait for the upload to happens :D

'from a import A' is wrong because it doesn't work unless you add the pyreverse_test directory to the python path.

[Julfried]

Ups sorry, it seems that the mermaid renderer does not work on mobile. I fixed that, now it should just write the mermaid code.

@bMorgan01 from a import A is invalid if used in a package context, like in your example. If you add __init__.py python treats the folder as a package. This explains the difference.

[bMorgan01]

With this project:

test
├── __init__.py      # I tested with and without and it worked in both cases
├── a.py
├── b.py
└── main.py

a.py:

class A:
    def __init__(self):
        self.var = 2

b.py:

from a import A

class B:
    def __init__(self):
        self.a_obj = A()

main.py:

from b import B

def main():
    print("Value of var: ", B().a_obj.var)

if __name__ == "__main__":
    main()

    /repos/test
··• python main.py 
Value of var:  2

I get Value of var: 2 as expected.

This also works if __init__.py is not included.

I think this a very valid way of managing a small python project, and I think pyreverse should be able to support it.

[DudeNr33]

tl;dr: pyreverse prodcues the expected output if you add --source-roots ./ to the command. I believe this is the only viable option to do it, as changing the default behavior of Pylint/Pyreverse would introduce breaking changes.

Python itself and Pylint behave differently regarding what directory is added to sys.path if --source-roots is not specified. That's why the example works when invoking Python, but not when running Pyreverse.

• Python will add the parent directory of the invoked script file, or the current directory if invoked without a module (e.g. when running with the -m option)
• Pylint will search from the current directory upwards and add the first directory without an __init__.py file

Thus Pyreverse will add not the test directory, but its parent to sys.path and will then fail to resolve the absolute import - at least for the case where test/__init__.py is present. Without test/__init__.py it will add test to sys.path, but crashes afterwards because it tries to import it.

This logic for appending to sys.path can't be changed easily. If you have a project with a package structure (i.e. subdirectories containing __init__.py files), Pylint should be able to lint them when invoking pylint mypkg/subpkg/module.py.
Notice that running python mypkg/subpkg/module.py would fail if you tried to use absolute imports in module.py. That's because it would only add mypkg/subpkg to sys.path, while Pylint (and therefore also Pyreverse) will walk up the directories until it finds the parent directory of mypkg.

[bMorgan01]

tl;dr: pyreverse prodcues the expected output if you add --source-roots ./ to the command. I believe this is the only viable option to do it, as changing the default behavior of Pylint/Pyreverse would introduce breaking changes.

That's a super clean solution! It would've meant (two years ago) that I didn't have to redo every import in my project with many files.

Getting nice UML output wasn't that important to me at the time, so I just gave up.

[Julfried]

I guess this can be closed then