diff --git a/crates/ty_python_core/src/builder.rs b/crates/ty_python_core/src/builder.rs index aae20b116d4d4..e374df02920a0 100644 --- a/crates/ty_python_core/src/builder.rs +++ b/crates/ty_python_core/src/builder.rs @@ -228,6 +228,39 @@ impl ConditionFlowSnapshot { } } +/// Whether evaluation produces a result object or chooses a control-flow path. +/// +/// In `Value` context, the enclosing code receives the expression's result object. For example, +/// `result = x and y` produces `x` if `x` is falsy, or `y` otherwise. This also applies to expressions +/// that return `bool`: the comparison in `result = x > 0` has value context. +/// +/// In `Condition` context, the enclosing code only needs to know which branch to take. For example, +/// CPython evaluates `if x and y` by testing `x` and, only if `x` is truthy, testing `y`. If `x` tests +/// falsy, that one truthiness check is enough to skip the body: `x` is not tested again as the +/// result of `x and y`. +/// +/// This distinction matters when an operand's `__bool__` can change between calls: +/// +/// ```python +/// if x and False: # A falsy x skips the body; a truthy x reaches False. +/// ... # Unreachable in either case. +/// saved = x and False # Can produce x after checking that it is falsy. +/// if saved: # Can call x.__bool__ again, which may now return True. +/// ... # Reachable. +/// ``` +/// +/// The context propagates through `and`, `or`, `not`, and the branches of conditional expressions. +/// Condition context does not propagate through calls or assignment expressions: in +/// `if f(x and False)`, the call's result controls the branch, but its argument is evaluated in +/// value context. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum ExpressionContext { + /// Produce the expression's result object for the enclosing code to use. + Value, + /// Choose the truthy or falsy control-flow path without preserving the result object. + Condition, +} + pub(super) struct SemanticIndexBuilder<'db, 'ast> { // Builder state db: &'db dyn Db, @@ -2153,12 +2186,16 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { &mut self, predicate_node: &'ast ast::Expr, ) -> (PredicateOrLiteral<'db>, ScopedPredicateId) { - let predicate = self.build_predicate(predicate_node); + let predicate = self.build_predicate(predicate_node, ExpressionContext::Condition); let predicate_id = self.record_narrowing_constraint(predicate); (predicate, predicate_id) } - fn build_predicate(&mut self, predicate_node: &'ast ast::Expr) -> PredicateOrLiteral<'db> { + fn build_predicate( + &mut self, + predicate_node: &'ast ast::Expr, + context: ExpressionContext, + ) -> PredicateOrLiteral<'db> { // Some commonly used test expressions are eagerly evaluated as `true` // or `false` here for performance reasons. This list does not need to // be exhaustive. More complex expressions will still evaluate to the @@ -2190,7 +2227,23 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { match resolve_to_literal(predicate_node) { Some(literal) => PredicateOrLiteral::Literal(literal), None => PredicateOrLiteral::Predicate(Predicate { - node: PredicateNode::Expression(expression), + node: match (context, predicate_node) { + ( + ExpressionContext::Condition, + ast::Expr::BoolOp(_) + | ast::Expr::If(_) + | ast::Expr::UnaryOp(ast::ExprUnaryOp { + op: ast::UnaryOp::Not, + .. + }), + ) => PredicateNode::Condition(expression), + (ExpressionContext::Condition, ast::Expr::Compare(compare)) + if compare.ops.len() > 1 => + { + PredicateNode::ChainedComparisonCondition(expression) + } + _ => PredicateNode::Expression(expression), + }, is_positive: true, }), } @@ -2249,7 +2302,9 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { let place_table = self.current_place_table(); match pred.node { - PredicateNode::Expression(expression) => { + PredicateNode::Expression(expression) + | PredicateNode::Condition(expression) + | PredicateNode::ChainedComparisonCondition(expression) => { let expression_node = expression.node_ref(self.db).node(self.module); let mut places = PossiblyNarrowedPlacesBuilder::new(self.db, place_table) .expression(expression_node); @@ -3008,7 +3063,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { /// print(last) /// ``` fn visit_comprehension_filter(&mut self, if_expr: &'ast ast::Expr) -> FlowSnapshot { - self.visit_expr(if_expr); + self.visit_expr_with_context(if_expr, ExpressionContext::Condition); let condition_flow_snapshot = self.flow_snapshot_for_condition(if_expr); let filtered_out = if let Some(snapshots) = condition_flow_snapshot.into_branches() { self.flow_restore(snapshots.truthy); @@ -3273,13 +3328,265 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { .get_or_init(|| source_text(self.db, self.file.file(self.db))) } + fn visit_expr_with_context(&mut self, expr: &'ast ast::Expr, context: ExpressionContext) { + self.with_semantic_checker(|semantic, builder| semantic.visit_expr(expr, builder)); + + self.scopes_by_expression + .record_expression(expr, self.current_scope()); + + match expr { + ast::Expr::Name(ast::ExprName { ctx, .. }) + | ast::Expr::Attribute(ast::ExprAttribute { ctx, .. }) + | ast::Expr::Subscript(ast::ExprSubscript { ctx, .. }) => { + // Record place effects after walking the expression. For names, this is + // equivalent because `walk_expr` is a no-op; for attribute/subscript places, + // child evaluation can introduce bindings (for example via walrus operators), + // and those bindings need to exist before we register parent/member associations. + let mut deferred_effects = None; + if let Some(mut place_expr) = PlaceExpr::try_from_expr(expr) { + if let Some(method_scope_id) = self.is_method_or_eagerly_executed_in_method() + && let PlaceExpr::Member(member) = &mut place_expr + && member.is_instance_attribute_candidate() + && let Some(attribute) = expr.as_attribute_expr() + { + // We specifically mark direct attribute assignments to the first + // parameter of a method, i.e. typically `self` or `cls`. + // However, we must check that the symbol hasn't been shadowed by an + // intermediate scope (e.g., a comprehension variable: `for self in [...]`) + // and that the AST base is still the original name rather than a + // rebinding expression such as `(self := other).x`. + let accessed_object_refers_to_first_parameter = + self.current_first_parameter_name.is_some_and(|first| { + attribute + .value + .as_name_expr() + .is_some_and(|name| name.id == first) + && !self.is_symbol_bound_in_intermediate_eager_scopes( + first, + method_scope_id, + ) + }); + + if accessed_object_refers_to_first_parameter { + member.mark_instance_attribute(); + } + } + + let (is_use, is_definition) = match (ctx, self.current_assignment()) { + (ast::ExprContext::Store, Some(CurrentAssignment::AugAssign(_))) => { + // Record the target load now; the definition is recorded separately + // after visiting the right-hand side. + (true, false) + } + (ast::ExprContext::Load, _) => (true, false), + (ast::ExprContext::Store, _) => (false, true), + (ast::ExprContext::Del, _) => (true, true), + (ast::ExprContext::Invalid, _) => (false, false), + }; + deferred_effects = Some((place_expr, is_use, is_definition)); + } + + walk_expr(self, expr); + + let is_use = deferred_effects + .as_ref() + .is_some_and(|(_, is_use, _)| *is_use); + let can_raise = self.place_access_can_raise(expr, is_use); + self.record_exception_checkpoint_if(can_raise); + + if let Some((place_expr, is_use, is_definition)) = deferred_effects { + let place_id = self.add_place(place_expr); + + if is_use { + self.record_place_use(place_id, expr); + + // Keep track of any uses of unannotated collection initializers. + if let Some(collection_def) = + self.unannotated_collection_initializer_binding(expr) + && let Some(current_statement) = self.current_statements.last_mut() + { + current_statement + .collection_uses + .push((collection_def, expr.into())); + } + } + + if is_definition { + self.record_place_definition(place_id, expr); + } + + if let Some(unpack_position) = self + .current_assignment_mut() + .and_then(CurrentAssignment::unpack_position_mut) + { + *unpack_position = UnpackPosition::Other; + } + } + } + ast::Expr::Named(node) => { + self.visit_expr(&node.value); + + // See https://peps.python.org/pep-0572/#differences-between-assignment-expressions-and-assignment-statements + if node.target.is_name_expr() { + self.push_assignment(CurrentAssignment::Named(node)); + self.visit_expr(&node.target); + self.pop_assignment(); + } else { + self.visit_expr(&node.target); + } + } + ast::Expr::Lambda(lambda) => { + self.current_statement_mut() + .expect("every lambda expression is part of a statement") + .lambda_expressions + .push(lambda); + + if let Some(parameters) = &lambda.parameters { + // The default value of the parameters needs to be evaluated in the + // enclosing scope. + for default in parameters + .iter_non_variadic_params() + .filter_map(|param| param.default.as_deref()) + { + self.visit_expr(default); + } + self.visit_parameters(parameters); + } + self.push_scope(NodeWithScopeRef::Lambda(lambda)); + + // Add symbols and definitions for the parameters to the lambda scope. + if let Some(parameters) = lambda.parameters.as_ref() { + self.declare_lambda_parameters(parameters, lambda); + } + + self.visit_expr(lambda.body.as_ref()); + self.pop_scope(); + } + ast::Expr::If(node) => self.visit_if_expression(node, context), + ast::Expr::ListComp( + list_comprehension @ ast::ExprListComp { + elt, generators, .. + }, + ) => { + let scope = self.with_generators_scope( + NodeWithScopeRef::ListComprehension(list_comprehension), + generators, + |builder| builder.visit_expr(elt), + ); + if self.async_comprehensions.contains(&scope) { + self.mark_current_comprehension_async(); + } + } + ast::Expr::SetComp( + set_comprehension @ ast::ExprSetComp { + elt, generators, .. + }, + ) => { + let scope = self.with_generators_scope( + NodeWithScopeRef::SetComprehension(set_comprehension), + generators, + |builder| builder.visit_expr(elt), + ); + if self.async_comprehensions.contains(&scope) { + self.mark_current_comprehension_async(); + } + } + ast::Expr::Generator( + generator @ ast::ExprGenerator { + elt, generators, .. + }, + ) => { + self.with_generators_scope( + NodeWithScopeRef::GeneratorExpression(generator), + generators, + |builder| builder.visit_expr(elt), + ); + } + ast::Expr::DictComp( + dict_comprehension @ ast::ExprDictComp { + key, + value, + generators, + .. + }, + ) => { + let scope = self.with_generators_scope( + NodeWithScopeRef::DictComprehension(dict_comprehension), + generators, + |builder| { + if let Some(key) = key { + builder.visit_expr(key); + } + builder.visit_expr(value); + }, + ); + if self.async_comprehensions.contains(&scope) { + self.mark_current_comprehension_async(); + } + } + ast::Expr::Call(_) | ast::Expr::BinOp(_) => { + walk_expr(self, expr); + self.record_exception_checkpoint(); + } + ast::Expr::UnaryOp(unary) => { + self.visit_expr_with_context( + &unary.operand, + if unary.op == ast::UnaryOp::Not { + context + } else { + ExpressionContext::Value + }, + ); + self.record_exception_checkpoint_if( + unary.op != ast::UnaryOp::Not + || !Self::condition_evaluation_is_known_safe(&unary.operand), + ); + } + ast::Expr::Compare(ast::ExprCompare { + left, + ops, + comparators, + .. + }) => { + self.visit_expr(left); + for (op, comparator) in ops.iter().zip(comparators) { + self.visit_expr(comparator); + self.record_exception_checkpoint_if(!matches!( + op, + ast::CmpOp::Is | ast::CmpOp::IsNot + )); + } + } + ast::Expr::BoolOp(node) => self.visit_bool_expression(node, context), + ast::Expr::StringLiteral(_) => { + walk_expr(self, expr); + } + ast::Expr::Yield(_) | ast::Expr::YieldFrom(_) => { + let scope = self.current_scope(); + if self.scopes[scope].kind() == ScopeKind::Function { + self.generator_functions.insert(scope); + } + walk_expr(self, expr); + self.record_exception_checkpoint(); + } + ast::Expr::Await(_) => { + self.mark_current_comprehension_async(); + walk_expr(self, expr); + self.record_exception_checkpoint(); + } + _ => { + walk_expr(self, expr); + } + } + } + /// Visits a conditional expression without reserving its flow snapshots in every recursive /// expression-visitor frame. This matters for deeply nested expressions in unoptimized builds. - fn visit_if_expression(&mut self, node: &'ast ast::ExprIf) { + fn visit_if_expression(&mut self, node: &'ast ast::ExprIf, context: ExpressionContext) { let ast::ExprIf { body, test, orelse, .. } = node; - self.visit_expr(test); + self.visit_expr_with_context(test, ExpressionContext::Condition); let condition_flow_snapshot = self.flow_snapshot_for_condition(test); let falsy = if let Some(snapshots) = condition_flow_snapshot.into_branches() { self.flow_restore(snapshots.truthy); @@ -3292,7 +3599,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { let in_type_checking_block = self.in_type_checking_block; self.current_use_def_map_mut() .record_range_reachability(body.range(), in_type_checking_block); - self.visit_expr(body); + self.visit_expr_with_context(body, context); let post_body = self.flow_snapshot(); self.flow_restore(falsy); @@ -3301,12 +3608,12 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { let in_type_checking_block = self.in_type_checking_block; self.current_use_def_map_mut() .record_range_reachability(orelse.range(), in_type_checking_block); - self.visit_expr(orelse); + self.visit_expr_with_context(orelse, context); self.flow_merge(post_body); } /// Keeps short-circuit flow snapshots out of the common recursive expression-visitor frame. - fn visit_bool_expression(&mut self, node: &'ast ast::ExprBoolOp) { + fn visit_bool_expression(&mut self, node: &'ast ast::ExprBoolOp, context: ExpressionContext) { let ast::ExprBoolOp { values, op, .. } = node; let mut snapshots = vec![]; let mut reachability_constraints = vec![]; @@ -3321,7 +3628,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { let in_type_checking_block = self.in_type_checking_block; self.current_use_def_map_mut() .record_range_reachability(value.range(), in_type_checking_block); - self.visit_expr(value); + self.visit_expr_with_context(value, context); // Only non-final values can short-circuit this boolean operation. The final // value can still have its own outcome-specific flow if it is nested. @@ -3330,7 +3637,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { value, )); let condition_flow_snapshots = self.take_condition_flow_snapshots(value); - let predicate = self.build_predicate(value); + let predicate = self.build_predicate(value, context); let possibly_narrowed = self.compute_possibly_narrowed_places(&predicate); let predicate_id = match op { ast::BoolOp::And => self.add_predicate(predicate), @@ -3870,9 +4177,9 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { // `msg` branch back into the following flow, since there is no way of getting out // of that branch. Code after the assertion starts from the condition's truthy flow. - self.visit_expr(test); + self.visit_expr_with_context(test, ExpressionContext::Condition); let condition_flow_snapshot = self.flow_snapshot_for_condition(test); - let predicate = self.build_predicate(test); + let predicate = self.build_predicate(test, ExpressionContext::Condition); if msg.is_some() || self @@ -4043,7 +4350,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { } } ast::Stmt::If(node) => { - self.visit_expr(&node.test); + self.visit_expr_with_context(&node.test, ExpressionContext::Condition); let condition_flow_snapshot = self.flow_snapshot_for_condition(&node.test); let mut falsy = if let Some(snapshots) = condition_flow_snapshot.into_branches() { self.flow_restore(snapshots.truthy); @@ -4097,7 +4404,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { self.record_negated_reachability_constraint(last_reachability_constraint); let next_falsy = if let Some(elif_test) = clause_test { - self.visit_expr(elif_test); + self.visit_expr_with_context(elif_test, ExpressionContext::Condition); // A test expression is evaluated whether the branch is taken or not let condition_flow_snapshot = self.flow_snapshot_for_condition(elif_test); let next_falsy = @@ -4180,7 +4487,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { // Visit the test expression after creating loop headers, so that loop-back values // are visible. - self.visit_expr(test); + self.visit_expr_with_context(test, ExpressionContext::Condition); let condition_flow_snapshot = self.flow_snapshot_for_condition(test); // Take the pre_loop snapshot from the post-test fallback flow before restoring the @@ -4558,7 +4865,7 @@ impl<'db, 'ast> SemanticIndexBuilder<'db, 'ast> { // while the next case is reached through `!P || (P && !G)`. Save `P && !G` // separately so it can be merged with the pattern-failure state after the body. let match_success_guard_failure = case.guard.as_ref().map(|guard| { - self.visit_expr(guard); + self.visit_expr_with_context(guard, ExpressionContext::Condition); let condition_flow_snapshot = self.flow_snapshot_for_condition(guard); let falsy = if let Some(snapshots) = condition_flow_snapshot.into_branches() { @@ -5270,248 +5577,9 @@ impl<'ast> Visitor<'ast> for SemanticIndexBuilder<'_, 'ast> { } fn visit_expr(&mut self, expr: &'ast ast::Expr) { - self.with_semantic_checker(|semantic, context| semantic.visit_expr(expr, context)); - - self.scopes_by_expression - .record_expression(expr, self.current_scope()); - - match expr { - ast::Expr::Name(ast::ExprName { ctx, .. }) - | ast::Expr::Attribute(ast::ExprAttribute { ctx, .. }) - | ast::Expr::Subscript(ast::ExprSubscript { ctx, .. }) => { - // Record place effects after walking the expression. For names, this is - // equivalent because `walk_expr` is a no-op; for attribute/subscript places, - // child evaluation can introduce bindings (for example via walrus operators), - // and those bindings need to exist before we register parent/member associations. - let mut deferred_effects = None; - if let Some(mut place_expr) = PlaceExpr::try_from_expr(expr) { - if let Some(method_scope_id) = self.is_method_or_eagerly_executed_in_method() - && let PlaceExpr::Member(member) = &mut place_expr - && member.is_instance_attribute_candidate() - && let Some(attribute) = expr.as_attribute_expr() - { - // We specifically mark direct attribute assignments to the first - // parameter of a method, i.e. typically `self` or `cls`. - // However, we must check that the symbol hasn't been shadowed by an - // intermediate scope (e.g., a comprehension variable: `for self in [...]`) - // and that the AST base is still the original name rather than a - // rebinding expression such as `(self := other).x`. - let accessed_object_refers_to_first_parameter = - self.current_first_parameter_name.is_some_and(|first| { - attribute - .value - .as_name_expr() - .is_some_and(|name| name.id == first) - && !self.is_symbol_bound_in_intermediate_eager_scopes( - first, - method_scope_id, - ) - }); - - if accessed_object_refers_to_first_parameter { - member.mark_instance_attribute(); - } - } - - let (is_use, is_definition) = match (ctx, self.current_assignment()) { - (ast::ExprContext::Store, Some(CurrentAssignment::AugAssign(_))) => { - // Record the target load now; the definition is recorded separately - // after visiting the right-hand side. - (true, false) - } - (ast::ExprContext::Load, _) => (true, false), - (ast::ExprContext::Store, _) => (false, true), - (ast::ExprContext::Del, _) => (true, true), - (ast::ExprContext::Invalid, _) => (false, false), - }; - deferred_effects = Some((place_expr, is_use, is_definition)); - } - - walk_expr(self, expr); - - let is_use = deferred_effects - .as_ref() - .is_some_and(|(_, is_use, _)| *is_use); - let can_raise = self.place_access_can_raise(expr, is_use); - self.record_exception_checkpoint_if(can_raise); - - if let Some((place_expr, is_use, is_definition)) = deferred_effects { - let place_id = self.add_place(place_expr); - - if is_use { - self.record_place_use(place_id, expr); - - // Keep track of any uses of unannotated collection initializers. - if let Some(collection_def) = - self.unannotated_collection_initializer_binding(expr) - && let Some(current_statement) = self.current_statements.last_mut() - { - current_statement - .collection_uses - .push((collection_def, expr.into())); - } - } - - if is_definition { - self.record_place_definition(place_id, expr); - } - - if let Some(unpack_position) = self - .current_assignment_mut() - .and_then(CurrentAssignment::unpack_position_mut) - { - *unpack_position = UnpackPosition::Other; - } - } - } - ast::Expr::Named(node) => { - self.visit_expr(&node.value); - - // See https://peps.python.org/pep-0572/#differences-between-assignment-expressions-and-assignment-statements - if node.target.is_name_expr() { - self.push_assignment(CurrentAssignment::Named(node)); - self.visit_expr(&node.target); - self.pop_assignment(); - } else { - self.visit_expr(&node.target); - } - } - ast::Expr::Lambda(lambda) => { - self.current_statement_mut() - .expect("every lambda expression is part of a statement") - .lambda_expressions - .push(lambda); - - if let Some(parameters) = &lambda.parameters { - // The default value of the parameters needs to be evaluated in the - // enclosing scope. - for default in parameters - .iter_non_variadic_params() - .filter_map(|param| param.default.as_deref()) - { - self.visit_expr(default); - } - self.visit_parameters(parameters); - } - self.push_scope(NodeWithScopeRef::Lambda(lambda)); - - // Add symbols and definitions for the parameters to the lambda scope. - if let Some(parameters) = lambda.parameters.as_ref() { - self.declare_lambda_parameters(parameters, lambda); - } - - self.visit_expr(lambda.body.as_ref()); - self.pop_scope(); - } - ast::Expr::If(node) => self.visit_if_expression(node), - ast::Expr::ListComp( - list_comprehension @ ast::ExprListComp { - elt, generators, .. - }, - ) => { - let scope = self.with_generators_scope( - NodeWithScopeRef::ListComprehension(list_comprehension), - generators, - |builder| builder.visit_expr(elt), - ); - if self.async_comprehensions.contains(&scope) { - self.mark_current_comprehension_async(); - } - } - ast::Expr::SetComp( - set_comprehension @ ast::ExprSetComp { - elt, generators, .. - }, - ) => { - let scope = self.with_generators_scope( - NodeWithScopeRef::SetComprehension(set_comprehension), - generators, - |builder| builder.visit_expr(elt), - ); - if self.async_comprehensions.contains(&scope) { - self.mark_current_comprehension_async(); - } - } - ast::Expr::Generator( - generator @ ast::ExprGenerator { - elt, generators, .. - }, - ) => { - self.with_generators_scope( - NodeWithScopeRef::GeneratorExpression(generator), - generators, - |builder| builder.visit_expr(elt), - ); - } - ast::Expr::DictComp( - dict_comprehension @ ast::ExprDictComp { - key, - value, - generators, - .. - }, - ) => { - let scope = self.with_generators_scope( - NodeWithScopeRef::DictComprehension(dict_comprehension), - generators, - |builder| { - if let Some(key) = key { - builder.visit_expr(key); - } - builder.visit_expr(value); - }, - ); - if self.async_comprehensions.contains(&scope) { - self.mark_current_comprehension_async(); - } - } - ast::Expr::Call(_) | ast::Expr::BinOp(_) => { - walk_expr(self, expr); - self.record_exception_checkpoint(); - } - ast::Expr::UnaryOp(unary) => { - walk_expr(self, expr); - self.record_exception_checkpoint_if( - unary.op != ast::UnaryOp::Not - || !Self::condition_evaluation_is_known_safe(&unary.operand), - ); - } - ast::Expr::Compare(ast::ExprCompare { - left, - ops, - comparators, - .. - }) => { - self.visit_expr(left); - for (op, comparator) in ops.iter().zip(comparators) { - self.visit_expr(comparator); - self.record_exception_checkpoint_if(!matches!( - op, - ast::CmpOp::Is | ast::CmpOp::IsNot - )); - } - } - ast::Expr::BoolOp(node) => self.visit_bool_expression(node), - ast::Expr::StringLiteral(_) => { - walk_expr(self, expr); - } - ast::Expr::Yield(_) | ast::Expr::YieldFrom(_) => { - let scope = self.current_scope(); - if self.scopes[scope].kind() == ScopeKind::Function { - self.generator_functions.insert(scope); - } - walk_expr(self, expr); - self.record_exception_checkpoint(); - } - ast::Expr::Await(_) => { - self.mark_current_comprehension_async(); - walk_expr(self, expr); - self.record_exception_checkpoint(); - } - _ => { - walk_expr(self, expr); - } - } + // Generic AST walking evaluates child expressions as values. Short-circuit syntax + // propagates condition context explicitly through `visit_expr_with_context`. + self.visit_expr_with_context(expr, ExpressionContext::Value); } fn visit_parameters(&mut self, parameters: &'ast ast::Parameters) { diff --git a/crates/ty_python_core/src/lib.rs b/crates/ty_python_core/src/lib.rs index a8a1b2c9c32df..d01e9adb6bc37 100644 --- a/crates/ty_python_core/src/lib.rs +++ b/crates/ty_python_core/src/lib.rs @@ -972,6 +972,27 @@ impl Truthiness { if condition { self.negate() } else { self } } + #[must_use] + pub fn and(self, other: Self) -> Self { + match self { + Truthiness::AlwaysTrue => other, + Truthiness::AlwaysFalse => self, + Truthiness::Ambiguous => match other { + Truthiness::AlwaysFalse => Truthiness::AlwaysFalse, + Truthiness::AlwaysTrue | Truthiness::Ambiguous => Truthiness::Ambiguous, + }, + } + } + + /// Like [`Truthiness::and`], but evaluates `other` only when `self` may be true. + #[must_use] + pub fn and_else(self, other: impl FnOnce() -> Self) -> Self { + match self { + Truthiness::AlwaysFalse => self, + Truthiness::AlwaysTrue | Truthiness::Ambiguous => self.and(other()), + } + } + #[must_use] pub fn or(self, other: Self) -> Self { match self { @@ -1078,6 +1099,7 @@ mod tests { use ruff_python_ast as ast; use ruff_text_size::{Ranged, TextRange}; + use super::Truthiness::{AlwaysFalse, AlwaysTrue, Ambiguous}; use super::*; use crate::{ @@ -1137,6 +1159,35 @@ mod tests { .collect() } + #[test] + fn truthiness_and() { + for (left, right, expected) in [ + (AlwaysTrue, AlwaysTrue, AlwaysTrue), + (AlwaysTrue, AlwaysFalse, AlwaysFalse), + (AlwaysTrue, Ambiguous, Ambiguous), + (AlwaysFalse, AlwaysTrue, AlwaysFalse), + (AlwaysFalse, AlwaysFalse, AlwaysFalse), + (AlwaysFalse, Ambiguous, AlwaysFalse), + (Ambiguous, AlwaysTrue, Ambiguous), + (Ambiguous, AlwaysFalse, AlwaysFalse), + (Ambiguous, Ambiguous, Ambiguous), + ] { + assert_eq!(left.and(right), expected, "{left:?}.and({right:?})"); + + let mut calls = 0; + let lazy_result = left.and_else(|| { + calls += 1; + right + }); + assert_eq!(lazy_result, expected, "{left:?}.and_else(|| {right:?})"); + assert_eq!( + calls, + usize::from(left != AlwaysFalse), + "{left:?}.and_else call count" + ); + } + } + #[test] fn empty() { let TestCase { db, file } = test_case(""); diff --git a/crates/ty_python_core/src/predicate.rs b/crates/ty_python_core/src/predicate.rs index 68ffc52942130..7b04b4b9b0235 100644 --- a/crates/ty_python_core/src/predicate.rs +++ b/crates/ty_python_core/src/predicate.rs @@ -114,7 +114,16 @@ pub struct CallableAndCallExpr<'db> { #[derive(Clone, Copy, Debug, Hash, PartialEq, Eq, get_size2::GetSize, salsa::SalsaValue)] pub enum PredicateNode<'db> { + /// The truthiness of an expression's resulting value. Expression(Expression<'db>), + /// A boolean operation, `not`, or conditional expression evaluated directly as a condition. + /// + /// In `if x and False`, the truthy branch is unreachable. But after `y = x and False`, + /// `if y` may be truthy: it can call `x.__bool__` a second time and get a different result. + Condition(Expression<'db>), + /// A chained comparison evaluated directly as a condition. Its inferred truthiness is + /// available without walking the expression again. + ChainedComparisonCondition(Expression<'db>), /// Whether a context manager's exit return type allows an exception to be suppressed. /// /// Resolved during type inference because the context manager's type is unavailable during diff --git a/crates/ty_python_semantic/resources/mdtest/annotations/annotated.md b/crates/ty_python_semantic/resources/mdtest/annotations/annotated.md index 846c92e4ca1c8..6290a26b0fd9b 100644 --- a/crates/ty_python_semantic/resources/mdtest/annotations/annotated.md +++ b/crates/ty_python_semantic/resources/mdtest/annotations/annotated.md @@ -37,6 +37,15 @@ def convert(value: "Annotated[str, dict(**{'name': 'value'})]") -> "Annotated[in return 1 ``` +Conditional expressions are also valid metadata and do not affect the annotated type. + +```py +def flag() -> bool: + return True + +conditional_value: "Annotated[int, 1 if flag() else 2]" = 1 +``` + ## Inside `type[...]` `Annotated` can wrap a class or specialized generic class inside `type[...]` without changing the diff --git a/crates/ty_python_semantic/resources/mdtest/boolean/short_circuit.md b/crates/ty_python_semantic/resources/mdtest/boolean/short_circuit.md index 4c314e3cba76d..6286b718a4c48 100644 --- a/crates/ty_python_semantic/resources/mdtest/boolean/short_circuit.md +++ b/crates/ty_python_semantic/resources/mdtest/boolean/short_circuit.md @@ -192,3 +192,321 @@ def match_guard(flag: bool, subject: object): def comprehension_filter(flag: bool): [reveal_type(x) for _ in range(1) if flag and (x := 1)] # revealed: Literal[1] ``` + +## Reachability of compound conditions + +An `and` condition with an always-falsy operand cannot ever take the truthy branch. Similarly, an +`or` condition with an always-truthy operand cannot ever take the falsy branch. + +This perhaps seems obvious, but it's not! Given the expression `value and False`, `value` could be +some object whose `__bool__` can return `False` on one call and `True` on the next. The evaluation +of `value and False` tests `value`, gets `False`, and short-circuits, meaning the entire expression +`value and False` evaluates to `value`. Now if we re-check truthiness of `value`, we can't +necessarily assume we get `False` again. + +For code which saves the `and` expression to a variable, this is correct, and we do model this +possibility: + +```py +def saved_condition(value: object): + saved = value and False + + # We know that `saved` is not always truthy; we don't know that it's always falsy. + reveal_type(saved) # revealed: ~AlwaysTruthy + + if saved: + # So this branch is reachable: + reveal_type(value) # revealed: object +``` + +But if the condition is tested directly, it works differently (at least in CPython). A short-circuit +within a branch condition doesn't just short-circuit to an evaluation of the expression; it +short-circuits directly to a control-flow decision, bypassing a final evaluation of the entire +condition expression, and avoiding the need for a second `__bool__` check. We model this +distinction: + +```py +def conditions(value: object): + if value and False: + # This branch is not reachable; `value.__bool__` is only tested once. If it's false, this + # branch is skipped immediately, if it's true, `False` is always false and this branch is + # still skipped. + reveal_type(value) # revealed: Never + + if value or True: + pass + else: + reveal_type(value) # revealed: Never + + if not (value and False): + pass + else: + reveal_type(value) # revealed: Never + + if (value and False) or not (value or True): + reveal_type(value) # revealed: Never +``` + +Short-circuiting also skips later operands within a condition, including after nested boolean +operations. + +```py +def nested_operands(value: object): + if (value and False) and reveal_type(value): # revealed: Never + pass + + if (value or True) or reveal_type(value): # revealed: Never + pass + + if not (value or True) and reveal_type(value): # revealed: Never + pass +``` + +The same short-circuit rules apply to loop conditions, assertions, conditional expressions, +comprehension filters, and match guards. + +```py +def other_conditions(value: object): + while value and False: + reveal_type(value) # revealed: Never + + assert value or True, reveal_type(value) # revealed: Never + + reveal_type(value) if value and False else None # revealed: Never + + [reveal_type(item) for item in range(1) if value and False] # revealed: Never + + match value: + case _ if value and False: + reveal_type(value) # revealed: Never + + assert value and False + reveal_type(value) # revealed: Never +``` + +## Conditions with impossible operands + +Narrowing can make a later operand impossible to evaluate. A `bool` cannot also be a `str`, so +`value` has type `Never` when it is tested again in each condition below. Only the earlier +short-circuit path can complete: falsy for `and`, truthy for `or`. We reveal an unrelated `marker` +to check that the whole branch is unreachable, independently of narrowing `value` itself. + +```py +def impossible_operands(value: bool, marker: int): + if isinstance(value, str) and value: + reveal_type(marker) # revealed: Never + + if not isinstance(value, str) or value: + pass + else: + reveal_type(marker) # revealed: Never + + if isinstance(value, str) and not value: + reveal_type(marker) # revealed: Never +``` + +These outcomes are preserved inside larger conditions, even when another operand has mutable +truthiness. + +```py +def nested_impossible_operands(other: object, value: bool, marker: int): + if other and (isinstance(value, str) and value): + reveal_type(marker) # revealed: Never + + if other or (not isinstance(value, str) or value): + pass + else: + reveal_type(marker) # revealed: Never +``` + +## Conditions with aliased `Never` operands + +A call cannot produce a result when its return type is an alias of `Never`. Only the preceding +short-circuit path can complete. + +```toml +[environment] +python-version = "3.12" +``` + +```py +from typing import Never + +type Bottom = Never + +def stop() -> Bottom: + raise RuntimeError + +def aliased_operand(flag: bool, marker: int): + if flag and stop(): + reveal_type(marker) # revealed: Never + + if flag or stop(): + pass + else: + reveal_type(marker) # revealed: Never +``` + +A union of aliases of `Never` still cannot produce a result. + +```py +type OtherBottom = Never +type BottomUnion = Bottom | OtherBottom + +def stop_union() -> BottomUnion: + raise RuntimeError + +def union_operand(flag: bool, marker: int): + if flag and stop_union(): + reveal_type(marker) # revealed: Never +``` + +## Conditional expressions used as conditions + +When a conditional expression (an `if/else` expression) is itself a condition, its selected branch +is evaluated as a condition too. The unselected branch does not affect whether the condition is +truthy. + +```py +def conditional_expressions(value: object, flag: bool): + if (value and False) if flag else False: + reveal_type(value) # revealed: Never + + if True if flag else (value or True): + pass + else: + reveal_type(value) # revealed: Never + + if True if value and False else False: + reveal_type(value) # revealed: Never +``` + +A branch narrowed to `Never` cannot contribute a result. The other branch alone determines the +conditional expression's truthiness. + +```py +def impossible_branches(value: bool, marker: int): + if value if isinstance(value, str) else False: + reveal_type(marker) # revealed: Never + + if True if not isinstance(value, str) else value: + pass + else: + reveal_type(marker) # revealed: Never +``` + +## Chained comparison conditions + +A comparison chain used as a condition is falsy if any comparison is always falsy, even if an +earlier comparison returns an arbitrary object. + +```py +class Comparable: + def __lt__(self, other: int) -> object: + return object() + +def comparisons(value: Comparable): + if value < 1 < 0: + reveal_type(value) # revealed: Never + + if value < 1 < 0 < 1: + reveal_type(value) # revealed: Never + + if (value < 1 < 0) and reveal_type(value): # revealed: Never + pass + + if not (value < 1 < 0): + pass + else: + reveal_type(value) # revealed: Never +``` + +Saving the result of a comparison chain can cause a non-boolean comparison result to be tested +twice. Its truthiness can change between those tests, so the truthy branch remains reachable. +References to `value` in these branches retain its type; in unreachable code they would have type +`Never`. + +```py +def saved_comparison(value: Comparable): + result = value < 1 < 0 + if result: + reveal_type(value) # revealed: Comparable + + if result := value < 1 < 0: + reveal_type(value) # revealed: Comparable +``` + +An unreachable assignment does not affect the inferred type of a loop variable. Inferring `value` +and deciding whether its assignment is reachable depend on each other, but `1 < 0` still makes the +branch unreachable. + +```py +def loop_condition(flag: bool): + value = 0 + while flag: + if value < 1 < 0: + value = Comparable() + reveal_type(value) # revealed: Literal[0] +``` + +## Re-testing boolean expression results + +Saving the result of `value and False` and then testing it can call `value.__bool__` twice. The +second call may return a different result, so the truthy branch remains reachable. Assignment +expressions and nested boolean operations in value contexts can also cause this extra test. + +```py +class MutableTruthiness: + truthy: bool = False + + def __bool__(self) -> bool: + self.truthy = not self.truthy + return self.truthy + +def expressions(value: MutableTruthiness, flag: bool): + saved = value and False + if saved: + reveal_type(value) # revealed: MutableTruthiness + + if saved := value and False: + reveal_type(value) # revealed: MutableTruthiness & ~AlwaysFalsy + + saved = (value and False) if flag else False + if saved: + reveal_type(value) # revealed: MutableTruthiness + + result = (value and False) and reveal_type(value) # revealed: MutableTruthiness & ~AlwaysFalsy + result = (not (value or True)) or reveal_type(value) # revealed: MutableTruthiness +``` + +Call arguments are evaluated as values, even when the call is itself used as a condition. Nested +boolean operations in an argument can therefore re-test an intermediate result. + +```py +def call_argument(value: MutableTruthiness): + if bool((value and False) and reveal_type(value)): # revealed: MutableTruthiness & ~AlwaysFalsy + pass +``` + +An assignment expression evaluates its right-hand side as a value before testing the assigned +object. Nested boolean operations on that right-hand side can therefore re-test an intermediate +result, even when the assignment expression is a condition. + +```py +def assignment_expression(value: MutableTruthiness, marker: int): + if saved := (value and False) and reveal_type(marker): # revealed: int + pass +``` + +A comprehension's filters are conditions, but its element is evaluated as a value, even when the +comprehension itself controls a branch. + +```py +def comprehension_element(value: MutableTruthiness, marker: int): + if [ + (value and False) and reveal_type(marker) # revealed: int + for _ in range(1) + if value or True + ]: + pass +``` diff --git a/crates/ty_python_semantic/resources/mdtest/conditional/if_expression.md b/crates/ty_python_semantic/resources/mdtest/conditional/if_expression.md index 082e0d43dbbfd..fba82cbd9e488 100644 --- a/crates/ty_python_semantic/resources/mdtest/conditional/if_expression.md +++ b/crates/ty_python_semantic/resources/mdtest/conditional/if_expression.md @@ -36,6 +36,74 @@ def _(flag: bool): reveal_type(x) # revealed: Literal[1] | None ``` +## Statically known compound conditions + +Short-circuit conditions can select a single branch even when an operand has mutable truthiness. +Saving the condition's value and testing it again does not provide the same guarantee. + +```py +def _(value: object): + reveal_type(1 if value and False else 2) # revealed: Literal[2] + reveal_type(1 if value or True else 2) # revealed: Literal[1] + + saved = value and False + reveal_type(1 if saved else 2) # revealed: Literal[1, 2] +``` + +A comparison chain can select a single branch even when an individual comparison returns an +arbitrary object. Saving the chain's result allows that object's truthiness to be tested again. + +```py +class Comparable: + def __lt__(self, other: int) -> object: + return object() + +def _(value: Comparable): + reveal_type(1 if value < 1 < 0 else 2) # revealed: Literal[2] + + saved = value < 1 < 0 + reveal_type(1 if saved else 2) # revealed: Literal[1, 2] +``` + +An operand narrowed to `Never` cannot produce a result. Nested conditions preserve the remaining +short-circuit outcome when selecting a branch. + +```py +def _(other: object, value: bool): + reveal_type(1 if other and (isinstance(value, str) and value) else 2) # revealed: Literal[2] + reveal_type(1 if other or (not isinstance(value, str) or value) else 2) # revealed: Literal[1] +``` + +## Conditions with operands equivalent to `Never` + +A call whose return type is an alias of `Never` cannot produce a result. The preceding short-circuit +outcome alone selects the conditional expression's branch. + +```toml +[environment] +python-version = "3.12" +``` + +```py +from typing import Never + +type Bottom = Never + +def stop() -> Bottom: + raise RuntimeError + +def _(flag: bool): + reveal_type(1 if flag and stop() else 2) # revealed: Literal[2] + reveal_type(1 if flag or stop() else 2) # revealed: Literal[1] +``` + +A type variable bounded by `Never` also cannot produce a result. + +```py +def _[T: Never](flag: bool, value: T): + reveal_type(1 if flag and value else 2) # revealed: Literal[2] +``` + ## Condition with object that implements `__bool__` incorrectly ```py diff --git a/crates/ty_python_semantic/src/reachability.rs b/crates/ty_python_semantic/src/reachability.rs index fed33131357f0..84b94aee43d1f 100644 --- a/crates/ty_python_semantic/src/reachability.rs +++ b/crates/ty_python_semantic/src/reachability.rs @@ -204,12 +204,14 @@ use crate::{ CallableTypes, ComparisonSoundnessPolicy, EnumClassLiteral, KnownInstanceType, NarrowingConstraint, SpecialFormType, Type, TypeContext, UnionType, callable_pattern_type, definite_match_pattern_type, definite_match_pattern_type_for_subject, equality_truthiness, - expand_type, infer_narrowing_constraints, infer_same_file_expression_type, - mapping_pattern_type, pattern_binding_fallthrough_type, sequence_pattern_type_builder, - singleton_pattern_type, + expand_type, infer_expression_types, infer_narrowing_constraints, + infer_same_file_expression_type, mapping_pattern_type, pattern_binding_fallthrough_type, + sequence_pattern_type_builder, singleton_pattern_type, }, }; +use ruff_db::parsed::parsed_module; use ruff_index::{Idx, IndexSlice}; +use ruff_python_ast as ast; use ruff_python_ast::name::Name; use ruff_text_size::TextRange; use rustc_hash::{FxHashMap, FxHashSet}; @@ -544,6 +546,8 @@ const REACHABILITY_EVALUATION_CHUNK_SIZE: usize = 256; fn predicate_scope<'db>(db: &'db dyn Db, predicate: &Predicate<'db>) -> ScopeId<'db> { match predicate.node { PredicateNode::Expression(expression) + | PredicateNode::Condition(expression) + | PredicateNode::ChainedComparisonCondition(expression) | PredicateNode::ContextManagerSuppresses { expression, .. } => expression.scope(db), PredicateNode::IsNonTerminalCall(CallableAndCallExpr { callable, .. }) => { callable.scope(db) @@ -1589,6 +1593,78 @@ fn analyze_non_empty_iterable(db: &dyn Db, iterable: Expression) -> Truthiness { } } +/// Evaluate a condition without re-testing intermediate short-circuit results. +/// +/// `None` means evaluation cannot produce a result, as for an operand narrowed to `Never`. +/// This differs from ambiguous truthiness: in `flag and raises()`, where `raises()` returns +/// `Never`, only the falsy short-circuit path can complete. For `flag or raises()`, only the +/// truthy path can complete. Callers that cannot represent the absence of a result can +/// conservatively map `None` to [`Truthiness::Ambiguous`]. +pub(crate) fn analyze_condition_expression( + node: &ast::Expr, + leaf_truthiness: &impl Fn(&ast::Expr) -> Option, +) -> Option { + match node { + ast::Expr::BoolOp(ast::ExprBoolOp { op, values, .. }) => { + let short_circuit = Truthiness::from(op.is_or()); + let mut result = short_circuit.negate(); + for value in values { + let Some(truthiness) = analyze_condition_expression(value, leaf_truthiness) else { + return result.is_ambiguous().then_some(short_circuit); + }; + if truthiness == short_circuit { + return Some(short_circuit); + } + if truthiness.is_ambiguous() { + result = Truthiness::Ambiguous; + } + } + Some(result) + } + ast::Expr::UnaryOp(ast::ExprUnaryOp { + op: ast::UnaryOp::Not, + operand, + .. + }) => analyze_condition_expression(operand, leaf_truthiness).map(Truthiness::negate), + ast::Expr::If(ast::ExprIf { + test, body, orelse, .. + }) => match analyze_condition_expression(test, leaf_truthiness)? { + Truthiness::AlwaysTrue => analyze_condition_expression(body, leaf_truthiness), + Truthiness::AlwaysFalse => analyze_condition_expression(orelse, leaf_truthiness), + Truthiness::Ambiguous => { + let body_truthiness = analyze_condition_expression(body, leaf_truthiness); + let orelse_truthiness = analyze_condition_expression(orelse, leaf_truthiness); + match (body_truthiness, orelse_truthiness) { + (None, truthiness) | (truthiness, None) => truthiness, + (Some(body), Some(orelse)) => Some(if body == orelse { + body + } else { + Truthiness::Ambiguous + }), + } + } + }, + _ => leaf_truthiness(node), + } +} + +#[salsa::tracked( + returns(copy), + cycle_initial = |_, _, _| Truthiness::Ambiguous, + heap_size = get_size2::GetSize::get_heap_size +)] +fn analyze_condition<'db>(db: &'db dyn Db, expression: Expression<'db>) -> Truthiness { + let env = ProgramEnvironment::from_scope(expression.scope(db)); + let module = parsed_module(db, expression.python_file(db)).load(db); + let inference = infer_expression_types(db, expression, TypeContext::default()); + analyze_condition_expression(expression.node_ref(db).node(&module), &|node| { + inference + .comparison_truthiness(node) + .or_else(|| inference.expression_type(node).bool_if_inhabited(db, &env)) + }) + .unwrap_or(Truthiness::Ambiguous) +} + fn analyze_single(db: &dyn Db, env: &ProgramEnvironment<'_>, predicate: &Predicate) -> Truthiness { let _span = tracing::trace_span!("analyze_single", ?predicate).entered(); @@ -1598,6 +1674,17 @@ fn analyze_single(db: &dyn Db, env: &ProgramEnvironment<'_>, predicate: &Predica .bool(db, env) .negate_if(!predicate.is_positive) } + PredicateNode::Condition(test_expr) => { + analyze_condition(db, test_expr).negate_if(!predicate.is_positive) + } + PredicateNode::ChainedComparisonCondition(test_expr) => { + let inference = infer_expression_types(db, test_expr, TypeContext::default()); + let expression = test_expr.node_ref(db); + inference + .comparison_truthiness(expression) + .unwrap_or_else(|| inference.expression_type(expression).bool(db, env)) + .negate_if(!predicate.is_positive) + } PredicateNode::ContextManagerSuppresses { expression, is_async, diff --git a/crates/ty_python_semantic/src/types/bool.rs b/crates/ty_python_semantic/src/types/bool.rs index 69f1e313d4860..9835f965eae9f 100644 --- a/crates/ty_python_semantic/src/types/bool.rs +++ b/crates/ty_python_semantic/src/types/bool.rs @@ -27,6 +27,23 @@ impl<'db> Type<'db> { .unwrap_or_else(|err| err.fallback_truthiness()) } + /// Like [`Self::bool`], but returns `None` for a type equivalent to [`Type::Never`]. + /// + /// An uninhabited type cannot produce either boolean outcome, unlike + /// [`Truthiness::Ambiguous`]. Condition analysis uses this distinction to retain the + /// short-circuit outcome of expressions like `flag and stop()`, where `stop` returns `Never`. + /// The equivalence check also handles aliases and type variables bounded by `Never`. + /// + /// This classifies a value type, not a compound condition's evaluation. It preserves + /// [`Self::bool`]'s error fallback and conservative handling of `__bool__` returning `Never`. + pub(crate) fn bool_if_inhabited( + &self, + db: &'db dyn Db, + env: &ProgramEnvironment<'db>, + ) -> Option { + (!self.is_equivalent_to(db, env, Type::Never)).then(|| self.bool(db, env)) + } + /// Resolves the boolean value of a type. /// /// This is used to determine the value that would be returned diff --git a/crates/ty_python_semantic/src/types/infer.rs b/crates/ty_python_semantic/src/types/infer.rs index 5cc8c5b73d7e1..4f7c6422bd3ae 100644 --- a/crates/ty_python_semantic/src/types/infer.rs +++ b/crates/ty_python_semantic/src/types/infer.rs @@ -70,7 +70,7 @@ use ty_python_core::expression::Expression; use ty_python_core::scope::ScopeId; use ty_python_core::statement::StatementInner; use ty_python_core::unpack::Unpack; -use ty_python_core::{ExpressionNodeKey, SemanticIndex, Statement, semantic_index}; +use ty_python_core::{ExpressionNodeKey, SemanticIndex, Statement, Truthiness, semantic_index}; mod builder; mod comparisons; @@ -1706,6 +1706,33 @@ struct ExpressionInferenceExtra<'db> { /// Metadata for type expressions in this region. type_expression_flags: FrozenMap, + /// A comparison chain's truthiness when evaluated directly as a condition. + /// + /// Expression types describe the objects produced by evaluation, which is not always enough + /// to determine a condition's outcome. If `x < 1` returns an object with mutable truthiness, + /// `saved = x < 1 < 0` can store that object after it tests falsy; `if saved:` can then test it + /// again and get `True`. In contrast, `if x < 1 < 0:` cannot enter its body: either the first + /// comparison tests falsy or the final comparison `1 < 0` does. Its condition truthiness is + /// `AlwaysFalse`, but its value type must still include objects returned by the first comparison. + /// + /// The same distinction matters for `and`/`or`, but their operands have separate expression + /// nodes with inferred types. [`crate::reachability::analyze_condition_expression`] can + /// reconstruct their condition truthiness by recursively visiting those operands, without + /// relying on the compound expression's value type. + /// + /// A comparison chain instead has one `ExprCompare` node with the operands and operators. + /// In `x < 1 < 0`, neither `x < 1` nor `1 < 0` has its own expression node, so their result + /// types are not recorded in [`ExpressionInference::expressions`]. We retain their combined + /// condition truthiness here while those types are available during comparison inference. + /// A single comparison needs no override: there is no intermediate truthiness check. + /// + /// When an `and`/`or` condition has a comparison chain as an operand, the recursive condition + /// analysis uses this map for that operand. + /// + /// Inference normally stores only differences from the truthiness of the chain's value type. + /// Cycle recovery also retains earlier overrides to keep widening monotonic. + comparison_truthiness: FrozenMap, + /// The constraints on any collection initializers that are accessed in this region. collection_use_constraints: CollectionUseConstraints<'db>, @@ -1760,6 +1787,10 @@ impl<'db> ExpressionInference<'db> { } } + if cycle.iteration() > crate::TAINTED_CYCLES { + self.widen_comparison_truthiness(db, env, previous); + } + for (expr, ty) in &mut self.expressions { let previous_ty = previous.expression_type(*expr); *ty = ty.cycle_normalized(db, env, previous_ty, cycle); @@ -1779,6 +1810,42 @@ impl<'db> ExpressionInference<'db> { self } + /// Sparse overrides can appear or disappear as operand types change. Compare the effective + /// truthiness in both iterations, including previous-only overrides, so widening cannot make + /// a condition alternate between definite outcomes. + fn widen_comparison_truthiness( + &mut self, + db: &'db dyn Db, + env: &ProgramEnvironment<'db>, + previous: &Self, + ) { + let comparison_truthiness: FrozenMap<_, _> = self + .extra + .iter() + .chain(previous.extra.iter()) + .flat_map(|extra| &extra.comparison_truthiness) + .map(|(expression, _)| { + let truthiness = self + .comparison_truthiness(*expression) + .unwrap_or_else(|| self.expression_type(*expression).bool(db, env)); + let previous_truthiness = previous + .comparison_truthiness(*expression) + .unwrap_or_else(|| previous.expression_type(*expression).bool(db, env)); + ( + *expression, + if truthiness == previous_truthiness { + truthiness + } else { + Truthiness::Ambiguous + }, + ) + }) + .collect(); + if comparison_truthiness.iter().next().is_some() { + self.extra.get_or_insert_default().comparison_truthiness = comparison_truthiness; + } + } + fn try_expression_type(&self, expression: impl Into) -> Option> { self.expressions .get(&expression.into()) @@ -1791,6 +1858,17 @@ impl<'db> ExpressionInference<'db> { .unwrap_or_else(Type::unknown) } + pub(crate) fn comparison_truthiness( + &self, + expression: impl Into, + ) -> Option { + self.extra + .as_deref()? + .comparison_truthiness + .get(&expression.into()) + .copied() + } + fn collection_use_constraints( &self, collection_def: Definition<'db>, diff --git a/crates/ty_python_semantic/src/types/infer/builder.rs b/crates/ty_python_semantic/src/types/infer/builder.rs index f61f059f3bd48..d89fa21be49bf 100644 --- a/crates/ty_python_semantic/src/types/infer/builder.rs +++ b/crates/ty_python_semantic/src/types/infer/builder.rs @@ -46,7 +46,9 @@ use crate::place_load::{ ImplicitPlaceLoad, PlaceExprPrefixLoad, PlaceExprPrefixLoads, PlaceLoadFailure, PlaceLoadMode, PlaceLoadResolutionStep, PlaceLoadSource, PlaceLoadSourceKind, resolve_place_load, }; -use crate::reachability::{ReachabilityEvaluationCache, evaluate_reachability_with_cache}; +use crate::reachability::{ + ReachabilityEvaluationCache, analyze_condition_expression, evaluate_reachability_with_cache, +}; use crate::types::add_inferred_python_version_hint_to_diagnostic; use crate::types::attribute_write::{AssignmentAttributeMembers, assignment_attribute_members}; use crate::types::call::bind::{ @@ -263,6 +265,11 @@ pub(super) struct TypeInferenceBuilder<'db, 'ast> { /// The types of every expression in this region. expressions: FxHashMap>, + /// Truthiness overrides for evaluating comparison chains directly as conditions. + /// See [`ExpressionInferenceExtra::comparison_truthiness`] for why these are stored + /// separately from expression types. + comparison_truthiness: FxHashMap, + /// An expression cache shared across builders during multi-inference. expression_cache: Option>>>, @@ -484,6 +491,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { called_functions: FxIndexSet::default(), deferred_state: DeferredExpressionState::None, expressions: FxHashMap::default(), + comparison_truthiness: FxHashMap::default(), expression_cache: None, reachability_cache: OnceCell::new(), qualifiers: FxHashMap::default(), @@ -567,8 +575,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { #[cfg(debug_assertions)] assert_eq!(self.scope, inference.scope); - self.expressions - .extend(inference.expressions.iter().copied()); + self.extend_expression_types(inference.expressions.iter().copied()); self.declarations.extend(inference.declarations(definition)); if !matches!(self.region, InferenceRegion::Scope(..)) { @@ -644,8 +651,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { #[cfg(debug_assertions)] assert_eq!(self.scope, inference.scope); - self.expressions - .extend(inference.expressions.iter().copied()); + self.extend_expression_types(inference.expressions.iter().copied()); self.declarations.extend(inference.declarations()); if !matches!(self.region, InferenceRegion::Scope(..)) { @@ -687,12 +693,29 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } } + /// Replacing an expression's type also replaces any truthiness override. A newly inferred + /// comparison may no longer need an override, so extending the sparse map alone is not enough. + fn extend_expression_types( + &mut self, + expressions: impl IntoIterator)>, + ) { + if self.comparison_truthiness.is_empty() { + self.expressions.extend(expressions); + } else { + for (expression, ty) in expressions { + self.expressions.insert(expression, ty); + self.comparison_truthiness.remove(&expression); + } + } + } + /// Merges expression results without claiming bindings owned by their enclosing statement. fn extend_expression_without_bindings(&mut self, inference: &ExpressionInference<'db>) { - self.expressions - .extend(inference.expressions.iter().copied()); + self.extend_expression_types(inference.expressions.iter().copied()); if let Some(extra) = &inference.extra { + self.comparison_truthiness + .extend(extra.comparison_truthiness.iter().copied()); self.context.extend(&extra.diagnostics); self.extend_cycle_recovery(extra.cycle_recovery); self.called_functions @@ -721,8 +744,13 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { #[cfg(debug_assertions)] assert_eq!(self.scope, inference.scope); - self.expressions - .extend(inference.expressions.iter().map(|(key, ty)| (*key, *ty))); + self.extend_expression_types(inference.expressions.iter().map(|(key, ty)| (*key, *ty))); + self.comparison_truthiness.extend( + inference + .comparison_truthiness + .iter() + .map(|(key, truthiness)| (*key, *truthiness)), + ); self.context.extend(&inference.diagnostics); self.extend_cycle_recovery(inference.cycle_recovery); self.called_functions @@ -760,7 +788,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } fn extend_scope(&mut self, inference: &ScopeInference<'db>) { - self.expressions.extend(inference.expressions.iter()); + self.extend_expression_types(inference.expressions.iter()); if let Some(extra) = &inference.extra { self.context.extend(&extra.diagnostics); @@ -8500,10 +8528,20 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { (body_ty, orelse_ty) }; - match test_ty.try_bool(db, env).unwrap_or_else(|err| { - err.report_diagnostic(&self.context, &**test); - err.fallback_truthiness() - }) { + let test_truthiness = match test_ty.try_bool(db, env) { + Ok(_) => analyze_condition_expression(test, &|node| { + self.comparison_truthiness + .get(&node.into()) + .copied() + .or_else(|| self.expression_type(node).bool_if_inhabited(db, env)) + }) + .unwrap_or(Truthiness::Ambiguous), + Err(err) => { + err.report_diagnostic(&self.context, &**test); + err.fallback_truthiness() + } + }; + match test_truthiness { Truthiness::AlwaysTrue => body_ty, Truthiness::AlwaysFalse => orelse_ty, Truthiness::Ambiguous => UnionType::from_two_elements(db, env, body_ty, orelse_ty), @@ -11097,12 +11135,15 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { (ty, value.range()) }, ) + .value_type } /// Computes the output of a chain of (one) boolean operation, consuming as input an iterator /// of operations and calling the `infer_ty` for each to infer their types. /// The iterator is consumed even if the boolean evaluation can be short-circuited, /// in order to ensure the invariant that all expressions are evaluated when inferring types. + /// Returns the value type and the combined truthiness of all but the final operand, which + /// is not converted to a boolean when evaluating the chain as a value. /// /// `infer_ty` receives the unguarded union of previous operand types that may contribute to the /// result. This can be used as a type context without losing generic specialization information @@ -11114,16 +11155,17 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { track_peer_types: bool, operations: Iterator, needs_peer_type: NeedsPeerType, - infer_ty: InferType, - ) -> Type<'db> + mut infer_ty: InferType, + ) -> ChainedBooleanResult<'db> where Iterator: IntoIterator, NeedsPeerType: Fn(&Item) -> bool, - InferType: Fn(&mut Self, Item, Option>) -> (Type<'db>, TextRange), + InferType: FnMut(&mut Self, Item, Option>) -> (Type<'db>, TextRange), { let db = self.db(); let env = self.program_environment(); let mut done = false; + let mut preceding_truthiness = Truthiness::from(op.is_and()); let mut peer_types: Option> = None; let elements = operations @@ -11148,6 +11190,10 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { err.report_diagnostic(&self.context, range); err.fallback_truthiness() }); + preceding_truthiness = match op { + ast::BoolOp::And => preceding_truthiness.and(truthiness), + ast::BoolOp::Or => preceding_truthiness.or(truthiness), + }; if done { return Type::Never; @@ -11182,7 +11228,11 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } }); - UnionType::from_elements(db, env, elements) + let value_type = UnionType::from_elements(db, env, elements); + ChainedBooleanResult { + value_type, + preceding_truthiness, + } } fn infer_compare_expression(&mut self, compare: &ast::ExprCompare) -> Type<'db> { @@ -11196,6 +11246,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } = compare; self.infer_expression(left, TypeContext::default()); + let mut last_comparison_ty = Type::unknown(); // https://docs.python.org/3/reference/expressions.html#comparisons // > Formally, if `a, b, c, …, y, z` are expressions and `op1, op2, …, opN` are comparison @@ -11204,7 +11255,10 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { // // As some operators (==, !=, <, <=, >, >=) *can* return an arbitrary type, the logic below // is shared with the one in `infer_binary_type_comparison`. - self.infer_chained_boolean_types( + let ChainedBooleanResult { + value_type: ty, + preceding_truthiness, + } = self.infer_chained_boolean_types( ast::BoolOp::And, false, std::iter::once(&**left) @@ -11246,9 +11300,32 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } }); + last_comparison_ty = ty; (ty, range) }, - ) + ); + + if ops.len() > 1 { + // Individual comparisons within a chain have no expression nodes whose result types + // reachability can look up later. Retain their combined condition truthiness here; + // `and`/`or` conditions can instead be reconstructed by walking their operand nodes. + // See `ExpressionInferenceExtra::comparison_truthiness` for why the chain's value + // type is not sufficient. + // + // As a condition, the chain is truthy only if both its prefix and final comparison are + // truthy. Skip the final comparison's truthiness computation when the prefix is + // already always false. + let truthiness = preceding_truthiness + .and_else(|| last_comparison_ty.bool(db, self.program_environment())); + let expression = ast::ExprRef::Compare(compare).into(); + if truthiness != ty.bool(db, self.program_environment()) { + self.comparison_truthiness.insert(expression, truthiness); + } else { + self.comparison_truthiness.remove(&expression); + } + } + + ty } fn infer_type_parameters(&mut self, type_parameters: &ast::TypeParams) { @@ -11283,6 +11360,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { let Self { context, expressions, + comparison_truthiness, qualifiers: _, type_expression_flags, collection_use_constraints, @@ -11324,6 +11402,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { FullExpressionCacheEntry { expressions, + comparison_truthiness, type_expression_flags, collection_use_constraints, string_annotations, @@ -11343,6 +11422,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { let Self { context, expressions, + comparison_truthiness: _, qualifiers, type_expression_flags, mut collection_use_constraints, @@ -11456,6 +11536,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { let Self { context, expressions, + comparison_truthiness: _, bindings, called_functions, expression_cache: _, @@ -11504,6 +11585,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { let Self { context, expressions, + comparison_truthiness: _, qualifiers, type_expression_flags, mut collection_use_constraints, @@ -11645,6 +11727,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { type_expression_flags, mut collection_use_constraints, expressions, + comparison_truthiness: _, scope, cycle_recovery, qualifiers, @@ -11726,6 +11809,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { context: _, collection_use_constraints: _, expressions: _, + comparison_truthiness: _, string_annotations: _, expected_types: _, scope: _, @@ -11785,6 +11869,7 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { let Self { context, expressions, + comparison_truthiness, type_expression_flags, collection_use_constraints, string_annotations, @@ -11824,7 +11909,8 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { "speculative `TypeInferenceBuilder` should only be used for expression inference" ); - self.expressions.extend(expressions.iter()); + self.extend_expression_types(expressions); + self.comparison_truthiness.extend(comparison_truthiness); self.context.extend(&diagnostics); self.extend_cycle_recovery(cycle_recovery); self.string_annotations @@ -11852,6 +11938,20 @@ impl<'db, 'ast> TypeInferenceBuilder<'db, 'ast> { } } +/// The inferred result of a boolean or comparison chain. +struct ChainedBooleanResult<'db> { + value_type: Type<'db>, + /// Combined truthiness of all operands except the last. + /// + /// For `a < b < c`, evaluating the chain as a value tests `a < b` to decide whether to + /// short-circuit, but returns `b < c` without testing it if evaluation continues. + /// Keeping the preceding checks separate lets comparison inference combine this result + /// with the final comparison's truthiness when analyzing the chain as a condition. + /// Using `value_type` instead would model testing the returned object again, which can + /// give a different answer when a comparison returns an object with mutable truthiness. + preceding_truthiness: Truthiness, +} + /// An expression cache shared across builders during multi-inference. /// /// This provides a cheap way of reusing inference results without the overhead @@ -11937,6 +12037,7 @@ enum ExpressionCacheEntry<'db> { /// that is otherwise performed for Salsa results. struct FullExpressionCacheEntry<'db> { expressions: FxHashMap>, + comparison_truthiness: FxHashMap, type_expression_flags: FxHashMap, collection_use_constraints: CollectionUseConstraints<'db>, string_annotations: FxHashSet, @@ -11961,6 +12062,7 @@ impl<'db> FullExpressionCacheEntry<'db> { fn is_single_expression(&self, expression: ExpressionNodeKey, ty: Type<'db>) -> bool { self.expressions.len() == 1 && self.expressions.get(&expression) == Some(&ty) + && self.comparison_truthiness.is_empty() && self.type_expression_flags.is_empty() && self.collection_use_constraints.is_empty() && self.string_annotations.is_empty() @@ -11976,6 +12078,7 @@ impl<'db> FullExpressionCacheEntry<'db> { region: InferenceRegion<'db>, ) -> ExpressionInference<'db> { let extra = (!self.string_annotations.is_empty() + || !self.comparison_truthiness.is_empty() || !self.type_expression_flags.is_empty() || !self.collection_use_constraints.is_empty() || !self.expected_types.is_empty() @@ -11997,6 +12100,7 @@ impl<'db> FullExpressionCacheEntry<'db> { self.diagnostics.shrink_to_fit(); Box::new(ExpressionInferenceExtra { string_annotations: FrozenSet::from(self.string_annotations), + comparison_truthiness: FrozenMap::from(self.comparison_truthiness), expected_types: FrozenMap::from(self.expected_types), type_expression_flags: FrozenMap::from(self.type_expression_flags), bindings: self.bindings.into_boxed_slice(), diff --git a/crates/ty_python_semantic/src/types/infer/tests.rs b/crates/ty_python_semantic/src/types/infer/tests.rs index 97f466c39dac3..994aee3a0947d 100644 --- a/crates/ty_python_semantic/src/types/infer/tests.rs +++ b/crates/ty_python_semantic/src/types/infer/tests.rs @@ -520,6 +520,72 @@ fn simple_assignment_does_not_enter_salsa_cycle() { assert_eq!(cycles, Vec::::new()); } +/// Checks widening when a comparison truthiness override is present in only one iteration. +/// +/// A missing override falls back to the expression type's truthiness. Widening must compare the +/// effective truthiness from both iterations, including this fallback. Discarding an override from +/// the previous iteration could otherwise make a previously ambiguous condition definite again. +/// +/// We construct inference results directly because mdtests cannot prescribe intermediate Salsa +/// results. A Python cycle can converge before widening starts, or drop an override without +/// changing any final types or diagnostics. No known Python example exposes the failures checked +/// here, so this is defensive coverage of the widening invariant. +#[test] +fn comparison_truthiness_widens_across_sparse_cycle_results() -> anyhow::Result<()> { + let mut db = setup_db(); + db.write_dedented("src/comparison.py", "0 < 1 < 2")?; + let file = program_file(&db, system_path_to_file(&db, "src/comparison.py")?); + let module = parsed_module(&db, file.python_file(&db)).load(&db); + let Some(ast::Stmt::Expr(statement)) = module.syntax().body.first() else { + anyhow::bail!("expected a comparison expression statement"); + }; + let expression = ExpressionNodeKey::from(statement.value.as_ref()); + let scope = global_scope(&db, file); + let env = ProgramEnvironment::from_scope(scope); + let inference = |ty, truthiness: Option| ExpressionInference { + expressions: [(expression, ty)].into_iter().collect(), + extra: truthiness.map(|truthiness| { + Box::new(ExpressionInferenceExtra { + comparison_truthiness: [(expression, truthiness)].into_iter().collect(), + ..ExpressionInferenceExtra::default() + }) + }), + #[cfg(debug_assertions)] + scope, + }; + + // A previously widened condition stays ambiguous even when the new result omits its + // override and has a definite value-type fallback. + let previous = inference(Type::bool_literal(false), Some(Truthiness::Ambiguous)); + let mut current = inference(Type::bool_literal(false), None); + current.widen_comparison_truthiness(&db, &env, &previous); + assert_eq!( + current.comparison_truthiness(expression), + Some(Truthiness::Ambiguous) + ); + + // A new override is compared with the previous result's value-type fallback. + let previous = inference(Type::bool_literal(true), None); + let mut current = inference(Type::unknown(), Some(Truthiness::AlwaysFalse)); + current.widen_comparison_truthiness(&db, &env, &previous); + assert_eq!( + current.comparison_truthiness(expression), + Some(Truthiness::Ambiguous) + ); + + // Matching effective truthiness stays precise. Keep the override even though it agrees with + // the current type: subsequent type widening can make that fallback ambiguous again. + let previous = inference(Type::unknown(), Some(Truthiness::AlwaysFalse)); + let mut current = inference(Type::bool_literal(false), None); + current.widen_comparison_truthiness(&db, &env, &previous); + assert_eq!( + current.comparison_truthiness(expression), + Some(Truthiness::AlwaysFalse) + ); + + Ok(()) +} + /// Test that a symbol known to be unbound in a scope does not still trigger cycle-causing /// reachability-constraint checks in that scope. #[test] diff --git a/crates/ty_python_semantic/src/types/narrow.rs b/crates/ty_python_semantic/src/types/narrow.rs index 2a455433f2507..d5ee177a3bee6 100644 --- a/crates/ty_python_semantic/src/types/narrow.rs +++ b/crates/ty_python_semantic/src/types/narrow.rs @@ -84,7 +84,9 @@ pub(crate) fn infer_narrowing_constraints<'db>( Option>, ) { let constraints = match predicate.node { - PredicateNode::Expression(expression) => { + PredicateNode::Expression(expression) + | PredicateNode::Condition(expression) + | PredicateNode::ChainedComparisonCondition(expression) => { let constraints = all_narrowing_constraints_for_expression(db, expression); ( constraints.get(place, true).cloned(), @@ -1511,7 +1513,9 @@ impl<'db, 'ast> NarrowingConstraintsBuilder<'db, 'ast> { fn finish(mut self) -> Option> { let constraints: Option> = match self.predicate { - PredicateNode::Expression(expression) => { + PredicateNode::Expression(expression) + | PredicateNode::Condition(expression) + | PredicateNode::ChainedComparisonCondition(expression) => { self.evaluate_expression_predicate(expression, self.is_positive) } PredicateNode::Pattern(pattern) => { @@ -3296,6 +3300,8 @@ impl<'db> NarrowingConstraintsBuilder<'db, '_> { let db = self.db; match self.predicate { PredicateNode::Expression(expression) + | PredicateNode::Condition(expression) + | PredicateNode::ChainedComparisonCondition(expression) | PredicateNode::ContextManagerSuppresses { expression, .. } => expression.scope(db), PredicateNode::Pattern(pattern) => pattern.scope(db), PredicateNode::FinallyNormalPathImpossible { scope, .. } => scope, diff --git a/hawk.toml b/hawk.toml index ee551be1dd58b..63f0cc6a07fc4 100644 --- a/hawk.toml +++ b/hawk.toml @@ -878,14 +878,6 @@ kind = "inherent_method" level = "expect" reason = "command descriptions expose a complete argument builder and inspection API" -[[override]] -lint = "hawk::dead_public" -crate = "ty_python_core" -item = "expression::Expression::<'db>::python_file" -kind = "inherent_method" -level = "expect" -reason = "semantic ingredients expose consistent file, scope, and program accessors" - [[override]] lint = "hawk::dead_public" crate = "ty_python_core"