Added check for "unsafe overlaps" for a runtime-checkable protocol wh…

…en used in an `issubclass` or `isinstance` check. This addresses #6882. (#6933)

packages/pyright-internal/src/analyzer/checker.ts

@@ -105,7 +105,7 @@ import { getParameterListDetails } from './parameterUtils';
 import * as ParseTreeUtils from './parseTreeUtils';
 import { ParseTreeWalker } from './parseTreeWalker';
 import { validateClassPattern } from './patternMatching';
-import { isMethodOnlyProtocol } from './protocols';
+import { isMethodOnlyProtocol, isProtocolUnsafeOverlap } from './protocols';
 import { RegionComment, RegionCommentType, getRegionComments } from './regions';
 import { ScopeType } from './scope';
 import { getScopeForNode } from './scopeUtils';
@@ -3762,6 +3762,14 @@ export class Checker extends ParseTreeWalker {
                             arg1IncludesSubclasses = true;
                         }
                     }
+
+                    if (arg0Type) {
+                        this._validateUnsafeProtocolOverlap(
+                            node.arguments[0].valueExpression,
+                            convertToInstance(arg1Subtype),
+                            isInstanceCheck ? arg0Type : convertToInstance(arg0Type)
+                        );
+                    }
                 } else {
                     // The isinstance and issubclass call supports a variation where the second
                     // parameter is a tuple of classes.
@@ -3774,6 +3782,14 @@ export class Checker extends ParseTreeWalker {
                                     if (typeArg.type.includeSubclasses) {
                                         arg1IncludesSubclasses = true;
                                     }
+
+                                    if (arg0Type) {
+                                        this._validateUnsafeProtocolOverlap(
+                                            node.arguments[0].valueExpression,
+                                            convertToInstance(typeArg.type),
+                                            isInstanceCheck ? arg0Type : convertToInstance(arg0Type)
+                                        );
+                                    }
                                 } else {
                                     isValidType = false;
                                 }
@@ -3916,6 +3932,39 @@ export class Checker extends ParseTreeWalker {
         }
     }
 
