Fixed false positive error with reportUnnecessaryIsInstance diagnostic check with the provided class is dynamic.

This commit is contained in:
Eric Traut 2021-12-14 21:35:35 -08:00
parent df3313c2cd
commit 6da2e0ec81
3 changed files with 53 additions and 11 deletions

View File

@ -2496,6 +2496,8 @@ export class Checker extends ParseTreeWalker {
]; ];
const classTypeList: ClassType[] = []; const classTypeList: ClassType[] = [];
let arg1IncludesSubclasses = false;
doForEachSubtype(arg1Type, (arg1Subtype) => { doForEachSubtype(arg1Type, (arg1Subtype) => {
if (isClass(arg1Subtype)) { if (isClass(arg1Subtype)) {
if (TypeBase.isInstantiable(arg1Subtype)) { if (TypeBase.isInstantiable(arg1Subtype)) {
@ -2506,18 +2508,33 @@ export class Checker extends ParseTreeWalker {
) { ) {
isValidType = false; isValidType = false;
} }
if (arg1Subtype.includeSubclasses) {
arg1IncludesSubclasses = true;
}
} else { } else {
// The isinstance and issubclass call supports a variation where the second // The isinstance and issubclass call supports a variation where the second
// parameter is a tuple of classes. // parameter is a tuple of classes.
if (isTupleClass(arg1Subtype) && arg1Subtype.tupleTypeArguments) { if (isTupleClass(arg1Subtype)) {
arg1Subtype.tupleTypeArguments.forEach((typeArg) => { if (arg1Subtype.tupleTypeArguments) {
if (isInstantiableClass(typeArg)) { arg1Subtype.tupleTypeArguments.forEach((typeArg) => {
classTypeList.push(typeArg); if (isInstantiableClass(typeArg)) {
} else { classTypeList.push(typeArg);
isValidType = false;
} if (typeArg.includeSubclasses) {
}); arg1IncludesSubclasses = true;
}
} else {
isValidType = false;
}
});
}
} else {
if (arg1Subtype.includeSubclasses) {
arg1IncludesSubclasses = true;
}
} }
if ( if (
ClassType.isBuiltIn(arg1Subtype) && ClassType.isBuiltIn(arg1Subtype) &&
nonstandardClassTypes.some((name) => name === arg1Subtype.details.name) nonstandardClassTypes.some((name) => name === arg1Subtype.details.name)
@ -2639,7 +2656,9 @@ export class Checker extends ParseTreeWalker {
return combineTypes(objTypeList); 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._evaluator.addDiagnostic(
this._fileInfo.diagnosticRuleSet.reportUnnecessaryIsInstance, this._fileInfo.diagnosticRuleSet.reportUnnecessaryIsInstance,
DiagnosticRule.reportUnnecessaryIsInstance, DiagnosticRule.reportUnnecessaryIsInstance,

View File

@ -201,7 +201,7 @@ test('UnnecessaryIsInstance1', () => {
// Turn on errors. // Turn on errors.
configOptions.diagnosticRuleSet.reportUnnecessaryIsInstance = 'error'; configOptions.diagnosticRuleSet.reportUnnecessaryIsInstance = 'error';
analysisResults = TestUtils.typeAnalyzeSampleFiles(['unnecessaryIsInstance1.py'], configOptions); analysisResults = TestUtils.typeAnalyzeSampleFiles(['unnecessaryIsInstance1.py'], configOptions);
TestUtils.validateResults(analysisResults, 4); TestUtils.validateResults(analysisResults, 5);
}); });
test('UnnecessaryIsSubclass1', () => { test('UnnecessaryIsSubclass1', () => {

View File

@ -1,6 +1,14 @@
# This sample tests unnecessary isinstance error reporting. # 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 from unknown_import import CustomClass1
@ -67,3 +75,18 @@ def func3(obj: BaseClass):
if isinstance(obj, (ClassA, ClassB, ClassC)): if isinstance(obj, (ClassA, ClassB, ClassC)):
t_2: Literal["ClassA | ClassB"] = reveal_type(obj) 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)