From 6da2e0ec816a4859c7138f4b3442f77ebfa92c11 Mon Sep 17 00:00:00 2001
From: Eric Traut <erictr@microsoft.com>
Date: Tue, 14 Dec 2021 21:35:35 -0800
Subject: [PATCH] Fixed false positive error with reportUnnecessaryIsInstance
 diagnostic check with the provided class is dynamic.

---
 .../pyright-internal/src/analyzer/checker.ts  | 37 ++++++++++++++-----
 .../src/tests/checker.test.ts                 |  2 +-
 .../tests/samples/unnecessaryIsInstance1.py   | 25 ++++++++++++-
 3 files changed, 53 insertions(+), 11 deletions(-)

diff --git a/packages/pyright-internal/src/analyzer/checker.ts b/packages/pyright-internal/src/analyzer/checker.ts
index 8c0df7237..526118adc 100644
--- a/packages/pyright-internal/src/analyzer/checker.ts
+++ b/packages/pyright-internal/src/analyzer/checker.ts
@@ -2496,6 +2496,8 @@ export class Checker extends ParseTreeWalker {
         ];
 
         const classTypeList: ClassType[] = [];
+        let arg1IncludesSubclasses = false;
+
         doForEachSubtype(arg1Type, (arg1Subtype) => {
             if (isClass(arg1Subtype)) {
                 if (TypeBase.isInstantiable(arg1Subtype)) {
@@ -2506,18 +2508,33 @@ export class Checker extends ParseTreeWalker {
                     ) {
                         isValidType = false;
                     }
+
+                    if (arg1Subtype.includeSubclasses) {
+                        arg1IncludesSubclasses = true;
+                    }
                 } else {
                     // The isinstance and issubclass call supports a variation where the second
                     // parameter is a tuple of classes.
-                    if (isTupleClass(arg1Subtype) && arg1Subtype.tupleTypeArguments) {
-                        arg1Subtype.tupleTypeArguments.forEach((typeArg) => {
-                            if (isInstantiableClass(typeArg)) {
-                                classTypeList.push(typeArg);
-                            } else {
-                                isValidType = false;
-                            }
-                        });
+                    if (isTupleClass(arg1Subtype)) {
+                        if (arg1Subtype.tupleTypeArguments) {
+                            arg1Subtype.tupleTypeArguments.forEach((typeArg) => {
+                                if (isInstantiableClass(typeArg)) {
+                                    classTypeList.push(typeArg);
+
+                                    if (typeArg.includeSubclasses) {
+                                        arg1IncludesSubclasses = true;
+                                    }
+                                } else {
+                                    isValidType = false;
+                                }
+                            });
+                        }
+                    } else {
+                        if (arg1Subtype.includeSubclasses) {
+                            arg1IncludesSubclasses = true;
+                        }
                     }
+
                     if (
                         ClassType.isBuiltIn(arg1Subtype) &&
                         nonstandardClassTypes.some((name) => name === arg1Subtype.details.name)
@@ -2639,7 +2656,9 @@ export class Checker extends ParseTreeWalker {
             return combineTypes(objTypeList);
         };
 
-        if (isTypeSame(filteredType, arg0Type, /* ignorePseudoGeneric */ true)) {
+        // If arg1IncludesSubclasses is true, it contains a Type[X] class rather than X. A Type[X]
+        // could be a subclass of X, so the "unnecessary isinstance check" may be legit.
+        if (!arg1IncludesSubclasses && isTypeSame(filteredType, arg0Type, /* ignorePseudoGeneric */ true)) {
             this._evaluator.addDiagnostic(
                 this._fileInfo.diagnosticRuleSet.reportUnnecessaryIsInstance,
                 DiagnosticRule.reportUnnecessaryIsInstance,
diff --git a/packages/pyright-internal/src/tests/checker.test.ts b/packages/pyright-internal/src/tests/checker.test.ts
index e5ede80bb..7b1badfd9 100644
--- a/packages/pyright-internal/src/tests/checker.test.ts
+++ b/packages/pyright-internal/src/tests/checker.test.ts
@@ -201,7 +201,7 @@ test('UnnecessaryIsInstance1', () => {
     // Turn on errors.
     configOptions.diagnosticRuleSet.reportUnnecessaryIsInstance = 'error';
     analysisResults = TestUtils.typeAnalyzeSampleFiles(['unnecessaryIsInstance1.py'], configOptions);
-    TestUtils.validateResults(analysisResults, 4);
+    TestUtils.validateResults(analysisResults, 5);
 });
 
 test('UnnecessaryIsSubclass1', () => {
diff --git a/packages/pyright-internal/src/tests/samples/unnecessaryIsInstance1.py b/packages/pyright-internal/src/tests/samples/unnecessaryIsInstance1.py
index 78aad531d..1a9343f4e 100644
--- a/packages/pyright-internal/src/tests/samples/unnecessaryIsInstance1.py
+++ b/packages/pyright-internal/src/tests/samples/unnecessaryIsInstance1.py
@@ -1,6 +1,14 @@
 # This sample tests unnecessary isinstance error reporting.
 
-from typing import ClassVar, Literal, Protocol, TypedDict, Union, runtime_checkable
+from typing import (
+    ClassVar,
+    Literal,
+    Protocol,
+    Type,
+    TypedDict,
+    Union,
+    runtime_checkable,
+)
 
 from unknown_import import CustomClass1
 
@@ -67,3 +75,18 @@ def func3(obj: BaseClass):
 
     if isinstance(obj, (ClassA, ClassB, ClassC)):
         t_2: Literal["ClassA | ClassB"] = reveal_type(obj)
+
+
+class A:
+    pass
+
+
+class B(A):
+    pass
+
+
+def func4(a: A, cls: Type[A]) -> None:
+    isinstance(a, cls)
+
+    # This should generate an error because it's always true.
+    isinstance(a, A)