Backed out recent performance optimization that resulted in a regress…

…ion for protocol matching in certain circumstances. This addresses #9153, #9132, and #9130. (#9164)
microsoft · Oct 7, 2024 · 23b4668 · 23b4668
1 parent 649a2ab
commit 23b4668
Show file tree

Hide file tree

Showing 4 changed files with 45 additions and 3 deletions.
diff --git a/packages/pyright-internal/src/analyzer/constraintTracker.ts b/packages/pyright-internal/src/analyzer/constraintTracker.ts
@@ -237,6 +237,14 @@ export class ConstraintTracker {
         }
     }
 
+    isSame(other: ConstraintTracker) {
+        if (other._constraintSets.length !== this._constraintSets.length) {
+            return false;
+        }
+
+        return this._constraintSets.every((set, index) => set.isSame(other._constraintSets[index]));
+    }
+
     isEmpty() {
         return this._constraintSets.every((set) => set.isEmpty());
     }

diff --git a/packages/pyright-internal/src/analyzer/protocols.ts b/packages/pyright-internal/src/analyzer/protocols.ts
@@ -58,6 +58,7 @@ interface ProtocolCompatibility {
     srcType: Type;
     destType: Type;
     flags: AssignTypeFlags;
+    constraints: ConstraintTracker | undefined;
     isCompatible: boolean;
 }
 
@@ -125,6 +126,7 @@ export function assignClassToProtocol(
 
     protocolAssignmentStack.push({ srcType, destType });
     let isCompatible = true;
+    const clonedConstraints = constraints?.clone();
 
     try {
         isCompatible = assignToProtocolInternal(evaluator, destType, srcType, diag, constraints, flags, recursionCount);
@@ -138,7 +140,7 @@ export function assignClassToProtocol(
     protocolAssignmentStack.pop();
 
     // Cache the results for next time.
-    setProtocolCompatibility(destType, srcType, flags, isCompatible);
+    setProtocolCompatibility(destType, srcType, flags, clonedConstraints, isCompatible);
 
     return isCompatible;
 }
@@ -229,7 +231,12 @@ function getProtocolCompatibility(
     }
 
     const entry = entries.find((entry) => {
-        return isTypeSame(entry.destType, destType) && isTypeSame(entry.srcType, srcType) && entry.flags === flags;
+        return (
+            isTypeSame(entry.destType, destType) &&
+            isTypeSame(entry.srcType, srcType) &&
+            entry.flags === flags &&
+            isConstraintTrackerSame(constraints, entry.constraints)
+        );
     });
 
     return entry?.isCompatible;
@@ -239,6 +246,7 @@ function setProtocolCompatibility(
     destType: ClassType,
     srcType: ClassType,
     flags: AssignTypeFlags,
+    constraints: ConstraintTracker | undefined,
     isCompatible: boolean
 ) {
     let map = srcType.shared.protocolCompatibility as Map<string, ProtocolCompatibility[]> | undefined;
@@ -253,13 +261,27 @@ function setProtocolCompatibility(
         map.set(destType.shared.fullName, entries);
     }
 
-    entries.push({ destType, srcType, flags, isCompatible });
+    entries.push({
+        destType,
+        srcType,
+        flags,
+        constraints: constraints,
+        isCompatible,
+    });
 
     if (entries.length > maxProtocolCompatibilityCacheEntries) {
         entries.shift();
     }
 }
 
+function isConstraintTrackerSame(context1: ConstraintTracker | undefined, context2: ConstraintTracker | undefined) {
+    if (!context1 || !context2) {
+        return context1 === context2;
+    }
+
+    return context1.isSame(context2);
+}
+
 function assignToProtocolInternal(
     evaluator: TypeEvaluator,
     destType: ClassType,

diff --git a/packages/pyright-internal/src/tests/samples/protocol51.py b/packages/pyright-internal/src/tests/samples/protocol51.py
@@ -0,0 +1,6 @@
+# This sample tests a regression case related to the protocol compatibility
+# cache.
+
+
+int(round(1.2, 0))
+round(3.4, 2)
diff --git a/packages/pyright-internal/src/tests/typeEvaluator7.test.ts b/packages/pyright-internal/src/tests/typeEvaluator7.test.ts
@@ -593,6 +593,12 @@ test('Protocol50', () => {
     TestUtils.validateResults(analysisResults, 0);
 });
 
+test('Protocol51', () => {
+    const analysisResults = TestUtils.typeAnalyzeSampleFiles(['protocol51.py']);
+
+    TestUtils.validateResults(analysisResults, 0);
+});
+
 test('ProtocolExplicit1', () => {
     const analysisResults = TestUtils.typeAnalyzeSampleFiles(['protocolExplicit1.py']);