From 5c2d7315cda6071a6ac876889aa17fc0d7464974 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Fri, 31 Jan 2025 16:32:03 -0500 Subject: [PATCH 01/27] Add TDD implementation --- crates/red_knot_python_semantic/src/lib.rs | 1 + .../src/semantic_index/constraint.rs | 6 +- crates/red_knot_python_semantic/src/tdd.rs | 597 ++++++++++++++++++ crates/ruff_python_ast/src/nodes.rs | 2 +- 4 files changed, 602 insertions(+), 4 deletions(-) create mode 100644 crates/red_knot_python_semantic/src/tdd.rs diff --git a/crates/red_knot_python_semantic/src/lib.rs b/crates/red_knot_python_semantic/src/lib.rs index 010de76df07f5..eedbf67f8ac3d 100644 --- a/crates/red_knot_python_semantic/src/lib.rs +++ b/crates/red_knot_python_semantic/src/lib.rs @@ -27,6 +27,7 @@ pub(crate) mod site_packages; mod stdlib; mod suppression; pub(crate) mod symbol; +mod tdd; pub mod types; mod unpack; mod util; diff --git a/crates/red_knot_python_semantic/src/semantic_index/constraint.rs b/crates/red_knot_python_semantic/src/semantic_index/constraint.rs index d99eb44b85520..ed0760a8bc3cf 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/constraint.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/constraint.rs @@ -5,20 +5,20 @@ use crate::db::Db; use crate::semantic_index::expression::Expression; use crate::semantic_index::symbol::{FileScopeId, ScopeId}; -#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] pub(crate) struct Constraint<'db> { pub(crate) node: ConstraintNode<'db>, pub(crate) is_positive: bool, } -#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, Hash, PartialEq, Eq)] pub(crate) enum ConstraintNode<'db> { Expression(Expression<'db>), Pattern(PatternConstraint<'db>), } /// Pattern kinds for which we support type narrowing and/or static visibility analysis. -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, Hash, PartialEq)] pub(crate) enum PatternConstraintKind<'db> { Singleton(Singleton, Option>), Value(Expression<'db>, Option>), diff --git a/crates/red_knot_python_semantic/src/tdd.rs b/crates/red_knot_python_semantic/src/tdd.rs new file mode 100644 index 0000000000000..8e5d46f9a6869 --- /dev/null +++ b/crates/red_knot_python_semantic/src/tdd.rs @@ -0,0 +1,597 @@ +//! # Visibility constraints +//! +//! During semantic index building, we collect visibility constraints for each binding and +//! declaration. These constraints are then used during type-checking to determine the static +//! visibility of a certain definition. This allows us to re-analyze control flow during type +//! checking, potentially "hiding" some branches that we can statically determine to never be +//! taken. Consider the following example first. We added implicit "unbound" definitions at the +//! start of the scope. Note how visibility constraints can apply to bindings outside of the +//! if-statement: +//! ```py +//! x = # not a live binding for the use of x below, shadowed by `x = 1` +//! y = # visibility constraint: ~test +//! +//! x = 1 # visibility constraint: ~test +//! if test: +//! x = 2 # visibility constraint: test +//! +//! y = 2 # visibility constraint: test +//! +//! use(x) +//! use(y) +//! ``` +//! The static truthiness of the `test` condition can either be always-false, ambiguous, or +//! always-true. Similarly, we have the same three options when evaluating a visibility constraint. +//! This outcome determines the visibility of a definition: always-true means that the definition +//! is definitely visible for a given use, always-false means that the definition is definitely +//! not visible, and ambiguous means that we might see this definition or not. In the latter case, +//! we need to consider both options during type inference and boundness analysis. For the example +//! above, these are the possible type inference / boundness results for the uses of `x` and `y`: +//! +//! ```text +//! | `test` truthiness | `~test` truthiness | type of `x` | boundness of `y` | +//! |-------------------|--------------------|-----------------|------------------| +//! | always false | always true | `Literal[1]` | unbound | +//! | ambiguous | ambiguous | `Literal[1, 2]` | possibly unbound | +//! | always true | always false | `Literal[2]` | bound | +//! ``` +//! +//! ### Sequential constraints (ternary AND) +//! +//! As we have seen above, visibility constraints can apply outside of a control flow element. +//! So we need to consider the possibility that multiple constraints apply to the same binding. +//! Here, we consider what happens if multiple `if`-statements lead to a sequence of constraints. +//! Consider the following example: +//! ```py +//! x = 0 +//! +//! if test1: +//! x = 1 +//! +//! if test2: +//! x = 2 +//! ``` +//! The binding `x = 2` is easy to analyze. Its visibility corresponds to the truthiness of `test2`. +//! For the `x = 1` binding, things are a bit more interesting. It is always visible if `test1` is +//! always-true *and* `test2` is always-false. It is never visible if `test1` is always-false *or* +//! `test2` is always-true. And it is ambiguous otherwise. This corresponds to a ternary *test1 AND +//! ~test2* operation in three-valued Kleene logic [Kleene]: +//! +//! ```text +//! | AND | always-false | ambiguous | always-true | +//! |--------------|--------------|--------------|--------------| +//! | always false | always-false | always-false | always-false | +//! | ambiguous | always-false | ambiguous | ambiguous | +//! | always true | always-false | ambiguous | always-true | +//! ``` +//! +//! The `x = 0` binding can be handled similarly, with the difference that both `test1` and `test2` +//! are negated: +//! ```py +//! x = 0 # ~test1 AND ~test2 +//! +//! if test1: +//! x = 1 # test1 AND ~test2 +//! +//! if test2: +//! x = 2 # test2 +//! ``` +//! +//! ### Merged constraints (ternary OR) +//! +//! Finally, we consider what happens in "parallel" control flow. Consider the following example +//! where we have omitted the test condition for the outer `if` for clarity: +//! ```py +//! x = 0 +//! +//! if <…>: +//! if test1: +//! x = 1 +//! else: +//! if test2: +//! x = 2 +//! +//! use(x) +//! ``` +//! At the usage of `x`, i.e. after control flow has been merged again, the visibility of the `x = +//! 0` binding behaves as follows: the binding is always visible if `test1` is always-false *or* +//! `test2` is always-false; and it is never visible if `test1` is always-true *and* `test2` is +//! always-true. This corresponds to a ternary *OR* operation in Kleene logic: +//! +//! ```text +//! | OR | always-false | ambiguous | always-true | +//! |--------------|--------------|--------------|--------------| +//! | always false | always-false | ambiguous | always-true | +//! | ambiguous | ambiguous | ambiguous | always-true | +//! | always true | always-true | always-true | always-true | +//! ``` +//! +//! Using this, we can annotate the visibility constraints for the example above: +//! ```py +//! x = 0 # ~test1 OR ~test2 +//! +//! if <…>: +//! if test1: +//! x = 1 # test1 +//! else: +//! if test2: +//! x = 2 # test2 +//! +//! use(x) +//! ``` +//! +//! ### Explicit ambiguity +//! +//! In some cases, we explicitly add an “ambiguous” constraint to all bindings +//! in a certain control flow path. We do this when branching on something that we can not (or +//! intentionally do not want to) analyze statically. `for` loops are one example: +//! ```py +//! x = +//! +//! for _ in range(2): +//! x = 1 +//! ``` +//! Here, we report an ambiguous visibility constraint before branching off. If we don't do this, +//! the `x = ` binding would be considered unconditionally visible in the no-loop case. +//! And since the other branch does not have the live `x = ` binding, we would incorrectly +//! create a state where the `x = ` binding is always visible. +//! +//! +//! ### Properties +//! +//! The ternary `AND` and `OR` operations have the property that `~a OR ~b = ~(a AND b)`. This +//! means we could, in principle, get rid of either of these two to simplify the representation. +//! +//! However, we already apply negative constraints `~test1` and `~test2` to the "branches not +//! taken" in the example above. This means that the tree-representation `~test1 OR ~test2` is much +//! cheaper/shallower than basically creating `~(~(~test1) AND ~(~test2))`. Similarly, if we wanted +//! to get rid of `AND`, we would also have to create additional nodes. So for performance reasons, +//! there is a small "duplication" in the code between those two constraint types. +//! +//! [Kleene]: + +use std::cmp::Ordering; + +use rustc_hash::FxHashMap; + +use crate::semantic_index::{ + ast_ids::HasScopedExpressionId, + constraint::{Constraint, ConstraintNode, PatternConstraintKind}, +}; +use crate::types::{infer_expression_types, Truthiness}; +use crate::Db; + +/// A ternary formula that defines under what conditions a binding is visible. (A ternary formula +/// is just like a boolean formula, but with `Ambiguous` as a third potential result. See the +/// module documentation for more details.) +/// +/// The primitive atoms of the formula are [`Constraint`]s, which express some property of the +/// runtime state of the code that we are analyzing. +/// +/// We assume that each atom has a stable value each time that the formula is evaluated. An atom +/// that resolves to `Ambiguous` might be true or false, and we can't tell which — but within that +/// evaluation, we assume that the atom has the _same_ unknown value each time it appears. That +/// allows us to perform simplifications like `A ∨ !A → true` and `A ∧ !A → false`. +/// +/// That means that when you are constructing a formula, you might need to create distinct atoms +/// for a particular [`Constraint`], if your formula needs to consider how a particular runtime +/// property might be different at different points in the execution of the program. +/// +/// Visibility constraints are normalized, so equivalent constraints are guaranteed to have equal +/// IDs. +#[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] +pub(crate) struct ScopedVisibilityConstraintId(u32); + +impl std::fmt::Debug for ScopedVisibilityConstraintId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_tuple("ScopedVisibilityConstraintId") + .field(&self.into_node_kind()) + .finish() + } +} + +// Internal details: +// +// There are 3 terminals, with hard-coded constraint IDs: true, ambiguous, and false. +// +// _Atoms_ are the underlying Constraints, which are the variables that are evaluated by the +// ternary function. +// +// _Interior nodes_ provide the TDD structure for the formula. Interior nodes are stored in an +// arena Vec, with the constraint ID providing an index into the arena. + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum NodeKind { + AlwaysTrue, + Ambiguous, + AlwaysFalse, + Interior(u32), +} + +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +struct InteriorNode { + atom: Atom, + if_true: ScopedVisibilityConstraintId, + if_ambiguous: ScopedVisibilityConstraintId, + if_false: ScopedVisibilityConstraintId, +} + +#[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] +struct Atom(u32); + +impl Atom { + fn into_index_and_copy(self) -> (u32, u8) { + let copy = self.0 >> 24; + let index = self.0 & 0x00ff_ffff; + (index, copy as u8) + } + + fn from_index_and_copy(index: u32, copy: u8) -> Self { + debug_assert!(index <= 0x00ff_ffff); + Self((u32::from(copy) << 24) | index) + } +} + +impl std::fmt::Debug for Atom { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let (index, copy) = self.into_index_and_copy(); + f.debug_tuple("Atom").field(&index).field(©).finish() + } +} + +impl ScopedVisibilityConstraintId { + /// A special ID that is used for an "always true" / "always visible" constraint. + pub(crate) const ALWAYS_TRUE: ScopedVisibilityConstraintId = + ScopedVisibilityConstraintId(0xffff_ffff); + + /// A special ID that is used for an ambiguous constraint. + pub(crate) const AMBIGUOUS: ScopedVisibilityConstraintId = + ScopedVisibilityConstraintId(0xffff_fffe); + + /// A special ID that is used for an "always false" / "never visible" constraint. + pub(crate) const ALWAYS_FALSE: ScopedVisibilityConstraintId = + ScopedVisibilityConstraintId(0xffff_fffd); + + fn into_node_kind(self) -> NodeKind { + if self == Self::ALWAYS_TRUE { + NodeKind::AlwaysTrue + } else if self == Self::AMBIGUOUS { + NodeKind::Ambiguous + } else if self == Self::ALWAYS_FALSE { + NodeKind::AlwaysFalse + } else { + NodeKind::Interior(self.0) + } + } +} + +impl From for ScopedVisibilityConstraintId { + fn from(kind: NodeKind) -> ScopedVisibilityConstraintId { + match kind { + NodeKind::AlwaysTrue => Self::ALWAYS_TRUE, + NodeKind::Ambiguous => Self::AMBIGUOUS, + NodeKind::AlwaysFalse => Self::ALWAYS_FALSE, + NodeKind::Interior(index) => { + // We have verified elsewhere that index is within the expected range + Self(index) + } + } + } +} + +#[derive(Debug, PartialEq, Eq)] +pub(crate) struct VisibilityConstraints<'db> { + constraints: Vec>, + interiors: Vec, +} + +#[derive(Debug, Default, PartialEq, Eq)] +pub(crate) struct VisibilityConstraintsBuilder<'db> { + constraints: Vec>, + interiors: Vec, + constraint_cache: FxHashMap, u32>, + interior_cache: FxHashMap, + not_cache: FxHashMap, + and_cache: FxHashMap< + (ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), + ScopedVisibilityConstraintId, + >, + or_cache: FxHashMap< + (ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), + ScopedVisibilityConstraintId, + >, +} + +impl<'db> VisibilityConstraintsBuilder<'db> { + pub(crate) fn build(self) -> VisibilityConstraints<'db> { + VisibilityConstraints { + constraints: self.constraints, + interiors: self.interiors, + } + } + + /// Adds a constraint, ensuring that we only store any particular constraint once. + #[allow(clippy::cast_possible_truncation)] + fn add_constraint(&mut self, constraint: Constraint<'db>, copy: u8) -> Atom { + let index = *self.constraint_cache.entry(constraint).or_insert_with(|| { + let index = self.constraints.len() as u32; + self.constraints.push(constraint); + index + }); + Atom::from_index_and_copy(index, copy) + } + + /// Adds an interior node, ensuring that we always use the same visibility constraint ID for + /// equal nodes. + #[allow(clippy::cast_possible_truncation)] + fn add_interior(&mut self, node: InteriorNode) -> ScopedVisibilityConstraintId { + // Reduce! + if node.if_true == node.if_ambiguous && node.if_true == node.if_false { + return node.if_true; + } + + let index = *self.interior_cache.entry(node).or_insert_with(|| { + let index = self.interiors.len() as u32; + self.interiors.push(node); + index + }); + debug_assert!(index < 0x8000_0000); + NodeKind::Interior(index).into() + } + + pub(crate) fn add_atom( + &mut self, + constraint: Constraint<'db>, + copy: u8, + ) -> ScopedVisibilityConstraintId { + let atom = self.add_constraint(constraint, copy); + self.add_interior(InteriorNode { + atom, + if_true: ScopedVisibilityConstraintId::ALWAYS_TRUE, + if_ambiguous: ScopedVisibilityConstraintId::AMBIGUOUS, + if_false: ScopedVisibilityConstraintId::ALWAYS_FALSE, + }) + } + + pub(crate) fn add_not_constraint( + &mut self, + a: ScopedVisibilityConstraintId, + ) -> ScopedVisibilityConstraintId { + if let Some(cached) = self.not_cache.get(&a) { + return *cached; + } + let a_node = match a.into_node_kind() { + NodeKind::AlwaysTrue => return ScopedVisibilityConstraintId::ALWAYS_FALSE, + NodeKind::Ambiguous => return ScopedVisibilityConstraintId::AMBIGUOUS, + NodeKind::AlwaysFalse => return ScopedVisibilityConstraintId::ALWAYS_TRUE, + NodeKind::Interior(index) => self.interiors[index as usize], + }; + let if_true = self.add_not_constraint(a_node.if_true); + let if_ambiguous = self.add_not_constraint(a_node.if_ambiguous); + let if_false = self.add_not_constraint(a_node.if_false); + let result = self.add_interior(InteriorNode { + atom: a_node.atom, + if_true, + if_ambiguous, + if_false, + }); + self.not_cache.insert(a, result); + result + } + + pub(crate) fn add_or_constraint( + &mut self, + a: ScopedVisibilityConstraintId, + b: ScopedVisibilityConstraintId, + ) -> ScopedVisibilityConstraintId { + if let Some(cached) = self.or_cache.get(&(a, b)) { + return *cached; + } + + let (atom, if_true, if_ambiguous, if_false) = match (a.into_node_kind(), b.into_node_kind()) + { + (NodeKind::AlwaysTrue, _) | (_, NodeKind::AlwaysTrue) => { + return ScopedVisibilityConstraintId::ALWAYS_TRUE + } + (NodeKind::AlwaysFalse, _) => return b, + (_, NodeKind::AlwaysFalse) => return a, + (NodeKind::Ambiguous, NodeKind::Ambiguous) => { + return ScopedVisibilityConstraintId::AMBIGUOUS + } + + (NodeKind::Ambiguous, NodeKind::Interior(b_index)) => { + let b_node = self.interiors[b_index as usize]; + ( + b_node.atom, + self.add_or_constraint(a, b_node.if_true), + self.add_or_constraint(a, b_node.if_ambiguous), + self.add_or_constraint(a, b_node.if_false), + ) + } + (NodeKind::Interior(a_index), NodeKind::Ambiguous) => { + let a_node = self.interiors[a_index as usize]; + ( + a_node.atom, + self.add_or_constraint(a_node.if_true, b), + self.add_or_constraint(a_node.if_ambiguous, b), + self.add_or_constraint(a_node.if_false, b), + ) + } + + (NodeKind::Interior(a_index), NodeKind::Interior(b_index)) => { + let a_node = self.interiors[a_index as usize]; + let b_node = self.interiors[b_index as usize]; + match a_node.atom.cmp(&b_node.atom) { + Ordering::Equal => ( + a_node.atom, + self.add_or_constraint(a_node.if_true, b_node.if_true), + self.add_or_constraint(a_node.if_ambiguous, b_node.if_ambiguous), + self.add_or_constraint(a_node.if_false, b_node.if_false), + ), + Ordering::Less => ( + a_node.atom, + self.add_or_constraint(a_node.if_true, b), + self.add_or_constraint(a_node.if_ambiguous, b), + self.add_or_constraint(a_node.if_false, b), + ), + Ordering::Greater => ( + b_node.atom, + self.add_or_constraint(a, b_node.if_true), + self.add_or_constraint(a, b_node.if_ambiguous), + self.add_or_constraint(a, b_node.if_false), + ), + } + } + }; + + let result = self.add_interior(InteriorNode { + atom, + if_true, + if_ambiguous, + if_false, + }); + self.or_cache.insert((a, b), result); + result + } + + pub(crate) fn add_and_constraint( + &mut self, + a: ScopedVisibilityConstraintId, + b: ScopedVisibilityConstraintId, + ) -> ScopedVisibilityConstraintId { + if let Some(cached) = self.and_cache.get(&(a, b)) { + return *cached; + } + + let (atom, if_true, if_ambiguous, if_false) = match (a.into_node_kind(), b.into_node_kind()) + { + (NodeKind::AlwaysFalse, _) | (_, NodeKind::AlwaysFalse) => { + return ScopedVisibilityConstraintId::ALWAYS_FALSE + } + (NodeKind::AlwaysTrue, _) => return b, + (_, NodeKind::AlwaysTrue) => return a, + (NodeKind::Ambiguous, NodeKind::Ambiguous) => { + return ScopedVisibilityConstraintId::AMBIGUOUS + } + + (NodeKind::Ambiguous, NodeKind::Interior(b_index)) => { + let b_node = self.interiors[b_index as usize]; + ( + b_node.atom, + self.add_and_constraint(a, b_node.if_true), + self.add_and_constraint(a, b_node.if_ambiguous), + self.add_and_constraint(a, b_node.if_false), + ) + } + (NodeKind::Interior(a_index), NodeKind::Ambiguous) => { + let a_node = self.interiors[a_index as usize]; + ( + a_node.atom, + self.add_and_constraint(a_node.if_true, b), + self.add_and_constraint(a_node.if_ambiguous, b), + self.add_and_constraint(a_node.if_false, b), + ) + } + + (NodeKind::Interior(a_index), NodeKind::Interior(b_index)) => { + let a_node = self.interiors[a_index as usize]; + let b_node = self.interiors[b_index as usize]; + match a_node.atom.cmp(&b_node.atom) { + Ordering::Equal => ( + a_node.atom, + self.add_and_constraint(a_node.if_true, b_node.if_true), + self.add_and_constraint(a_node.if_ambiguous, b_node.if_ambiguous), + self.add_and_constraint(a_node.if_false, b_node.if_false), + ), + Ordering::Less => ( + a_node.atom, + self.add_and_constraint(a_node.if_true, b), + self.add_and_constraint(a_node.if_ambiguous, b), + self.add_and_constraint(a_node.if_false, b), + ), + Ordering::Greater => ( + b_node.atom, + self.add_and_constraint(a, b_node.if_true), + self.add_and_constraint(a, b_node.if_ambiguous), + self.add_and_constraint(a, b_node.if_false), + ), + } + } + }; + + let result = self.add_interior(InteriorNode { + atom, + if_true, + if_ambiguous, + if_false, + }); + self.and_cache.insert((a, b), result); + result + } +} + +impl<'db> VisibilityConstraints<'db> { + /// Analyze the statically known visibility for a given visibility constraint. + pub(crate) fn evaluate(&self, db: &'db dyn Db, id: ScopedVisibilityConstraintId) -> Truthiness { + let node = match id.into_node_kind() { + NodeKind::AlwaysTrue => return Truthiness::AlwaysTrue, + NodeKind::Ambiguous => return Truthiness::Ambiguous, + NodeKind::AlwaysFalse => return Truthiness::AlwaysFalse, + NodeKind::Interior(index) => self.interiors[index as usize], + }; + let (index, _) = node.atom.into_index_and_copy(); + let constraint = &self.constraints[index as usize]; + match Self::analyze_single(db, constraint) { + Truthiness::AlwaysTrue => self.evaluate(db, node.if_true), + Truthiness::Ambiguous => self.evaluate(db, node.if_ambiguous), + Truthiness::AlwaysFalse => self.evaluate(db, node.if_false), + } + } + + fn analyze_single(db: &dyn Db, constraint: &Constraint) -> Truthiness { + match constraint.node { + ConstraintNode::Expression(test_expr) => { + let inference = infer_expression_types(db, test_expr); + let scope = test_expr.scope(db); + let ty = inference + .expression_type(test_expr.node_ref(db).scoped_expression_id(db, scope)); + + ty.bool(db).negate_if(!constraint.is_positive) + } + ConstraintNode::Pattern(inner) => match inner.kind(db) { + PatternConstraintKind::Value(value, guard) => { + let subject_expression = inner.subject(db); + let inference = infer_expression_types(db, *subject_expression); + let scope = subject_expression.scope(db); + let subject_ty = inference.expression_type( + subject_expression + .node_ref(db) + .scoped_expression_id(db, scope), + ); + + let inference = infer_expression_types(db, *value); + let scope = value.scope(db); + let value_ty = inference + .expression_type(value.node_ref(db).scoped_expression_id(db, scope)); + + if subject_ty.is_single_valued(db) { + let truthiness = + Truthiness::from(subject_ty.is_equivalent_to(db, value_ty)); + + if truthiness.is_always_true() && guard.is_some() { + // Fall back to ambiguous, the guard might change the result. + Truthiness::Ambiguous + } else { + truthiness + } + } else { + Truthiness::Ambiguous + } + } + PatternConstraintKind::Singleton(..) + | PatternConstraintKind::Class(..) + | PatternConstraintKind::Unsupported => Truthiness::Ambiguous, + }, + } + } +} diff --git a/crates/ruff_python_ast/src/nodes.rs b/crates/ruff_python_ast/src/nodes.rs index 618fa5c71c281..ff9dbab59000c 100644 --- a/crates/ruff_python_ast/src/nodes.rs +++ b/crates/ruff_python_ast/src/nodes.rs @@ -3528,7 +3528,7 @@ impl From for Name { } } -#[derive(Clone, Copy, Debug, PartialEq)] +#[derive(Clone, Copy, Debug, Hash, PartialEq)] pub enum Singleton { None, True, From 1116336df26376e5050f9c58fd26741356eb9e8b Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Fri, 31 Jan 2025 16:49:40 -0500 Subject: [PATCH 02/27] Replace the old implementation with the new TDD one! --- .../mdtest/statically_known_branches.md | 5 +- crates/red_knot_python_semantic/src/lib.rs | 1 - crates/red_knot_python_semantic/src/tdd.rs | 597 ------------------ .../src/visibility_constraints.rs | 447 ++++++++----- 4 files changed, 305 insertions(+), 745 deletions(-) delete mode 100644 crates/red_knot_python_semantic/src/tdd.rs diff --git a/crates/red_knot_python_semantic/resources/mdtest/statically_known_branches.md b/crates/red_knot_python_semantic/resources/mdtest/statically_known_branches.md index 5b5f7c686c7b3..c2f77b26a7e89 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/statically_known_branches.md +++ b/crates/red_knot_python_semantic/resources/mdtest/statically_known_branches.md @@ -1494,6 +1494,8 @@ from module import symbol We currently have a limitation in the complexity (depth) of the visibility constraints that are supported. This is to avoid pathological cases that would require us to recurse deeply. +TODO: We don't! Remove this! + ```py x = 1 @@ -1516,8 +1518,7 @@ False or False or False or False or \ False or False or False or False or \ False or False or False or (y := 2) # fmt: skip -# TODO: This should ideally be `Literal[2]` as well: -reveal_type(y) # revealed: Literal[1, 2] +reveal_type(y) # revealed: Literal[2] ``` ## Unsupported features diff --git a/crates/red_knot_python_semantic/src/lib.rs b/crates/red_knot_python_semantic/src/lib.rs index eedbf67f8ac3d..010de76df07f5 100644 --- a/crates/red_knot_python_semantic/src/lib.rs +++ b/crates/red_knot_python_semantic/src/lib.rs @@ -27,7 +27,6 @@ pub(crate) mod site_packages; mod stdlib; mod suppression; pub(crate) mod symbol; -mod tdd; pub mod types; mod unpack; mod util; diff --git a/crates/red_knot_python_semantic/src/tdd.rs b/crates/red_knot_python_semantic/src/tdd.rs deleted file mode 100644 index 8e5d46f9a6869..0000000000000 --- a/crates/red_knot_python_semantic/src/tdd.rs +++ /dev/null @@ -1,597 +0,0 @@ -//! # Visibility constraints -//! -//! During semantic index building, we collect visibility constraints for each binding and -//! declaration. These constraints are then used during type-checking to determine the static -//! visibility of a certain definition. This allows us to re-analyze control flow during type -//! checking, potentially "hiding" some branches that we can statically determine to never be -//! taken. Consider the following example first. We added implicit "unbound" definitions at the -//! start of the scope. Note how visibility constraints can apply to bindings outside of the -//! if-statement: -//! ```py -//! x = # not a live binding for the use of x below, shadowed by `x = 1` -//! y = # visibility constraint: ~test -//! -//! x = 1 # visibility constraint: ~test -//! if test: -//! x = 2 # visibility constraint: test -//! -//! y = 2 # visibility constraint: test -//! -//! use(x) -//! use(y) -//! ``` -//! The static truthiness of the `test` condition can either be always-false, ambiguous, or -//! always-true. Similarly, we have the same three options when evaluating a visibility constraint. -//! This outcome determines the visibility of a definition: always-true means that the definition -//! is definitely visible for a given use, always-false means that the definition is definitely -//! not visible, and ambiguous means that we might see this definition or not. In the latter case, -//! we need to consider both options during type inference and boundness analysis. For the example -//! above, these are the possible type inference / boundness results for the uses of `x` and `y`: -//! -//! ```text -//! | `test` truthiness | `~test` truthiness | type of `x` | boundness of `y` | -//! |-------------------|--------------------|-----------------|------------------| -//! | always false | always true | `Literal[1]` | unbound | -//! | ambiguous | ambiguous | `Literal[1, 2]` | possibly unbound | -//! | always true | always false | `Literal[2]` | bound | -//! ``` -//! -//! ### Sequential constraints (ternary AND) -//! -//! As we have seen above, visibility constraints can apply outside of a control flow element. -//! So we need to consider the possibility that multiple constraints apply to the same binding. -//! Here, we consider what happens if multiple `if`-statements lead to a sequence of constraints. -//! Consider the following example: -//! ```py -//! x = 0 -//! -//! if test1: -//! x = 1 -//! -//! if test2: -//! x = 2 -//! ``` -//! The binding `x = 2` is easy to analyze. Its visibility corresponds to the truthiness of `test2`. -//! For the `x = 1` binding, things are a bit more interesting. It is always visible if `test1` is -//! always-true *and* `test2` is always-false. It is never visible if `test1` is always-false *or* -//! `test2` is always-true. And it is ambiguous otherwise. This corresponds to a ternary *test1 AND -//! ~test2* operation in three-valued Kleene logic [Kleene]: -//! -//! ```text -//! | AND | always-false | ambiguous | always-true | -//! |--------------|--------------|--------------|--------------| -//! | always false | always-false | always-false | always-false | -//! | ambiguous | always-false | ambiguous | ambiguous | -//! | always true | always-false | ambiguous | always-true | -//! ``` -//! -//! The `x = 0` binding can be handled similarly, with the difference that both `test1` and `test2` -//! are negated: -//! ```py -//! x = 0 # ~test1 AND ~test2 -//! -//! if test1: -//! x = 1 # test1 AND ~test2 -//! -//! if test2: -//! x = 2 # test2 -//! ``` -//! -//! ### Merged constraints (ternary OR) -//! -//! Finally, we consider what happens in "parallel" control flow. Consider the following example -//! where we have omitted the test condition for the outer `if` for clarity: -//! ```py -//! x = 0 -//! -//! if <…>: -//! if test1: -//! x = 1 -//! else: -//! if test2: -//! x = 2 -//! -//! use(x) -//! ``` -//! At the usage of `x`, i.e. after control flow has been merged again, the visibility of the `x = -//! 0` binding behaves as follows: the binding is always visible if `test1` is always-false *or* -//! `test2` is always-false; and it is never visible if `test1` is always-true *and* `test2` is -//! always-true. This corresponds to a ternary *OR* operation in Kleene logic: -//! -//! ```text -//! | OR | always-false | ambiguous | always-true | -//! |--------------|--------------|--------------|--------------| -//! | always false | always-false | ambiguous | always-true | -//! | ambiguous | ambiguous | ambiguous | always-true | -//! | always true | always-true | always-true | always-true | -//! ``` -//! -//! Using this, we can annotate the visibility constraints for the example above: -//! ```py -//! x = 0 # ~test1 OR ~test2 -//! -//! if <…>: -//! if test1: -//! x = 1 # test1 -//! else: -//! if test2: -//! x = 2 # test2 -//! -//! use(x) -//! ``` -//! -//! ### Explicit ambiguity -//! -//! In some cases, we explicitly add an “ambiguous” constraint to all bindings -//! in a certain control flow path. We do this when branching on something that we can not (or -//! intentionally do not want to) analyze statically. `for` loops are one example: -//! ```py -//! x = -//! -//! for _ in range(2): -//! x = 1 -//! ``` -//! Here, we report an ambiguous visibility constraint before branching off. If we don't do this, -//! the `x = ` binding would be considered unconditionally visible in the no-loop case. -//! And since the other branch does not have the live `x = ` binding, we would incorrectly -//! create a state where the `x = ` binding is always visible. -//! -//! -//! ### Properties -//! -//! The ternary `AND` and `OR` operations have the property that `~a OR ~b = ~(a AND b)`. This -//! means we could, in principle, get rid of either of these two to simplify the representation. -//! -//! However, we already apply negative constraints `~test1` and `~test2` to the "branches not -//! taken" in the example above. This means that the tree-representation `~test1 OR ~test2` is much -//! cheaper/shallower than basically creating `~(~(~test1) AND ~(~test2))`. Similarly, if we wanted -//! to get rid of `AND`, we would also have to create additional nodes. So for performance reasons, -//! there is a small "duplication" in the code between those two constraint types. -//! -//! [Kleene]: - -use std::cmp::Ordering; - -use rustc_hash::FxHashMap; - -use crate::semantic_index::{ - ast_ids::HasScopedExpressionId, - constraint::{Constraint, ConstraintNode, PatternConstraintKind}, -}; -use crate::types::{infer_expression_types, Truthiness}; -use crate::Db; - -/// A ternary formula that defines under what conditions a binding is visible. (A ternary formula -/// is just like a boolean formula, but with `Ambiguous` as a third potential result. See the -/// module documentation for more details.) -/// -/// The primitive atoms of the formula are [`Constraint`]s, which express some property of the -/// runtime state of the code that we are analyzing. -/// -/// We assume that each atom has a stable value each time that the formula is evaluated. An atom -/// that resolves to `Ambiguous` might be true or false, and we can't tell which — but within that -/// evaluation, we assume that the atom has the _same_ unknown value each time it appears. That -/// allows us to perform simplifications like `A ∨ !A → true` and `A ∧ !A → false`. -/// -/// That means that when you are constructing a formula, you might need to create distinct atoms -/// for a particular [`Constraint`], if your formula needs to consider how a particular runtime -/// property might be different at different points in the execution of the program. -/// -/// Visibility constraints are normalized, so equivalent constraints are guaranteed to have equal -/// IDs. -#[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] -pub(crate) struct ScopedVisibilityConstraintId(u32); - -impl std::fmt::Debug for ScopedVisibilityConstraintId { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_tuple("ScopedVisibilityConstraintId") - .field(&self.into_node_kind()) - .finish() - } -} - -// Internal details: -// -// There are 3 terminals, with hard-coded constraint IDs: true, ambiguous, and false. -// -// _Atoms_ are the underlying Constraints, which are the variables that are evaluated by the -// ternary function. -// -// _Interior nodes_ provide the TDD structure for the formula. Interior nodes are stored in an -// arena Vec, with the constraint ID providing an index into the arena. - -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -enum NodeKind { - AlwaysTrue, - Ambiguous, - AlwaysFalse, - Interior(u32), -} - -#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] -struct InteriorNode { - atom: Atom, - if_true: ScopedVisibilityConstraintId, - if_ambiguous: ScopedVisibilityConstraintId, - if_false: ScopedVisibilityConstraintId, -} - -#[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] -struct Atom(u32); - -impl Atom { - fn into_index_and_copy(self) -> (u32, u8) { - let copy = self.0 >> 24; - let index = self.0 & 0x00ff_ffff; - (index, copy as u8) - } - - fn from_index_and_copy(index: u32, copy: u8) -> Self { - debug_assert!(index <= 0x00ff_ffff); - Self((u32::from(copy) << 24) | index) - } -} - -impl std::fmt::Debug for Atom { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - let (index, copy) = self.into_index_and_copy(); - f.debug_tuple("Atom").field(&index).field(©).finish() - } -} - -impl ScopedVisibilityConstraintId { - /// A special ID that is used for an "always true" / "always visible" constraint. - pub(crate) const ALWAYS_TRUE: ScopedVisibilityConstraintId = - ScopedVisibilityConstraintId(0xffff_ffff); - - /// A special ID that is used for an ambiguous constraint. - pub(crate) const AMBIGUOUS: ScopedVisibilityConstraintId = - ScopedVisibilityConstraintId(0xffff_fffe); - - /// A special ID that is used for an "always false" / "never visible" constraint. - pub(crate) const ALWAYS_FALSE: ScopedVisibilityConstraintId = - ScopedVisibilityConstraintId(0xffff_fffd); - - fn into_node_kind(self) -> NodeKind { - if self == Self::ALWAYS_TRUE { - NodeKind::AlwaysTrue - } else if self == Self::AMBIGUOUS { - NodeKind::Ambiguous - } else if self == Self::ALWAYS_FALSE { - NodeKind::AlwaysFalse - } else { - NodeKind::Interior(self.0) - } - } -} - -impl From for ScopedVisibilityConstraintId { - fn from(kind: NodeKind) -> ScopedVisibilityConstraintId { - match kind { - NodeKind::AlwaysTrue => Self::ALWAYS_TRUE, - NodeKind::Ambiguous => Self::AMBIGUOUS, - NodeKind::AlwaysFalse => Self::ALWAYS_FALSE, - NodeKind::Interior(index) => { - // We have verified elsewhere that index is within the expected range - Self(index) - } - } - } -} - -#[derive(Debug, PartialEq, Eq)] -pub(crate) struct VisibilityConstraints<'db> { - constraints: Vec>, - interiors: Vec, -} - -#[derive(Debug, Default, PartialEq, Eq)] -pub(crate) struct VisibilityConstraintsBuilder<'db> { - constraints: Vec>, - interiors: Vec, - constraint_cache: FxHashMap, u32>, - interior_cache: FxHashMap, - not_cache: FxHashMap, - and_cache: FxHashMap< - (ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), - ScopedVisibilityConstraintId, - >, - or_cache: FxHashMap< - (ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), - ScopedVisibilityConstraintId, - >, -} - -impl<'db> VisibilityConstraintsBuilder<'db> { - pub(crate) fn build(self) -> VisibilityConstraints<'db> { - VisibilityConstraints { - constraints: self.constraints, - interiors: self.interiors, - } - } - - /// Adds a constraint, ensuring that we only store any particular constraint once. - #[allow(clippy::cast_possible_truncation)] - fn add_constraint(&mut self, constraint: Constraint<'db>, copy: u8) -> Atom { - let index = *self.constraint_cache.entry(constraint).or_insert_with(|| { - let index = self.constraints.len() as u32; - self.constraints.push(constraint); - index - }); - Atom::from_index_and_copy(index, copy) - } - - /// Adds an interior node, ensuring that we always use the same visibility constraint ID for - /// equal nodes. - #[allow(clippy::cast_possible_truncation)] - fn add_interior(&mut self, node: InteriorNode) -> ScopedVisibilityConstraintId { - // Reduce! - if node.if_true == node.if_ambiguous && node.if_true == node.if_false { - return node.if_true; - } - - let index = *self.interior_cache.entry(node).or_insert_with(|| { - let index = self.interiors.len() as u32; - self.interiors.push(node); - index - }); - debug_assert!(index < 0x8000_0000); - NodeKind::Interior(index).into() - } - - pub(crate) fn add_atom( - &mut self, - constraint: Constraint<'db>, - copy: u8, - ) -> ScopedVisibilityConstraintId { - let atom = self.add_constraint(constraint, copy); - self.add_interior(InteriorNode { - atom, - if_true: ScopedVisibilityConstraintId::ALWAYS_TRUE, - if_ambiguous: ScopedVisibilityConstraintId::AMBIGUOUS, - if_false: ScopedVisibilityConstraintId::ALWAYS_FALSE, - }) - } - - pub(crate) fn add_not_constraint( - &mut self, - a: ScopedVisibilityConstraintId, - ) -> ScopedVisibilityConstraintId { - if let Some(cached) = self.not_cache.get(&a) { - return *cached; - } - let a_node = match a.into_node_kind() { - NodeKind::AlwaysTrue => return ScopedVisibilityConstraintId::ALWAYS_FALSE, - NodeKind::Ambiguous => return ScopedVisibilityConstraintId::AMBIGUOUS, - NodeKind::AlwaysFalse => return ScopedVisibilityConstraintId::ALWAYS_TRUE, - NodeKind::Interior(index) => self.interiors[index as usize], - }; - let if_true = self.add_not_constraint(a_node.if_true); - let if_ambiguous = self.add_not_constraint(a_node.if_ambiguous); - let if_false = self.add_not_constraint(a_node.if_false); - let result = self.add_interior(InteriorNode { - atom: a_node.atom, - if_true, - if_ambiguous, - if_false, - }); - self.not_cache.insert(a, result); - result - } - - pub(crate) fn add_or_constraint( - &mut self, - a: ScopedVisibilityConstraintId, - b: ScopedVisibilityConstraintId, - ) -> ScopedVisibilityConstraintId { - if let Some(cached) = self.or_cache.get(&(a, b)) { - return *cached; - } - - let (atom, if_true, if_ambiguous, if_false) = match (a.into_node_kind(), b.into_node_kind()) - { - (NodeKind::AlwaysTrue, _) | (_, NodeKind::AlwaysTrue) => { - return ScopedVisibilityConstraintId::ALWAYS_TRUE - } - (NodeKind::AlwaysFalse, _) => return b, - (_, NodeKind::AlwaysFalse) => return a, - (NodeKind::Ambiguous, NodeKind::Ambiguous) => { - return ScopedVisibilityConstraintId::AMBIGUOUS - } - - (NodeKind::Ambiguous, NodeKind::Interior(b_index)) => { - let b_node = self.interiors[b_index as usize]; - ( - b_node.atom, - self.add_or_constraint(a, b_node.if_true), - self.add_or_constraint(a, b_node.if_ambiguous), - self.add_or_constraint(a, b_node.if_false), - ) - } - (NodeKind::Interior(a_index), NodeKind::Ambiguous) => { - let a_node = self.interiors[a_index as usize]; - ( - a_node.atom, - self.add_or_constraint(a_node.if_true, b), - self.add_or_constraint(a_node.if_ambiguous, b), - self.add_or_constraint(a_node.if_false, b), - ) - } - - (NodeKind::Interior(a_index), NodeKind::Interior(b_index)) => { - let a_node = self.interiors[a_index as usize]; - let b_node = self.interiors[b_index as usize]; - match a_node.atom.cmp(&b_node.atom) { - Ordering::Equal => ( - a_node.atom, - self.add_or_constraint(a_node.if_true, b_node.if_true), - self.add_or_constraint(a_node.if_ambiguous, b_node.if_ambiguous), - self.add_or_constraint(a_node.if_false, b_node.if_false), - ), - Ordering::Less => ( - a_node.atom, - self.add_or_constraint(a_node.if_true, b), - self.add_or_constraint(a_node.if_ambiguous, b), - self.add_or_constraint(a_node.if_false, b), - ), - Ordering::Greater => ( - b_node.atom, - self.add_or_constraint(a, b_node.if_true), - self.add_or_constraint(a, b_node.if_ambiguous), - self.add_or_constraint(a, b_node.if_false), - ), - } - } - }; - - let result = self.add_interior(InteriorNode { - atom, - if_true, - if_ambiguous, - if_false, - }); - self.or_cache.insert((a, b), result); - result - } - - pub(crate) fn add_and_constraint( - &mut self, - a: ScopedVisibilityConstraintId, - b: ScopedVisibilityConstraintId, - ) -> ScopedVisibilityConstraintId { - if let Some(cached) = self.and_cache.get(&(a, b)) { - return *cached; - } - - let (atom, if_true, if_ambiguous, if_false) = match (a.into_node_kind(), b.into_node_kind()) - { - (NodeKind::AlwaysFalse, _) | (_, NodeKind::AlwaysFalse) => { - return ScopedVisibilityConstraintId::ALWAYS_FALSE - } - (NodeKind::AlwaysTrue, _) => return b, - (_, NodeKind::AlwaysTrue) => return a, - (NodeKind::Ambiguous, NodeKind::Ambiguous) => { - return ScopedVisibilityConstraintId::AMBIGUOUS - } - - (NodeKind::Ambiguous, NodeKind::Interior(b_index)) => { - let b_node = self.interiors[b_index as usize]; - ( - b_node.atom, - self.add_and_constraint(a, b_node.if_true), - self.add_and_constraint(a, b_node.if_ambiguous), - self.add_and_constraint(a, b_node.if_false), - ) - } - (NodeKind::Interior(a_index), NodeKind::Ambiguous) => { - let a_node = self.interiors[a_index as usize]; - ( - a_node.atom, - self.add_and_constraint(a_node.if_true, b), - self.add_and_constraint(a_node.if_ambiguous, b), - self.add_and_constraint(a_node.if_false, b), - ) - } - - (NodeKind::Interior(a_index), NodeKind::Interior(b_index)) => { - let a_node = self.interiors[a_index as usize]; - let b_node = self.interiors[b_index as usize]; - match a_node.atom.cmp(&b_node.atom) { - Ordering::Equal => ( - a_node.atom, - self.add_and_constraint(a_node.if_true, b_node.if_true), - self.add_and_constraint(a_node.if_ambiguous, b_node.if_ambiguous), - self.add_and_constraint(a_node.if_false, b_node.if_false), - ), - Ordering::Less => ( - a_node.atom, - self.add_and_constraint(a_node.if_true, b), - self.add_and_constraint(a_node.if_ambiguous, b), - self.add_and_constraint(a_node.if_false, b), - ), - Ordering::Greater => ( - b_node.atom, - self.add_and_constraint(a, b_node.if_true), - self.add_and_constraint(a, b_node.if_ambiguous), - self.add_and_constraint(a, b_node.if_false), - ), - } - } - }; - - let result = self.add_interior(InteriorNode { - atom, - if_true, - if_ambiguous, - if_false, - }); - self.and_cache.insert((a, b), result); - result - } -} - -impl<'db> VisibilityConstraints<'db> { - /// Analyze the statically known visibility for a given visibility constraint. - pub(crate) fn evaluate(&self, db: &'db dyn Db, id: ScopedVisibilityConstraintId) -> Truthiness { - let node = match id.into_node_kind() { - NodeKind::AlwaysTrue => return Truthiness::AlwaysTrue, - NodeKind::Ambiguous => return Truthiness::Ambiguous, - NodeKind::AlwaysFalse => return Truthiness::AlwaysFalse, - NodeKind::Interior(index) => self.interiors[index as usize], - }; - let (index, _) = node.atom.into_index_and_copy(); - let constraint = &self.constraints[index as usize]; - match Self::analyze_single(db, constraint) { - Truthiness::AlwaysTrue => self.evaluate(db, node.if_true), - Truthiness::Ambiguous => self.evaluate(db, node.if_ambiguous), - Truthiness::AlwaysFalse => self.evaluate(db, node.if_false), - } - } - - fn analyze_single(db: &dyn Db, constraint: &Constraint) -> Truthiness { - match constraint.node { - ConstraintNode::Expression(test_expr) => { - let inference = infer_expression_types(db, test_expr); - let scope = test_expr.scope(db); - let ty = inference - .expression_type(test_expr.node_ref(db).scoped_expression_id(db, scope)); - - ty.bool(db).negate_if(!constraint.is_positive) - } - ConstraintNode::Pattern(inner) => match inner.kind(db) { - PatternConstraintKind::Value(value, guard) => { - let subject_expression = inner.subject(db); - let inference = infer_expression_types(db, *subject_expression); - let scope = subject_expression.scope(db); - let subject_ty = inference.expression_type( - subject_expression - .node_ref(db) - .scoped_expression_id(db, scope), - ); - - let inference = infer_expression_types(db, *value); - let scope = value.scope(db); - let value_ty = inference - .expression_type(value.node_ref(db).scoped_expression_id(db, scope)); - - if subject_ty.is_single_valued(db) { - let truthiness = - Truthiness::from(subject_ty.is_equivalent_to(db, value_ty)); - - if truthiness.is_always_true() && guard.is_some() { - // Fall back to ambiguous, the guard might change the result. - Truthiness::Ambiguous - } else { - truthiness - } - } else { - Truthiness::Ambiguous - } - } - PatternConstraintKind::Singleton(..) - | PatternConstraintKind::Class(..) - | PatternConstraintKind::Unsupported => Truthiness::Ambiguous, - }, - } - } -} diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 6c0126e50dd91..8e5d46f9a6869 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -150,7 +150,9 @@ //! //! [Kleene]: -use ruff_index::{newtype_index, IndexVec}; +use std::cmp::Ordering; + +use rustc_hash::FxHashMap; use crate::semantic_index::{ ast_ids::HasScopedExpressionId, @@ -159,14 +161,6 @@ use crate::semantic_index::{ use crate::types::{infer_expression_types, Truthiness}; use crate::Db; -/// The maximum depth of recursion when evaluating visibility constraints. -/// -/// This is a performance optimization that prevents us from descending deeply in case of -/// pathological cases. The actual limit here has been derived from performance testing on -/// the `black` codebase. When increasing the limit beyond 32, we see a 5x runtime increase -/// resulting from a few files with a lot of boolean expressions and `if`-statements. -const MAX_RECURSION_DEPTH: usize = 24; - /// A ternary formula that defines under what conditions a binding is visible. (A ternary formula /// is just like a boolean formula, but with `Ambiguous` as a third potential result. See the /// module documentation for more details.) @@ -182,75 +176,167 @@ const MAX_RECURSION_DEPTH: usize = 24; /// That means that when you are constructing a formula, you might need to create distinct atoms /// for a particular [`Constraint`], if your formula needs to consider how a particular runtime /// property might be different at different points in the execution of the program. -#[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) struct VisibilityConstraint<'db>(VisibilityConstraintInner<'db>); +/// +/// Visibility constraints are normalized, so equivalent constraints are guaranteed to have equal +/// IDs. +#[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] +pub(crate) struct ScopedVisibilityConstraintId(u32); + +impl std::fmt::Debug for ScopedVisibilityConstraintId { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_tuple("ScopedVisibilityConstraintId") + .field(&self.into_node_kind()) + .finish() + } +} -#[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) enum VisibilityConstraintInner<'db> { +// Internal details: +// +// There are 3 terminals, with hard-coded constraint IDs: true, ambiguous, and false. +// +// _Atoms_ are the underlying Constraints, which are the variables that are evaluated by the +// ternary function. +// +// _Interior nodes_ provide the TDD structure for the formula. Interior nodes are stored in an +// arena Vec, with the constraint ID providing an index into the arena. + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +enum NodeKind { AlwaysTrue, - AlwaysFalse, Ambiguous, - VisibleIf(Constraint<'db>, u8), - VisibleIfNot(ScopedVisibilityConstraintId), - KleeneAnd(ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), - KleeneOr(ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), + AlwaysFalse, + Interior(u32), +} + +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +struct InteriorNode { + atom: Atom, + if_true: ScopedVisibilityConstraintId, + if_ambiguous: ScopedVisibilityConstraintId, + if_false: ScopedVisibilityConstraintId, } -/// A newtype-index for a visibility constraint in a particular scope. -#[newtype_index] -pub(crate) struct ScopedVisibilityConstraintId; +#[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] +struct Atom(u32); + +impl Atom { + fn into_index_and_copy(self) -> (u32, u8) { + let copy = self.0 >> 24; + let index = self.0 & 0x00ff_ffff; + (index, copy as u8) + } + + fn from_index_and_copy(index: u32, copy: u8) -> Self { + debug_assert!(index <= 0x00ff_ffff); + Self((u32::from(copy) << 24) | index) + } +} + +impl std::fmt::Debug for Atom { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let (index, copy) = self.into_index_and_copy(); + f.debug_tuple("Atom").field(&index).field(©).finish() + } +} impl ScopedVisibilityConstraintId { /// A special ID that is used for an "always true" / "always visible" constraint. - /// When we create a new [`VisibilityConstraints`] object, this constraint is always - /// present at index 0. pub(crate) const ALWAYS_TRUE: ScopedVisibilityConstraintId = - ScopedVisibilityConstraintId::from_u32(0); + ScopedVisibilityConstraintId(0xffff_ffff); + + /// A special ID that is used for an ambiguous constraint. + pub(crate) const AMBIGUOUS: ScopedVisibilityConstraintId = + ScopedVisibilityConstraintId(0xffff_fffe); /// A special ID that is used for an "always false" / "never visible" constraint. - /// When we create a new [`VisibilityConstraints`] object, this constraint is always - /// present at index 1. pub(crate) const ALWAYS_FALSE: ScopedVisibilityConstraintId = - ScopedVisibilityConstraintId::from_u32(1); + ScopedVisibilityConstraintId(0xffff_fffd); + + fn into_node_kind(self) -> NodeKind { + if self == Self::ALWAYS_TRUE { + NodeKind::AlwaysTrue + } else if self == Self::AMBIGUOUS { + NodeKind::Ambiguous + } else if self == Self::ALWAYS_FALSE { + NodeKind::AlwaysFalse + } else { + NodeKind::Interior(self.0) + } + } +} - /// A special ID that is used for an ambiguous constraint. - /// When we create a new [`VisibilityConstraints`] object, this constraint is always - /// present at index 2. - pub(crate) const AMBIGUOUS: ScopedVisibilityConstraintId = - ScopedVisibilityConstraintId::from_u32(2); +impl From for ScopedVisibilityConstraintId { + fn from(kind: NodeKind) -> ScopedVisibilityConstraintId { + match kind { + NodeKind::AlwaysTrue => Self::ALWAYS_TRUE, + NodeKind::Ambiguous => Self::AMBIGUOUS, + NodeKind::AlwaysFalse => Self::ALWAYS_FALSE, + NodeKind::Interior(index) => { + // We have verified elsewhere that index is within the expected range + Self(index) + } + } + } } #[derive(Debug, PartialEq, Eq)] pub(crate) struct VisibilityConstraints<'db> { - constraints: IndexVec>, + constraints: Vec>, + interiors: Vec, } -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, Default, PartialEq, Eq)] pub(crate) struct VisibilityConstraintsBuilder<'db> { - constraints: IndexVec>, -} - -impl Default for VisibilityConstraintsBuilder<'_> { - fn default() -> Self { - Self { - constraints: IndexVec::from_iter([ - VisibilityConstraint(VisibilityConstraintInner::AlwaysTrue), - VisibilityConstraint(VisibilityConstraintInner::AlwaysFalse), - VisibilityConstraint(VisibilityConstraintInner::Ambiguous), - ]), - } - } + constraints: Vec>, + interiors: Vec, + constraint_cache: FxHashMap, u32>, + interior_cache: FxHashMap, + not_cache: FxHashMap, + and_cache: FxHashMap< + (ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), + ScopedVisibilityConstraintId, + >, + or_cache: FxHashMap< + (ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), + ScopedVisibilityConstraintId, + >, } impl<'db> VisibilityConstraintsBuilder<'db> { pub(crate) fn build(self) -> VisibilityConstraints<'db> { VisibilityConstraints { constraints: self.constraints, + interiors: self.interiors, } } - fn add(&mut self, constraint: VisibilityConstraintInner<'db>) -> ScopedVisibilityConstraintId { - self.constraints.push(VisibilityConstraint(constraint)) + /// Adds a constraint, ensuring that we only store any particular constraint once. + #[allow(clippy::cast_possible_truncation)] + fn add_constraint(&mut self, constraint: Constraint<'db>, copy: u8) -> Atom { + let index = *self.constraint_cache.entry(constraint).or_insert_with(|| { + let index = self.constraints.len() as u32; + self.constraints.push(constraint); + index + }); + Atom::from_index_and_copy(index, copy) + } + + /// Adds an interior node, ensuring that we always use the same visibility constraint ID for + /// equal nodes. + #[allow(clippy::cast_possible_truncation)] + fn add_interior(&mut self, node: InteriorNode) -> ScopedVisibilityConstraintId { + // Reduce! + if node.if_true == node.if_ambiguous && node.if_true == node.if_false { + return node.if_true; + } + + let index = *self.interior_cache.entry(node).or_insert_with(|| { + let index = self.interiors.len() as u32; + self.interiors.push(node); + index + }); + debug_assert!(index < 0x8000_0000); + NodeKind::Interior(index).into() } pub(crate) fn add_atom( @@ -258,22 +344,39 @@ impl<'db> VisibilityConstraintsBuilder<'db> { constraint: Constraint<'db>, copy: u8, ) -> ScopedVisibilityConstraintId { - self.add(VisibilityConstraintInner::VisibleIf(constraint, copy)) + let atom = self.add_constraint(constraint, copy); + self.add_interior(InteriorNode { + atom, + if_true: ScopedVisibilityConstraintId::ALWAYS_TRUE, + if_ambiguous: ScopedVisibilityConstraintId::AMBIGUOUS, + if_false: ScopedVisibilityConstraintId::ALWAYS_FALSE, + }) } pub(crate) fn add_not_constraint( &mut self, a: ScopedVisibilityConstraintId, ) -> ScopedVisibilityConstraintId { - if a == ScopedVisibilityConstraintId::ALWAYS_FALSE { - ScopedVisibilityConstraintId::ALWAYS_TRUE - } else if a == ScopedVisibilityConstraintId::ALWAYS_TRUE { - ScopedVisibilityConstraintId::ALWAYS_FALSE - } else if a == ScopedVisibilityConstraintId::AMBIGUOUS { - ScopedVisibilityConstraintId::AMBIGUOUS - } else { - self.add(VisibilityConstraintInner::VisibleIfNot(a)) + if let Some(cached) = self.not_cache.get(&a) { + return *cached; } + let a_node = match a.into_node_kind() { + NodeKind::AlwaysTrue => return ScopedVisibilityConstraintId::ALWAYS_FALSE, + NodeKind::Ambiguous => return ScopedVisibilityConstraintId::AMBIGUOUS, + NodeKind::AlwaysFalse => return ScopedVisibilityConstraintId::ALWAYS_TRUE, + NodeKind::Interior(index) => self.interiors[index as usize], + }; + let if_true = self.add_not_constraint(a_node.if_true); + let if_ambiguous = self.add_not_constraint(a_node.if_ambiguous); + let if_false = self.add_not_constraint(a_node.if_false); + let result = self.add_interior(InteriorNode { + atom: a_node.atom, + if_true, + if_ambiguous, + if_false, + }); + self.not_cache.insert(a, result); + result } pub(crate) fn add_or_constraint( @@ -281,24 +384,74 @@ impl<'db> VisibilityConstraintsBuilder<'db> { a: ScopedVisibilityConstraintId, b: ScopedVisibilityConstraintId, ) -> ScopedVisibilityConstraintId { - if a == ScopedVisibilityConstraintId::ALWAYS_TRUE - || b == ScopedVisibilityConstraintId::ALWAYS_TRUE - { - return ScopedVisibilityConstraintId::ALWAYS_TRUE; - } else if a == ScopedVisibilityConstraintId::ALWAYS_FALSE { - return b; - } else if b == ScopedVisibilityConstraintId::ALWAYS_FALSE { - return a; + if let Some(cached) = self.or_cache.get(&(a, b)) { + return *cached; } - match (&self.constraints[a], &self.constraints[b]) { - (_, VisibilityConstraint(VisibilityConstraintInner::VisibleIfNot(id))) if a == *id => { - ScopedVisibilityConstraintId::ALWAYS_TRUE + + let (atom, if_true, if_ambiguous, if_false) = match (a.into_node_kind(), b.into_node_kind()) + { + (NodeKind::AlwaysTrue, _) | (_, NodeKind::AlwaysTrue) => { + return ScopedVisibilityConstraintId::ALWAYS_TRUE } - (VisibilityConstraint(VisibilityConstraintInner::VisibleIfNot(id)), _) if *id == b => { - ScopedVisibilityConstraintId::ALWAYS_TRUE + (NodeKind::AlwaysFalse, _) => return b, + (_, NodeKind::AlwaysFalse) => return a, + (NodeKind::Ambiguous, NodeKind::Ambiguous) => { + return ScopedVisibilityConstraintId::AMBIGUOUS } - _ => self.add(VisibilityConstraintInner::KleeneOr(a, b)), - } + + (NodeKind::Ambiguous, NodeKind::Interior(b_index)) => { + let b_node = self.interiors[b_index as usize]; + ( + b_node.atom, + self.add_or_constraint(a, b_node.if_true), + self.add_or_constraint(a, b_node.if_ambiguous), + self.add_or_constraint(a, b_node.if_false), + ) + } + (NodeKind::Interior(a_index), NodeKind::Ambiguous) => { + let a_node = self.interiors[a_index as usize]; + ( + a_node.atom, + self.add_or_constraint(a_node.if_true, b), + self.add_or_constraint(a_node.if_ambiguous, b), + self.add_or_constraint(a_node.if_false, b), + ) + } + + (NodeKind::Interior(a_index), NodeKind::Interior(b_index)) => { + let a_node = self.interiors[a_index as usize]; + let b_node = self.interiors[b_index as usize]; + match a_node.atom.cmp(&b_node.atom) { + Ordering::Equal => ( + a_node.atom, + self.add_or_constraint(a_node.if_true, b_node.if_true), + self.add_or_constraint(a_node.if_ambiguous, b_node.if_ambiguous), + self.add_or_constraint(a_node.if_false, b_node.if_false), + ), + Ordering::Less => ( + a_node.atom, + self.add_or_constraint(a_node.if_true, b), + self.add_or_constraint(a_node.if_ambiguous, b), + self.add_or_constraint(a_node.if_false, b), + ), + Ordering::Greater => ( + b_node.atom, + self.add_or_constraint(a, b_node.if_true), + self.add_or_constraint(a, b_node.if_ambiguous), + self.add_or_constraint(a, b_node.if_false), + ), + } + } + }; + + let result = self.add_interior(InteriorNode { + atom, + if_true, + if_ambiguous, + if_false, + }); + self.or_cache.insert((a, b), result); + result } pub(crate) fn add_and_constraint( @@ -306,88 +459,92 @@ impl<'db> VisibilityConstraintsBuilder<'db> { a: ScopedVisibilityConstraintId, b: ScopedVisibilityConstraintId, ) -> ScopedVisibilityConstraintId { - if a == ScopedVisibilityConstraintId::ALWAYS_FALSE - || b == ScopedVisibilityConstraintId::ALWAYS_FALSE - { - return ScopedVisibilityConstraintId::ALWAYS_FALSE; - } else if a == ScopedVisibilityConstraintId::ALWAYS_TRUE { - return b; - } else if b == ScopedVisibilityConstraintId::ALWAYS_TRUE { - return a; + if let Some(cached) = self.and_cache.get(&(a, b)) { + return *cached; } - match (&self.constraints[a], &self.constraints[b]) { - (_, VisibilityConstraint(VisibilityConstraintInner::VisibleIfNot(id))) if a == *id => { - ScopedVisibilityConstraintId::ALWAYS_FALSE + + let (atom, if_true, if_ambiguous, if_false) = match (a.into_node_kind(), b.into_node_kind()) + { + (NodeKind::AlwaysFalse, _) | (_, NodeKind::AlwaysFalse) => { + return ScopedVisibilityConstraintId::ALWAYS_FALSE } - (VisibilityConstraint(VisibilityConstraintInner::VisibleIfNot(id)), _) if *id == b => { - ScopedVisibilityConstraintId::ALWAYS_FALSE + (NodeKind::AlwaysTrue, _) => return b, + (_, NodeKind::AlwaysTrue) => return a, + (NodeKind::Ambiguous, NodeKind::Ambiguous) => { + return ScopedVisibilityConstraintId::AMBIGUOUS } - _ => self.add(VisibilityConstraintInner::KleeneAnd(a, b)), - } - } -} - -impl<'db> VisibilityConstraints<'db> { - /// Analyze the statically known visibility for a given visibility constraint. - pub(crate) fn evaluate(&self, db: &'db dyn Db, id: ScopedVisibilityConstraintId) -> Truthiness { - self.evaluate_impl(db, id, MAX_RECURSION_DEPTH) - } - - fn evaluate_impl( - &self, - db: &'db dyn Db, - id: ScopedVisibilityConstraintId, - max_depth: usize, - ) -> Truthiness { - if max_depth == 0 { - return Truthiness::Ambiguous; - } - let VisibilityConstraint(visibility_constraint) = &self.constraints[id]; - match visibility_constraint { - VisibilityConstraintInner::AlwaysTrue => Truthiness::AlwaysTrue, - VisibilityConstraintInner::AlwaysFalse => Truthiness::AlwaysFalse, - VisibilityConstraintInner::Ambiguous => Truthiness::Ambiguous, - VisibilityConstraintInner::VisibleIf(constraint, _) => { - Self::analyze_single(db, constraint) + (NodeKind::Ambiguous, NodeKind::Interior(b_index)) => { + let b_node = self.interiors[b_index as usize]; + ( + b_node.atom, + self.add_and_constraint(a, b_node.if_true), + self.add_and_constraint(a, b_node.if_ambiguous), + self.add_and_constraint(a, b_node.if_false), + ) } - VisibilityConstraintInner::VisibleIfNot(negated) => { - self.evaluate_impl(db, *negated, max_depth - 1).negate() + (NodeKind::Interior(a_index), NodeKind::Ambiguous) => { + let a_node = self.interiors[a_index as usize]; + ( + a_node.atom, + self.add_and_constraint(a_node.if_true, b), + self.add_and_constraint(a_node.if_ambiguous, b), + self.add_and_constraint(a_node.if_false, b), + ) } - VisibilityConstraintInner::KleeneAnd(lhs, rhs) => { - let lhs = self.evaluate_impl(db, *lhs, max_depth - 1); - if lhs == Truthiness::AlwaysFalse { - return Truthiness::AlwaysFalse; - } - - let rhs = self.evaluate_impl(db, *rhs, max_depth - 1); - - if rhs == Truthiness::AlwaysFalse { - Truthiness::AlwaysFalse - } else if lhs == Truthiness::AlwaysTrue && rhs == Truthiness::AlwaysTrue { - Truthiness::AlwaysTrue - } else { - Truthiness::Ambiguous + (NodeKind::Interior(a_index), NodeKind::Interior(b_index)) => { + let a_node = self.interiors[a_index as usize]; + let b_node = self.interiors[b_index as usize]; + match a_node.atom.cmp(&b_node.atom) { + Ordering::Equal => ( + a_node.atom, + self.add_and_constraint(a_node.if_true, b_node.if_true), + self.add_and_constraint(a_node.if_ambiguous, b_node.if_ambiguous), + self.add_and_constraint(a_node.if_false, b_node.if_false), + ), + Ordering::Less => ( + a_node.atom, + self.add_and_constraint(a_node.if_true, b), + self.add_and_constraint(a_node.if_ambiguous, b), + self.add_and_constraint(a_node.if_false, b), + ), + Ordering::Greater => ( + b_node.atom, + self.add_and_constraint(a, b_node.if_true), + self.add_and_constraint(a, b_node.if_ambiguous), + self.add_and_constraint(a, b_node.if_false), + ), } } - VisibilityConstraintInner::KleeneOr(lhs_id, rhs_id) => { - let lhs = self.evaluate_impl(db, *lhs_id, max_depth - 1); - - if lhs == Truthiness::AlwaysTrue { - return Truthiness::AlwaysTrue; - } - - let rhs = self.evaluate_impl(db, *rhs_id, max_depth - 1); + }; + + let result = self.add_interior(InteriorNode { + atom, + if_true, + if_ambiguous, + if_false, + }); + self.and_cache.insert((a, b), result); + result + } +} - if rhs == Truthiness::AlwaysTrue { - Truthiness::AlwaysTrue - } else if lhs == Truthiness::AlwaysFalse && rhs == Truthiness::AlwaysFalse { - Truthiness::AlwaysFalse - } else { - Truthiness::Ambiguous - } - } +impl<'db> VisibilityConstraints<'db> { + /// Analyze the statically known visibility for a given visibility constraint. + pub(crate) fn evaluate(&self, db: &'db dyn Db, id: ScopedVisibilityConstraintId) -> Truthiness { + let node = match id.into_node_kind() { + NodeKind::AlwaysTrue => return Truthiness::AlwaysTrue, + NodeKind::Ambiguous => return Truthiness::Ambiguous, + NodeKind::AlwaysFalse => return Truthiness::AlwaysFalse, + NodeKind::Interior(index) => self.interiors[index as usize], + }; + let (index, _) = node.atom.into_index_and_copy(); + let constraint = &self.constraints[index as usize]; + match Self::analyze_single(db, constraint) { + Truthiness::AlwaysTrue => self.evaluate(db, node.if_true), + Truthiness::Ambiguous => self.evaluate(db, node.if_ambiguous), + Truthiness::AlwaysFalse => self.evaluate(db, node.if_false), } } From 5b6bce522802543db26d16f6509a99a0b59da008 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Fri, 31 Jan 2025 20:15:08 -0500 Subject: [PATCH 03/27] Remove NodeKind --- .../src/visibility_constraints.rs | 243 ++++++++---------- 1 file changed, 108 insertions(+), 135 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 8e5d46f9a6869..2f4fbb712959d 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -179,14 +179,18 @@ use crate::Db; /// /// Visibility constraints are normalized, so equivalent constraints are guaranteed to have equal /// IDs. -#[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] +#[derive(Clone, Copy, Eq, Hash, PartialEq)] pub(crate) struct ScopedVisibilityConstraintId(u32); impl std::fmt::Debug for ScopedVisibilityConstraintId { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_tuple("ScopedVisibilityConstraintId") - .field(&self.into_node_kind()) - .finish() + let mut f = f.debug_tuple("ScopedVisibilityConstraintId"); + match *self { + ALWAYS_TRUE => f.field(&format_args!("AlwaysTrue")).finish(), + AMBIGUOUS => f.field(&format_args!("Ambiguous")).finish(), + ALWAYS_FALSE => f.field(&format_args!("AlwaysFalse")).finish(), + _ => f.field(&self.0).finish(), + } } } @@ -200,14 +204,6 @@ impl std::fmt::Debug for ScopedVisibilityConstraintId { // _Interior nodes_ provide the TDD structure for the formula. Interior nodes are stored in an // arena Vec, with the constraint ID providing an index into the arena. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] -enum NodeKind { - AlwaysTrue, - Ambiguous, - AlwaysFalse, - Interior(u32), -} - #[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] struct InteriorNode { atom: Atom, @@ -252,32 +248,15 @@ impl ScopedVisibilityConstraintId { pub(crate) const ALWAYS_FALSE: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId(0xffff_fffd); - fn into_node_kind(self) -> NodeKind { - if self == Self::ALWAYS_TRUE { - NodeKind::AlwaysTrue - } else if self == Self::AMBIGUOUS { - NodeKind::Ambiguous - } else if self == Self::ALWAYS_FALSE { - NodeKind::AlwaysFalse - } else { - NodeKind::Interior(self.0) - } + fn is_terminal(self) -> bool { + self.0 >= SMALLEST_TERMINAL } } -impl From for ScopedVisibilityConstraintId { - fn from(kind: NodeKind) -> ScopedVisibilityConstraintId { - match kind { - NodeKind::AlwaysTrue => Self::ALWAYS_TRUE, - NodeKind::Ambiguous => Self::AMBIGUOUS, - NodeKind::AlwaysFalse => Self::ALWAYS_FALSE, - NodeKind::Interior(index) => { - // We have verified elsewhere that index is within the expected range - Self(index) - } - } - } -} +const ALWAYS_TRUE: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::ALWAYS_TRUE; +const AMBIGUOUS: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::AMBIGUOUS; +const ALWAYS_FALSE: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::ALWAYS_FALSE; +const SMALLEST_TERMINAL: u32 = ALWAYS_FALSE.0; #[derive(Debug, PartialEq, Eq)] pub(crate) struct VisibilityConstraints<'db> { @@ -310,6 +289,29 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } } + fn interior_node(&self, a: ScopedVisibilityConstraintId) -> InteriorNode { + debug_assert!(!a.is_terminal()); + self.interiors[a.0 as usize] + } + + /// Returns whether `a` or `b` has a "larger" atom. Terminals are considered to have a larger + /// atom than any internal node. + fn cmp_atoms( + &self, + a: ScopedVisibilityConstraintId, + b: ScopedVisibilityConstraintId, + ) -> Ordering { + if a == b { + Ordering::Equal + } else if a.is_terminal() { + Ordering::Greater + } else if b.is_terminal() { + Ordering::Less + } else { + self.interior_node(a).atom.cmp(&self.interior_node(b).atom) + } + } + /// Adds a constraint, ensuring that we only store any particular constraint once. #[allow(clippy::cast_possible_truncation)] fn add_constraint(&mut self, constraint: Constraint<'db>, copy: u8) -> Atom { @@ -335,8 +337,8 @@ impl<'db> VisibilityConstraintsBuilder<'db> { self.interiors.push(node); index }); - debug_assert!(index < 0x8000_0000); - NodeKind::Interior(index).into() + debug_assert!(index < SMALLEST_TERMINAL); + ScopedVisibilityConstraintId(index) } pub(crate) fn add_atom( @@ -347,9 +349,9 @@ impl<'db> VisibilityConstraintsBuilder<'db> { let atom = self.add_constraint(constraint, copy); self.add_interior(InteriorNode { atom, - if_true: ScopedVisibilityConstraintId::ALWAYS_TRUE, - if_ambiguous: ScopedVisibilityConstraintId::AMBIGUOUS, - if_false: ScopedVisibilityConstraintId::ALWAYS_FALSE, + if_true: ALWAYS_TRUE, + if_ambiguous: AMBIGUOUS, + if_false: ALWAYS_FALSE, }) } @@ -360,12 +362,14 @@ impl<'db> VisibilityConstraintsBuilder<'db> { if let Some(cached) = self.not_cache.get(&a) { return *cached; } - let a_node = match a.into_node_kind() { - NodeKind::AlwaysTrue => return ScopedVisibilityConstraintId::ALWAYS_FALSE, - NodeKind::Ambiguous => return ScopedVisibilityConstraintId::AMBIGUOUS, - NodeKind::AlwaysFalse => return ScopedVisibilityConstraintId::ALWAYS_TRUE, - NodeKind::Interior(index) => self.interiors[index as usize], - }; + if a == ALWAYS_TRUE { + return ALWAYS_FALSE; + } else if a == AMBIGUOUS { + return AMBIGUOUS; + } else if a == ALWAYS_FALSE { + return ALWAYS_TRUE; + } + let a_node = self.interiors[a.0 as usize]; let if_true = self.add_not_constraint(a_node.if_true); let if_ambiguous = self.add_not_constraint(a_node.if_ambiguous); let if_false = self.add_not_constraint(a_node.if_false); @@ -388,28 +392,26 @@ impl<'db> VisibilityConstraintsBuilder<'db> { return *cached; } - let (atom, if_true, if_ambiguous, if_false) = match (a.into_node_kind(), b.into_node_kind()) - { - (NodeKind::AlwaysTrue, _) | (_, NodeKind::AlwaysTrue) => { - return ScopedVisibilityConstraintId::ALWAYS_TRUE - } - (NodeKind::AlwaysFalse, _) => return b, - (_, NodeKind::AlwaysFalse) => return a, - (NodeKind::Ambiguous, NodeKind::Ambiguous) => { - return ScopedVisibilityConstraintId::AMBIGUOUS - } + match (a, b) { + (ALWAYS_TRUE, _) | (_, ALWAYS_TRUE) => return ALWAYS_TRUE, + (ALWAYS_FALSE, other) | (other, ALWAYS_FALSE) => return other, + (AMBIGUOUS, AMBIGUOUS) => return AMBIGUOUS, + _ => {} + } - (NodeKind::Ambiguous, NodeKind::Interior(b_index)) => { - let b_node = self.interiors[b_index as usize]; + let (atom, if_true, if_ambiguous, if_false) = match self.cmp_atoms(a, b) { + Ordering::Equal => { + let a_node = self.interior_node(a); + let b_node = self.interior_node(b); ( - b_node.atom, - self.add_or_constraint(a, b_node.if_true), - self.add_or_constraint(a, b_node.if_ambiguous), - self.add_or_constraint(a, b_node.if_false), + a_node.atom, + self.add_or_constraint(a_node.if_true, b_node.if_true), + self.add_or_constraint(a_node.if_ambiguous, b_node.if_ambiguous), + self.add_or_constraint(a_node.if_false, b_node.if_false), ) } - (NodeKind::Interior(a_index), NodeKind::Ambiguous) => { - let a_node = self.interiors[a_index as usize]; + Ordering::Less => { + let a_node = self.interior_node(a); ( a_node.atom, self.add_or_constraint(a_node.if_true, b), @@ -417,30 +419,14 @@ impl<'db> VisibilityConstraintsBuilder<'db> { self.add_or_constraint(a_node.if_false, b), ) } - - (NodeKind::Interior(a_index), NodeKind::Interior(b_index)) => { - let a_node = self.interiors[a_index as usize]; - let b_node = self.interiors[b_index as usize]; - match a_node.atom.cmp(&b_node.atom) { - Ordering::Equal => ( - a_node.atom, - self.add_or_constraint(a_node.if_true, b_node.if_true), - self.add_or_constraint(a_node.if_ambiguous, b_node.if_ambiguous), - self.add_or_constraint(a_node.if_false, b_node.if_false), - ), - Ordering::Less => ( - a_node.atom, - self.add_or_constraint(a_node.if_true, b), - self.add_or_constraint(a_node.if_ambiguous, b), - self.add_or_constraint(a_node.if_false, b), - ), - Ordering::Greater => ( - b_node.atom, - self.add_or_constraint(a, b_node.if_true), - self.add_or_constraint(a, b_node.if_ambiguous), - self.add_or_constraint(a, b_node.if_false), - ), - } + Ordering::Greater => { + let b_node = self.interior_node(b); + ( + b_node.atom, + self.add_or_constraint(a, b_node.if_true), + self.add_or_constraint(a, b_node.if_ambiguous), + self.add_or_constraint(a, b_node.if_false), + ) } }; @@ -463,28 +449,26 @@ impl<'db> VisibilityConstraintsBuilder<'db> { return *cached; } - let (atom, if_true, if_ambiguous, if_false) = match (a.into_node_kind(), b.into_node_kind()) - { - (NodeKind::AlwaysFalse, _) | (_, NodeKind::AlwaysFalse) => { - return ScopedVisibilityConstraintId::ALWAYS_FALSE - } - (NodeKind::AlwaysTrue, _) => return b, - (_, NodeKind::AlwaysTrue) => return a, - (NodeKind::Ambiguous, NodeKind::Ambiguous) => { - return ScopedVisibilityConstraintId::AMBIGUOUS - } + match (a, b) { + (ALWAYS_FALSE, _) | (_, ALWAYS_FALSE) => return ALWAYS_FALSE, + (ALWAYS_TRUE, other) | (other, ALWAYS_TRUE) => return other, + (AMBIGUOUS, AMBIGUOUS) => return AMBIGUOUS, + _ => {} + } - (NodeKind::Ambiguous, NodeKind::Interior(b_index)) => { - let b_node = self.interiors[b_index as usize]; + let (atom, if_true, if_ambiguous, if_false) = match self.cmp_atoms(a, b) { + Ordering::Equal => { + let a_node = self.interior_node(a); + let b_node = self.interior_node(b); ( - b_node.atom, - self.add_and_constraint(a, b_node.if_true), - self.add_and_constraint(a, b_node.if_ambiguous), - self.add_and_constraint(a, b_node.if_false), + a_node.atom, + self.add_and_constraint(a_node.if_true, b_node.if_true), + self.add_and_constraint(a_node.if_ambiguous, b_node.if_ambiguous), + self.add_and_constraint(a_node.if_false, b_node.if_false), ) } - (NodeKind::Interior(a_index), NodeKind::Ambiguous) => { - let a_node = self.interiors[a_index as usize]; + Ordering::Less => { + let a_node = self.interior_node(a); ( a_node.atom, self.add_and_constraint(a_node.if_true, b), @@ -492,30 +476,14 @@ impl<'db> VisibilityConstraintsBuilder<'db> { self.add_and_constraint(a_node.if_false, b), ) } - - (NodeKind::Interior(a_index), NodeKind::Interior(b_index)) => { - let a_node = self.interiors[a_index as usize]; - let b_node = self.interiors[b_index as usize]; - match a_node.atom.cmp(&b_node.atom) { - Ordering::Equal => ( - a_node.atom, - self.add_and_constraint(a_node.if_true, b_node.if_true), - self.add_and_constraint(a_node.if_ambiguous, b_node.if_ambiguous), - self.add_and_constraint(a_node.if_false, b_node.if_false), - ), - Ordering::Less => ( - a_node.atom, - self.add_and_constraint(a_node.if_true, b), - self.add_and_constraint(a_node.if_ambiguous, b), - self.add_and_constraint(a_node.if_false, b), - ), - Ordering::Greater => ( - b_node.atom, - self.add_and_constraint(a, b_node.if_true), - self.add_and_constraint(a, b_node.if_ambiguous), - self.add_and_constraint(a, b_node.if_false), - ), - } + Ordering::Greater => { + let b_node = self.interior_node(b); + ( + b_node.atom, + self.add_and_constraint(a, b_node.if_true), + self.add_and_constraint(a, b_node.if_ambiguous), + self.add_and_constraint(a, b_node.if_false), + ) } }; @@ -531,13 +499,18 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } impl<'db> VisibilityConstraints<'db> { + fn interior_node(&self, a: ScopedVisibilityConstraintId) -> InteriorNode { + debug_assert!(!a.is_terminal()); + self.interiors[a.0 as usize] + } + /// Analyze the statically known visibility for a given visibility constraint. pub(crate) fn evaluate(&self, db: &'db dyn Db, id: ScopedVisibilityConstraintId) -> Truthiness { - let node = match id.into_node_kind() { - NodeKind::AlwaysTrue => return Truthiness::AlwaysTrue, - NodeKind::Ambiguous => return Truthiness::Ambiguous, - NodeKind::AlwaysFalse => return Truthiness::AlwaysFalse, - NodeKind::Interior(index) => self.interiors[index as usize], + let node = match id { + ALWAYS_TRUE => return Truthiness::AlwaysTrue, + AMBIGUOUS => return Truthiness::Ambiguous, + ALWAYS_FALSE => return Truthiness::AlwaysFalse, + _ => self.interior_node(id), }; let (index, _) = node.atom.into_index_and_copy(); let constraint = &self.constraints[index as usize]; From 48fce2600ca08c98c3af90e419435109e0491633 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Fri, 31 Jan 2025 20:54:09 -0500 Subject: [PATCH 04/27] Check cache second --- .../src/visibility_constraints.rs | 55 +++++++++++-------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 2f4fbb712959d..0c4fc48e727cd 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -359,9 +359,6 @@ impl<'db> VisibilityConstraintsBuilder<'db> { &mut self, a: ScopedVisibilityConstraintId, ) -> ScopedVisibilityConstraintId { - if let Some(cached) = self.not_cache.get(&a) { - return *cached; - } if a == ALWAYS_TRUE { return ALWAYS_FALSE; } else if a == AMBIGUOUS { @@ -369,6 +366,10 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } else if a == ALWAYS_FALSE { return ALWAYS_TRUE; } + + if let Some(cached) = self.not_cache.get(&a) { + return *cached; + } let a_node = self.interiors[a.0 as usize]; let if_true = self.add_not_constraint(a_node.if_true); let if_ambiguous = self.add_not_constraint(a_node.if_ambiguous); @@ -388,10 +389,6 @@ impl<'db> VisibilityConstraintsBuilder<'db> { a: ScopedVisibilityConstraintId, b: ScopedVisibilityConstraintId, ) -> ScopedVisibilityConstraintId { - if let Some(cached) = self.or_cache.get(&(a, b)) { - return *cached; - } - match (a, b) { (ALWAYS_TRUE, _) | (_, ALWAYS_TRUE) => return ALWAYS_TRUE, (ALWAYS_FALSE, other) | (other, ALWAYS_FALSE) => return other, @@ -399,6 +396,10 @@ impl<'db> VisibilityConstraintsBuilder<'db> { _ => {} } + if let Some(cached) = self.or_cache.get(&(a, b)) { + return *cached; + } + let (atom, if_true, if_ambiguous, if_false) = match self.cmp_atoms(a, b) { Ordering::Equal => { let a_node = self.interior_node(a); @@ -445,10 +446,6 @@ impl<'db> VisibilityConstraintsBuilder<'db> { a: ScopedVisibilityConstraintId, b: ScopedVisibilityConstraintId, ) -> ScopedVisibilityConstraintId { - if let Some(cached) = self.and_cache.get(&(a, b)) { - return *cached; - } - match (a, b) { (ALWAYS_FALSE, _) | (_, ALWAYS_FALSE) => return ALWAYS_FALSE, (ALWAYS_TRUE, other) | (other, ALWAYS_TRUE) => return other, @@ -456,6 +453,10 @@ impl<'db> VisibilityConstraintsBuilder<'db> { _ => {} } + if let Some(cached) = self.and_cache.get(&(a, b)) { + return *cached; + } + let (atom, if_true, if_ambiguous, if_false) = match self.cmp_atoms(a, b) { Ordering::Equal => { let a_node = self.interior_node(a); @@ -505,19 +506,25 @@ impl<'db> VisibilityConstraints<'db> { } /// Analyze the statically known visibility for a given visibility constraint. - pub(crate) fn evaluate(&self, db: &'db dyn Db, id: ScopedVisibilityConstraintId) -> Truthiness { - let node = match id { - ALWAYS_TRUE => return Truthiness::AlwaysTrue, - AMBIGUOUS => return Truthiness::Ambiguous, - ALWAYS_FALSE => return Truthiness::AlwaysFalse, - _ => self.interior_node(id), - }; - let (index, _) = node.atom.into_index_and_copy(); - let constraint = &self.constraints[index as usize]; - match Self::analyze_single(db, constraint) { - Truthiness::AlwaysTrue => self.evaluate(db, node.if_true), - Truthiness::Ambiguous => self.evaluate(db, node.if_ambiguous), - Truthiness::AlwaysFalse => self.evaluate(db, node.if_false), + pub(crate) fn evaluate( + &self, + db: &'db dyn Db, + mut id: ScopedVisibilityConstraintId, + ) -> Truthiness { + loop { + let node = match id { + ALWAYS_TRUE => return Truthiness::AlwaysTrue, + AMBIGUOUS => return Truthiness::Ambiguous, + ALWAYS_FALSE => return Truthiness::AlwaysFalse, + _ => self.interior_node(id), + }; + let (index, _) = node.atom.into_index_and_copy(); + let constraint = &self.constraints[index as usize]; + match Self::analyze_single(db, constraint) { + Truthiness::AlwaysTrue => id = node.if_true, + Truthiness::Ambiguous => id = node.if_ambiguous, + Truthiness::AlwaysFalse => id = node.if_false, + } } } From 3e4058c7bca7ea821f3ac1c15e11f69c9fa8cf9a Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 10:12:14 -0500 Subject: [PATCH 05/27] Add additional comments --- .../src/visibility_constraints.rs | 29 +++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 0c4fc48e727cd..9e024fbff324e 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -212,22 +212,33 @@ struct InteriorNode { if_false: ScopedVisibilityConstraintId, } +/// A "variable" that is evaluated as part of a TDD ternary function. For visibility constraints, +/// this is (one of the copies of) a `Constraint` that represents some runtime property of the +/// Python code that we are evaluating. We intern these constraints in an arena +/// ([VisibilityConstraints::constraints]). An atom consists of an index into this arena, and a +/// copy number. #[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] struct Atom(u32); impl Atom { + /// Deconstruct an atom into a constraint index and a copy number. + #[inline(always)] fn into_index_and_copy(self) -> (u32, u8) { let copy = self.0 >> 24; let index = self.0 & 0x00ff_ffff; (index, copy as u8) } + /// Construct an atom from a constraint index and a copy number. + #[inline(always)] fn from_index_and_copy(index: u32, copy: u8) -> Self { debug_assert!(index <= 0x00ff_ffff); Self((u32::from(copy) << 24) | index) } } +// A custom Debug implementation that prints out the constraint index and copy number as distinct +// fields. impl std::fmt::Debug for Atom { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let (index, copy) = self.into_index_and_copy(); @@ -253,11 +264,14 @@ impl ScopedVisibilityConstraintId { } } +// Rebind some constants locally so that we don't need as many qualifiers below. const ALWAYS_TRUE: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::ALWAYS_TRUE; const AMBIGUOUS: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::AMBIGUOUS; const ALWAYS_FALSE: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::ALWAYS_FALSE; const SMALLEST_TERMINAL: u32 = ALWAYS_FALSE.0; +/// A collection of visibility constraints. This is currently stored in `UseDefMap`, which means we +/// maintain a separate set of visibility constraints for each scope in file. #[derive(Debug, PartialEq, Eq)] pub(crate) struct VisibilityConstraints<'db> { constraints: Vec>, @@ -294,8 +308,9 @@ impl<'db> VisibilityConstraintsBuilder<'db> { self.interiors[a.0 as usize] } - /// Returns whether `a` or `b` has a "larger" atom. Terminals are considered to have a larger - /// atom than any internal node. + /// Returns whether `a` or `b` has a "larger" atom. TDDs are ordered such that interior nodes + /// can only have edges to "larger" nodes. Terminals are considered to have a larger atom than + /// any internal node, since they are leaf nodes. fn cmp_atoms( &self, a: ScopedVisibilityConstraintId, @@ -341,6 +356,9 @@ impl<'db> VisibilityConstraintsBuilder<'db> { ScopedVisibilityConstraintId(index) } + /// Adds a new visibility constraint that checks a single [`Constraint`]. Provide different + /// values for `copy` if you need to model that the constraint can evaluate to different + /// results at different points in the execution of the program being modeled. pub(crate) fn add_atom( &mut self, constraint: Constraint<'db>, @@ -355,6 +373,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { }) } + /// Adds a new visibility constraint that is the ternary NOT of an existing one. pub(crate) fn add_not_constraint( &mut self, a: ScopedVisibilityConstraintId, @@ -384,6 +403,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { result } + /// Adds a new visibility constraint that is the ternary OR of two existing ones. pub(crate) fn add_or_constraint( &mut self, a: ScopedVisibilityConstraintId, @@ -396,6 +416,8 @@ impl<'db> VisibilityConstraintsBuilder<'db> { _ => {} } + // OR is commutative, which lets us halve the cache requirements + let (a, b) = if b.0 < a.0 { (b, a) } else { (a, b) }; if let Some(cached) = self.or_cache.get(&(a, b)) { return *cached; } @@ -441,6 +463,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { result } + /// Adds a new visibility constraint that is the ternary AND of two existing ones. pub(crate) fn add_and_constraint( &mut self, a: ScopedVisibilityConstraintId, @@ -453,6 +476,8 @@ impl<'db> VisibilityConstraintsBuilder<'db> { _ => {} } + // AND is commutative, which lets us halve the cache requirements + let (a, b) = if b.0 < a.0 { (b, a) } else { (a, b) }; if let Some(cached) = self.and_cache.get(&(a, b)) { return *cached; } From eaec3770e225ec5608095f4600117edeff9c2203 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 10:29:44 -0500 Subject: [PATCH 06/27] Remove recursion depth limit test --- .../mdtest/statically_known_branches.md | 32 ------------------- 1 file changed, 32 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/statically_known_branches.md b/crates/red_knot_python_semantic/resources/mdtest/statically_known_branches.md index c2f77b26a7e89..8734a5897347c 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/statically_known_branches.md +++ b/crates/red_knot_python_semantic/resources/mdtest/statically_known_branches.md @@ -1489,38 +1489,6 @@ if True: from module import symbol ``` -## Known limitations - -We currently have a limitation in the complexity (depth) of the visibility constraints that are -supported. This is to avoid pathological cases that would require us to recurse deeply. - -TODO: We don't! Remove this! - -```py -x = 1 - -False or False or False or False or \ - False or False or False or False or \ - False or False or False or False or \ - False or False or False or False or \ - False or False or False or False or \ - False or False or (x := 2) # fmt: skip - -# This still works fine: -reveal_type(x) # revealed: Literal[2] - -y = 1 - -False or False or False or False or \ - False or False or False or False or \ - False or False or False or False or \ - False or False or False or False or \ - False or False or False or False or \ - False or False or False or (y := 2) # fmt: skip - -reveal_type(y) # revealed: Literal[2] -``` - ## Unsupported features We do not support full unreachable code analysis yet. We also raise diagnostics from From dd03d79d48c23fb637f424a7889c4acc7203b077 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 10:49:35 -0500 Subject: [PATCH 07/27] clippy --- crates/red_knot_python_semantic/src/visibility_constraints.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 9e024fbff324e..2cde70a5a20d6 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -215,13 +215,14 @@ struct InteriorNode { /// A "variable" that is evaluated as part of a TDD ternary function. For visibility constraints, /// this is (one of the copies of) a `Constraint` that represents some runtime property of the /// Python code that we are evaluating. We intern these constraints in an arena -/// ([VisibilityConstraints::constraints]). An atom consists of an index into this arena, and a +/// ([`VisibilityConstraints::constraints`]). An atom consists of an index into this arena, and a /// copy number. #[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] struct Atom(u32); impl Atom { /// Deconstruct an atom into a constraint index and a copy number. + #[allow(clippy::inline_always)] #[inline(always)] fn into_index_and_copy(self) -> (u32, u8) { let copy = self.0 >> 24; @@ -230,6 +231,7 @@ impl Atom { } /// Construct an atom from a constraint index and a copy number. + #[allow(clippy::inline_always)] #[inline(always)] fn from_index_and_copy(index: u32, copy: u8) -> Self { debug_assert!(index <= 0x00ff_ffff); From 2c2aba4f98ab8d80307284cf63ab773e31d2cfd2 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 13:17:54 -0500 Subject: [PATCH 08/27] easy on the inline --- .../red_knot_python_semantic/src/visibility_constraints.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 2cde70a5a20d6..8a737cf6106de 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -222,8 +222,7 @@ struct Atom(u32); impl Atom { /// Deconstruct an atom into a constraint index and a copy number. - #[allow(clippy::inline_always)] - #[inline(always)] + #[inline] fn into_index_and_copy(self) -> (u32, u8) { let copy = self.0 >> 24; let index = self.0 & 0x00ff_ffff; @@ -231,8 +230,7 @@ impl Atom { } /// Construct an atom from a constraint index and a copy number. - #[allow(clippy::inline_always)] - #[inline(always)] + #[inline] fn from_index_and_copy(index: u32, copy: u8) -> Self { debug_assert!(index <= 0x00ff_ffff); Self((u32::from(copy) << 24) | index) From f76824c340fac8657882106ef93818982e6946ed Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 13:49:18 -0500 Subject: [PATCH 09/27] Reduce C && !C to false (etc) correctly --- .../red_knot_python_semantic/src/visibility_constraints.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 8a737cf6106de..bcb5cf21498ba 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -342,8 +342,9 @@ impl<'db> VisibilityConstraintsBuilder<'db> { /// equal nodes. #[allow(clippy::cast_possible_truncation)] fn add_interior(&mut self, node: InteriorNode) -> ScopedVisibilityConstraintId { - // Reduce! - if node.if_true == node.if_ambiguous && node.if_true == node.if_false { + // If the true and false branches lead to the same node, we can override the ambiguous + // branch to go there too. And this node is then redundant and can be reduced. + if node.if_true == node.if_false { return node.if_true; } From 2c98b82ef2fc82fc539df62b24962e8260c21491 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 13:58:58 -0500 Subject: [PATCH 10/27] Remove simplify calls!!! --- .../resources/mdtest/terminal_statements.md | 3 +- .../src/semantic_index/builder.rs | 24 ++----------- .../src/semantic_index/use_def.rs | 34 ------------------- .../semantic_index/use_def/symbol_state.rs | 10 ------ 4 files changed, 4 insertions(+), 67 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md b/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md index 6a3427e211c84..5888847b84f7d 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md +++ b/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md @@ -635,6 +635,5 @@ def _(cond: bool): if True: return - # TODO: Literal["a"] - reveal_type(x) # revealed: Literal["a", "b"] + reveal_type(x) # revealed: Literal["a"] ``` diff --git a/crates/red_knot_python_semantic/src/semantic_index/builder.rs b/crates/red_knot_python_semantic/src/semantic_index/builder.rs index ca75b22379ac3..9dac8ef56583d 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -413,13 +413,6 @@ impl<'db> SemanticIndexBuilder<'db> { .record_visibility_constraint(ScopedVisibilityConstraintId::AMBIGUOUS); } - /// Simplifies (resets) visibility constraints on all live bindings and declarations that did - /// not see any new definitions since the given snapshot. - fn simplify_visibility_constraints(&mut self, snapshot: FlowSnapshot) { - self.current_use_def_map_mut() - .simplify_visibility_constraints(snapshot); - } - fn push_assignment(&mut self, assignment: CurrentAssignment<'db>) { self.current_assignments.push(assignment); } @@ -1073,8 +1066,6 @@ where for post_clause_state in post_clauses { self.flow_merge(post_clause_state); } - - self.simplify_visibility_constraints(no_branch_taken); } ast::Stmt::While(ast::StmtWhile { test, @@ -1130,7 +1121,7 @@ where // To model this correctly, we need two copies of the while condition constraint, // since the first and later evaluations might produce different results. let post_body = self.flow_snapshot(); - self.flow_restore(pre_loop.clone()); + self.flow_restore(pre_loop); self.record_negated_visibility_constraint(first_vis_constraint_id); self.flow_merge(post_body); self.record_negated_constraint(constraint); @@ -1145,8 +1136,6 @@ where self.record_visibility_constraint_id(body_vis_constraint_id); self.flow_merge(snapshot); } - - self.simplify_visibility_constraints(pre_loop); } ast::Stmt::With(ast::StmtWith { items, @@ -1291,7 +1280,7 @@ where .is_some_and(|case| case.guard.is_none() && case.pattern.is_wildcard()) { post_case_snapshots.push(self.flow_snapshot()); - self.flow_restore(after_subject.clone()); + self.flow_restore(after_subject); for id in &vis_constraints { self.record_negated_visibility_constraint(*id); @@ -1301,8 +1290,6 @@ where for post_clause_state in post_case_snapshots { self.flow_merge(post_clause_state); } - - self.simplify_visibility_constraints(after_subject); } ast::Stmt::Try(ast::StmtTry { body, @@ -1583,13 +1570,12 @@ where self.visit_expr(body); let visibility_constraint = self.record_visibility_constraint(constraint); let post_body = self.flow_snapshot(); - self.flow_restore(pre_if.clone()); + self.flow_restore(pre_if); self.record_negated_constraint(constraint); self.visit_expr(orelse); self.record_negated_visibility_constraint(visibility_constraint); self.flow_merge(post_body); - self.simplify_visibility_constraints(pre_if); } ast::Expr::ListComp( list_comprehension @ ast::ExprListComp { @@ -1646,8 +1632,6 @@ where range: _, op, }) => { - let pre_op = self.flow_snapshot(); - let mut snapshots = vec![]; let mut visibility_constraints = vec![]; @@ -1694,8 +1678,6 @@ where for snapshot in snapshots { self.flow_merge(snapshot); } - - self.simplify_visibility_constraints(pre_op); } _ => { walk_expr(self, expr); diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs index 861a4c3132bd4..f8d4ae8f23f6a 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs @@ -575,40 +575,6 @@ impl<'db> UseDefMapBuilder<'db> { .add_and_constraint(self.scope_start_visibility, constraint); } - /// This method resets the visibility constraints for all symbols to a previous state - /// *if* there have been no new declarations or bindings since then. Consider the - /// following example: - /// ```py - /// x = 0 - /// y = 0 - /// if test_a: - /// y = 1 - /// elif test_b: - /// y = 2 - /// elif test_c: - /// y = 3 - /// - /// # RESET - /// ``` - /// We build a complex visibility constraint for the `y = 0` binding. We build the same - /// constraint for the `x = 0` binding as well, but at the `RESET` point, we can get rid - /// of it, as the `if`-`elif`-`elif` chain doesn't include any new bindings of `x`. - pub(super) fn simplify_visibility_constraints(&mut self, snapshot: FlowSnapshot) { - debug_assert!(self.symbol_states.len() >= snapshot.symbol_states.len()); - - self.scope_start_visibility = snapshot.scope_start_visibility; - - // Note that this loop terminates when we reach a symbol not present in the snapshot. - // This means we keep visibility constraints for all new symbols, which is intended, - // since these symbols have been introduced in the corresponding branch, which might - // be subject to visibility constraints. We only simplify/reset visibility constraints - // for symbols that have the same bindings and declarations present compared to the - // snapshot. - for (current, snapshot) in self.symbol_states.iter_mut().zip(snapshot.symbol_states) { - current.simplify_visibility_constraints(snapshot); - } - } - pub(super) fn record_declaration( &mut self, symbol: ScopedSymbolId, diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs index 4c0c72bc6226a..ce9c577a267f9 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs @@ -371,16 +371,6 @@ impl SymbolState { .record_visibility_constraint(visibility_constraints, constraint); } - pub(super) fn simplify_visibility_constraints(&mut self, snapshot_state: SymbolState) { - if self.bindings.live_bindings == snapshot_state.bindings.live_bindings { - self.bindings.visibility_constraints = snapshot_state.bindings.visibility_constraints; - } - if self.declarations.live_declarations == snapshot_state.declarations.live_declarations { - self.declarations.visibility_constraints = - snapshot_state.declarations.visibility_constraints; - } - } - /// Record a newly-encountered declaration of this symbol. pub(super) fn record_declaration(&mut self, declaration_id: ScopedDefinitionId) { self.declarations.record_declaration(declaration_id); From 0fa10158638a9b6565f5428f4aba1c51d64e413a Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 14:16:38 -0500 Subject: [PATCH 11/27] Skip calculating ambiguous branch if it's not needed --- .../src/visibility_constraints.rs | 84 +++++++++++-------- 1 file changed, 48 insertions(+), 36 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index bcb5cf21498ba..fa6a19d0e549e 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -427,30 +427,36 @@ impl<'db> VisibilityConstraintsBuilder<'db> { Ordering::Equal => { let a_node = self.interior_node(a); let b_node = self.interior_node(b); - ( - a_node.atom, - self.add_or_constraint(a_node.if_true, b_node.if_true), - self.add_or_constraint(a_node.if_ambiguous, b_node.if_ambiguous), - self.add_or_constraint(a_node.if_false, b_node.if_false), - ) + let if_true = self.add_or_constraint(a_node.if_true, b_node.if_true); + let if_false = self.add_or_constraint(a_node.if_false, b_node.if_false); + let if_ambiguous = if if_true == if_false { + if_true + } else { + self.add_or_constraint(a_node.if_ambiguous, b_node.if_ambiguous) + }; + (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Less => { let a_node = self.interior_node(a); - ( - a_node.atom, - self.add_or_constraint(a_node.if_true, b), - self.add_or_constraint(a_node.if_ambiguous, b), - self.add_or_constraint(a_node.if_false, b), - ) + let if_true = self.add_or_constraint(a_node.if_true, b); + let if_false = self.add_or_constraint(a_node.if_false, b); + let if_ambiguous = if if_true == if_false { + if_true + } else { + self.add_or_constraint(a_node.if_ambiguous, b) + }; + (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Greater => { let b_node = self.interior_node(b); - ( - b_node.atom, - self.add_or_constraint(a, b_node.if_true), - self.add_or_constraint(a, b_node.if_ambiguous), - self.add_or_constraint(a, b_node.if_false), - ) + let if_true = self.add_or_constraint(a, b_node.if_true); + let if_false = self.add_or_constraint(a, b_node.if_false); + let if_ambiguous = if if_true == if_false { + if_true + } else { + self.add_or_constraint(a, b_node.if_ambiguous) + }; + (b_node.atom, if_true, if_ambiguous, if_false) } }; @@ -487,30 +493,36 @@ impl<'db> VisibilityConstraintsBuilder<'db> { Ordering::Equal => { let a_node = self.interior_node(a); let b_node = self.interior_node(b); - ( - a_node.atom, - self.add_and_constraint(a_node.if_true, b_node.if_true), - self.add_and_constraint(a_node.if_ambiguous, b_node.if_ambiguous), - self.add_and_constraint(a_node.if_false, b_node.if_false), - ) + let if_true = self.add_and_constraint(a_node.if_true, b_node.if_true); + let if_false = self.add_and_constraint(a_node.if_false, b_node.if_false); + let if_ambiguous = if if_true == if_false { + if_true + } else { + self.add_and_constraint(a_node.if_ambiguous, b_node.if_ambiguous) + }; + (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Less => { let a_node = self.interior_node(a); - ( - a_node.atom, - self.add_and_constraint(a_node.if_true, b), - self.add_and_constraint(a_node.if_ambiguous, b), - self.add_and_constraint(a_node.if_false, b), - ) + let if_true = self.add_and_constraint(a_node.if_true, b); + let if_false = self.add_and_constraint(a_node.if_false, b); + let if_ambiguous = if if_true == if_false { + if_true + } else { + self.add_and_constraint(a_node.if_ambiguous, b) + }; + (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Greater => { let b_node = self.interior_node(b); - ( - b_node.atom, - self.add_and_constraint(a, b_node.if_true), - self.add_and_constraint(a, b_node.if_ambiguous), - self.add_and_constraint(a, b_node.if_false), - ) + let if_true = self.add_and_constraint(a, b_node.if_true); + let if_false = self.add_and_constraint(a, b_node.if_false); + let if_ambiguous = if if_true == if_false { + if_true + } else { + self.add_and_constraint(a, b_node.if_ambiguous) + }; + (b_node.atom, if_true, if_ambiguous, if_false) } }; From 6fb24c6cbe0c1b60a9d99da3884c1eb51a7e875e Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 14:19:53 -0500 Subject: [PATCH 12/27] Revert "Remove simplify calls!!!" This reverts commit 2f74a5153079c8dd0c9a8327f84a705a7b705ba7. --- .../resources/mdtest/terminal_statements.md | 3 +- .../src/semantic_index/builder.rs | 24 +++++++++++-- .../src/semantic_index/use_def.rs | 34 +++++++++++++++++++ .../semantic_index/use_def/symbol_state.rs | 10 ++++++ 4 files changed, 67 insertions(+), 4 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md b/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md index 5888847b84f7d..6a3427e211c84 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md +++ b/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md @@ -635,5 +635,6 @@ def _(cond: bool): if True: return - reveal_type(x) # revealed: Literal["a"] + # TODO: Literal["a"] + reveal_type(x) # revealed: Literal["a", "b"] ``` diff --git a/crates/red_knot_python_semantic/src/semantic_index/builder.rs b/crates/red_knot_python_semantic/src/semantic_index/builder.rs index 9dac8ef56583d..ca75b22379ac3 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -413,6 +413,13 @@ impl<'db> SemanticIndexBuilder<'db> { .record_visibility_constraint(ScopedVisibilityConstraintId::AMBIGUOUS); } + /// Simplifies (resets) visibility constraints on all live bindings and declarations that did + /// not see any new definitions since the given snapshot. + fn simplify_visibility_constraints(&mut self, snapshot: FlowSnapshot) { + self.current_use_def_map_mut() + .simplify_visibility_constraints(snapshot); + } + fn push_assignment(&mut self, assignment: CurrentAssignment<'db>) { self.current_assignments.push(assignment); } @@ -1066,6 +1073,8 @@ where for post_clause_state in post_clauses { self.flow_merge(post_clause_state); } + + self.simplify_visibility_constraints(no_branch_taken); } ast::Stmt::While(ast::StmtWhile { test, @@ -1121,7 +1130,7 @@ where // To model this correctly, we need two copies of the while condition constraint, // since the first and later evaluations might produce different results. let post_body = self.flow_snapshot(); - self.flow_restore(pre_loop); + self.flow_restore(pre_loop.clone()); self.record_negated_visibility_constraint(first_vis_constraint_id); self.flow_merge(post_body); self.record_negated_constraint(constraint); @@ -1136,6 +1145,8 @@ where self.record_visibility_constraint_id(body_vis_constraint_id); self.flow_merge(snapshot); } + + self.simplify_visibility_constraints(pre_loop); } ast::Stmt::With(ast::StmtWith { items, @@ -1280,7 +1291,7 @@ where .is_some_and(|case| case.guard.is_none() && case.pattern.is_wildcard()) { post_case_snapshots.push(self.flow_snapshot()); - self.flow_restore(after_subject); + self.flow_restore(after_subject.clone()); for id in &vis_constraints { self.record_negated_visibility_constraint(*id); @@ -1290,6 +1301,8 @@ where for post_clause_state in post_case_snapshots { self.flow_merge(post_clause_state); } + + self.simplify_visibility_constraints(after_subject); } ast::Stmt::Try(ast::StmtTry { body, @@ -1570,12 +1583,13 @@ where self.visit_expr(body); let visibility_constraint = self.record_visibility_constraint(constraint); let post_body = self.flow_snapshot(); - self.flow_restore(pre_if); + self.flow_restore(pre_if.clone()); self.record_negated_constraint(constraint); self.visit_expr(orelse); self.record_negated_visibility_constraint(visibility_constraint); self.flow_merge(post_body); + self.simplify_visibility_constraints(pre_if); } ast::Expr::ListComp( list_comprehension @ ast::ExprListComp { @@ -1632,6 +1646,8 @@ where range: _, op, }) => { + let pre_op = self.flow_snapshot(); + let mut snapshots = vec![]; let mut visibility_constraints = vec![]; @@ -1678,6 +1694,8 @@ where for snapshot in snapshots { self.flow_merge(snapshot); } + + self.simplify_visibility_constraints(pre_op); } _ => { walk_expr(self, expr); diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs index f8d4ae8f23f6a..861a4c3132bd4 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs @@ -575,6 +575,40 @@ impl<'db> UseDefMapBuilder<'db> { .add_and_constraint(self.scope_start_visibility, constraint); } + /// This method resets the visibility constraints for all symbols to a previous state + /// *if* there have been no new declarations or bindings since then. Consider the + /// following example: + /// ```py + /// x = 0 + /// y = 0 + /// if test_a: + /// y = 1 + /// elif test_b: + /// y = 2 + /// elif test_c: + /// y = 3 + /// + /// # RESET + /// ``` + /// We build a complex visibility constraint for the `y = 0` binding. We build the same + /// constraint for the `x = 0` binding as well, but at the `RESET` point, we can get rid + /// of it, as the `if`-`elif`-`elif` chain doesn't include any new bindings of `x`. + pub(super) fn simplify_visibility_constraints(&mut self, snapshot: FlowSnapshot) { + debug_assert!(self.symbol_states.len() >= snapshot.symbol_states.len()); + + self.scope_start_visibility = snapshot.scope_start_visibility; + + // Note that this loop terminates when we reach a symbol not present in the snapshot. + // This means we keep visibility constraints for all new symbols, which is intended, + // since these symbols have been introduced in the corresponding branch, which might + // be subject to visibility constraints. We only simplify/reset visibility constraints + // for symbols that have the same bindings and declarations present compared to the + // snapshot. + for (current, snapshot) in self.symbol_states.iter_mut().zip(snapshot.symbol_states) { + current.simplify_visibility_constraints(snapshot); + } + } + pub(super) fn record_declaration( &mut self, symbol: ScopedSymbolId, diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs index ce9c577a267f9..4c0c72bc6226a 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs @@ -371,6 +371,16 @@ impl SymbolState { .record_visibility_constraint(visibility_constraints, constraint); } + pub(super) fn simplify_visibility_constraints(&mut self, snapshot_state: SymbolState) { + if self.bindings.live_bindings == snapshot_state.bindings.live_bindings { + self.bindings.visibility_constraints = snapshot_state.bindings.visibility_constraints; + } + if self.declarations.live_declarations == snapshot_state.declarations.live_declarations { + self.declarations.visibility_constraints = + snapshot_state.declarations.visibility_constraints; + } + } + /// Record a newly-encountered declaration of this symbol. pub(super) fn record_declaration(&mut self, declaration_id: ScopedDefinitionId) { self.declarations.record_declaration(declaration_id); From 6e47aa98f8af9031d8208f35cef2e94cfdbbf37a Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 16:25:38 -0500 Subject: [PATCH 13/27] Reapply "Remove simplify calls!!!" This reverts commit 6fb24c6cbe0c1b60a9d99da3884c1eb51a7e875e. --- .../resources/mdtest/terminal_statements.md | 3 +- .../src/semantic_index/builder.rs | 24 ++----------- .../src/semantic_index/use_def.rs | 34 ------------------- .../semantic_index/use_def/symbol_state.rs | 10 ------ 4 files changed, 4 insertions(+), 67 deletions(-) diff --git a/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md b/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md index 6a3427e211c84..5888847b84f7d 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md +++ b/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md @@ -635,6 +635,5 @@ def _(cond: bool): if True: return - # TODO: Literal["a"] - reveal_type(x) # revealed: Literal["a", "b"] + reveal_type(x) # revealed: Literal["a"] ``` diff --git a/crates/red_knot_python_semantic/src/semantic_index/builder.rs b/crates/red_knot_python_semantic/src/semantic_index/builder.rs index ca75b22379ac3..9dac8ef56583d 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -413,13 +413,6 @@ impl<'db> SemanticIndexBuilder<'db> { .record_visibility_constraint(ScopedVisibilityConstraintId::AMBIGUOUS); } - /// Simplifies (resets) visibility constraints on all live bindings and declarations that did - /// not see any new definitions since the given snapshot. - fn simplify_visibility_constraints(&mut self, snapshot: FlowSnapshot) { - self.current_use_def_map_mut() - .simplify_visibility_constraints(snapshot); - } - fn push_assignment(&mut self, assignment: CurrentAssignment<'db>) { self.current_assignments.push(assignment); } @@ -1073,8 +1066,6 @@ where for post_clause_state in post_clauses { self.flow_merge(post_clause_state); } - - self.simplify_visibility_constraints(no_branch_taken); } ast::Stmt::While(ast::StmtWhile { test, @@ -1130,7 +1121,7 @@ where // To model this correctly, we need two copies of the while condition constraint, // since the first and later evaluations might produce different results. let post_body = self.flow_snapshot(); - self.flow_restore(pre_loop.clone()); + self.flow_restore(pre_loop); self.record_negated_visibility_constraint(first_vis_constraint_id); self.flow_merge(post_body); self.record_negated_constraint(constraint); @@ -1145,8 +1136,6 @@ where self.record_visibility_constraint_id(body_vis_constraint_id); self.flow_merge(snapshot); } - - self.simplify_visibility_constraints(pre_loop); } ast::Stmt::With(ast::StmtWith { items, @@ -1291,7 +1280,7 @@ where .is_some_and(|case| case.guard.is_none() && case.pattern.is_wildcard()) { post_case_snapshots.push(self.flow_snapshot()); - self.flow_restore(after_subject.clone()); + self.flow_restore(after_subject); for id in &vis_constraints { self.record_negated_visibility_constraint(*id); @@ -1301,8 +1290,6 @@ where for post_clause_state in post_case_snapshots { self.flow_merge(post_clause_state); } - - self.simplify_visibility_constraints(after_subject); } ast::Stmt::Try(ast::StmtTry { body, @@ -1583,13 +1570,12 @@ where self.visit_expr(body); let visibility_constraint = self.record_visibility_constraint(constraint); let post_body = self.flow_snapshot(); - self.flow_restore(pre_if.clone()); + self.flow_restore(pre_if); self.record_negated_constraint(constraint); self.visit_expr(orelse); self.record_negated_visibility_constraint(visibility_constraint); self.flow_merge(post_body); - self.simplify_visibility_constraints(pre_if); } ast::Expr::ListComp( list_comprehension @ ast::ExprListComp { @@ -1646,8 +1632,6 @@ where range: _, op, }) => { - let pre_op = self.flow_snapshot(); - let mut snapshots = vec![]; let mut visibility_constraints = vec![]; @@ -1694,8 +1678,6 @@ where for snapshot in snapshots { self.flow_merge(snapshot); } - - self.simplify_visibility_constraints(pre_op); } _ => { walk_expr(self, expr); diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs index 861a4c3132bd4..f8d4ae8f23f6a 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs @@ -575,40 +575,6 @@ impl<'db> UseDefMapBuilder<'db> { .add_and_constraint(self.scope_start_visibility, constraint); } - /// This method resets the visibility constraints for all symbols to a previous state - /// *if* there have been no new declarations or bindings since then. Consider the - /// following example: - /// ```py - /// x = 0 - /// y = 0 - /// if test_a: - /// y = 1 - /// elif test_b: - /// y = 2 - /// elif test_c: - /// y = 3 - /// - /// # RESET - /// ``` - /// We build a complex visibility constraint for the `y = 0` binding. We build the same - /// constraint for the `x = 0` binding as well, but at the `RESET` point, we can get rid - /// of it, as the `if`-`elif`-`elif` chain doesn't include any new bindings of `x`. - pub(super) fn simplify_visibility_constraints(&mut self, snapshot: FlowSnapshot) { - debug_assert!(self.symbol_states.len() >= snapshot.symbol_states.len()); - - self.scope_start_visibility = snapshot.scope_start_visibility; - - // Note that this loop terminates when we reach a symbol not present in the snapshot. - // This means we keep visibility constraints for all new symbols, which is intended, - // since these symbols have been introduced in the corresponding branch, which might - // be subject to visibility constraints. We only simplify/reset visibility constraints - // for symbols that have the same bindings and declarations present compared to the - // snapshot. - for (current, snapshot) in self.symbol_states.iter_mut().zip(snapshot.symbol_states) { - current.simplify_visibility_constraints(snapshot); - } - } - pub(super) fn record_declaration( &mut self, symbol: ScopedSymbolId, diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs index 4c0c72bc6226a..ce9c577a267f9 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs @@ -371,16 +371,6 @@ impl SymbolState { .record_visibility_constraint(visibility_constraints, constraint); } - pub(super) fn simplify_visibility_constraints(&mut self, snapshot_state: SymbolState) { - if self.bindings.live_bindings == snapshot_state.bindings.live_bindings { - self.bindings.visibility_constraints = snapshot_state.bindings.visibility_constraints; - } - if self.declarations.live_declarations == snapshot_state.declarations.live_declarations { - self.declarations.visibility_constraints = - snapshot_state.declarations.visibility_constraints; - } - } - /// Record a newly-encountered declaration of this symbol. pub(super) fn record_declaration(&mut self, declaration_id: ScopedDefinitionId) { self.declarations.record_declaration(declaration_id); From 61b23359c62422f5e3e497a7945f3b5fde2ec297 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 16:27:10 -0500 Subject: [PATCH 14/27] Add .pyi xfail for RET503 --- crates/red_knot_project/tests/check.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/red_knot_project/tests/check.rs b/crates/red_knot_project/tests/check.rs index 6cdd13f32f16a..711cec75f2d23 100644 --- a/crates/red_knot_project/tests/check.rs +++ b/crates/red_knot_project/tests/check.rs @@ -270,6 +270,8 @@ impl SourceOrderVisitor<'_> for PullTypesVisitor<'_> { /// Whether or not the .py/.pyi version of this file is expected to fail #[rustfmt::skip] const KNOWN_FAILURES: &[(&str, bool, bool)] = &[ + // related to circular references in nested functions + ("crates/ruff_linter/resources/test/fixtures/flake8_return/RET503.py", false, true), // related to circular references in class definitions ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_26.py", true, true), ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_27.py", true, true), From bd5ebf7a27c84bcce1e2a6f6d5ac9899ced8e493 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 17:36:51 -0500 Subject: [PATCH 15/27] Use IndexVec --- .../src/visibility_constraints.rs | 58 +++++++++++-------- 1 file changed, 34 insertions(+), 24 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index fa6a19d0e549e..ab71f05bf7b3b 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -152,6 +152,7 @@ use std::cmp::Ordering; +use ruff_index::{newtype_index, IndexVec}; use rustc_hash::FxHashMap; use crate::semantic_index::{ @@ -212,6 +213,12 @@ struct InteriorNode { if_false: ScopedVisibilityConstraintId, } +#[newtype_index] +struct ConstraintId; + +#[newtype_index] +struct InteriorNodeId; + /// A "variable" that is evaluated as part of a TDD ternary function. For visibility constraints, /// this is (one of the copies of) a `Constraint` that represents some runtime property of the /// Python code that we are evaluating. We intern these constraints in an arena @@ -223,15 +230,16 @@ struct Atom(u32); impl Atom { /// Deconstruct an atom into a constraint index and a copy number. #[inline] - fn into_index_and_copy(self) -> (u32, u8) { + fn into_index_and_copy(self) -> (ConstraintId, u8) { let copy = self.0 >> 24; let index = self.0 & 0x00ff_ffff; - (index, copy as u8) + (index.into(), copy as u8) } /// Construct an atom from a constraint index and a copy number. #[inline] - fn from_index_and_copy(index: u32, copy: u8) -> Self { + fn from_index_and_copy(index: ConstraintId, copy: u8) -> Self { + let index = index.as_u32(); debug_assert!(index <= 0x00ff_ffff); Self((u32::from(copy) << 24) | index) } @@ -242,7 +250,10 @@ impl Atom { impl std::fmt::Debug for Atom { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let (index, copy) = self.into_index_and_copy(); - f.debug_tuple("Atom").field(&index).field(©).finish() + f.debug_tuple("Atom") + .field(&index.as_u32()) + .field(©) + .finish() } } @@ -274,16 +285,16 @@ const SMALLEST_TERMINAL: u32 = ALWAYS_FALSE.0; /// maintain a separate set of visibility constraints for each scope in file. #[derive(Debug, PartialEq, Eq)] pub(crate) struct VisibilityConstraints<'db> { - constraints: Vec>, - interiors: Vec, + constraints: IndexVec>, + interiors: IndexVec, } #[derive(Debug, Default, PartialEq, Eq)] pub(crate) struct VisibilityConstraintsBuilder<'db> { - constraints: Vec>, - interiors: Vec, - constraint_cache: FxHashMap, u32>, - interior_cache: FxHashMap, + constraints: IndexVec>, + interiors: IndexVec, + constraint_cache: FxHashMap, ConstraintId>, + interior_cache: FxHashMap, not_cache: FxHashMap, and_cache: FxHashMap< (ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), @@ -305,7 +316,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { fn interior_node(&self, a: ScopedVisibilityConstraintId) -> InteriorNode { debug_assert!(!a.is_terminal()); - self.interiors[a.0 as usize] + self.interiors[InteriorNodeId::from(a.0)] } /// Returns whether `a` or `b` has a "larger" atom. TDDs are ordered such that interior nodes @@ -330,11 +341,10 @@ impl<'db> VisibilityConstraintsBuilder<'db> { /// Adds a constraint, ensuring that we only store any particular constraint once. #[allow(clippy::cast_possible_truncation)] fn add_constraint(&mut self, constraint: Constraint<'db>, copy: u8) -> Atom { - let index = *self.constraint_cache.entry(constraint).or_insert_with(|| { - let index = self.constraints.len() as u32; - self.constraints.push(constraint); - index - }); + let index = *self + .constraint_cache + .entry(constraint) + .or_insert_with(|| self.constraints.push(constraint)); Atom::from_index_and_copy(index, copy) } @@ -348,11 +358,11 @@ impl<'db> VisibilityConstraintsBuilder<'db> { return node.if_true; } - let index = *self.interior_cache.entry(node).or_insert_with(|| { - let index = self.interiors.len() as u32; - self.interiors.push(node); - index - }); + let index = *self + .interior_cache + .entry(node) + .or_insert_with(|| self.interiors.push(node)); + let index = index.as_u32(); debug_assert!(index < SMALLEST_TERMINAL); ScopedVisibilityConstraintId(index) } @@ -390,7 +400,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { if let Some(cached) = self.not_cache.get(&a) { return *cached; } - let a_node = self.interiors[a.0 as usize]; + let a_node = self.interior_node(a); let if_true = self.add_not_constraint(a_node.if_true); let if_ambiguous = self.add_not_constraint(a_node.if_ambiguous); let if_false = self.add_not_constraint(a_node.if_false); @@ -540,7 +550,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { impl<'db> VisibilityConstraints<'db> { fn interior_node(&self, a: ScopedVisibilityConstraintId) -> InteriorNode { debug_assert!(!a.is_terminal()); - self.interiors[a.0 as usize] + self.interiors[InteriorNodeId::from(a.0)] } /// Analyze the statically known visibility for a given visibility constraint. @@ -557,7 +567,7 @@ impl<'db> VisibilityConstraints<'db> { _ => self.interior_node(id), }; let (index, _) = node.atom.into_index_and_copy(); - let constraint = &self.constraints[index as usize]; + let constraint = &self.constraints[index]; match Self::analyze_single(db, constraint) { Truthiness::AlwaysTrue => id = node.if_true, Truthiness::Ambiguous => id = node.if_ambiguous, From d7f070c2ad8f7161f68d2e051b0baf75cb1e6c56 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 17:43:51 -0500 Subject: [PATCH 16/27] Handle two-terminal case in cmp_atoms --- crates/red_knot_python_semantic/src/visibility_constraints.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index ab71f05bf7b3b..846a629ee5694 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -327,7 +327,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { a: ScopedVisibilityConstraintId, b: ScopedVisibilityConstraintId, ) -> Ordering { - if a == b { + if a == b || (a.is_terminal() && b.is_terminal()) { Ordering::Equal } else if a.is_terminal() { Ordering::Greater From 7eb1f2bb41cdd7fd685af1de4ed0cfd65ff397c7 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 17:45:34 -0500 Subject: [PATCH 17/27] Remove unneeded clippy allows --- crates/red_knot_python_semantic/src/visibility_constraints.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 846a629ee5694..6900d261afb00 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -339,7 +339,6 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } /// Adds a constraint, ensuring that we only store any particular constraint once. - #[allow(clippy::cast_possible_truncation)] fn add_constraint(&mut self, constraint: Constraint<'db>, copy: u8) -> Atom { let index = *self .constraint_cache @@ -350,7 +349,6 @@ impl<'db> VisibilityConstraintsBuilder<'db> { /// Adds an interior node, ensuring that we always use the same visibility constraint ID for /// equal nodes. - #[allow(clippy::cast_possible_truncation)] fn add_interior(&mut self, node: InteriorNode) -> ScopedVisibilityConstraintId { // If the true and false branches lead to the same node, we can override the ambiguous // branch to go there too. And this node is then redundant and can be reduced. From cf6b269c94a42969d715c956e7704c05e5d3c56d Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 19:18:17 -0500 Subject: [PATCH 18/27] Custom Idx impl for Atom --- .../src/visibility_constraints.rs | 56 +++++++++++-------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 6900d261afb00..67d4b2a832be2 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -152,7 +152,7 @@ use std::cmp::Ordering; -use ruff_index::{newtype_index, IndexVec}; +use ruff_index::{newtype_index, Idx, IndexVec}; use rustc_hash::FxHashMap; use crate::semantic_index::{ @@ -213,9 +213,6 @@ struct InteriorNode { if_false: ScopedVisibilityConstraintId, } -#[newtype_index] -struct ConstraintId; - #[newtype_index] struct InteriorNodeId; @@ -230,18 +227,19 @@ struct Atom(u32); impl Atom { /// Deconstruct an atom into a constraint index and a copy number. #[inline] - fn into_index_and_copy(self) -> (ConstraintId, u8) { + fn into_index_and_copy(self) -> (u32, u8) { let copy = self.0 >> 24; let index = self.0 & 0x00ff_ffff; - (index.into(), copy as u8) + (index, copy as u8) } - /// Construct an atom from a constraint index and a copy number. #[inline] - fn from_index_and_copy(index: ConstraintId, copy: u8) -> Self { - let index = index.as_u32(); - debug_assert!(index <= 0x00ff_ffff); - Self((u32::from(copy) << 24) | index) + fn copy_of(mut self, copy: u8) -> Self { + // Clear out the previous copy number + self.0 &= 0x00ff_ffff; + // OR in the new one + self.0 |= u32::from(copy) << 24; + self } } @@ -250,10 +248,22 @@ impl Atom { impl std::fmt::Debug for Atom { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let (index, copy) = self.into_index_and_copy(); - f.debug_tuple("Atom") - .field(&index.as_u32()) - .field(©) - .finish() + f.debug_tuple("Atom").field(&index).field(©).finish() + } +} + +impl Idx for Atom { + #[inline] + fn new(value: usize) -> Self { + assert!(value <= 0x00ff_ffff); + #[allow(clippy::cast_possible_truncation)] + Self(value as u32) + } + + #[inline] + fn index(self) -> usize { + let (index, _) = self.into_index_and_copy(); + index as usize } } @@ -285,15 +295,15 @@ const SMALLEST_TERMINAL: u32 = ALWAYS_FALSE.0; /// maintain a separate set of visibility constraints for each scope in file. #[derive(Debug, PartialEq, Eq)] pub(crate) struct VisibilityConstraints<'db> { - constraints: IndexVec>, + constraints: IndexVec>, interiors: IndexVec, } #[derive(Debug, Default, PartialEq, Eq)] pub(crate) struct VisibilityConstraintsBuilder<'db> { - constraints: IndexVec>, + constraints: IndexVec>, interiors: IndexVec, - constraint_cache: FxHashMap, ConstraintId>, + constraint_cache: FxHashMap, Atom>, interior_cache: FxHashMap, not_cache: FxHashMap, and_cache: FxHashMap< @@ -340,11 +350,10 @@ impl<'db> VisibilityConstraintsBuilder<'db> { /// Adds a constraint, ensuring that we only store any particular constraint once. fn add_constraint(&mut self, constraint: Constraint<'db>, copy: u8) -> Atom { - let index = *self - .constraint_cache + self.constraint_cache .entry(constraint) - .or_insert_with(|| self.constraints.push(constraint)); - Atom::from_index_and_copy(index, copy) + .or_insert_with(|| self.constraints.push(constraint)) + .copy_of(copy) } /// Adds an interior node, ensuring that we always use the same visibility constraint ID for @@ -564,8 +573,7 @@ impl<'db> VisibilityConstraints<'db> { ALWAYS_FALSE => return Truthiness::AlwaysFalse, _ => self.interior_node(id), }; - let (index, _) = node.atom.into_index_and_copy(); - let constraint = &self.constraints[index]; + let constraint = &self.constraints[node.atom]; match Self::analyze_single(db, constraint) { Truthiness::AlwaysTrue => id = node.if_true, Truthiness::Ambiguous => id = node.if_ambiguous, From a387636558f5ccd0c157ae72249b825825fac990 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 19:26:44 -0500 Subject: [PATCH 19/27] Add custom Idx impl for ScopedVisibilityConstraintId --- .../src/visibility_constraints.rs | 65 +++++++++---------- 1 file changed, 32 insertions(+), 33 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 67d4b2a832be2..0c987bb51ebff 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -152,7 +152,7 @@ use std::cmp::Ordering; -use ruff_index::{newtype_index, Idx, IndexVec}; +use ruff_index::{Idx, IndexVec}; use rustc_hash::FxHashMap; use crate::semantic_index::{ @@ -213,9 +213,6 @@ struct InteriorNode { if_false: ScopedVisibilityConstraintId, } -#[newtype_index] -struct InteriorNodeId; - /// A "variable" that is evaluated as part of a TDD ternary function. For visibility constraints, /// this is (one of the copies of) a `Constraint` that represents some runtime property of the /// Python code that we are evaluating. We intern these constraints in an arena @@ -285,6 +282,21 @@ impl ScopedVisibilityConstraintId { } } +impl Idx for ScopedVisibilityConstraintId { + #[inline] + fn new(value: usize) -> Self { + assert!(value <= (SMALLEST_TERMINAL as usize)); + #[allow(clippy::cast_possible_truncation)] + Self(value as u32) + } + + #[inline] + fn index(self) -> usize { + debug_assert!(!self.is_terminal()); + self.0 as usize + } +} + // Rebind some constants locally so that we don't need as many qualifiers below. const ALWAYS_TRUE: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::ALWAYS_TRUE; const AMBIGUOUS: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::AMBIGUOUS; @@ -296,15 +308,15 @@ const SMALLEST_TERMINAL: u32 = ALWAYS_FALSE.0; #[derive(Debug, PartialEq, Eq)] pub(crate) struct VisibilityConstraints<'db> { constraints: IndexVec>, - interiors: IndexVec, + interiors: IndexVec, } #[derive(Debug, Default, PartialEq, Eq)] pub(crate) struct VisibilityConstraintsBuilder<'db> { constraints: IndexVec>, - interiors: IndexVec, + interiors: IndexVec, constraint_cache: FxHashMap, Atom>, - interior_cache: FxHashMap, + interior_cache: FxHashMap, not_cache: FxHashMap, and_cache: FxHashMap< (ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), @@ -324,11 +336,6 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } } - fn interior_node(&self, a: ScopedVisibilityConstraintId) -> InteriorNode { - debug_assert!(!a.is_terminal()); - self.interiors[InteriorNodeId::from(a.0)] - } - /// Returns whether `a` or `b` has a "larger" atom. TDDs are ordered such that interior nodes /// can only have edges to "larger" nodes. Terminals are considered to have a larger atom than /// any internal node, since they are leaf nodes. @@ -344,7 +351,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } else if b.is_terminal() { Ordering::Less } else { - self.interior_node(a).atom.cmp(&self.interior_node(b).atom) + self.interiors[a].atom.cmp(&self.interiors[b].atom) } } @@ -365,13 +372,10 @@ impl<'db> VisibilityConstraintsBuilder<'db> { return node.if_true; } - let index = *self + *self .interior_cache .entry(node) - .or_insert_with(|| self.interiors.push(node)); - let index = index.as_u32(); - debug_assert!(index < SMALLEST_TERMINAL); - ScopedVisibilityConstraintId(index) + .or_insert_with(|| self.interiors.push(node)) } /// Adds a new visibility constraint that checks a single [`Constraint`]. Provide different @@ -407,7 +411,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { if let Some(cached) = self.not_cache.get(&a) { return *cached; } - let a_node = self.interior_node(a); + let a_node = self.interiors[a]; let if_true = self.add_not_constraint(a_node.if_true); let if_ambiguous = self.add_not_constraint(a_node.if_ambiguous); let if_false = self.add_not_constraint(a_node.if_false); @@ -442,8 +446,8 @@ impl<'db> VisibilityConstraintsBuilder<'db> { let (atom, if_true, if_ambiguous, if_false) = match self.cmp_atoms(a, b) { Ordering::Equal => { - let a_node = self.interior_node(a); - let b_node = self.interior_node(b); + let a_node = self.interiors[a]; + let b_node = self.interiors[b]; let if_true = self.add_or_constraint(a_node.if_true, b_node.if_true); let if_false = self.add_or_constraint(a_node.if_false, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -454,7 +458,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Less => { - let a_node = self.interior_node(a); + let a_node = self.interiors[a]; let if_true = self.add_or_constraint(a_node.if_true, b); let if_false = self.add_or_constraint(a_node.if_false, b); let if_ambiguous = if if_true == if_false { @@ -465,7 +469,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Greater => { - let b_node = self.interior_node(b); + let b_node = self.interiors[b]; let if_true = self.add_or_constraint(a, b_node.if_true); let if_false = self.add_or_constraint(a, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -508,8 +512,8 @@ impl<'db> VisibilityConstraintsBuilder<'db> { let (atom, if_true, if_ambiguous, if_false) = match self.cmp_atoms(a, b) { Ordering::Equal => { - let a_node = self.interior_node(a); - let b_node = self.interior_node(b); + let a_node = self.interiors[a]; + let b_node = self.interiors[b]; let if_true = self.add_and_constraint(a_node.if_true, b_node.if_true); let if_false = self.add_and_constraint(a_node.if_false, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -520,7 +524,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Less => { - let a_node = self.interior_node(a); + let a_node = self.interiors[a]; let if_true = self.add_and_constraint(a_node.if_true, b); let if_false = self.add_and_constraint(a_node.if_false, b); let if_ambiguous = if if_true == if_false { @@ -531,7 +535,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Greater => { - let b_node = self.interior_node(b); + let b_node = self.interiors[b]; let if_true = self.add_and_constraint(a, b_node.if_true); let if_false = self.add_and_constraint(a, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -555,11 +559,6 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } impl<'db> VisibilityConstraints<'db> { - fn interior_node(&self, a: ScopedVisibilityConstraintId) -> InteriorNode { - debug_assert!(!a.is_terminal()); - self.interiors[InteriorNodeId::from(a.0)] - } - /// Analyze the statically known visibility for a given visibility constraint. pub(crate) fn evaluate( &self, @@ -571,7 +570,7 @@ impl<'db> VisibilityConstraints<'db> { ALWAYS_TRUE => return Truthiness::AlwaysTrue, AMBIGUOUS => return Truthiness::Ambiguous, ALWAYS_FALSE => return Truthiness::AlwaysFalse, - _ => self.interior_node(id), + _ => self.interiors[id], }; let constraint = &self.constraints[node.atom]; match Self::analyze_single(db, constraint) { From ff472b74fb03c1b669ac3d57f755535d2bbd472c Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 20:39:47 -0500 Subject: [PATCH 20/27] Go back to Vec I guess --- .../src/visibility_constraints.rs | 108 ++++++++---------- 1 file changed, 45 insertions(+), 63 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 0c987bb51ebff..1c2cd8de519b5 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -152,7 +152,6 @@ use std::cmp::Ordering; -use ruff_index::{Idx, IndexVec}; use rustc_hash::FxHashMap; use crate::semantic_index::{ @@ -231,12 +230,9 @@ impl Atom { } #[inline] - fn copy_of(mut self, copy: u8) -> Self { - // Clear out the previous copy number - self.0 &= 0x00ff_ffff; - // OR in the new one - self.0 |= u32::from(copy) << 24; - self + fn from_index_and_copy(index: u32, copy: u8) -> Self { + debug_assert!(index <= 0x00ff_ffff); + Self((u32::from(copy) << 24) | index) } } @@ -249,21 +245,6 @@ impl std::fmt::Debug for Atom { } } -impl Idx for Atom { - #[inline] - fn new(value: usize) -> Self { - assert!(value <= 0x00ff_ffff); - #[allow(clippy::cast_possible_truncation)] - Self(value as u32) - } - - #[inline] - fn index(self) -> usize { - let (index, _) = self.into_index_and_copy(); - index as usize - } -} - impl ScopedVisibilityConstraintId { /// A special ID that is used for an "always true" / "always visible" constraint. pub(crate) const ALWAYS_TRUE: ScopedVisibilityConstraintId = @@ -282,21 +263,6 @@ impl ScopedVisibilityConstraintId { } } -impl Idx for ScopedVisibilityConstraintId { - #[inline] - fn new(value: usize) -> Self { - assert!(value <= (SMALLEST_TERMINAL as usize)); - #[allow(clippy::cast_possible_truncation)] - Self(value as u32) - } - - #[inline] - fn index(self) -> usize { - debug_assert!(!self.is_terminal()); - self.0 as usize - } -} - // Rebind some constants locally so that we don't need as many qualifiers below. const ALWAYS_TRUE: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::ALWAYS_TRUE; const AMBIGUOUS: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::AMBIGUOUS; @@ -307,16 +273,16 @@ const SMALLEST_TERMINAL: u32 = ALWAYS_FALSE.0; /// maintain a separate set of visibility constraints for each scope in file. #[derive(Debug, PartialEq, Eq)] pub(crate) struct VisibilityConstraints<'db> { - constraints: IndexVec>, - interiors: IndexVec, + constraints: Vec>, + interiors: Vec, } #[derive(Debug, Default, PartialEq, Eq)] pub(crate) struct VisibilityConstraintsBuilder<'db> { - constraints: IndexVec>, - interiors: IndexVec, - constraint_cache: FxHashMap, Atom>, - interior_cache: FxHashMap, + constraints: Vec>, + interiors: Vec, + constraint_cache: FxHashMap, u32>, + interior_cache: FxHashMap, not_cache: FxHashMap, and_cache: FxHashMap< (ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), @@ -336,6 +302,11 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } } + fn interior_node(&self, a: ScopedVisibilityConstraintId) -> InteriorNode { + debug_assert!(!a.is_terminal()); + self.interiors[a.0 as usize] + } + /// Returns whether `a` or `b` has a "larger" atom. TDDs are ordered such that interior nodes /// can only have edges to "larger" nodes. Terminals are considered to have a larger atom than /// any internal node, since they are leaf nodes. @@ -351,16 +322,18 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } else if b.is_terminal() { Ordering::Less } else { - self.interiors[a].atom.cmp(&self.interiors[b].atom) + self.interior_node(a).atom.cmp(&self.interior_node(b).atom) } } /// Adds a constraint, ensuring that we only store any particular constraint once. fn add_constraint(&mut self, constraint: Constraint<'db>, copy: u8) -> Atom { - self.constraint_cache - .entry(constraint) - .or_insert_with(|| self.constraints.push(constraint)) - .copy_of(copy) + let index = *self.constraint_cache.entry(constraint).or_insert_with(|| { + let index = self.constraints.len() as u32; + self.constraints.push(constraint); + index + }); + Atom::from_index_and_copy(index, copy) } /// Adds an interior node, ensuring that we always use the same visibility constraint ID for @@ -372,10 +345,13 @@ impl<'db> VisibilityConstraintsBuilder<'db> { return node.if_true; } - *self - .interior_cache - .entry(node) - .or_insert_with(|| self.interiors.push(node)) + let index = *self.interior_cache.entry(node).or_insert_with(|| { + let index = self.interiors.len() as u32; + self.interiors.push(node); + index + }); + debug_assert!(index < SMALLEST_TERMINAL); + ScopedVisibilityConstraintId(index) } /// Adds a new visibility constraint that checks a single [`Constraint`]. Provide different @@ -411,7 +387,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { if let Some(cached) = self.not_cache.get(&a) { return *cached; } - let a_node = self.interiors[a]; + let a_node = self.interiors[a.0 as usize]; let if_true = self.add_not_constraint(a_node.if_true); let if_ambiguous = self.add_not_constraint(a_node.if_ambiguous); let if_false = self.add_not_constraint(a_node.if_false); @@ -446,8 +422,8 @@ impl<'db> VisibilityConstraintsBuilder<'db> { let (atom, if_true, if_ambiguous, if_false) = match self.cmp_atoms(a, b) { Ordering::Equal => { - let a_node = self.interiors[a]; - let b_node = self.interiors[b]; + let a_node = self.interior_node(a); + let b_node = self.interior_node(b); let if_true = self.add_or_constraint(a_node.if_true, b_node.if_true); let if_false = self.add_or_constraint(a_node.if_false, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -458,7 +434,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Less => { - let a_node = self.interiors[a]; + let a_node = self.interior_node(a); let if_true = self.add_or_constraint(a_node.if_true, b); let if_false = self.add_or_constraint(a_node.if_false, b); let if_ambiguous = if if_true == if_false { @@ -469,7 +445,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Greater => { - let b_node = self.interiors[b]; + let b_node = self.interior_node(b); let if_true = self.add_or_constraint(a, b_node.if_true); let if_false = self.add_or_constraint(a, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -512,8 +488,8 @@ impl<'db> VisibilityConstraintsBuilder<'db> { let (atom, if_true, if_ambiguous, if_false) = match self.cmp_atoms(a, b) { Ordering::Equal => { - let a_node = self.interiors[a]; - let b_node = self.interiors[b]; + let a_node = self.interior_node(a); + let b_node = self.interior_node(b); let if_true = self.add_and_constraint(a_node.if_true, b_node.if_true); let if_false = self.add_and_constraint(a_node.if_false, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -524,7 +500,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Less => { - let a_node = self.interiors[a]; + let a_node = self.interior_node(a); let if_true = self.add_and_constraint(a_node.if_true, b); let if_false = self.add_and_constraint(a_node.if_false, b); let if_ambiguous = if if_true == if_false { @@ -535,7 +511,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Greater => { - let b_node = self.interiors[b]; + let b_node = self.interior_node(b); let if_true = self.add_and_constraint(a, b_node.if_true); let if_false = self.add_and_constraint(a, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -559,6 +535,11 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } impl<'db> VisibilityConstraints<'db> { + fn interior_node(&self, a: ScopedVisibilityConstraintId) -> InteriorNode { + debug_assert!(!a.is_terminal()); + self.interiors[a.0 as usize] + } + /// Analyze the statically known visibility for a given visibility constraint. pub(crate) fn evaluate( &self, @@ -570,9 +551,10 @@ impl<'db> VisibilityConstraints<'db> { ALWAYS_TRUE => return Truthiness::AlwaysTrue, AMBIGUOUS => return Truthiness::Ambiguous, ALWAYS_FALSE => return Truthiness::AlwaysFalse, - _ => self.interiors[id], + _ => self.interior_node(id), }; - let constraint = &self.constraints[node.atom]; + let (index, _) = node.atom.into_index_and_copy(); + let constraint = &self.constraints[index as usize]; match Self::analyze_single(db, constraint) { Truthiness::AlwaysTrue => id = node.if_true, Truthiness::Ambiguous => id = node.if_ambiguous, From 977da30cbc866fd04221076236652029affa09ef Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Tue, 4 Feb 2025 09:27:20 -0500 Subject: [PATCH 21/27] Revert "Go back to Vec I guess" This reverts commit ff472b74fb03c1b669ac3d57f755535d2bbd472c. --- .../src/visibility_constraints.rs | 108 ++++++++++-------- 1 file changed, 63 insertions(+), 45 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 1c2cd8de519b5..0c987bb51ebff 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -152,6 +152,7 @@ use std::cmp::Ordering; +use ruff_index::{Idx, IndexVec}; use rustc_hash::FxHashMap; use crate::semantic_index::{ @@ -230,9 +231,12 @@ impl Atom { } #[inline] - fn from_index_and_copy(index: u32, copy: u8) -> Self { - debug_assert!(index <= 0x00ff_ffff); - Self((u32::from(copy) << 24) | index) + fn copy_of(mut self, copy: u8) -> Self { + // Clear out the previous copy number + self.0 &= 0x00ff_ffff; + // OR in the new one + self.0 |= u32::from(copy) << 24; + self } } @@ -245,6 +249,21 @@ impl std::fmt::Debug for Atom { } } +impl Idx for Atom { + #[inline] + fn new(value: usize) -> Self { + assert!(value <= 0x00ff_ffff); + #[allow(clippy::cast_possible_truncation)] + Self(value as u32) + } + + #[inline] + fn index(self) -> usize { + let (index, _) = self.into_index_and_copy(); + index as usize + } +} + impl ScopedVisibilityConstraintId { /// A special ID that is used for an "always true" / "always visible" constraint. pub(crate) const ALWAYS_TRUE: ScopedVisibilityConstraintId = @@ -263,6 +282,21 @@ impl ScopedVisibilityConstraintId { } } +impl Idx for ScopedVisibilityConstraintId { + #[inline] + fn new(value: usize) -> Self { + assert!(value <= (SMALLEST_TERMINAL as usize)); + #[allow(clippy::cast_possible_truncation)] + Self(value as u32) + } + + #[inline] + fn index(self) -> usize { + debug_assert!(!self.is_terminal()); + self.0 as usize + } +} + // Rebind some constants locally so that we don't need as many qualifiers below. const ALWAYS_TRUE: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::ALWAYS_TRUE; const AMBIGUOUS: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::AMBIGUOUS; @@ -273,16 +307,16 @@ const SMALLEST_TERMINAL: u32 = ALWAYS_FALSE.0; /// maintain a separate set of visibility constraints for each scope in file. #[derive(Debug, PartialEq, Eq)] pub(crate) struct VisibilityConstraints<'db> { - constraints: Vec>, - interiors: Vec, + constraints: IndexVec>, + interiors: IndexVec, } #[derive(Debug, Default, PartialEq, Eq)] pub(crate) struct VisibilityConstraintsBuilder<'db> { - constraints: Vec>, - interiors: Vec, - constraint_cache: FxHashMap, u32>, - interior_cache: FxHashMap, + constraints: IndexVec>, + interiors: IndexVec, + constraint_cache: FxHashMap, Atom>, + interior_cache: FxHashMap, not_cache: FxHashMap, and_cache: FxHashMap< (ScopedVisibilityConstraintId, ScopedVisibilityConstraintId), @@ -302,11 +336,6 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } } - fn interior_node(&self, a: ScopedVisibilityConstraintId) -> InteriorNode { - debug_assert!(!a.is_terminal()); - self.interiors[a.0 as usize] - } - /// Returns whether `a` or `b` has a "larger" atom. TDDs are ordered such that interior nodes /// can only have edges to "larger" nodes. Terminals are considered to have a larger atom than /// any internal node, since they are leaf nodes. @@ -322,18 +351,16 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } else if b.is_terminal() { Ordering::Less } else { - self.interior_node(a).atom.cmp(&self.interior_node(b).atom) + self.interiors[a].atom.cmp(&self.interiors[b].atom) } } /// Adds a constraint, ensuring that we only store any particular constraint once. fn add_constraint(&mut self, constraint: Constraint<'db>, copy: u8) -> Atom { - let index = *self.constraint_cache.entry(constraint).or_insert_with(|| { - let index = self.constraints.len() as u32; - self.constraints.push(constraint); - index - }); - Atom::from_index_and_copy(index, copy) + self.constraint_cache + .entry(constraint) + .or_insert_with(|| self.constraints.push(constraint)) + .copy_of(copy) } /// Adds an interior node, ensuring that we always use the same visibility constraint ID for @@ -345,13 +372,10 @@ impl<'db> VisibilityConstraintsBuilder<'db> { return node.if_true; } - let index = *self.interior_cache.entry(node).or_insert_with(|| { - let index = self.interiors.len() as u32; - self.interiors.push(node); - index - }); - debug_assert!(index < SMALLEST_TERMINAL); - ScopedVisibilityConstraintId(index) + *self + .interior_cache + .entry(node) + .or_insert_with(|| self.interiors.push(node)) } /// Adds a new visibility constraint that checks a single [`Constraint`]. Provide different @@ -387,7 +411,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { if let Some(cached) = self.not_cache.get(&a) { return *cached; } - let a_node = self.interiors[a.0 as usize]; + let a_node = self.interiors[a]; let if_true = self.add_not_constraint(a_node.if_true); let if_ambiguous = self.add_not_constraint(a_node.if_ambiguous); let if_false = self.add_not_constraint(a_node.if_false); @@ -422,8 +446,8 @@ impl<'db> VisibilityConstraintsBuilder<'db> { let (atom, if_true, if_ambiguous, if_false) = match self.cmp_atoms(a, b) { Ordering::Equal => { - let a_node = self.interior_node(a); - let b_node = self.interior_node(b); + let a_node = self.interiors[a]; + let b_node = self.interiors[b]; let if_true = self.add_or_constraint(a_node.if_true, b_node.if_true); let if_false = self.add_or_constraint(a_node.if_false, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -434,7 +458,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Less => { - let a_node = self.interior_node(a); + let a_node = self.interiors[a]; let if_true = self.add_or_constraint(a_node.if_true, b); let if_false = self.add_or_constraint(a_node.if_false, b); let if_ambiguous = if if_true == if_false { @@ -445,7 +469,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Greater => { - let b_node = self.interior_node(b); + let b_node = self.interiors[b]; let if_true = self.add_or_constraint(a, b_node.if_true); let if_false = self.add_or_constraint(a, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -488,8 +512,8 @@ impl<'db> VisibilityConstraintsBuilder<'db> { let (atom, if_true, if_ambiguous, if_false) = match self.cmp_atoms(a, b) { Ordering::Equal => { - let a_node = self.interior_node(a); - let b_node = self.interior_node(b); + let a_node = self.interiors[a]; + let b_node = self.interiors[b]; let if_true = self.add_and_constraint(a_node.if_true, b_node.if_true); let if_false = self.add_and_constraint(a_node.if_false, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -500,7 +524,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Less => { - let a_node = self.interior_node(a); + let a_node = self.interiors[a]; let if_true = self.add_and_constraint(a_node.if_true, b); let if_false = self.add_and_constraint(a_node.if_false, b); let if_ambiguous = if if_true == if_false { @@ -511,7 +535,7 @@ impl<'db> VisibilityConstraintsBuilder<'db> { (a_node.atom, if_true, if_ambiguous, if_false) } Ordering::Greater => { - let b_node = self.interior_node(b); + let b_node = self.interiors[b]; let if_true = self.add_and_constraint(a, b_node.if_true); let if_false = self.add_and_constraint(a, b_node.if_false); let if_ambiguous = if if_true == if_false { @@ -535,11 +559,6 @@ impl<'db> VisibilityConstraintsBuilder<'db> { } impl<'db> VisibilityConstraints<'db> { - fn interior_node(&self, a: ScopedVisibilityConstraintId) -> InteriorNode { - debug_assert!(!a.is_terminal()); - self.interiors[a.0 as usize] - } - /// Analyze the statically known visibility for a given visibility constraint. pub(crate) fn evaluate( &self, @@ -551,10 +570,9 @@ impl<'db> VisibilityConstraints<'db> { ALWAYS_TRUE => return Truthiness::AlwaysTrue, AMBIGUOUS => return Truthiness::Ambiguous, ALWAYS_FALSE => return Truthiness::AlwaysFalse, - _ => self.interior_node(id), + _ => self.interiors[id], }; - let (index, _) = node.atom.into_index_and_copy(); - let constraint = &self.constraints[index as usize]; + let constraint = &self.constraints[node.atom]; match Self::analyze_single(db, constraint) { Truthiness::AlwaysTrue => id = node.if_true, Truthiness::Ambiguous => id = node.if_ambiguous, From 086bb584676e6dfd884d10d3f92c1ec74ad0abe3 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Mon, 3 Feb 2025 14:19:53 -0500 Subject: [PATCH 22/27] Revert "Remove simplify calls!!!" This reverts commit 2f74a5153079c8dd0c9a8327f84a705a7b705ba7. --- crates/red_knot_project/tests/check.rs | 2 -- .../resources/mdtest/terminal_statements.md | 3 +- .../src/semantic_index/builder.rs | 24 +++++++++++-- .../src/semantic_index/use_def.rs | 34 +++++++++++++++++++ .../semantic_index/use_def/symbol_state.rs | 10 ++++++ 5 files changed, 67 insertions(+), 6 deletions(-) diff --git a/crates/red_knot_project/tests/check.rs b/crates/red_knot_project/tests/check.rs index 711cec75f2d23..6cdd13f32f16a 100644 --- a/crates/red_knot_project/tests/check.rs +++ b/crates/red_knot_project/tests/check.rs @@ -270,8 +270,6 @@ impl SourceOrderVisitor<'_> for PullTypesVisitor<'_> { /// Whether or not the .py/.pyi version of this file is expected to fail #[rustfmt::skip] const KNOWN_FAILURES: &[(&str, bool, bool)] = &[ - // related to circular references in nested functions - ("crates/ruff_linter/resources/test/fixtures/flake8_return/RET503.py", false, true), // related to circular references in class definitions ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_26.py", true, true), ("crates/ruff_linter/resources/test/fixtures/pyflakes/F821_27.py", true, true), diff --git a/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md b/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md index 5888847b84f7d..6a3427e211c84 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md +++ b/crates/red_knot_python_semantic/resources/mdtest/terminal_statements.md @@ -635,5 +635,6 @@ def _(cond: bool): if True: return - reveal_type(x) # revealed: Literal["a"] + # TODO: Literal["a"] + reveal_type(x) # revealed: Literal["a", "b"] ``` diff --git a/crates/red_knot_python_semantic/src/semantic_index/builder.rs b/crates/red_knot_python_semantic/src/semantic_index/builder.rs index 9dac8ef56583d..ca75b22379ac3 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -413,6 +413,13 @@ impl<'db> SemanticIndexBuilder<'db> { .record_visibility_constraint(ScopedVisibilityConstraintId::AMBIGUOUS); } + /// Simplifies (resets) visibility constraints on all live bindings and declarations that did + /// not see any new definitions since the given snapshot. + fn simplify_visibility_constraints(&mut self, snapshot: FlowSnapshot) { + self.current_use_def_map_mut() + .simplify_visibility_constraints(snapshot); + } + fn push_assignment(&mut self, assignment: CurrentAssignment<'db>) { self.current_assignments.push(assignment); } @@ -1066,6 +1073,8 @@ where for post_clause_state in post_clauses { self.flow_merge(post_clause_state); } + + self.simplify_visibility_constraints(no_branch_taken); } ast::Stmt::While(ast::StmtWhile { test, @@ -1121,7 +1130,7 @@ where // To model this correctly, we need two copies of the while condition constraint, // since the first and later evaluations might produce different results. let post_body = self.flow_snapshot(); - self.flow_restore(pre_loop); + self.flow_restore(pre_loop.clone()); self.record_negated_visibility_constraint(first_vis_constraint_id); self.flow_merge(post_body); self.record_negated_constraint(constraint); @@ -1136,6 +1145,8 @@ where self.record_visibility_constraint_id(body_vis_constraint_id); self.flow_merge(snapshot); } + + self.simplify_visibility_constraints(pre_loop); } ast::Stmt::With(ast::StmtWith { items, @@ -1280,7 +1291,7 @@ where .is_some_and(|case| case.guard.is_none() && case.pattern.is_wildcard()) { post_case_snapshots.push(self.flow_snapshot()); - self.flow_restore(after_subject); + self.flow_restore(after_subject.clone()); for id in &vis_constraints { self.record_negated_visibility_constraint(*id); @@ -1290,6 +1301,8 @@ where for post_clause_state in post_case_snapshots { self.flow_merge(post_clause_state); } + + self.simplify_visibility_constraints(after_subject); } ast::Stmt::Try(ast::StmtTry { body, @@ -1570,12 +1583,13 @@ where self.visit_expr(body); let visibility_constraint = self.record_visibility_constraint(constraint); let post_body = self.flow_snapshot(); - self.flow_restore(pre_if); + self.flow_restore(pre_if.clone()); self.record_negated_constraint(constraint); self.visit_expr(orelse); self.record_negated_visibility_constraint(visibility_constraint); self.flow_merge(post_body); + self.simplify_visibility_constraints(pre_if); } ast::Expr::ListComp( list_comprehension @ ast::ExprListComp { @@ -1632,6 +1646,8 @@ where range: _, op, }) => { + let pre_op = self.flow_snapshot(); + let mut snapshots = vec![]; let mut visibility_constraints = vec![]; @@ -1678,6 +1694,8 @@ where for snapshot in snapshots { self.flow_merge(snapshot); } + + self.simplify_visibility_constraints(pre_op); } _ => { walk_expr(self, expr); diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs index f8d4ae8f23f6a..861a4c3132bd4 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def.rs @@ -575,6 +575,40 @@ impl<'db> UseDefMapBuilder<'db> { .add_and_constraint(self.scope_start_visibility, constraint); } + /// This method resets the visibility constraints for all symbols to a previous state + /// *if* there have been no new declarations or bindings since then. Consider the + /// following example: + /// ```py + /// x = 0 + /// y = 0 + /// if test_a: + /// y = 1 + /// elif test_b: + /// y = 2 + /// elif test_c: + /// y = 3 + /// + /// # RESET + /// ``` + /// We build a complex visibility constraint for the `y = 0` binding. We build the same + /// constraint for the `x = 0` binding as well, but at the `RESET` point, we can get rid + /// of it, as the `if`-`elif`-`elif` chain doesn't include any new bindings of `x`. + pub(super) fn simplify_visibility_constraints(&mut self, snapshot: FlowSnapshot) { + debug_assert!(self.symbol_states.len() >= snapshot.symbol_states.len()); + + self.scope_start_visibility = snapshot.scope_start_visibility; + + // Note that this loop terminates when we reach a symbol not present in the snapshot. + // This means we keep visibility constraints for all new symbols, which is intended, + // since these symbols have been introduced in the corresponding branch, which might + // be subject to visibility constraints. We only simplify/reset visibility constraints + // for symbols that have the same bindings and declarations present compared to the + // snapshot. + for (current, snapshot) in self.symbol_states.iter_mut().zip(snapshot.symbol_states) { + current.simplify_visibility_constraints(snapshot); + } + } + pub(super) fn record_declaration( &mut self, symbol: ScopedSymbolId, diff --git a/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs b/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs index ce9c577a267f9..4c0c72bc6226a 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs @@ -371,6 +371,16 @@ impl SymbolState { .record_visibility_constraint(visibility_constraints, constraint); } + pub(super) fn simplify_visibility_constraints(&mut self, snapshot_state: SymbolState) { + if self.bindings.live_bindings == snapshot_state.bindings.live_bindings { + self.bindings.visibility_constraints = snapshot_state.bindings.visibility_constraints; + } + if self.declarations.live_declarations == snapshot_state.declarations.live_declarations { + self.declarations.visibility_constraints = + snapshot_state.declarations.visibility_constraints; + } + } + /// Record a newly-encountered declaration of this symbol. pub(super) fn record_declaration(&mut self, declaration_id: ScopedDefinitionId) { self.declarations.record_declaration(declaration_id); From 20ada6a5f2369b5af21fecbbd8d0b0728d512606 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Tue, 4 Feb 2025 13:25:42 -0500 Subject: [PATCH 23/27] Move finish calls --- .../src/visibility_constraints.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 0c987bb51ebff..426e180acc439 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -187,11 +187,12 @@ impl std::fmt::Debug for ScopedVisibilityConstraintId { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let mut f = f.debug_tuple("ScopedVisibilityConstraintId"); match *self { - ALWAYS_TRUE => f.field(&format_args!("AlwaysTrue")).finish(), - AMBIGUOUS => f.field(&format_args!("Ambiguous")).finish(), - ALWAYS_FALSE => f.field(&format_args!("AlwaysFalse")).finish(), - _ => f.field(&self.0).finish(), - } + ALWAYS_TRUE => f.field(&format_args!("AlwaysTrue")), + AMBIGUOUS => f.field(&format_args!("Ambiguous")), + ALWAYS_FALSE => f.field(&format_args!("AlwaysFalse")), + _ => f.field(&self.0), + }; + f.finish() } } From 82e8e3e0cae53451b52be430e32e5387c6be83a0 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Tue, 4 Feb 2025 13:51:24 -0500 Subject: [PATCH 24/27] Add module docs describing TDDs --- .../src/visibility_constraints.rs | 39 +++++++++++++++---- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index 426e180acc439..e6e8ad1978698 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -137,18 +137,41 @@ //! create a state where the `x = ` binding is always visible. //! //! -//! ### Properties +//! ### Representing formulas //! -//! The ternary `AND` and `OR` operations have the property that `~a OR ~b = ~(a AND b)`. This -//! means we could, in principle, get rid of either of these two to simplify the representation. +//! Given everything above, we can represent a visibility constraint as a _ternary formula_. This +//! is like a boolean formula (which maps several true/false variables to a single true/false +//! result), but which allows the third "ambiguous" value in addition to "true" and "false". //! -//! However, we already apply negative constraints `~test1` and `~test2` to the "branches not -//! taken" in the example above. This means that the tree-representation `~test1 OR ~test2` is much -//! cheaper/shallower than basically creating `~(~(~test1) AND ~(~test2))`. Similarly, if we wanted -//! to get rid of `AND`, we would also have to create additional nodes. So for performance reasons, -//! there is a small "duplication" in the code between those two constraint types. +//! [_Binary decision diagrams_][bdd] (BDDs) are a common way to represent boolean formulas when +//! doing program analysis. We extend this to a _ternary decision diagram_ (TDD) to support +//! ambiguous values. +//! +//! A TDD is a graph, and a ternary formula is represented by a node in this graph. There are three +//! possible leaf nodes representing the "true", "false", and "ambiguous" constant functions. +//! Interior nodes consist of a ternary variable to evaluate, and outgoing edges for whether the +//! variable evaluates to true, false, or ambiguous. +//! +//! Our TDDs are _reduced_ and _ordered_ (as is typical for BDDs). +//! +//! An ordered TDD means that variables appear in the same order in all paths within the graph. +//! +//! A reduced TDD means two things: First, we intern the graph nodes, so that we only keep a single +//! copy of interior nodes with the same contents. Second, we eliminate any nodes that are "noops", +//! where the "true" and "false" outgoing edges lead to the same node. (This implies that it +//! doesn't matter what value that variable has when evaluating the formula, and we can leave it +//! out of the evaluation chain completely.) +//! +//! Reduced and ordered decision diagrams are _normal forms_, which means that two equivalent +//! formulas (which have the same outputs for every combination of inputs) are represented by +//! exactly the same graph node. (Because of interning, this is not _equal_ nodes, but _identical_ +//! ones.) That means that we can compare formulas for equivalence in constant time, and in +//! particular, can check whether a visibility constraint is statically always true or false, +//! regardless of any Python program state, by seeing if the constraint's formula is the "true" or +//! "false" leaf node. //! //! [Kleene]: +//! [bdd]: https://en.wikipedia.org/wiki/Binary_decision_diagram use std::cmp::Ordering; From 7e286a76de53212517ba2a448c4fef003fa7699d Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Tue, 4 Feb 2025 13:53:21 -0500 Subject: [PATCH 25/27] Make SMALLEST_TERMINAL an id --- .../red_knot_python_semantic/src/visibility_constraints.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index e6e8ad1978698..f12eff7dbdf24 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -302,14 +302,14 @@ impl ScopedVisibilityConstraintId { ScopedVisibilityConstraintId(0xffff_fffd); fn is_terminal(self) -> bool { - self.0 >= SMALLEST_TERMINAL + self.0 >= SMALLEST_TERMINAL.0 } } impl Idx for ScopedVisibilityConstraintId { #[inline] fn new(value: usize) -> Self { - assert!(value <= (SMALLEST_TERMINAL as usize)); + assert!(value <= (SMALLEST_TERMINAL.0 as usize)); #[allow(clippy::cast_possible_truncation)] Self(value as u32) } @@ -325,7 +325,7 @@ impl Idx for ScopedVisibilityConstraintId { const ALWAYS_TRUE: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::ALWAYS_TRUE; const AMBIGUOUS: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::AMBIGUOUS; const ALWAYS_FALSE: ScopedVisibilityConstraintId = ScopedVisibilityConstraintId::ALWAYS_FALSE; -const SMALLEST_TERMINAL: u32 = ALWAYS_FALSE.0; +const SMALLEST_TERMINAL: ScopedVisibilityConstraintId = ALWAYS_FALSE; /// A collection of visibility constraints. This is currently stored in `UseDefMap`, which means we /// maintain a separate set of visibility constraints for each scope in file. From 33773153b82eda13e0eba7190f95fef26f33a442 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Tue, 4 Feb 2025 14:00:56 -0500 Subject: [PATCH 26/27] Document copy number better --- .../src/visibility_constraints.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index f12eff7dbdf24..c256f34b33c48 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -238,10 +238,16 @@ struct InteriorNode { } /// A "variable" that is evaluated as part of a TDD ternary function. For visibility constraints, -/// this is (one of the copies of) a `Constraint` that represents some runtime property of the -/// Python code that we are evaluating. We intern these constraints in an arena -/// ([`VisibilityConstraints::constraints`]). An atom consists of an index into this arena, and a -/// copy number. +/// this is a `Constraint` that represents some runtime property of the Python code that we are +/// evaluating. We intern these constraints in an arena ([`VisibilityConstraints::constraints`]). +/// An atom is then an index into this arena. +/// +/// By using a 32-bit index, we would typically allow 4 billion distinct constraints within a +/// scope. However, we sometimes have to model how a `Constraint` can have a different runtime +/// value at different points in the execution of the program. To handle this, we reserve the top +/// byte of an atom to represent a "copy number". This is just an opaque value that allows +/// different `Atom`s to evaluate the same `Constraint`. This yields a maximum of 16 million +/// distinct `Constraint`s in a scope, and 256 possible copies of each of those constraints. #[derive(Clone, Copy, Eq, Hash, Ord, PartialEq, PartialOrd)] struct Atom(u32); From ec35dcfab7f915271081ff734cfded8d714c7b55 Mon Sep 17 00:00:00 2001 From: Douglas Creager Date: Tue, 4 Feb 2025 14:12:24 -0500 Subject: [PATCH 27/27] Document format_args usage --- crates/red_knot_python_semantic/src/visibility_constraints.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/red_knot_python_semantic/src/visibility_constraints.rs b/crates/red_knot_python_semantic/src/visibility_constraints.rs index c256f34b33c48..2acd79ffa677f 100644 --- a/crates/red_knot_python_semantic/src/visibility_constraints.rs +++ b/crates/red_knot_python_semantic/src/visibility_constraints.rs @@ -210,6 +210,9 @@ impl std::fmt::Debug for ScopedVisibilityConstraintId { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { let mut f = f.debug_tuple("ScopedVisibilityConstraintId"); match *self { + // We use format_args instead of rendering the strings directly so that we don't get + // any quotes in the output: ScopedVisibilityConstraintId(AlwaysTrue) instead of + // ScopedVisibilityConstraintId("AlwaysTrue"). ALWAYS_TRUE => f.field(&format_args!("AlwaysTrue")), AMBIGUOUS => f.field(&format_args!("Ambiguous")), ALWAYS_FALSE => f.field(&format_args!("AlwaysFalse")),