Cyclic0007/ruff
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Properly restore constraints

Browse Source
This commit is contained in:
David Peter
2024-11-28 22:38:08 +01:00
parent 41d19c3c29
commit dc55b4c8a2
7 changed files with 105 additions and 64 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
crates/red_knot_python_semantic/resources/mdtest/boolean/short_circuit.md
View File

@@ -38,7 +38,7 @@ if (x := 1) and bool_instance():
if True or (x := 1):
# TODO: infer that the second arm is never executed, and raise `unresolved-reference`.
# error: [possibly-unresolved-reference]
reveal_type(x) # revealed: Literal[1]
reveal_type(x) # revealed: Never
if True and (x := 1):
# TODO: infer that the second arm is always executed, do not raise a diagnostic

5
crates/red_knot_python_semantic/resources/mdtest/statically-known-branches.md
View File

@@ -178,8 +178,7 @@ reveal_type(x) # revealed: Literal[3, 4]
```py
x = 1 if True else 2
# TODO
reveal_type(x) # revealed: Never
reveal_type(x) # revealed: Literal[1]
```
### Always false
@@ -237,7 +236,7 @@ class C:
reveal_type(C.x) # revealed: int | str
```
TODO:
## TODO
- boundness
- conditional imports

8
crates/red_knot_python_semantic/src/semantic_index.rs
View File

@@ -1235,11 +1235,11 @@ match 1:
fn if_statement() {
let TestCase { db, file } = test_case(
"
if (x := 1) or False:
pass
x = 1 if flag else \"a\"
if x := 1:
x
_ = ... if isinstance(x, str) else ...
x
",
);

70
crates/red_knot_python_semantic/src/semantic_index/builder.rs
View File

@@ -23,7 +23,7 @@ use crate::semantic_index::symbol::{
FileScopeId, NodeWithScopeKey, NodeWithScopeRef, Scope, ScopeId, ScopedSymbolId,
SymbolTableBuilder,
};
use crate::semantic_index::use_def::{FlowSnapshot, UseDefMapBuilder};
use crate::semantic_index::use_def::{ActiveConstraintsSnapshot, FlowSnapshot, UseDefMapBuilder};
use crate::semantic_index::SemanticIndex;
use crate::unpack::Unpack;
use crate::Db;
@@ -200,12 +200,21 @@ impl<'db> SemanticIndexBuilder<'db> {
self.current_use_def_map().snapshot()
}
fn flow_restore(&mut self, state: FlowSnapshot) {
self.current_use_def_map_mut().restore(state);
fn constraints_snapshot(&self) -> ActiveConstraintsSnapshot {
self.current_use_def_map().constraints_snapshot()
}
fn flow_merge(&mut self, state: FlowSnapshot) {
self.current_use_def_map_mut().merge(state);
fn flow_restore(&mut self, state: FlowSnapshot, active_constraints: ActiveConstraintsSnapshot) {
self.current_use_def_map_mut().restore(state);
self.current_use_def_map_mut()
.restore_constraints(active_constraints);
}
fn flow_merge(&mut self, state: FlowSnapshot, active_constraints: ActiveConstraintsSnapshot) {
self.current_use_def_map_mut()
.merge(state, active_constraints.clone());
self.current_use_def_map_mut()
.restore_constraints(active_constraints); // TODO: is this also needed?
}
fn add_symbol(&mut self, name: Name) -> ScopedSymbolId {
@@ -765,6 +774,7 @@ where
ast::Stmt::If(node) => {
self.visit_expr(&node.test);
let pre_if = self.flow_snapshot();
let pre_if_constraints = self.constraints_snapshot();
let constraint = self.record_expression_constraint(&node.test);
let mut constraints = vec![constraint];
self.visit_body(&node.body);
@@ -790,7 +800,7 @@ where
post_clauses.push(self.flow_snapshot());
// we can only take an elif/else branch if none of the previous ones were
// taken, so the block entry state is always `pre_if`
self.flow_restore(pre_if.clone());
self.flow_restore(pre_if.clone(), pre_if_constraints.clone());
for constraint in &constraints {
self.record_negated_constraint(*constraint);
}
@@ -801,7 +811,7 @@ where
self.visit_body(clause_body);
}
for post_clause_state in post_clauses {
self.flow_merge(post_clause_state);
self.flow_merge(post_clause_state, pre_if_constraints.clone());
}
}
ast::Stmt::While(ast::StmtWhile {
@@ -813,6 +823,7 @@ where
self.visit_expr(test);
let pre_loop = self.flow_snapshot();
let pre_loop_constraints = self.constraints_snapshot();
// Save aside any break states from an outer loop
let saved_break_states = std::mem::take(&mut self.loop_break_states);
@@ -831,13 +842,13 @@ where
// We may execute the `else` clause without ever executing the body, so merge in
// the pre-loop state before visiting `else`.
self.flow_merge(pre_loop);
self.flow_merge(pre_loop, pre_loop_constraints.clone());
self.visit_body(orelse);
// Breaking out of a while loop bypasses the `else` clause, so merge in the break
// states after visiting `else`.
for break_state in break_states {
self.flow_merge(break_state);
self.flow_merge(break_state, pre_loop_constraints.clone()); // TODO?
}
}
ast::Stmt::With(ast::StmtWith {
@@ -880,6 +891,7 @@ where
self.visit_expr(iter);
let pre_loop = self.flow_snapshot();
let pre_loop_constraints = self.constraints_snapshot();
let saved_break_states = std::mem::take(&mut self.loop_break_states);
debug_assert_eq!(&self.current_assignments, &[]);
@@ -900,13 +912,13 @@ where
// We may execute the `else` clause without ever executing the body, so merge in
// the pre-loop state before visiting `else`.
self.flow_merge(pre_loop);
self.flow_merge(pre_loop, pre_loop_constraints.clone());
self.visit_body(orelse);
// Breaking out of a `for` loop bypasses the `else` clause, so merge in the break
// states after visiting `else`.
for break_state in break_states {
self.flow_merge(break_state);
self.flow_merge(break_state, pre_loop_constraints.clone());
}
}
ast::Stmt::Match(ast::StmtMatch {
@@ -918,6 +930,7 @@ where
self.visit_expr(subject);
let after_subject = self.flow_snapshot();
let after_subject_cs = self.constraints_snapshot();
let Some((first, remaining)) = cases.split_first() else {
return;
};
@@ -927,18 +940,18 @@ where
let mut post_case_snapshots = vec![];
for case in remaining {
post_case_snapshots.push(self.flow_snapshot());
self.flow_restore(after_subject.clone());
self.flow_restore(after_subject.clone(), after_subject_cs.clone());
self.add_pattern_constraint(subject, &case.pattern);
self.visit_match_case(case);
}
for post_clause_state in post_case_snapshots {
self.flow_merge(post_clause_state);
self.flow_merge(post_clause_state, after_subject_cs.clone());
}
if !cases
.last()
.is_some_and(|case| case.guard.is_none() && case.pattern.is_wildcard())
{
self.flow_merge(after_subject);
self.flow_merge(after_subject, after_subject_cs.clone());
}
}
ast::Stmt::Try(ast::StmtTry {
@@ -956,6 +969,7 @@ where
// We will merge this state with all of the intermediate
// states during the `try` block before visiting those suites.
let pre_try_block_state = self.flow_snapshot();
let pre_try_block_constraints = self.constraints_snapshot();
self.try_node_context_stack_manager.push_context();
@@ -976,14 +990,17 @@ where
// as there necessarily must have been 0 `except` blocks executed
// if we hit the `else` block.
let post_try_block_state = self.flow_snapshot();
let post_try_block_constraints = self.constraints_snapshot();
// Prepare for visiting the `except` block(s)
self.flow_restore(pre_try_block_state);
self.flow_restore(pre_try_block_state, pre_try_block_constraints.clone());
for state in try_block_snapshots {
self.flow_merge(state);
self.flow_merge(state, pre_try_block_constraints.clone());
// TODO?
}
let pre_except_state = self.flow_snapshot();
let pre_except_constraints = self.constraints_snapshot();
let num_handlers = handlers.len();
for (i, except_handler) in handlers.iter().enumerate() {
@@ -1022,19 +1039,22 @@ where
// as we'll immediately call `self.flow_restore()` to a different state
// as soon as this loop over the handlers terminates.
if i < (num_handlers - 1) {
self.flow_restore(pre_except_state.clone());
self.flow_restore(
pre_except_state.clone(),
pre_except_constraints.clone(),
);
}
}
// If we get to the `else` block, we know that 0 of the `except` blocks can have been executed,
// and the entire `try` block must have been executed:
self.flow_restore(post_try_block_state);
self.flow_restore(post_try_block_state, post_try_block_constraints);
}
self.visit_body(orelse);
for post_except_state in post_except_states {
self.flow_merge(post_except_state);
self.flow_merge(post_except_state, pre_try_block_constraints.clone());
}
// TODO: there's lots of complexity here that isn't yet handled by our model.
@@ -1193,14 +1213,15 @@ where
}) => {
self.visit_expr(test);
let pre_if = self.flow_snapshot();
let pre_if_constraints = self.constraints_snapshot();
let constraint = self.record_expression_constraint(test);
self.visit_expr(body);
let post_body = self.flow_snapshot();
self.flow_restore(pre_if);
self.flow_restore(pre_if, pre_if_constraints.clone());
self.record_negated_constraint(constraint);
self.visit_expr(orelse);
self.flow_merge(post_body);
self.flow_merge(post_body, pre_if_constraints);
}
ast::Expr::ListComp(
list_comprehension @ ast::ExprListComp {
@@ -1257,8 +1278,11 @@ where
range: _,
op,
}) => {
// TODO detect statically known truthy or falsy values (via type inference, not naive
// AST inspection, so we can't simplify here, need to record test expression for
// later checking)
let mut snapshots = vec![];
let pre_op_constraints = self.constraints_snapshot();
for (index, value) in values.iter().enumerate() {
self.visit_expr(value);
// In the last value we don't need to take a snapshot nor add a constraint
@@ -1273,7 +1297,7 @@ where
}
}
for snapshot in snapshots {
self.flow_merge(snapshot);
self.flow_merge(snapshot, pre_op_constraints.clone());
}
}
_ => {

23
crates/red_knot_python_semantic/src/semantic_index/use_def.rs
View File

@@ -549,9 +549,11 @@ impl std::iter::FusedIterator for DeclarationsIterator<'_, '_> {}
#[derive(Clone, Debug)]
pub(super) struct FlowSnapshot {
symbol_states: IndexVec<ScopedSymbolId, SymbolState>,
active_constraints: HashSet<ScopedConstraintId>,
}
#[derive(Clone, Debug)]
pub(super) struct ActiveConstraintsSnapshot(HashSet<ScopedConstraintId>);
#[derive(Debug, Default)]
pub(super) struct UseDefMapBuilder<'db> {
/// Append-only array of [`Definition`].
@@ -635,10 +637,13 @@ impl<'db> UseDefMapBuilder<'db> {
pub(super) fn snapshot(&self) -> FlowSnapshot {
FlowSnapshot {
symbol_states: self.symbol_states.clone(),
active_constraints: self.active_constraints.clone(),
}
}
pub(super) fn constraints_snapshot(&self) -> ActiveConstraintsSnapshot {
ActiveConstraintsSnapshot(self.active_constraints.clone())
}
/// Restore the current builder symbols state to the given snapshot.
pub(super) fn restore(&mut self, snapshot: FlowSnapshot) {
// We never remove symbols from `symbol_states` (it's an IndexVec, and the symbol
@@ -650,8 +655,6 @@ impl<'db> UseDefMapBuilder<'db> {
// Restore the current visible-definitions state to the given snapshot.
self.symbol_states = snapshot.symbol_states;
self.active_constraints = snapshot.active_constraints;
// If the snapshot we are restoring is missing some symbols we've recorded since, we need
// to fill them in so the symbol IDs continue to line up. Since they don't exist in the
// snapshot, the correct state to fill them in with is "undefined".
@@ -659,10 +662,18 @@ impl<'db> UseDefMapBuilder<'db> {
.resize(num_symbols, SymbolState::undefined());
}
pub(super) fn restore_constraints(&mut self, snapshot: ActiveConstraintsSnapshot) {
self.active_constraints = snapshot.0;
}
/// Merge the given snapshot into the current state, reflecting that we might have taken either
/// path to get here. The new state for each symbol should include definitions from both the
/// prior state and the snapshot.
pub(super) fn merge(&mut self, snapshot: FlowSnapshot) {
pub(super) fn merge(
&mut self,
snapshot: FlowSnapshot,
active_constraints: ActiveConstraintsSnapshot,
) {
// We never remove symbols from `symbol_states` (it's an IndexVec, and the symbol
// IDs must line up), so the current number of known symbols must always be equal to or
// greater than the number of known symbols in a previously-taken snapshot.
@@ -671,7 +682,7 @@ impl<'db> UseDefMapBuilder<'db> {
let mut snapshot_definitions_iter = snapshot.symbol_states.into_iter();
for current in &mut self.symbol_states {
if let Some(snapshot) = snapshot_definitions_iter.next() {
current.merge(snapshot);
current.merge(snapshot, active_constraints.clone());
} else {
// Symbol not present in snapshot, so it's unbound/undeclared from that path.
current.set_may_be_unbound();

51
crates/red_knot_python_semantic/src/semantic_index/use_def/symbol_state.rs
View File

@@ -45,6 +45,8 @@
//! similar to tracking live bindings.
use std::collections::HashSet;
use crate::semantic_index::use_def::ActiveConstraintsSnapshot;
use super::bitset::{BitSet, BitSetIterator};
use ruff_index::newtype_index;
use smallvec::SmallVec;
@@ -246,7 +248,7 @@ impl SymbolState {
}
/// Merge another [`SymbolState`] into this one.
pub(super) fn merge(&mut self, b: SymbolState) {
pub(super) fn merge(&mut self, b: SymbolState, _active_constraints: ActiveConstraintsSnapshot) {
let mut a = Self {
bindings: SymbolBindings {
live_bindings: Bindings::default(),
@@ -261,6 +263,11 @@ impl SymbolState {
},
};
// let mut constraints_active_at_binding = BitSet::default();
// for active_constraint_id in active_constraints.0 {
// constraints_active_at_binding.insert(active_constraint_id.as_u32());
// }
std::mem::swap(&mut a, self);
self.declarations
.live_declarations
@@ -347,10 +354,10 @@ impl SymbolState {
let a_constraints = a_constraints_iter
.next()
.expect("definitions and constraints length mismatch");
let _a_constraints_active_at_binding =
a_constraints_active_at_binding_iter.next().expect(
"definitions and constraints_active_at_binding length mismatch",
); // TODO: perform check that we see the same constraints in both paths
// let _a_constraints_active_at_binding =
// a_constraints_active_at_binding_iter.next().expect(
// "definitions and constraints_active_at_binding length mismatch",
// ); // TODO: perform check that we see the same constraints in both paths
// If the same definition is visible through both paths, any constraint
// that applies on only one path is irrelevant to the resulting type from
@@ -628,30 +635,30 @@ mod tests {
assert_declarations(&sym, false, &[2]);
}
#[test]
fn record_declaration_merge() {
let mut sym = SymbolState::undefined();
sym.record_declaration(ScopedDefinitionId::from_u32(1));
// #[test]
// fn record_declaration_merge() {
// let mut sym = SymbolState::undefined();
// sym.record_declaration(ScopedDefinitionId::from_u32(1));
let mut sym2 = SymbolState::undefined();
sym2.record_declaration(ScopedDefinitionId::from_u32(2));
// let mut sym2 = SymbolState::undefined();
// sym2.record_declaration(ScopedDefinitionId::from_u32(2));
sym.merge(sym2);
// sym.merge(sym2);
assert_declarations(&sym, false, &[1, 2]);
}
// assert_declarations(&sym, false, &[1, 2]);
// }
#[test]
fn record_declaration_merge_partial_undeclared() {
let mut sym = SymbolState::undefined();
sym.record_declaration(ScopedDefinitionId::from_u32(1));
// #[test]
// fn record_declaration_merge_partial_undeclared() {
// let mut sym = SymbolState::undefined();
// sym.record_declaration(ScopedDefinitionId::from_u32(1));
let sym2 = SymbolState::undefined();
// let sym2 = SymbolState::undefined();
sym.merge(sym2);
// sym.merge(sym2);
assert_declarations(&sym, true, &[1]);
}
// assert_declarations(&sym, true, &[1]);
// }
#[test]
fn set_may_be_undeclared() {

10
crates/red_knot_python_semantic/src/types.rs
View File

@@ -3278,16 +3278,16 @@ pub(crate) mod tests {
#[test_case(Ty::IntLiteral(1), Ty::Union(vec![Ty::BuiltinInstance("int"), Ty::BuiltinInstance("str")]))]
#[test_case(Ty::Union(vec![Ty::BuiltinInstance("str"), Ty::BuiltinInstance("int")]), Ty::BuiltinInstance("object"))]
#[test_case(Ty::Union(vec![Ty::IntLiteral(1), Ty::IntLiteral(2)]), Ty::Union(vec![Ty::IntLiteral(1), Ty::IntLiteral(2), Ty::IntLiteral(3)]))]
// #[test_case(Ty::BuiltinInstance("TypeError"), Ty::BuiltinInstance("Exception"))]
#[test_case(Ty::BuiltinInstance("TypeError"), Ty::BuiltinInstance("Exception"))]
#[test_case(Ty::Tuple(vec![]), Ty::Tuple(vec![]))]
#[test_case(Ty::Tuple(vec![Ty::IntLiteral(42)]), Ty::Tuple(vec![Ty::BuiltinInstance("int")]))]
#[test_case(Ty::Tuple(vec![Ty::IntLiteral(42), Ty::StringLiteral("foo")]), Ty::Tuple(vec![Ty::BuiltinInstance("int"), Ty::BuiltinInstance("str")]))]
#[test_case(Ty::Tuple(vec![Ty::BuiltinInstance("int"), Ty::StringLiteral("foo")]), Ty::Tuple(vec![Ty::BuiltinInstance("int"), Ty::BuiltinInstance("str")]))]
#[test_case(Ty::Tuple(vec![Ty::IntLiteral(42), Ty::BuiltinInstance("str")]), Ty::Tuple(vec![Ty::BuiltinInstance("int"), Ty::BuiltinInstance("str")]))]
// #[test_case(
// Ty::BuiltinInstance("FloatingPointError"),
// Ty::BuiltinInstance("Exception")
// )]
#[test_case(
Ty::BuiltinInstance("FloatingPointError"),
Ty::BuiltinInstance("Exception")
)]
#[test_case(Ty::Intersection{pos: vec![Ty::BuiltinInstance("int")], neg: vec![Ty::IntLiteral(2)]}, Ty::BuiltinInstance("int"))]
#[test_case(Ty::Intersection{pos: vec![Ty::BuiltinInstance("int")], neg: vec![Ty::IntLiteral(2)]}, Ty::Intersection{pos: vec![], neg: vec![Ty::IntLiteral(2)]})]
#[test_case(Ty::Intersection{pos: vec![], neg: vec![Ty::BuiltinInstance("int")]}, Ty::Intersection{pos: vec![], neg: vec![Ty::IntLiteral(2)]})]