Skip to content

Commit

Permalink
Added new check for a common source of bugs where an equals operator …
Browse files Browse the repository at this point in the history
…within an if statement compares two values whose literal types do not overlap and will therefore never evaluate to True.
  • Loading branch information
msfterictraut committed Mar 23, 2021
1 parent e91e976 commit 740ba46
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 0 deletions.
36 changes: 36 additions & 0 deletions packages/pyright-internal/src/analyzer/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ import {
YieldFromNode,
YieldNode,
} from '../parser/parseNodes';
import { OperatorType } from '../parser/tokenizerTypes';
import { AnalyzerFileInfo } from './analyzerFileInfo';
import * as AnalyzerNodeInfo from './analyzerNodeInfo';
import { Declaration, DeclarationType } from './declaration';
Expand Down Expand Up @@ -116,6 +117,7 @@ import {
getDeclaredGeneratorReturnType,
getDeclaredGeneratorYieldType,
isEllipsisType,
isLiteralTypeOrUnion,
isNoReturnType,
isOpenEndedTupleClass,
isPartlyUnknown,
Expand Down Expand Up @@ -607,6 +609,40 @@ export class Checker extends ParseTreeWalker {
}

visitIf(node: IfNode): boolean {
// Check for expressions where a variable is being compared to
// a literal string or number. Look for a common bug where
// the comparison will always be False.
if (
node.testExpression.nodeType === ParseNodeType.BinaryOperation &&
node.testExpression.operator === OperatorType.Equals
) {
const rightType = this._evaluator.getType(node.testExpression.rightExpression);
if (rightType && isLiteralTypeOrUnion(rightType)) {
const leftType = this._evaluator.getType(node.testExpression.leftExpression);
if (leftType && isLiteralTypeOrUnion(leftType)) {
let isPossiblyTrue = false;

doForEachSubtype(leftType, (leftSubtype) => {
if (this._evaluator.canAssignType(rightType, leftSubtype, new DiagnosticAddendum())) {
isPossiblyTrue = true;
}
});

if (!isPossiblyTrue) {
this._evaluator.addDiagnostic(
this._fileInfo.diagnosticRuleSet.reportGeneralTypeIssues,
DiagnosticRule.reportGeneralTypeIssues,
Localizer.Diagnostic.comparisonAlwaysFalse().format({
leftType: this._evaluator.printType(leftType, /* expandTypeAlias */ true),
rightType: this._evaluator.printType(rightType, /* expandTypeAlias */ true),
}),
node.testExpression
);
}
}
}
}

this._evaluator.getType(node.testExpression);
return true;
}
Expand Down
4 changes: 4 additions & 0 deletions packages/pyright-internal/src/localization/localize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,10 @@ export namespace Localizer {
export const classGetItemClsParam = () => getRawString('Diagnostic.classGetItemClsParam');
export const classVarFirstArgMissing = () => getRawString('Diagnostic.classVarFirstArgMissing');
export const classVarTooManyArgs = () => getRawString('Diagnostic.classVarTooManyArgs');
export const comparisonAlwaysFalse = () =>
new ParameterizedString<{ leftType: string; rightType: string }>(
getRawString('Diagnostic.comparisonAlwaysFalse')
);
export const comprehensionInDict = () => getRawString('Diagnostic.comprehensionInDict');
export const comprehensionInSet = () => getRawString('Diagnostic.comprehensionInSet');
export const concatenateParamSpecMissing = () => getRawString('Diagnostic.concatenateParamSpecMissing');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
"classNotRuntimeSubscriptable": "Subscript for class \"{name}\" will generate runtime exception; enclose type annotation in quotes",
"classVarFirstArgMissing": "Expected a type argument after \"ClassVar\"",
"classVarTooManyArgs": "Expected only one type argument after \"ClassVar\"",
"comparisonAlwaysFalse": "Condition will always evaluate to False since the types \"{leftType}\" and \"{rightType}\" have no overlap",
"comprehensionInDict": "Comprehension cannot be used with other dictionary entries",
"comprehensionInSet": "Comprehension cannot be used with other set entries",
"concatenateParamSpecMissing": "Last type argument for \"Concatenate\" must be a ParamSpec",
Expand Down
27 changes: 27 additions & 0 deletions packages/pyright-internal/src/tests/samples/comparison1.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# This sample tests the check for non-overlapping types compared
# with equals comparison.

from typing import Literal


OS = Literal["Linux", "Darwin", "Windows"]


def func1(os: OS, val: Literal[1, "linux"]):
if os == "Linux":
return True

# This should generate an error because there is no overlap in types.
if os == "darwin":
return False

# This should generate an error because there is no overlap in types.
if os == val:
return False

# This should generate an error because there is no overlap in types.
if val == 2:
return False

if val == 1:
return True
5 changes: 5 additions & 0 deletions packages/pyright-internal/src/tests/typeEvaluator1.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1411,3 +1411,8 @@ test('List1', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['list1.py']);
TestUtils.validateResults(analysisResults, 0);
});

test('Comparison1', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['comparison1.py']);
TestUtils.validateResults(analysisResults, 3);
});

0 comments on commit 740ba46

Please sign in to comment.