SONARPY-4016 Fix S8511 FP on built-in containers with ABC bases (e.g. dict/MutableMapping) (#1066)

GitOrigin-RevId: db4db4ef4f05d00eda8061cde8d8e9ef536e08d7

Author: marc-jasper-sonarsource

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

@@ -86,13 +86,14 @@ private static List<Expression> collectPositionalBases(ArgList argList) {
   /**
    * 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}).
+   * later base. Both paths use a runtime-faithful view of built-in containers — see
+   * {@link ClassType#wouldHaveValidMro(List)}.
    */
   private static boolean hasMroConflict(List<ClassType> types, int conflictIndex) {
-    return isFullyResolved(types)
-      ? !ClassType.wouldHaveValidMro(types)
-      : (conflictIndex >= 0);
+    if (isFullyResolved(types)) {
+      return !ClassType.wouldHaveValidMro(types);
+    }
+    return conflictIndex >= 0;
   }
 
   /**
@@ -110,7 +111,10 @@ private static boolean isFullyResolved(List<ClassType> types) {
 
   /**
    * 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.
+   * class at index {@code j > i}, or {@code -1} if no such pair exists. Paths from any later base
+   * that pass through a virtual-ABC-subclassing builtin are cut off, so typeshed-only ABC edges
+   * (e.g. {@code dict → MutableMapping}) are not treated as real ancestry — see
+   * {@link #isOrExtendsClassAtRuntime(ClassType, ClassType)}.
    */
   private static int findAncestorConflictIndex(List<ClassType> types) {
     for (int i = 0; i < types.size() - 1; i++) {
@@ -128,19 +132,20 @@ private static boolean laterBaseExtendsEarlier(List<ClassType> types, int i) {
     }
     for (int j = i + 1; j < types.size(); j++) {
       ClassType typeJ = types.get(j);
-      if (typeJ != null && isOrExtendsClass(typeJ, typeI)) {
+      if (typeJ != null && isOrExtendsClassAtRuntime(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.
+   * Returns {@code true} if {@code ancestor} appears in {@code candidate}'s runtime type hierarchy.
+   * Traversal stops at virtual-ABC-subclassing builtins so their typeshed-only ABC parents are not
+   * reachable; see {@link ClassType#wouldHaveValidMro(List)}. Uses reference equality
+   * since {@link ClassType} instances are canonical within a single analysis.
    */
-  private static boolean isOrExtendsClass(ClassType candidate, ClassType ancestor) {
+  private static boolean isOrExtendsClassAtRuntime(ClassType candidate, ClassType ancestor) {
     Set<PythonType> visited = new HashSet<>();
     Deque<PythonType> queue = new ArrayDeque<>();
     queue.add(candidate);
@@ -152,7 +157,7 @@ private static boolean isOrExtendsClass(ClassType candidate, ClassType ancestor)
       if (current == ancestor) {
         return true;
       }
-      if (current instanceof ClassType ct) {
+      if (current instanceof ClassType ct && !ct.isVirtualAbcSubclassingBuiltin()) {
         enqueueDirectSuperclasses(ct, queue);
       }
     }

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

@@ -68,3 +68,98 @@ class CompliantTwoUnresolvedPeers(ExternalMixin, ExternalOther):
 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
+
+
+# --- typeshed lies about built-in containers inheriting from collections.abc ABCs ---
+# At runtime these built-ins only inherit from `object` and are merely virtual-subclass-registered
+# against the corresponding ABCs (via abc.ABCMeta.register). Python's MRO computation succeeds, so
+# the rule must NOT flag these.
+from collections.abc import (
+    MutableMapping,
+    Mapping,
+    MutableSequence,
+    Sequence,
+    MutableSet,
+    Set as AbstractSet,
+)
+
+
+class CompliantDictMixin(MutableMapping, dict):
+    pass
+
+
+class CompliantMappingDict(Mapping, dict):
+    pass
+
+
+class CompliantListMixin(MutableSequence, list):
+    pass
+
+
+class CompliantTupleMixin(Sequence, tuple):
+    pass
+
+
+class CompliantSetMixin(MutableSet, set):
+    pass
+
+
+class CompliantFrozenSetMixin(AbstractSet, frozenset):
+    pass
+
+
+class CompliantBytearrayMixin(MutableSequence, bytearray):
+    pass
+
+
+# Smoke tests for str/bytes — typeshed lists extra ABCs (Sequence, Hashable, …) that don't appear
+# in their runtime MRO.
+class CompliantStrMixin(Sequence, str):
+    pass
+
+
+class CompliantBytesMixin(Sequence, bytes):
+    pass
+
+
+# collections.deque has the same typeshed-vs-runtime mismatch as the builtin containers:
+# typeshed declares `class deque(MutableSequence[_T])` but at runtime deque.__bases__ == (object,),
+# with MutableSequence registered as a virtual subclass via abc.ABCMeta.register().
+from collections import deque
+
+
+class CompliantDequeMixin(MutableSequence, deque):
+    pass
+
+
+# Real conflict that involves a built-in container must still be flagged: dict before its own
+# user subclass is a genuine "earlier-is-ancestor-of-later" situation that Python rejects.
+class _UserDictSubclass(dict):
+    pass
+
+
+class StillBrokenWithUserDict(dict, _UserDictSubclass):  # 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
+
+
+# The reverse order is valid Python (dict comes after its subclass) and must not be flagged.
+class CompliantUserDictThenDict(_UserDictSubclass, dict):
+    pass
+
+
+# Compliant: ABC paired with a user-defined dict subclass. typeshed (falsely) lists MutableMapping
+# as ancestor of dict; the rule must not infer the same ancestry through `_UserDictSubclass`.
+# Fully-resolved bases — exercises the C3 path (wouldHaveValidMro).
+class CompliantAbcThenUserDict(MutableMapping, _UserDictSubclass):
+    pass
+
+
+# Same scenario but with an extra unresolved external mixin to force the heuristic path
+# (findAncestorConflictIndex / isOrExtendsClassAtRuntime).
+class _UserDictWithUnresolvedMixin(dict, ExternalMixin):
+    pass
+
+
+class CompliantAbcThenUserDictUnresolved(MutableMapping, _UserDictWithUnresolvedMixin):
+    pass

python-frontend/src/main/java/org/sonar/plugins/python/api/types/v2/ClassType.java

@@ -206,8 +206,12 @@ public Optional<List<ClassType>> mro() {
   }
 
   /**
-   * Returns whether C3 linearization would succeed for a hypothetical class whose direct bases are
-   * exactly {@code bases} in order.
+   * Returns whether C3 linearization would succeed at runtime for a hypothetical class whose direct
+   * bases are exactly {@code bases} in order. Uses Python's runtime view of built-in containers:
+   * typeshed-only inheritance edges from {@code dict}/{@code list}/{@code set}/… to their
+   * {@code collections.abc} / {@code typing} ABCs (which at runtime are
+   * {@code abc.ABCMeta.register()} virtual subclasses, not real bases) are ignored. This way, valid
+   * Python code such as {@code class N(MutableMapping, dict): pass} is not flagged.
    *
    * <p>Precondition: every element of {@code bases} is non-null and fully resolved (no unresolved
    * hierarchy), matching what {@link #mro()} requires for each base.
@@ -219,6 +223,15 @@ public static boolean wouldHaveValidMro(List<ClassType> bases) {
     return ClassTypeMroUtils.wouldHaveValidMro(bases);
   }
 
+  /**
+   * Returns {@code true} for built-in containers whose typeshed declaration includes fictional
+   * ABC inheritance that does not exist at runtime (e.g. {@code dict}, {@code list}, {@code set}).
+   * See {@link #wouldHaveValidMro(List)}.
+   */
+  public boolean isVirtualAbcSubclassingBuiltin() {
+    return ClassTypeMroUtils.isVirtualAbcSubclassingBuiltin(this);
+  }
+
   @Override
   public TriBool hasMember(String memberName) {
     // a ClassType is an object of class type, it has the same members as those present on any type

python-frontend/src/main/java/org/sonar/plugins/python/api/types/v2/ClassTypeMroUtils.java

@@ -27,27 +27,36 @@
 
 /**
  * C3 linearization (Method Resolution Order) for {@link ClassType}.
+ *
+ * <p>Built-in containers listed in {@link #VIRTUAL_ABC_SUBCLASSING_BUILTIN_FQNS} have typeshed
+ * declarations that lie about their bases (e.g. {@code class dict(MutableMapping, Generic)}),
+ * but at runtime they only inherit from {@code object} and the ABC relationship is a
+ * virtual-subclass registration via {@code abc.ABCMeta.register()}. {@link #compute(ClassType)}
+ * preserves the typeshed view (used by {@link ClassType#mro()}), while
+ * {@link #wouldHaveValidMro(List)} honours the runtime view (used to detect real MRO conflicts).
  */
 final class ClassTypeMroUtils {
 
+  private static final Set<String> VIRTUAL_ABC_SUBCLASSING_BUILTIN_FQNS = Set.of(
+    "dict", "list", "tuple", "set", "frozenset", "str", "bytes", "bytearray", "collections.deque"
+  );
+
   private ClassTypeMroUtils() {
   }
 
-  /**
-   * Computes the C3 MRO for {@code cls}, using a fresh memoisation cache.
-   */
   static Optional<List<ClassType>> compute(ClassType cls) {
-    return compute(cls, new HashMap<>(), new HashSet<>());
+    return compute(cls, new HashMap<>(), new HashSet<>(), false);
   }
 
   /**
-   * Returns whether C3 linearization would succeed for a hypothetical class whose direct bases are
-   * exactly {@code bases} in order — same behaviour as {@link ClassType#wouldHaveValidMro(List)}.
+   * Returns whether C3 would succeed at runtime for a hypothetical class whose direct bases are
+   * exactly {@code bases} in order. Backs {@link ClassType#wouldHaveValidMro(List)}.
    */
   static boolean wouldHaveValidMro(List<ClassType> bases) {
+    Map<ClassType, Optional<List<ClassType>>> cache = new HashMap<>();
     List<List<ClassType>> lists = new ArrayList<>();
     for (ClassType base : bases) {
-      Optional<List<ClassType>> mro = base.mro();
+      Optional<List<ClassType>> mro = compute(base, cache, new HashSet<>(), true);
       if (mro.isEmpty()) {
         return true;
       }
@@ -57,18 +66,34 @@ static boolean wouldHaveValidMro(List<ClassType> bases) {
     return c3Merge(lists) != null;
   }
 
+  static boolean isVirtualAbcSubclassingBuiltin(ClassType cls) {
+    String fqn = cls.fullyQualifiedName();
+    return fqn != null && VIRTUAL_ABC_SUBCLASSING_BUILTIN_FQNS.contains(fqn);
+  }
+
   /**
-   * Recursively computes the C3 MRO for {@code cls}, memoising results in {@code cache}.
-   * {@code visiting} guards against cycles.
+   * Recursively computes the C3 MRO for {@code cls}. {@code cache} memoises results across the
+   * traversal (callers may pass a fresh map); {@code visiting} guards against inheritance cycles.
    */
   private static Optional<List<ClassType>> compute(
     ClassType cls,
     Map<ClassType, Optional<List<ClassType>>> cache,
-    Set<ClassType> visiting
+    Set<ClassType> visiting,
+    boolean stripVirtualAbcInheritance
   ) {
     if (cache.containsKey(cls)) {
       return cache.get(cls);
     }
+    if (stripVirtualAbcInheritance && isVirtualAbcSubclassingBuiltin(cls)) {
+      // Runtime view: ignore the typeshed-declared parents but still keep {@code object} as the tail,
+      // so C3 rejects orderings like {@code class X(object, dict)} (object must come after dict).
+      // {@code object} is, by construction, the last element of {@code cls}'s non-stripped MRO.
+      Optional<List<ClassType>> fullMro = compute(cls, new HashMap<>(), new HashSet<>(), false);
+      List<ClassType> mro = fullMro.map(m -> List.of(cls, m.get(m.size() - 1))).orElseGet(() -> List.of(cls));
+      Optional<List<ClassType>> result = Optional.of(mro);
+      cache.put(cls, result);
+      return result;
+    }
     if (!visiting.add(cls)) {
       // Cycle detected — treat as failure.
       return Optional.empty();
@@ -84,10 +109,9 @@ private static Optional<List<ClassType>> compute(
 
     List<List<ClassType>> lists = new ArrayList<>();
     for (ClassType parent : parentTypes) {
-      Optional<List<ClassType>> parentMro = compute(parent, cache, visiting);
+      Optional<List<ClassType>> parentMro = compute(parent, cache, visiting, stripVirtualAbcInheritance);
       if (parentMro.isEmpty()) {
-        // A parent itself has an invalid MRO — Python would have raised TypeError
-        // when that parent was defined, so this class is not the root cause.
+        // A parent's MRO is invalid — Python would have raised TypeError there, not here.
         visiting.remove(cls);
         cache.put(cls, Optional.empty());
         return Optional.empty();
@@ -110,10 +134,7 @@ private static Optional<List<ClassType>> compute(
     return result;
   }
 
-  /**
-   * Performs the C3 merge of the given lists. Returns the merged sequence, or {@code null} if no
-   * valid merge exists (i.e. the MRO has a conflict).
-   */
+  /** Returns the C3 merge of {@code lists}, or {@code null} if no valid merge exists. */
   @CheckForNull
   private static List<ClassType> c3Merge(List<List<ClassType>> lists) {
     List<ClassType> result = new ArrayList<>();
@@ -131,10 +152,7 @@ private static List<ClassType> c3Merge(List<List<ClassType>> lists) {
     }
   }
 
-  /**
-   * Returns the first list head that does not appear as a non-head entry in any list, or {@code null}
-   * if no such head exists (merge failure).
-   */
+  /** Returns the first list head that doesn't appear as a non-head in any list, or {@code null}. */
   @CheckForNull
   private static ClassType findFirstValidMergeHead(List<List<ClassType>> lists) {
     for (List<ClassType> list : lists) {

python-frontend/src/test/java/org/sonar/plugins/python/api/types/v2/ClassTypeTest.java

@@ -967,6 +967,67 @@ void wouldHaveValidMro_returns_true_for_diamond_inheritance_bases() {
     assertThat(ClassType.wouldHaveValidMro(List.of(b, c))).isTrue();
   }
 
+  @Test
+  void wouldHaveValidMro_ignores_typeshed_virtual_abc_inheritance_for_dict() {
+    // typeshed declares `class dict(MutableMapping[_KT, _VT], Generic[_KT, _VT])`, but at runtime
+    // dict only inherits from `object` and the MutableMapping relationship is a virtual-subclass
+    // registration via `abc.ABCMeta.register()`. C3 succeeds at runtime for class N(MutableMapping, dict).
+    List<ClassType> types = classTypes(
+      "from collections.abc import MutableMapping",
+      "class Holder(MutableMapping, dict): ..."
+    );
+    ClassType holder = types.get(0);
+    assertThat(holder.superClasses()).hasSize(2);
+    var bases = holder.superClasses().stream()
+      .map(sw -> (ClassType) sw.type())
+      .toList();
+    assertThat(ClassType.wouldHaveValidMro(bases)).isTrue();
+  }
+
+  @Test
+  void wouldHaveValidMro_handles_user_subclass_of_builtin_container() {
+    // class MyDict(dict): ...; class X(MutableMapping, MyDict): ... is valid at runtime because
+    // MyDict's runtime MRO is [MyDict, dict, object] (no MutableMapping) — the typeshed link from
+    // dict to MutableMapping is fictional.
+    List<ClassType> types = classTypes(
+      "from collections.abc import MutableMapping",
+      "class MyDict(dict): ...",
+      "class X(MutableMapping, MyDict): ..."
+    );
+    ClassType x = types.get(1);
+    var bases = x.superClasses().stream()
+      .map(sw -> (ClassType) sw.type())
+      .toList();
+    assertThat(ClassType.wouldHaveValidMro(bases)).isTrue();
+  }
+
+  @Test
+  void wouldHaveValidMro_still_detects_real_conflict_with_builtin_container() {
+    // class MyDict(dict): ...; class X(dict, MyDict): ... is rejected at runtime because dict
+    // appears before its own subclass.
+    List<ClassType> types = classTypes(
+      "class MyDict(dict): ...",
+      "class X(dict, MyDict): ..."
+    );
+    ClassType x = types.get(1);
+    var bases = x.superClasses().stream()
+      .map(sw -> (ClassType) sw.type())
+      .toList();
+    assertThat(ClassType.wouldHaveValidMro(bases)).isFalse();
+  }
+
+  @Test
+  void wouldHaveValidMro_rejects_object_before_builtin_container() {
+    // class X(object, dict): ... is rejected at runtime ("Cannot create a consistent MRO …") because
+    // dict.__mro__ ends with object, so object must come after dict. The runtime-view MRO must keep
+    // the object tail to catch this — otherwise stripping dict's typeshed parents to just [dict]
+    // would let the merge succeed (false negative).
+    var objectType = (ClassType) PROJECT_LEVEL_TYPE_TABLE.getType("object");
+    var dictType = (ClassType) PROJECT_LEVEL_TYPE_TABLE.getType("dict");
+    assertThat(ClassType.wouldHaveValidMro(List.of(objectType, dictType))).isFalse();
+    assertThat(ClassType.wouldHaveValidMro(List.of(dictType, objectType))).isTrue();
+  }
+
   @Test
   void mro_unresolved_hierarchy_returns_empty() {
     // class Child(Unknown): ...  <- unresolved parent