py2puml icon indicating copy to clipboard operation
py2puml copied to clipboard

fix crash trying to extend None list member

Open wakamex opened this issue 1 year ago • 5 comments

attempting py2puml elfpy elfpy > elfpy.plantuml on https://github.com/element-fi/elf-simulations/tree/2349ebf365f5c3c589ad42d85a263a241010dac7

Traceback (most recent call last):
  File "/Users/mihai/.pyenv/versions/py2puml/bin/py2puml", line 8, in <module>
    sys.exit(run())
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/cli.py", line 24, in run
    print(''.join(py2puml(args.path, args.module)))
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/py2puml.py", line 12, in py2puml
    inspect_package(
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/inspection/inspectpackage.py", line 20, in inspect_package
    inspect_module(
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/inspection/inspectmodule.py", line 60, in inspect_module
    inspect_domain_definition(definition_type, root_module_name, domain_items_by_fqn, domain_relations)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/inspection/inspectmodule.py", line 47, in inspect_domain_definition
    inspect_class_type(
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/inspection/inspectclass.py", line 103, in inspect_class_type
    instance_attributes, compositions = parse_class_constructor(class_type, class_type_fqn, root_module_name)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/parseclassconstructor.py", line 42, in parse_class_constructor
    visitor.visit(constructor_ast)
  File "/Users/mihai/.pyenv/versions/3.10.10/lib/python3.10/ast.py", line 418, in visit
    return visitor(node)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 103, in generic_visit
    NodeVisitor.generic_visit(self, node)
  File "/Users/mihai/.pyenv/versions/3.10.10/lib/python3.10/ast.py", line 426, in generic_visit
    self.visit(item)
  File "/Users/mihai/.pyenv/versions/3.10.10/lib/python3.10/ast.py", line 418, in visit
    return visitor(node)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 113, in visit_FunctionDef
    self.generic_visit(node)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 103, in generic_visit
    NodeVisitor.generic_visit(self, node)
  File "/Users/mihai/.pyenv/versions/3.10.10/lib/python3.10/ast.py", line 426, in generic_visit
    self.visit(item)
  File "/Users/mihai/.pyenv/versions/3.10.10/lib/python3.10/ast.py", line 418, in visit
    return visitor(node)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 150, in visit_Assign
    self.extend_relations(full_namespaced_definitions)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 85, in extend_relations
    self.uml_relations_by_target_fqn.update({
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 88, in <dictcomp>
    if target_fqn.startswith(self.root_fqn) and (
AttributeError: 'NoneType' object has no attribute 'startswith'

wakamex avatar Apr 01 '23 02:04 wakamex

hello @wakamex, thank you for reporting the issue.

I should first investigate why target_fqn is None, this should not happen. I don't have much time to investigate the issue this week, I need some time. Forgive me in advance to keep you waiting :pray:

lucsorel avatar Apr 03 '23 15:04 lucsorel

the cause is assignments in base.py: https://github.com/element-fi/elf-simulations/blob/2349ebf365f5c3c589ad42d85a263a241010dac7/elfpy/markets/base.py#L108-L109

        self.market_state = market_state
        self.block_time = block_time

replacing our init function with the following also solves the problem (without requiring a tweak to py2puml):

    pricing_model: PricingModel
    market_state: State
    block_time: time.BlockTime

    def __init__(self, **kwargs):
        for key, value in kwargs.items():
            setattr(self, key, value)

along with moving theimport elfpy.time as time import outside of the if TYPE_CHECKING condition (diff https://github.com/element-fi/elf-simulations/commit/f8bc81394398f9f98891fa5aef7dda1b55ed47a7)

I'm not sure why this, but I suspect it's because we're doing something totally-not-good with our type hints

hopefully you understand this better than I do 😄

I added a commit here with the print statements I used to arrive at this conclusion

wakamex avatar Apr 03 '23 18:04 wakamex

the diff between the two resulting plantuml's https://github.com/element-fi/elf-simulations/commit/f22a7deb4ce451444e86ebef6774e87cc357319a

wakamex avatar Apr 03 '23 19:04 wakamex

thanks for investigating the issue :pray:

The main reason of the issue seems this:

moving the import elfpy.time as time import outside of the if TYPE_CHECKING condition

Indeed, py2puml combines 2 techniques when handling native Python classes:

  • it parses the abstract syntax tree (the source code) of the __init__ constructors to detect the self.attr = attr assignments, looking for the type annotation of attr (in the constructor signature) to affect it to self.attr in the PlantUML diagram
  • it imports the module of the class to understand the type of self.attr in order to draw relationships between your classes in the UML diagram

In you case, the type associated to block_time is not defined at the module level (in an import done at the top of the file) but dynamically when some code executes. Therefore, py2puml cannot assign a type to self.block_time. Dynamically importing modules is not a recommended practice and I didn't know that some projects actually do that (and there may be some good reasons to do it; I am not judging, I am only saying that it is a practice py2puml does not handle).

If you can, I would recommend to move such dynamic imports at the top of the module and restore your original constructor (with self.attr = attr assignments) so that py2puml can yield the proper class diagram: these properties are instance attributes, not class attributes.

I should definitely think of a safety net for these cases, such as skipping the potential relationship between this class and the attribute's class. Maybe by introducing a default fail-fast mode, and an optional fail-proof mode (producing a degraded class diagram).

Side remark: please, follow the contribution guidelines about the use of quotes and the formatting as described in https://github.com/lucsorel/py2puml/blob/main/CONTRIBUTING.md#code-styling :pray: Your IDE settings seems to reformat all the files you modified and replace single quotes with double quotes, making the codebase inconsistent and hard for me to read. And it makes the pull-request harder to understand.

I realize that I should add pre-commit hooks to the project in order to avoid these troubles now that several people are eager to contribute (and thanks again for reporting the issue) :smiley:

lucsorel avatar Apr 04 '23 20:04 lucsorel

hi @wakamex :smiley:

Did you manage to fix the crash by placing the import at the top of the Python file (without modifying the py2puml code)?

lucsorel avatar Apr 18 '23 14:04 lucsorel