Skip to content

Commit

Permalink
Improved consistency of unreachable code. Previously, unreachable cod…
Browse files Browse the repository at this point in the history
…e was not supported for `if` or `else` suites when the condition type was narrowed to `Never`. This addresses microsoft/pylance-release#6028.
  • Loading branch information
erictraut committed Jun 20, 2024
1 parent 81ff21e commit d5c4e8c
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 30 deletions.
26 changes: 10 additions & 16 deletions packages/pyright-internal/src/analyzer/codeFlowEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ export interface CodeFlowAnalyzer {
}

export interface CodeFlowEngine {
createCodeFlowAnalyzer: (typeAtStart: TypeResult | undefined) => CodeFlowAnalyzer;
createCodeFlowAnalyzer: () => CodeFlowAnalyzer;
isFlowNodeReachable: (flowNode: FlowNode, sourceFlowNode?: FlowNode, ignoreNoReturn?: boolean) => boolean;
narrowConstrainedTypeVar: (flowNode: FlowNode, typeVar: TypeVarType) => Type | undefined;
printControlFlowGraph: (
Expand Down Expand Up @@ -173,11 +173,7 @@ export function getCodeFlowEngine(
// Creates a new code flow analyzer that can be used to narrow the types
// of the expressions within an execution context. Each code flow analyzer
// instance maintains a cache of types it has already determined.
// The caller should pass a typeAtStart value because the code flow
// analyzer may cache types based on this value, but the typeAtStart
// may vary depending on the context in which the code flow analysis
// is performed.
function createCodeFlowAnalyzer(typeAtStart: TypeResult | undefined): CodeFlowAnalyzer {
function createCodeFlowAnalyzer(): CodeFlowAnalyzer {
const flowNodeTypeCacheSet = new Map<string, CodeFlowTypeCache>();

function getFlowNodeTypeCacheForReference(referenceKey: string) {
Expand Down Expand Up @@ -513,10 +509,6 @@ export function getCodeFlowEngine(
}
}

if (flowTypeResult && !isFlowNodeReachable(flowNode)) {
flowTypeResult = undefined;
}

return setCacheEntry(curFlowNode, flowTypeResult?.type, !!flowTypeResult?.isIncomplete);
}

Expand Down Expand Up @@ -1226,24 +1218,26 @@ export function getCodeFlowEngine(
curFlowNode.flags &
(FlowFlags.VariableAnnotation |
FlowFlags.Assignment |
FlowFlags.TrueCondition |
FlowFlags.FalseCondition |
FlowFlags.WildcardImport |
FlowFlags.NarrowForPattern |
FlowFlags.ExhaustedMatch)
) {
const typedFlowNode = curFlowNode as
| FlowVariableAnnotation
| FlowAssignment
| FlowCondition
| FlowWildcardImport
| FlowCondition
| FlowExhaustedMatch;
curFlowNode = typedFlowNode.antecedent;
continue;
}

if (curFlowNode.flags & (FlowFlags.TrueNeverCondition | FlowFlags.FalseNeverCondition)) {
if (
curFlowNode.flags &
(FlowFlags.TrueCondition |
FlowFlags.FalseCondition |
FlowFlags.TrueNeverCondition |
FlowFlags.FalseNeverCondition)
) {
const conditionalFlowNode = curFlowNode as FlowCondition;
if (conditionalFlowNode.reference) {
// Make sure the reference type has a declared type. If not,
Expand Down Expand Up @@ -1365,7 +1359,7 @@ export function getCodeFlowEngine(

// Protect against infinite recursion.
if (isReachableRecursionSet.has(flowNode.id)) {
return true;
return false;
}
isReachableRecursionSet.add(flowNode.id);

Expand Down
4 changes: 2 additions & 2 deletions packages/pyright-internal/src/analyzer/typeEvaluator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19876,7 +19876,7 @@ export function createTypeEvaluator(
}

// Allocate a new code flow analyzer.
const analyzer = codeFlowEngine.createCodeFlowAnalyzer(typeAtStart);
const analyzer = codeFlowEngine.createCodeFlowAnalyzer();
if (entries) {
entries.push({ typeAtStart, codeFlowAnalyzer: analyzer });
} else {
Expand Down Expand Up @@ -22181,7 +22181,7 @@ export function createTypeEvaluator(
const prevTypeCache = returnTypeInferenceTypeCache;
returnTypeInferenceContextStack.push({
functionNode,
codeFlowAnalyzer: codeFlowEngine.createCodeFlowAnalyzer(/* typeAtStart */ undefined),
codeFlowAnalyzer: codeFlowEngine.createCodeFlowAnalyzer(),
});

try {
Expand Down
2 changes: 1 addition & 1 deletion packages/pyright-internal/src/tests/checker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ test('With2', () => {
test('With3', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['with3.py']);

TestUtils.validateResults(analysisResults, 5);
TestUtils.validateResults(analysisResults, 6);
});

test('With4', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,21 +49,21 @@ def meth1(
# This should generate an error.
return [0]

if cond:
if cond or 3 > 2:
if isinstance(val1, str):
# This should generate an error.
return [0]
else:
return [0]

if cond:
if cond or 3 > 2:
if isinstance(val3, B):
return [B()]
else:
# This should generate an error.
return [C()]

if cond:
if cond or 3 > 2:
if not isinstance(val3, B) and not isinstance(val3, C):
return [A()]

Expand Down
11 changes: 7 additions & 4 deletions packages/pyright-internal/src/tests/samples/tryExcept4.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
# This sample validates that the exception type provided
# within a raise statement is valid.

a: bool = True
from random import random


a: bool = True if random() > 0.5 else False


class CustomException1(BaseException):
Expand All @@ -11,10 +14,10 @@ def __init__(self, code: int):

# This should generate an error because CustomException1
# requires an argument to instantiate.
if a:
if a or 2 > 1:
raise CustomException1

if a:
if a or 2 > 1:
raise CustomException1(3)


Expand All @@ -24,5 +27,5 @@ class CustomException2:

# This should generate an error because
# the exception doesn't derive from BaseException.
if a:
if a or 2 > 1:
raise CustomException2
18 changes: 17 additions & 1 deletion packages/pyright-internal/src/tests/samples/unreachable1.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# This sample tests the detection and reporting of unreachable code.

from abc import abstractmethod
import os
import sys
from abc import abstractmethod
from typing import NoReturn


Expand Down Expand Up @@ -109,3 +109,19 @@ def func10():
return
# This should be marked unreachable.
b = e.errno


def func11(obj: str) -> list:
if isinstance(obj, str):
return []
else:
# This should be marked as unreachable.
return obj


def func12(obj: str) -> list:
if isinstance(obj, str):
return []

# This should be marked as unreachable.
return obj
3 changes: 1 addition & 2 deletions packages/pyright-internal/src/tests/samples/with3.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ class A:
raise RuntimeError()
return

# This should generate an error because
# the code is not unreachable.
# This should generate an error.
c = "hi" + 3

with memoryview(x):
Expand Down
2 changes: 1 addition & 1 deletion packages/pyright-internal/src/tests/typeEvaluator1.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import * as TestUtils from './testUtils';
test('Unreachable1', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['unreachable1.py']);

TestUtils.validateResults(analysisResults, 0, 0, 2, 1, 4);
TestUtils.validateResults(analysisResults, 0, 0, 2, 1, 6);
});

test('Builtins1', () => {
Expand Down

0 comments on commit d5c4e8c

Please sign in to comment.