fix(jsii-pacmak): Python interfaces sometimes violate MRO (#4973)

The Method Resolution Order (MRO) rules for Python state (in brief) that all classes and interfaces in an inheritance declaration must be listed subclass-before-superclass, and in the same order on all classes.

`jsii-pacmak` was not guaranteeing order, leading to sometimes producing an order that violates MRO.

Update `jsii-pacmak` to update base classes and sort them in the right order.



---

By submitting this pull request, I confirm that my contribution is made under the terms of the [Apache 2.0 license].

[Apache 2.0 license]: https://www.apache.org/licenses/LICENSE-2.0

Author: rix0rrr

packages/jsii-pacmak/lib/targets/python.ts

@@ -32,6 +32,7 @@ import {
 import { die, toPythonIdentifier } from './python/util';
 import { toPythonVersionRange, toReleaseVersion } from './version-utils';
 import { assertSpecIsRosettaCompatible } from '../rosetta-assembly';
+import { topologicalSort } from '../toposort';
 
 import { TargetName } from './index';
 
@@ -367,7 +368,7 @@ interface ISortableType {
   dependsOn(resolver: TypeResolver): PythonType[];
 }
 
-function isSortableType(arg: unknown): arg is ISortableType {
+function isSortableType<T>(arg: T): arg is T & ISortableType {
   return (arg as Partial<ISortableType>).dependsOn !== undefined;
 }
 
@@ -450,8 +451,10 @@ abstract class BasePythonClassType implements PythonType, ISortableType {
   public emit(code: CodeMaker, context: EmitContext) {
     context = nestedContext(context, this.fqn);
 
-    const classParams = this.getClassParams(context);
-    openSignature(code, 'class', this.pythonName, classParams);
+    this.sortBasesForMro();
+
+    const baseClasses = this.getClassParams(context);
+    openSignature(code, 'class', this.pythonName, baseClasses);
 
     this.generator.emitDocString(code, this.apiLocation, this.docs, {
       documentableItem: `class-${this.pythonName}`,
@@ -479,6 +482,95 @@ abstract class BasePythonClassType implements PythonType, ISortableType {
     }
   }
 
+  /**
+   * Sort the this.bases array for proper Python MRO.
+   *
+   * If we don't do this, there is a chance that a superclass and subclass (or interface)
+   * have the parents in different order, which leads to an MRO violation.
+   *
+   * See: <https://docs.python.org/3/howto/mro.html>
+   *
+   * The bottom line is: child classes higher up in the hierarchy first.
+   * As an example, in the hierarchy:
+   *
+   * ```
+   *  ┌─────┐
+   *  │  A  │
+   *  └─────┘
+   *     ▲
+   *     ├─────┐
+   *     │  ┌─────┐
+   *     │  │  C  │
+   *     │  └─────┘
+   *     │     ▲
+   *     ├─────┘
+   *  ┌─────┐
+   *  │  B  │
+   *  └─────┘
+   * ```
+   *
+   * The order should be `class B(C, A)`, and not the other way around.
+   *
+   * This is yet another instance of topological sort.
+   */
+  protected sortBasesForMro() {
+    if (this.bases.length <= 1) {
+      return;
+    }
+
+    // Record original order
+    const originalOrder = new Map<spec.TypeReference, number>();
+    for (let i = 0; i < this.bases.length; i++) {
+      originalOrder.set(this.bases[i], i);
+    }
+
+    // Classify into sortable and non-sortable types
+    const sortableBases: spec.NamedTypeReference[] = [];
+    const nonSortableBases: spec.TypeReference[] = [];
+    for (const baseRef of this.bases) {
+      if (spec.isNamedTypeReference(baseRef)) {
+        sortableBases.push(baseRef);
+      } else {
+        nonSortableBases.push(baseRef);
+      }
+    }
+
+    // Toposort, in reverse (highest in the tree last)
+    const typeSystem = this.generator.reflectAssembly.system;
+    const sorted = topologicalSort(
+      sortableBases,
+      (t) => t.fqn,
+      (d) => typeBaseFqns(typeSystem.findFqn(d.fqn)),
+    );
+    sorted.reverse();
+
+    // Inside a trance, we keep original sort order in the source declarations.
+    for (const tranche of sorted) {
+      tranche.sort((a, b) => originalOrder.get(a)! - originalOrder.get(b)!);
+    }
+
+    // Replace original bases
+    this.bases.splice(
+      0,
+      this.bases.length,
+      ...sorted.flatMap((xs) => xs),
+      ...nonSortableBases,
+    );
+
+    function typeBaseFqns(x: reflect.Type): string[] {
+      if (x.isClassType()) {
+        return [
+          ...(x.base ? [x.base?.fqn] : []),
+          ...x.getInterfaces().map((i) => i.fqn),
+        ];
+      }
+      if (x.isInterfaceType()) {
+        return x.getInterfaces().map((i) => i.fqn);
+      }
+      return [];
+    }
+  }
+
   protected boundResolver(resolver: TypeResolver): TypeResolver {
     if (this.fqn == null) {
       return resolver;
@@ -1024,6 +1116,8 @@ abstract class BaseProperty implements PythonBase {
 
 class Interface extends BasePythonClassType {
   public emit(code: CodeMaker, context: EmitContext) {
+    this.sortBasesForMro();
+
     context = nestedContext(context, this.fqn);
     emitList(code, '@jsii.interface(', [`jsii_type="${this.fqn}"`], ')');
 
@@ -1114,6 +1208,8 @@ class Struct extends BasePythonClassType {
   }
 
   public emit(code: CodeMaker, context: EmitContext) {
+    this.sortBasesForMro();
+
     context = nestedContext(context, this.fqn);
     const baseInterfaces = this.getClassParams(context);
 

packages/jsii-pacmak/lib/toposort.ts

@@ -79,4 +79,4 @@ interface TopoElement<T> {
  * We do declare the type `Toposorted<A>` here so that if we ever change
  * the type, we can find all usage sites quickly.
  */
-export type Toposorted<A> = readonly A[][];
+export type Toposorted<A> = A[][];

packages/jsii-pacmak/lib/util.ts

@@ -464,3 +464,9 @@ export function zip<A, B>(xs: A[], ys: B[]): Array<[A, B]> {
   }
   return ret;
 }
+
+export function setAdd<T>(setA: Set<T>, setB: Iterable<T>) {
+  for (const b of setB) {
+    setA.add(b);
+  }
+}