Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions packages/pyright-internal/src/analyzer/operations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -765,6 +765,33 @@ export function getTypeOfUnaryOperation(
return { type, isIncomplete, magicMethodDeprecationInfo: deprecatedInfo };
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
[High] The isBoolLiteralName guard only checks for bool literals, but the same reassignment concern applies to any literal-typed Name. For example:

count = 42  # flow-narrowed to Literal[42]
result = "yes" if count else "no"

canBeFalsy(Literal[42]) returns false, so the else branch is pruned — but count is just as reassignable as a bool flag. Either generalize the guard to all literal-typed Names or add a comment explaining why bool is uniquely special here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
[Medium] The guard fires for any Name with a Literal[True/False] type, including legitimately flow-narrowed bools. Consider def f(x: bool): if x: return 1 if x else "no" — here x is narrowed to Literal[True] via control flow (not assignment), so the else branch should be prunable, but the guard blocks it. The guard can't distinguish "narrowed by flow" from "literal from assignment." This is worth a test case to document the accepted trade-off and a comment clarifying the limitation.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
[Medium] (Unresolved prior feedback) isBoolLiteralName only guards bool literals, not other literal types like Literal[42]. The Advocate's defense that bool is uniquely common as a mutable flag pattern is reasonable, but this design choice should be documented. Add a comment explaining why only bool is guarded — e.g., "Only bool is guarded because bool literals are the overwhelmingly common case for mutable flag variables used as ternary conditions; non-bool literals like Literal[42] are rarely used as ternary conditions where reassignment would cause over-narrowing."

// Helper function to check if an expression is a simple name or `not <Name>` with a literal bool type.
// We avoid narrowing for these cases because the variable could be reassigned.
function isBoolLiteralName(expr: ExpressionNode, exprType: Type, evaluator: TypeEvaluator): boolean {
// Check for simple name references
if (expr.nodeType === ParseNodeType.Name) {
return (
isClassInstance(exprType) &&
ClassType.isBuiltIn(exprType, 'bool') &&
exprType.priv.literalValue !== undefined
);
}

// Check for `not <Name>` expressions
if (expr.nodeType === ParseNodeType.UnaryOperation && expr.d.operator === OperatorType.Not) {
// Evaluate the inner expression's type (not the `not` expression's type).
// Note: makeTopLevelTypeVarsConcrete is needed here for the isClassInstance/
// ClassType.isBuiltIn checks, unlike the outer call site which was removed
// because canBeTruthy/canBeFalsy do it internally.
const innerType = evaluator.makeTopLevelTypeVarsConcrete(
evaluator.getTypeOfExpression(expr.d.expr).type
);
return isBoolLiteralName(expr.d.expr, innerType, evaluator);
}

return false;
}

export function getTypeOfTernaryOperation(
evaluator: TypeEvaluator,
node: TernaryNode,
Expand All @@ -778,7 +805,9 @@ export function getTypeOfTernaryOperation(
return { type: UnknownType.create() };
}

evaluator.getTypeOfExpression(node.d.testExpr);
// Get the narrowed type of the test expression at this point in the code flow.
const testExprTypeResult = evaluator.getTypeOfExpression(node.d.testExpr);
const testExprType = testExprTypeResult.type;

const typesToCombine: Type[] = [];
let isIncomplete = false;
Expand All @@ -790,7 +819,19 @@ export function getTypeOfTernaryOperation(
fileInfo.definedConstants
);

if (constExprValue !== false && evaluator.isNodeReachable(node.d.ifExpr)) {
// Check if we should apply flow-sensitive narrowing. We avoid narrowing for
// simple name references with literal bool types because the variable could
// be reassigned, even though the type is a literal. This also applies to
// `not <Name>` expressions to maintain consistency.
// Note: This guard is specific to ternary expressions. The and/or operators
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
[Medium] The comment explains that and/or operators "don't need this guard because they operate on already-evaluated types from their operands." Both the Skeptic and Architect note this distinction is debatable — and/or's leftType is also flow-narrowed via getTypeOfExpression. The observable inconsistency: maybe = True; maybe and 1 or "no" narrows (no guard), but 1 if maybe else "no" does not (guard fires). Consider acknowledging this inconsistency in the comment rather than asserting a structural distinction.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
[Medium] (Unresolved prior feedback) The comment "and/or operators don't need this guard because they operate on already-evaluated types from their operands" asserts a structural distinction that all four reviewers agree is inaccurate — and/or's leftType is also flow-narrowed via getTypeOfExpression. Revise to something like: "The and/or operators historically apply narrowing without this guard. The guard is added here as conservative behavior for the new ternary narrowing, since ternary conditions are more commonly simple name references that may have been reassigned."

// don't need this guard because they operate on already-evaluated types from
// their operands, not on types that may have been narrowed by upstream flow analysis.
const shouldApplyNarrowing = !isBoolLiteralName(node.d.testExpr, testExprType, evaluator);

// Determine if the if-branch is reachable based on static evaluation,
// general reachability, and flow-sensitive type narrowing.
const testCanBeTruthy = shouldApplyNarrowing ? evaluator.canBeTruthy(testExprType) : true;
if (constExprValue !== false && evaluator.isNodeReachable(node.d.ifExpr) && testCanBeTruthy) {
const ifType = evaluator.getTypeOfExpression(node.d.ifExpr, flags, inferenceContext);
typesToCombine.push(ifType.type);
if (ifType.isIncomplete) {
Expand All @@ -801,7 +842,10 @@ export function getTypeOfTernaryOperation(
}
}

if (constExprValue !== true && evaluator.isNodeReachable(node.d.elseExpr)) {
// Determine if the else-branch is reachable based on static evaluation,
// general reachability, and flow-sensitive type narrowing.
const testCanBeFalsy = shouldApplyNarrowing ? evaluator.canBeFalsy(testExprType) : true;
if (constExprValue !== true && evaluator.isNodeReachable(node.d.elseExpr) && testCanBeFalsy) {
const elseType = evaluator.getTypeOfExpression(node.d.elseExpr, flags, inferenceContext);
typesToCombine.push(elseType.type);
if (elseType.isIncomplete) {
Expand Down
106 changes: 106 additions & 0 deletions packages/pyright-internal/src/tests/samples/conditionalExpr1.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
# This sample tests type narrowing for conditional expressions (ternary operator)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
[Medium] Missing test case: def f(x: Literal[True]): return 1 if x else "no" — where the literal type comes from an explicit annotation, not assignment. This would document whether the guard's behavior for explicitly-typed parameters is accepted or surprising. Also consider adding a test for nested ternaries (a if x else (b if y else c)) to confirm correct narrowing propagation.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
[Medium] (Unresolved prior feedback) Missing test cases that would strengthen coverage: (1) def f(x: Literal[True]): return 1 if x else "no" — documents the guard's behavior for explicitly-annotated parameters where no reassignment occurred. (2) A nested ternary a if x else (b if y else c) for regression coverage. (3) A custom class without __bool__/__len__ to verify canBeFalsy is the deciding factor (not isNodeReachable).

# where the condition is narrowed such that one branch is known to be unreachable.

from typing import assert_type


class Node:
pass


class Wrapper:
def __init__(self, child: Node):
self.child = child


class SpecialNode(Node):
pass


def func1(plan: Wrapper | None):
# After the guard, plan is known to be truthy (not None) and
# plan.child is known to be SpecialNode.
if not (plan and isinstance(plan.child, SpecialNode)):
return

# This should be fine - direct assignment works.
ts1: SpecialNode = plan.child

# This should also be fine - the else branch (None) is unreachable
# because plan is known to be truthy in this context.
ts2: SpecialNode = plan.child if plan else None

# Also verify the inferred type.
assert_type(plan.child if plan else None, SpecialNode)


def func2(val: int | None):
# After this guard, val is known to be truthy (not None and not 0).
if not val:
return

# The else branch is unreachable since val is known to be truthy.
# Using different types to make the test meaningful.
ts1: int = val if val else "fallback"
assert_type(val if val else "fallback", int)


def func3(val: str | None):
# After this guard, val is known to be None (falsy).
if val:
return

# The if branch is unreachable since val is known to be falsy (None).
# Using different types to make the test meaningful - without pruning,
# this would be str | None instead of just None.
ts1: None = "unreachable" if val else None
assert_type("unreachable" if val else None, None)


def func4(val: int | None):
# After this guard, val is known to be not None (but could still be 0, which is falsy).
if val is None:
return

# The else branch is still reachable since val could be 0 (falsy).
# This test verifies that we don't over-narrow.
ts1: int | str = val if val else "zero"
assert_type(val if val else "zero", int | str)


def func5(val: int | None):
# After this guard, val is known to be truthy (not None and not 0).
if not val:
return

# The else branch is unreachable since val is known to be truthy.
# Using different types to make the test meaningful.
ts1: int = val if val else "fallback"
assert_type(val if val else "fallback", int)


def func6(val: list[int] | None):
# After this guard, val is known to be not None.
if val is None:
return

# However, val could still be an empty list (falsy), so the else branch
# is still reachable. Both branches should contribute to the type.
ts1: list[int] | str = val if val else "empty"
assert_type(val if val else "empty", list[int] | str)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot generated:
[High] func_bool_literal and func_bool_literal_not are vacuous — they pass identically with the isBoolLiteralName guard deleted. Pyright widens maybe = True to bool (not Literal[True]), so isBoolLiteralName returns false and the guard never fires. canBeTruthy(bool) and canBeFalsy(bool) both return true, producing int | str regardless.

Replace these with tests that actually produce Literal[True] via flow narrowing:

def func_guard_flow_narrowed(x: bool):
    if x:
        # x is Literal[True] here — guard should fire
        ts: int | str = 1 if x else "no"
        assert_type(1 if x else "no", int | str)

def func_guard_flow_narrowed_not(x: bool):
    if x:
        ts: int | str = 1 if not x else "yes"
        assert_type(1 if not x else "yes", int | str)



def func_bool_literal():
# Test that the bool literal guard prevents over-narrowing for mutable variables.
maybe = True
# Both branches should remain since maybe could be reassigned.
ts: int | str = 1 if maybe else "no"
assert_type(1 if maybe else "no", int | str)


def func_bool_literal_not():
# Test that the bool literal guard also applies to `not` expressions.
flag = True
# Both branches should remain since flag could be reassigned.
ts: int | str = 1 if not flag else "yes"
assert_type(1 if not flag else "yes", int | str)
5 changes: 5 additions & 0 deletions packages/pyright-internal/src/tests/typeEvaluator5.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,11 @@ test('Conditional1', () => {
TestUtils.validateResults(analysisResults, 15);
});

test('ConditionalExpr1', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['conditionalExpr1.py']);
TestUtils.validateResults(analysisResults, 0);
});

test('TypePrinter1', () => {
const analysisResults = TestUtils.typeAnalyzeSampleFiles(['typePrinter1.py']);
TestUtils.validateResults(analysisResults, 0);
Expand Down
Loading