Refactor pyreverse Association Logic

[Julfried]

Type of Changes

	Type
✓	🐛 Bug fix
✓	🔨 Refactoring

Description

Closes #9045
Closes #9267

Summary

Fixed incorrect UML semantics in pyreverse for aggregation vs composition relationships. The previous implementation had no explicit CompositionsHandler class and did not differentiate between Aggregation and Composition properly, treating most relationships as aggregation by default.

Problem

Pyreverse previously lacked proper UML relationship semantics. So I would proposes the following distinction for Python class relationships:

fields.py

# Test for https://github.com/pylint-dev/pylint/issues/9045

class P:
    pass

class A:
    x: P  # just type hint, no ownership → Association

class B:
    def __init__(self, x: P):
        self.x = x  # receives object, not created → Aggregation

class C:
    x: P
    def __init__(self, x: P):
        self.x = x  # receives object, not created → Aggregation

class D:
    x: P
    def __init__(self):
        self.x = P()  # creates object → Composition

class E:
    def __init__(self):
        self.x = P()  # creates object → Composition

fields.mmd

classDiagram
  class A {
    x
  }
  class B {
    x
  }
  class C {
    x
  }
  class D {
    x
  }
  class E {
    x
  }
  class P {
  }
  P --> A : x
  P --* D : x
  P --* E : x
  P --o B : x
  P --o C : x

Changes Made

1. Added CompositionsHandler

Created dedicated handler following chain of responsibility pattern:
CompositionsHandler → AggregationsHandler → AssociationsHandler

2. Updated Printers

• Added EdgeType.COMPOSITION enum value
• Updated MermaidJS and PlantUML printers with correct arrows per Mermaid docs
• For the Dot printer I used plain arrows, but I did not manage to find something in the dot language documentation

3. Prevented Duplicate Relationships

Modified extract_relationships() to process relationships by priority and avoid duplicates.

Currently I only made the modifications needed to implement the new Associations Logic. If you like the proposed distinction I would work on making the tests pass :)

[DudeNr33]

I like the addition! 👍

I only left two remarks for cases where at least from the comment it seems like the relationship is processed by the wrong handler. Am I missing something?

It would be great if you can add tests for all possible output formats as all printers where changed. You can specify multiple output formats for a single functional test in the [testoptions].

[Julfried]

Thanks for the review. I agree that the logic is not complete and it needs some more work and I need to figure out all the possible edge cases. Also I think some of the other tests have changed , and I need to rebase aswell. I think this will take some time, but lets see how it goes :)

[DudeNr33]

@Julfried please ping me once the PR is ready to be reviewed again. No hurry though, just want to point this out as I am not actively monitoring the open PRs and instead rely more on the email notifications. 🙂

[codecov]

Codecov Report

❌ Patch coverage is 98.26087% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.88%. Comparing base (25a0f9e) to head (d275c2f).
⚠️ Report is 44 commits behind head on main.

Files with missing lines	Patch %	Lines
pylint/pyreverse/inspector.py	97.97%	2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (98.26%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10397      +/-   ##
==========================================
- Coverage   95.91%   95.88%   -0.03%     
==========================================
  Files         176      176              
  Lines       19147    19227      +80     
==========================================
+ Hits        18364    18436      +72     
- Misses        783      791       +8     

Files with missing lines	Coverage Δ
pylint/pyreverse/diagrams.py	92.85% <100.00%> (-0.76%)	⬇️
pylint/pyreverse/dot_printer.py	85.71% <ø> (ø)
pylint/pyreverse/mermaidjs_printer.py	98.43% <ø> (ø)
pylint/pyreverse/plantuml_printer.py	100.00% <ø> (ø)
pylint/pyreverse/printer.py	100.00% <100.00%> (ø)
pylint/pyreverse/writer.py	98.93% <100.00%> (-1.07%)	⬇️
pylint/pyreverse/inspector.py	86.11% <97.97%> (+4.89%)	⬆️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Julfried]

@DudeNr33 Finally ready for review again. All functional tests are passing and the association logic is working correctly now.

Interestingly, I figured while playing around that this PR also fixes #10333 (duplicate arrows issue) as a side effect. The root cause was the same - improper relationship classification led to the same attribute being processed multiple times across different relationship types. Our priority-based processing with duplicate prevention solves this at the source.

This example from #10333:

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

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

    def func(self):
        self.a_obj = A()
        self.a_obj = A()

Now correctly produces single composition relationship instead of duplicates:

classDiagram
  class A {
    var : int
  }
  class B {
    a_obj
    func()
  }
  A --* B : a_obj

Should I add the functional test cases from #10333 to this PR, or handle them in a follow-up? The PR is getting quite large, but its up to you.

The core association logic is solid and ready for review either way.

[Pierre-Sassoulas]

Thank you, that looks pretty good !

[Pierre-Sassoulas]

I think it's better to add functional test cases for the other issue here as you fixed it here. Usually we do another MR because we don't realise an issue is fixed until later.

[Julfried]

I updated the functional tests, so now this also Closes #9267

The example from #8046 currently produces this output:

@startuml classes_toy_code
set namespaceSeparator none
class "ExampleClass" as parser.toy_code.ExampleClass {
  example1 : Optional[int]
  example2 : Optional[int]
}
@enduml

However I think this is not the output the was discussed in the issue, since the class level attributes are still not handled correctly.

Let me know what you think :)

[Pierre-Sassoulas]

LGTM, although I'll let @DudeNr33 review the fine details. Great work clearly a lot of care went into this. Could you try to cover the missing lines maybe ?

[Julfried]

Thanks for the review, I appreciate the kind words :)

I remember the patch coverage initially dropped because I updated a base class, and only two lines were uncovered at that point. Now I see more lines are reported as missing — could this be because my branch is not up to date with main? Would it make sense to try a rebase to see if that clears it up?

[Pierre-Sassoulas]

Yes, good idea !

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 04f4b2e