Skip to content

Commit

Permalink
Fixed bug in code flow engine that led to incorrect type evaluation o…
Browse files Browse the repository at this point in the history
…f a variable in a nested loop. This addresses https://github.com/microsoft/pylance-release/issues/4509.
  • Loading branch information
msfterictraut committed Jun 19, 2023
1 parent 056415f commit 23bcbce
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 4 deletions.
29 changes: 25 additions & 4 deletions packages/pyright-internal/src/analyzer/codeFlowEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -864,6 +864,7 @@ export function getCodeFlowEngine(
reference === undefined &&
cacheEntry.incompleteSubtypes?.some((subtype) => subtype.type !== undefined);
let firstAntecedentTypeIsIncomplete = false;
let firstAntecedentTypeIsPending = false;

loopNode.antecedents.forEach((antecedent, index) => {
// If we've trying to determine reachability and we've already proven
Expand All @@ -872,6 +873,10 @@ export function getCodeFlowEngine(
return;
}

if (firstAntecedentTypeIsPending && index > 0) {
return;
}

cacheEntry = getCacheEntry(loopNode)!;

// Is this entry marked "pending"? If so, we have recursed and there
Expand All @@ -883,9 +888,21 @@ export function getCodeFlowEngine(
index < cacheEntry.incompleteSubtypes.length &&
cacheEntry.incompleteSubtypes[index].isPending
) {
sawIncomplete = true;
sawPending = true;
return;
// In rare circumstances, it's possible for a code flow graph with
// nested loops to hit the case where the first antecedent is marked
// as pending. In this case, we'll evaluate only the first antecedent
// again even though it's pending. We're guaranteed to make forward
// progress with the first antecedent, and that will allow us to establish
// an initial type for this expression, but we don't want to evaluate
// any other antecedents in this case because this could result in
// infinite recursion.
if (index === 0) {
firstAntecedentTypeIsPending = true;
} else {
sawIncomplete = true;
sawPending = true;
return;
}
}

// Have we already been here (i.e. does the entry exist and is
Expand All @@ -895,7 +912,11 @@ export function getCodeFlowEngine(
cacheEntry.incompleteSubtypes !== undefined && index < cacheEntry.incompleteSubtypes.length
? cacheEntry.incompleteSubtypes[index]
: undefined;
if (subtypeEntry === undefined || (!subtypeEntry?.isPending && subtypeEntry?.isIncomplete)) {
if (
subtypeEntry === undefined ||
(!subtypeEntry?.isPending && subtypeEntry?.isIncomplete) ||
index === 0
) {
const entryEvaluationCount = subtypeEntry === undefined ? 0 : subtypeEntry.evaluationCount;

// Set this entry to "pending" to prevent infinite recursion.
Expand Down
22 changes: 22 additions & 0 deletions packages/pyright-internal/src/tests/samples/loops38.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
# This sample tests a code flow graph that includes a nested loop
# and a variable that is assigned only in the outer loop.


# pyright: strict


# * Code flow graph for func1:
# Assign[step+=1] ── True[step==0] ── Assign[node=] ── Loop ┬─ Loop ┬─ Assign[step=1] ── Start
# │ ╰ Circular(Assign[step+=1])
# ╰ FalseNever ─ False ─ Circular(Assign[node])
def func1(nodes: list[int]):
step = 1
while True:
for node in nodes:
if node or step == 0:
step += 1
break
else:
return


6 changes: 6 additions & 0 deletions packages/pyright-internal/src/tests/typeEvaluator3.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,12 @@ test('Loops37', () => {
TestUtils.validateResults(analysisResults, 0);
});

test('Loops38', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['loops38.py']);

TestUtils.validateResults(analysisResults, 0);
});

test('ForLoop1', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['forLoop1.py']);

Expand Down

0 comments on commit 23bcbce

Please sign in to comment.