Extended reportUnnecessaryComparison to cover more cases involving …

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

@@ -1277,7 +1277,7 @@ export class Checker extends ParseTreeWalker {
         } else if (node.d.operator === OperatorType.Is || node.d.operator === OperatorType.IsNot) {
             // Don't apply this rule if it's within an assert.
             if (!ParseTreeUtils.isWithinAssertExpression(node)) {
-                this._validateComparisonTypesForIsOperator(node);
+                this._validateComparisonTypes(node);
             }
         } else if (node.d.operator === OperatorType.In || node.d.operator === OperatorType.NotIn) {
             // Don't apply this rule if it's within an assert.
@@ -2028,47 +2028,6 @@ export class Checker extends ParseTreeWalker {
         }
     }
 
-    // Determines whether the types of the two operands for an "is" or "is not"
-    // operation have overlapping types.
-    private _validateComparisonTypesForIsOperator(node: BinaryOperationNode) {
-        const rightType = this._evaluator.getType(node.d.rightExpr);
-
-        if (!rightType || !isNoneInstance(rightType)) {
-            return;
-        }
-
-        const leftType = this._evaluator.getType(node.d.leftExpr);
-        if (!leftType) {
-            return;
-        }
-
-        let foundMatchForNone = false;
-        doForEachSubtype(leftType, (subtype) => {
-            subtype = this._evaluator.makeTopLevelTypeVarsConcrete(subtype);
-
-            if (this._evaluator.assignType(subtype, this._evaluator.getNoneType())) {
-                foundMatchForNone = true;
-            }
-        });
-
-        const getMessage = () => {
-            return node.d.operator === OperatorType.Is
-                ? LocMessage.comparisonAlwaysFalse()
-                : LocMessage.comparisonAlwaysTrue();
-        };
-
-        if (!foundMatchForNone) {
-            this._evaluator.addDiagnostic(
-                DiagnosticRule.reportUnnecessaryComparison,
-                getMessage().format({
-                    leftType: this._evaluator.printType(leftType, { expandTypeAlias: true }),
-                    rightType: this._evaluator.printType(rightType),
-                }),
-                node
-            );
-        }
-    }
-
     // Determines whether the types of the two operands for an == or != operation
     // have overlapping types.
     private _validateComparisonTypes(node: BinaryOperationNode) {
@@ -2096,7 +2055,7 @@ export class Checker extends ParseTreeWalker {
         }
 
         const getMessage = () => {
-            return node.d.operator === OperatorType.Equals
+            return node.d.operator === OperatorType.Equals || node.d.operator === OperatorType.Is
                 ? LocMessage.comparisonAlwaysFalse()
                 : LocMessage.comparisonAlwaysTrue();
         };
@@ -2138,23 +2097,24 @@ export class Checker extends ParseTreeWalker {
         } else {
             let isComparable = false;
 
-            doForEachSubtype(leftType, (leftSubtype) => {
+            this._evaluator.mapSubtypesExpandTypeVars(leftType, {}, (leftSubtype) => {
                 if (isComparable) {
                     return;
                 }
 
-                leftSubtype = this._evaluator.makeTopLevelTypeVarsConcrete(leftSubtype);
-                doForEachSubtype(rightType, (rightSubtype) => {
+                this._evaluator.mapSubtypesExpandTypeVars(rightType, {}, (rightSubtype) => {
                     if (isComparable) {
                         return;
                     }
 
-                    rightSubtype = this._evaluator.makeTopLevelTypeVarsConcrete(rightSubtype);
-
                     if (this._isTypeComparable(leftSubtype, rightSubtype)) {
                         isComparable = true;
                     }
+
+                    return rightSubtype;
                 });
+
+                return leftSubtype;
             });
 
             if (!isComparable) {
@@ -2186,11 +2146,7 @@ export class Checker extends ParseTreeWalker {
         }
 
         if (isModule(leftType) || isModule(rightType)) {
-            return isTypeSame(leftType, rightType);
-        }
-
-        if (isNoneInstance(leftType) || isNoneInstance(rightType)) {
-            return isTypeSame(leftType, rightType);
+            return isTypeSame(leftType, rightType, { ignoreConditions: true });
         }
 
         const isLeftCallable = isFunction(leftType) || isOverloaded(leftType);
@@ -2227,7 +2183,7 @@ export class Checker extends ParseTreeWalker {
         }
 
         if (isClassInstance(leftType)) {
-            if (isClassInstance(rightType)) {
+            if (isClass(rightType)) {
                 const genericLeftType = ClassType.specialize(leftType, /* typeArgs */ undefined);
                 const genericRightType = ClassType.specialize(rightType, /* typeArgs */ undefined);
 
@@ -2240,7 +2196,7 @@ export class Checker extends ParseTreeWalker {
 
                 // Assume that if the types are disjoint and built-in classes that they
                 // will never be comparable.
-                if (ClassType.isBuiltIn(leftType) && ClassType.isBuiltIn(rightType)) {
+                if (ClassType.isBuiltIn(leftType) && ClassType.isBuiltIn(rightType) && TypeBase.isInstance(rightType)) {
                     return false;
                 }
             }

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

@@ -6,8 +6,7 @@
 from dataclasses import dataclass
 
 
-def cond() -> bool:
-    ...
+def cond() -> bool: ...
 
 
 # This should generate a diagnostic when reportUnnecessaryComparison is enabled.
@@ -79,11 +78,41 @@ async def func5() -> None:
         pass
 
 
-def func6() -> Coroutine[Any, Any, int] | None:
-    ...
+def func6() -> Coroutine[Any, Any, int] | None: ...
 
 
 def func7():
     coro = func6()
     if coro:
         pass
+
+
+class A: ...
+
+
+def func8(x: A):
+    # This should generate an error if reportUnnecessaryComparison is enabled.
+    if x is True:
+        pass
+
+    # This should generate an error if reportUnnecessaryComparison is enabled.
+    if x is False:
+        pass
+
+    # This should generate an error if reportUnnecessaryComparison is enabled.
+    if x is not True:
+        pass
+
+    # This should generate an error if reportUnnecessaryComparison is enabled.
+    if x is not False:
+        pass
+
+
+def func9(x: object, y: type[object]):
+    if x is y:
+        pass
+
+
+def func10(x: object, y: type[object]):
+    if x is not y:
+        pass

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

@@ -590,7 +590,7 @@ test('Comparison2', () => {
 
     configOptions.diagnosticRuleSet.reportUnnecessaryComparison = 'error';
     const analysisResults2 = TestUtils.typeAnalyzeSampleFiles(['comparison2.py'], configOptions);
-    TestUtils.validateResults(analysisResults2, 11);
+    TestUtils.validateResults(analysisResults2, 15);
 });
 
 test('EmptyContainers1', () => {