SONARPY-4016 Rule S8511 Multiple inheritance should not create Method Resolution Order (MRO) conflicts (#1046)

Co-authored-by: Marc Jasper <marc.jasper@sonarsource.com>
GitOrigin-RevId: 162f6b70593e6b0898303e9f1909388427d190f6

Author: 2 people

python-checks/src/main/java/org/sonar/python/checks/MultipleInheritanceMROConflictCheck.java

@@ -0,0 +1,173 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) SonarSource Sàrl
+ * mailto:info AT sonarsource DOT com
+ *
+ * You can redistribute and/or modify this program under the terms of
+ * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the Sonar Source-Available License for more details.
+ *
+ * You should have received a copy of the Sonar Source-Available License
+ * along with this program; if not, see https://sonarsource.com/license/ssal/
+ */
+package org.sonar.python.checks;
+
+import java.util.ArrayDeque;
+import java.util.ArrayList;
+import java.util.Deque;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import javax.annotation.CheckForNull;
+import org.sonar.check.Rule;
+import org.sonar.plugins.python.api.PythonSubscriptionCheck;
+import org.sonar.plugins.python.api.SubscriptionContext;
+import org.sonar.plugins.python.api.tree.ArgList;
+import org.sonar.plugins.python.api.tree.Argument;
+import org.sonar.plugins.python.api.tree.ClassDef;
+import org.sonar.plugins.python.api.tree.Expression;
+import org.sonar.plugins.python.api.tree.RegularArgument;
+import org.sonar.plugins.python.api.tree.Tree;
+import org.sonar.plugins.python.api.types.v2.ClassType;
+import org.sonar.plugins.python.api.types.v2.PythonType;
+import org.sonar.plugins.python.api.types.v2.TypeWrapper;
+
+@Rule(key = "S8511")
+public class MultipleInheritanceMROConflictCheck extends PythonSubscriptionCheck {
+
+  private static final String MESSAGE = "Reorder or remove base classes to fix this MRO conflict.";
+  private static final String SECONDARY_MESSAGE = "This base class is an ancestor of another listed base class appearing after it.";
+
+  @Override
+  public void initialize(Context context) {
+    context.registerSyntaxNodeConsumer(Tree.Kind.CLASSDEF, MultipleInheritanceMROConflictCheck::checkClassDef);
+  }
+
+  private static void checkClassDef(SubscriptionContext ctx) {
+    ClassDef classDef = (ClassDef) ctx.syntaxNode();
+    ArgList argList = classDef.args();
+    if (argList == null) {
+      return;
+    }
+
+    List<Expression> bases = collectPositionalBases(argList);
+    if (bases.size() < 2) {
+      return;
+    }
+
+    List<ClassType> types = new ArrayList<>();
+    for (Expression base : bases) {
+      types.add(resolveClassType(base));
+    }
+
+    int conflictIndex = findAncestorConflictIndex(types);
+    if (hasMroConflict(types, conflictIndex)) {
+      PreciseIssue issue = ctx.addIssue(classDef.name(), MESSAGE);
+      if (conflictIndex >= 0) {
+        issue.secondary(bases.get(conflictIndex), SECONDARY_MESSAGE);
+      }
+    }
+  }
+
+  private static List<Expression> collectPositionalBases(ArgList argList) {
+    List<Expression> bases = new ArrayList<>();
+    for (Argument argument : argList.arguments()) {
+      if (argument instanceof RegularArgument regularArgument && regularArgument.keywordArgument() == null) {
+        bases.add(regularArgument.expression());
+      }
+    }
+    return bases;
+  }
+
+  /**
+   * Detects an MRO conflict: either via C3 when every base is fully resolved, or otherwise only
+   * when {@link #findAncestorConflictIndex} finds an earlier base that is a strict superclass of a
+   * later base in our type graph (same structural issue as {@code class C(A, B)} with {@code B}
+   * subclassing {@code A}).
+   */
+  private static boolean hasMroConflict(List<ClassType> types, int conflictIndex) {
+    return isFullyResolved(types)
+      ? !ClassType.wouldHaveValidMro(types)
+      : (conflictIndex >= 0);
+  }
+
+  /**
+   * Returns {@code true} if every base class is fully resolved (non-null, with a fully known type
+   * hierarchy). Only when this is true can we run the complete C3 algorithm safely.
+   */
+  private static boolean isFullyResolved(List<ClassType> types) {
+    for (ClassType type : types) {
+      if (type == null || type.hasUnresolvedHierarchy()) {
+        return false;
+      }
+    }
+    return true;
+  }
+
+  /**
+   * Returns the index {@code i} of the first base class that is an ancestor of some later base
+   * class at index {@code j > i}, or {@code -1} if no such pair exists.
+   */
+  private static int findAncestorConflictIndex(List<ClassType> types) {
+    for (int i = 0; i < types.size() - 1; i++) {
+      if (laterBaseExtendsEarlier(types, i)) {
+        return i;
+      }
+    }
+    return -1;
+  }
+
+  private static boolean laterBaseExtendsEarlier(List<ClassType> types, int i) {
+    ClassType typeI = types.get(i);
+    if (typeI == null) {
+      return false;
+    }
+    for (int j = i + 1; j < types.size(); j++) {
+      ClassType typeJ = types.get(j);
+      if (typeJ != null && isOrExtendsClass(typeJ, typeI)) {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  /**
+   * Returns {@code true} if {@code candidate} is or extends {@code ancestor} (i.e., {@code ancestor}
+   * appears anywhere in {@code candidate}'s type hierarchy). Uses reference equality since
+   * {@link ClassType} instances are canonical singletons within a single analysis.
+   */
+  private static boolean isOrExtendsClass(ClassType candidate, ClassType ancestor) {
+    Set<PythonType> visited = new HashSet<>();
+    Deque<PythonType> queue = new ArrayDeque<>();
+    queue.add(candidate);
+    while (!queue.isEmpty()) {
+      PythonType current = queue.poll();
+      if (!visited.add(current)) {
+        continue;
+      }
+      if (current == ancestor) {
+        return true;
+      }
+      if (current instanceof ClassType ct) {
+        enqueueDirectSuperclasses(ct, queue);
+      }
+    }
+    return false;
+  }
+
+  private static void enqueueDirectSuperclasses(ClassType ct, Deque<PythonType> queue) {
+    for (TypeWrapper sw : ct.superClasses()) {
+      queue.add(sw.type());
+    }
+  }
+
+  @CheckForNull
+  private static ClassType resolveClassType(Expression expression) {
+    PythonType type = expression.typeV2();
+    return (type instanceof ClassType classType) ? classType : null;
+  }
+}

python-checks/src/main/java/org/sonar/python/checks/OpenSourceCheckList.java

@@ -319,6 +319,7 @@ public Stream<Class<?>> getChecks() {
       MissingNewlineAtEndOfFileCheck.class,
       ModifiedParameterValueCheck.class,
       ModuleNameCheck.class,
+      MultipleInheritanceMROConflictCheck.class,
       MultipleWhitespaceCheck.class,
       MutableDefaultValueCheck.class,
       NeedlessPassCheck.class,

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8511.html

@@ -0,0 +1,53 @@
+<p>This rule raises an issue when a class definition with multiple inheritance results in an unresolvable Method Resolution Order (MRO).</p>
+<h2>Why is this an issue?</h2>
+<p>Python uses the C3 linearization algorithm to determine the Method Resolution Order (MRO) — the order in which base classes are searched when
+looking up methods and attributes. When a class is created with multiple inheritance, Python must be able to construct a valid MRO. If the inheritance
+structure violates the C3 linearization rules, Python raises a <code>TypeError</code> at class definition time, preventing the class from being
+created at all.</p>
+<p>The C3 algorithm enforces these key constraints:</p>
+<ul>
+  <li><strong>Local precedence ordering</strong>: If class <code>C</code> inherits from <code>A</code> then <code>B</code>, then <code>A</code> must
+  appear before <code>B</code> in the MRO.</li>
+  <li><strong>Monotonicity</strong>: If class <code>A</code> appears before class <code>B</code> in the MRO of a parent class, then <code>A</code>
+  must also appear before <code>B</code> in the MRO of any child class.</li>
+</ul>
+<p>MRO inconsistencies cause immediate runtime failures. The class cannot be instantiated or used, breaking the application at import time.</p>
+<h2>How to fix it</h2>
+<p>Restructure the inheritance hierarchy to respect the C3 linearization rules. Remove redundant base classes from the inheritance list, as a class
+does not need to directly inherit from a grandparent if it is already inherited through a parent.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+class A:
+    pass
+
+class B(A):
+    pass
+
+class C(A):
+    pass
+
+class D(B, A, C):  # Noncompliant: explicitly inheriting from A violates local precedence
+    pass
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+class A:
+    pass
+
+class B(A):
+    pass
+
+class C(A):
+    pass
+
+class D(B, C):  # Compliant: A is inherited through B and C
+    pass
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li>Python Documentation - <a href="https://docs.python.org/3/tutorial/classes.html#multiple-inheritance">Multiple Inheritance</a></li>
+  <li>Python Documentation - <a href="https://docs.python.org/3/library/stdtypes.html#class.__mro__"><code>__mro__</code> attribute</a></li>
+  <li>Python - <a href="https://www.python.org/download/releases/2.3/mro/">The Python 2.3 Method Resolution Order</a></li>
+</ul>

python-checks/src/main/resources/org/sonar/l10n/py/rules/python/S8511.json

@@ -0,0 +1,21 @@
+{
+  "title": "Multiple inheritance should not create Method Resolution Order (MRO) conflicts",
+  "type": "BUG",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "15 min"
+  },
+  "tags": [],
+  "defaultSeverity": "Critical",
+  "ruleSpecification": "RSPEC-8511",
+  "sqKey": "S8511",
+  "scope": "Main",
+  "quickfix": "unknown",
+  "code": {
+    "impacts": {
+      "RELIABILITY": "HIGH"
+    },
+    "attribute": "LOGICAL"
+  }
+}

python-checks/src/test/java/org/sonar/python/checks/MultipleInheritanceMROConflictCheckTest.java

@@ -0,0 +1,29 @@
+/*
+ * SonarQube Python Plugin
+ * Copyright (C) SonarSource Sàrl
+ * mailto:info AT sonarsource DOT com
+ *
+ * You can redistribute and/or modify this program under the terms of
+ * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the Sonar Source-Available License for more details.
+ *
+ * You should have received a copy of the Sonar Source-Available License
+ * along with this program; if not, see https://sonarsource.com/license/ssal/
+ */
+package org.sonar.python.checks;
+
+import org.junit.jupiter.api.Test;
+import org.sonar.python.checks.utils.PythonCheckVerifier;
+
+class MultipleInheritanceMROConflictCheckTest {
+
+  @Test
+  void test() {
+    PythonCheckVerifier.verify("src/test/resources/checks/multipleInheritanceMROConflict.py", new MultipleInheritanceMROConflictCheck());
+  }
+
+}

python-checks/src/test/resources/checks/multipleInheritanceMROConflict.py

@@ -0,0 +1,70 @@
+class Base:
+    pass
+
+class Mid(Base):
+    pass
+
+class Leaf(Mid):
+    pass
+
+
+class Broken(Base, Leaf):  # Noncompliant {{Reorder or remove base classes to fix this MRO conflict.}}
+#     ^^^^^^ ^^^^< {{This base class is an ancestor of another listed base class appearing after it.}}
+    pass
+
+
+class CommonRoot:
+    pass
+
+class LeftChild(CommonRoot):
+    pass
+
+class RightChild(CommonRoot):
+    pass
+
+class LeftFirst(LeftChild, RightChild):
+    pass
+
+class RightFirst(RightChild, LeftChild):
+    pass
+
+class C3Conflict(LeftFirst, RightFirst):  # Noncompliant {{Reorder or remove base classes to fix this MRO conflict.}}
+    pass
+
+
+def not_a_class():
+    pass
+
+
+class CompliantPartialResolution(not_a_class, Base):
+    pass
+
+
+class OnlyMetaclass(metaclass=type):
+    pass
+
+
+class CompliantC3Order(LeftFirst, RightChild):
+    pass
+
+
+# --- Super types not fully resolved (heuristic path; full C3 is skipped) ---
+from unknown_module import ExternalMixin, ExternalOther
+
+
+# Compliant: later base not fully resolved for C3 (unknown import / incomplete hierarchy), and the
+# heuristic does not show it extending Base.
+class MidWithUnresolvedMixin(Base, ExternalMixin):
+    pass
+
+
+# Compliant: two unrelated imported bases (incomplete hierarchies); no "earlier ancestor of later" pair in the model
+class CompliantTwoUnresolvedPeers(ExternalMixin, ExternalOther):
+    pass
+
+
+# Bases not fully resolved for C3 (second base has incomplete hierarchy), yet the heuristic still
+# finds that the second base subclasses Base — flagged without running full C3.
+class ConflictMid(Base, MidWithUnresolvedMixin):  # Noncompliant {{Reorder or remove base classes to fix this MRO conflict.}}
+#     ^^^^^^^^^^^ ^^^^< {{This base class is an ancestor of another listed base class appearing after it.}}
+    pass