+    private _validateUnsafeProtocolOverlap(errorNode: ExpressionNode, protocol: ClassType, testType: Type) {
+        // If this is a protocol class, check for an "unsafe overlap"
+        // with the arg0 type.
+        if (ClassType.isProtocolClass(protocol)) {
+            let isUnsafeOverlap = false;
+            const diag = new DiagnosticAddendum();
+
+            doForEachSubtype(testType, (testSubtype) => {
+                if (isClassInstance(testSubtype)) {
+                    if (isProtocolUnsafeOverlap(this._evaluator, protocol, testSubtype)) {
+                        isUnsafeOverlap = true;
+                        diag.addMessage(
+                            Localizer.DiagnosticAddendum.protocolUnsafeOverlap().format({
+                                name: testSubtype.details.name,
+                            })
+                        );
+                    }
+                }
+            });
+
+            if (isUnsafeOverlap) {
+                this._evaluator.addDiagnostic(
+                    this._fileInfo.diagnosticRuleSet.reportGeneralTypeIssues,
+                    DiagnosticRule.reportGeneralTypeIssues,
+                    Localizer.Diagnostic.protocolUnsafeOverlap().format({
+                        name: protocol.details.name,
+                    }) + diag.getString(),
+                    errorNode
+                );
+            }
+        }
+    }
+
     // Determines whether the specified type is allowed as the second argument
     // to an isinstance or issubclass check.
     private _isTypeSupportedTypeForIsInstance(type: Type, isInstanceCheck: boolean, diag: DiagnosticAddendum) {

packages/pyright-internal/src/analyzer/protocols.ts

@@ -193,6 +193,38 @@ export function isMethodOnlyProtocol(classType: ClassType): boolean {
     return true;
 }
 
+// Determines whether the classType has "unsafe overlap" with a runtime checkable protocol.
+// This can occur because the runtime doesn't do full type comparisons. It simply looks at
+// the presence of specific attributes.
+export function isProtocolUnsafeOverlap(evaluator: TypeEvaluator, protocol: ClassType, classType: ClassType): boolean {
+    // If the classType is compatible with the protocol, then it doesn't overlap unsafely.
+    if (evaluator.assignType(protocol, classType)) {
+        return false;
+    }
+
+    let isUnsafeOverlap = true;
+
+    protocol.details.mro.forEach((mroClass) => {
+        if (!isUnsafeOverlap || !isInstantiableClass(mroClass) || !ClassType.isProtocolClass(mroClass)) {
+            return;
+        }
+
+        mroClass.details.fields.forEach((destSymbol, name) => {
+            if (!isUnsafeOverlap || !destSymbol.isClassMember() || destSymbol.isIgnoredForProtocolMatch()) {
+                return;
+            }
+
+            // Does the classType have a member with the same name?
+            const srcMemberInfo = lookUpClassMember(classType, name);
+            if (!srcMemberInfo) {
+                isUnsafeOverlap = false;
+            }
+        });
+    });
+
+    return isUnsafeOverlap;
+}
+
 // Looks up the protocol compatibility in the cache. If it's not found,
 // return undefined.
 function getProtocolCompatibility(

packages/pyright-internal/src/analyzer/typeEvaluator.ts

@@ -6415,6 +6415,8 @@ export function createTypeEvaluator(importLookup: ImportLookup, evaluatorOptions
         return indexTypeResult;
     }
 
+    // If the list of type parameters includes a TypeVarTuple, we may need to adjust
+    // the supplied type arguments to map to the type parameter list.
     function adjustTypeArgumentsForVariadicTypeVar(
         typeArgs: TypeResultWithNode[],
         typeParameters: TypeVarType[],

packages/pyright-internal/src/localization/localize.ts

@@ -797,6 +797,8 @@ export namespace Localizer {
         export const protocolBaseClassWithTypeArgs = () => getRawString('Diagnostic.protocolBaseClassWithTypeArgs');
         export const protocolIllegal = () => getRawString('Diagnostic.protocolIllegal');
         export const protocolNotAllowed = () => getRawString('Diagnostic.protocolNotAllowed');
+        export const protocolUnsafeOverlap = () =>
+            new ParameterizedString<{ name: string }>(getRawString('Diagnostic.protocolUnsafeOverlap'));
         export const protocolVarianceContravariant = () =>
             new ParameterizedString<{ variable: string; class: string }>(
                 getRawString('Diagnostic.protocolVarianceContravariant')
@@ -1407,6 +1409,8 @@ export namespace Localizer {
             new ParameterizedString<{ sourceType: string; destType: string }>(
                 getRawString('DiagnosticAddendum.protocolSourceIsNotConcrete')
             );
+        export const protocolUnsafeOverlap = () =>
+            new ParameterizedString<{ name: string }>(getRawString('DiagnosticAddendum.protocolUnsafeOverlap'));
         export const pyrightCommentIgnoreTip = () => getRawString('DiagnosticAddendum.pyrightCommentIgnoreTip');
         export const readOnlyAttribute = () =>
             new ParameterizedString<{ name: string }>(getRawString('DiagnosticAddendum.readOnlyAttribute'));

packages/pyright-internal/src/localization/package.nls.en-us.json

@@ -377,6 +377,7 @@
         "protocolBaseClassWithTypeArgs": "Type arguments are not allowed with Protocol class when using type parameter syntax",
         "protocolIllegal": "Use of \"Protocol\" requires Python 3.7 or newer",
         "protocolNotAllowed": "\"Protocol\" cannot be used in this context",
+        "protocolUnsafeOverlap": "Class overlaps \"{name}\" unsafely and could produce a match at runtime",
         "protocolVarianceContravariant": "Type variable \"{variable}\" used in generic protocol \"{class}\" should be contravariant",
         "protocolVarianceCovariant": "Type variable \"{variable}\" used in generic protocol \"{class}\" should be covariant",
         "protocolVarianceInvariant": "Type variable \"{variable}\" used in generic protocol \"{class}\" should be invariant",
@@ -713,6 +714,7 @@
         "protocolMemberMissing": "\"{name}\" is not present",
         "protocolRequiresRuntimeCheckable": "Protocol class must be @runtime_checkable to be used with instance and class checks",
         "protocolSourceIsNotConcrete": "\"{sourceType}\" is not a concrete class type and cannot be assigned to type \"{destType}\"",
+        "protocolUnsafeOverlap": "Attributes of \"{name}\" have the same names as the protocol",
         "pyrightCommentIgnoreTip": "Use \"# pyright: ignore[<diagnostic rules>] to suppress diagnostics for a single line",
         "readOnlyAttribute": "Attribute \"{name}\" is read-only",
         "seeDeclaration": "See declaration",

packages/pyright-internal/src/tests/samples/protocol49.py

@@ -0,0 +1,41 @@
+# This sample tests the detection of a runtime checkable protocol
+# that unsafely overlaps a class within an isinstance or issubclass
+# call.
+
+# > Type checkers should reject an isinstance() or issubclass() call if there
+# > is an unsafe overlap between the type of the first argument and the protocol.
+
+
+from typing import Protocol, runtime_checkable
+
+
+@runtime_checkable
+class Proto3(Protocol):
+    def method1(self, a: int) -> int:
+        ...
+
+
+class Concrete3A:
+    def method1(self, a: str) -> None:
+        pass
+
+
+@runtime_checkable
+class Proto2(Protocol):
+    def other(self) -> None:
+        ...
+
+
+class Concrete3B:
+    method1: int = 1
+
+
+def func3():
+    if isinstance(Concrete3A(), Proto3):  # Type error: unsafe overlap
+        pass
+
+    if isinstance(Concrete3B(), (Proto3, Proto2)):  # Type error: unsafe overlap
+        pass
+
+    if issubclass(Concrete3A, (Proto3, Proto2)):  # Type error: unsafe overlap
+        pass

packages/pyright-internal/src/tests/typeEvaluator2.test.ts

@@ -1405,6 +1405,12 @@ test('Protocol48', () => {
     TestUtils.validateResults(analysisResults, 0);
 });
 
+test('Protocol49', () => {
+    const analysisResults = TestUtils.typeAnalyzeSampleFiles(['protocol49.py']);
+
+    TestUtils.validateResults(analysisResults, 3);
+});
+
 test('TypedDict1', () => {
     const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typedDict1.py']);