From 178d05810235e00eae6344e411e70ed803279eb1 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 12 Sep 2024 04:12:36 -0400 Subject: [PATCH 01/12] [red-knot] Add control flow for `try`/`except` blocks --- .../src/semantic_index/builder.rs | 70 +++++ .../src/types/infer.rs | 296 +++++++++++++++++- 2 files changed, 350 insertions(+), 16 deletions(-) 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 d73f554bd047a..0921cc7c1fa35 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -44,6 +44,12 @@ pub(super) struct SemanticIndexBuilder<'db> { current_match_case: Option>, /// Flow states at each `break` in the current loop. loop_break_states: Vec, + /// Flow states after each definition in the `try` branch + /// of the current `try`/`except` block. + try_block_definition_states: Vec, + /// Whether we are currently visiting a `try` branch + /// of a `try`/`except` block + visiting_try_block: bool, // Semantic Index fields scopes: IndexVec, @@ -67,6 +73,8 @@ impl<'db> SemanticIndexBuilder<'db> { current_assignment: None, current_match_case: None, loop_break_states: vec![], + try_block_definition_states: vec![], + visiting_try_block: false, scopes: IndexVec::new(), symbol_tables: IndexVec::new(), @@ -222,6 +230,10 @@ impl<'db> SemanticIndexBuilder<'db> { DefinitionCategory::Binding => use_def.record_binding(symbol, definition), } + if self.visiting_try_block { + self.try_block_definition_states.push(self.flow_snapshot()); + } + definition } @@ -738,8 +750,44 @@ where is_star, range: _, }) => { + // Save the state prior to visiting any of the `try` block. + // + // Potentially none of the `try` block could have been executed prior to executing + // the `except` block(s), so we will merge this state with all of the intermediate + // states during the `try` block before visiting the `except` blocks. + let pre_try_block_state = self.flow_snapshot(); + let saved_definition_states = std::mem::take(&mut self.try_block_definition_states); + let visiting_nested_try_block = self.visiting_try_block; + + // Visit the `try` block! + // + // If `visiting_try_block == true`, this informs `.add_definition()` to record the state + // in `self.try_block_definition_states` after each new definition is recorded. + self.visiting_try_block = true; self.visit_body(body); + self.visiting_try_block = visiting_nested_try_block; + + // Save the state immediately *after* visiting the `try` block + // but *before* we prepare for visiting the `except` block(s). + // + // We will revert to this state prior to visiting the the `else` block, + // as there necessarily must have been 0 `except` blocks executed + // if we hit the `else` block. + let post_try_block_state = self.flow_snapshot(); + + // Prepare for visiting the `except` block(s) + let visiting_try_block_states = std::mem::replace( + &mut self.try_block_definition_states, + saved_definition_states, + ); + self.flow_restore(pre_try_block_state.clone()); + for state in visiting_try_block_states { + self.flow_merge(state); + } + let pre_except_state = self.flow_snapshot(); + let mut post_except_states = vec![]; + // Visit the `except` blocks! for except_handler in handlers { let ast::ExceptHandler::ExceptHandler(except_handler) = except_handler; let ast::ExceptHandlerExceptHandler { @@ -769,9 +817,31 @@ where } self.visit_body(handler_body); + + // Each `except` block is mutually exclusive with all other `except` blocks. + post_except_states.push(self.flow_snapshot()); + self.flow_restore(pre_except_state.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.visit_body(orelse); + + // Visiting the `finally` block! + // + // The state here could be that we visited exactly one `except` block, + // or that we visited the `else` block. + // + // N.B. Not all `try`/`except` clauses have `else` and/or `finally`! + // (In fact, some of them have `finally` but not `except`!) + // That's actually an irrelevant detail for us here, though, because + // a nonexistent `else` or `finally` block is just represented + // as an empty `Vec` in our AST, and visiting an empty `Vec` will not + // change the flow state at all. + for state in post_except_states { + self.flow_merge(state); + } self.visit_body(finalbody); } _ => { diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index ba153d727606c..5e2c02524662e 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -4488,8 +4488,8 @@ mod tests { ", )?; - assert_public_ty(&db, "src/a.py", "e", "NameError"); - assert_public_ty(&db, "src/a.py", "f", "error"); + assert_public_ty(&db, "src/a.py", "e", "Unbound | NameError"); + assert_public_ty(&db, "src/a.py", "f", "Unbound | error"); assert_file_diagnostics(&db, "src/a.py", &[]); Ok(()) @@ -4517,7 +4517,7 @@ mod tests { &["Cannot resolve import 'nonexistent_module'."], ); assert_public_ty(&db, "src/a.py", "foo", "Unknown"); - assert_public_ty(&db, "src/a.py", "e", "Unknown"); + assert_public_ty(&db, "src/a.py", "e", "Unbound | Unknown"); Ok(()) } @@ -4544,10 +4544,10 @@ mod tests { // For these TODOs we need support for `tuple` types: - // TODO: Should be `RuntimeError | OSError` --Alex - assert_public_ty(&db, "src/a.py", "e", "Unknown"); - // TODO: Should be `AttributeError | TypeError` --Alex - assert_public_ty(&db, "src/a.py", "e", "Unknown"); + // TODO: Should be `Unbound | RuntimeError | OSError` --Alex + assert_public_ty(&db, "src/a.py", "e", "Unbound | Unknown"); + // TODO: Should be `Unbound | AttributeError | TypeError` --Alex + assert_public_ty(&db, "src/a.py", "e", "Unbound | Unknown"); Ok(()) } @@ -4567,7 +4567,7 @@ mod tests { )?; assert_file_diagnostics(&db, "src/a.py", &[]); - assert_public_ty(&db, "src/a.py", "e", "Unknown"); + assert_public_ty(&db, "src/a.py", "e", "Unbound | Unknown"); Ok(()) } @@ -4590,8 +4590,13 @@ mod tests { // TODO: once we support `sys.version_info` branches, // we can set `--target-version=py311` in this test - // and the inferred type will just be `BaseExceptionGroup` --Alex - assert_public_ty(&db, "src/a.py", "e", "Unknown | BaseExceptionGroup"); + // and the inferred type will just be `Unbound | BaseExceptionGroup` --Alex + assert_public_ty( + &db, + "src/a.py", + "e", + "Unbound | Unknown | BaseExceptionGroup", + ); Ok(()) } @@ -4614,10 +4619,15 @@ mod tests { // TODO: once we support `sys.version_info` branches, // we can set `--target-version=py311` in this test - // and the inferred type will just be `BaseExceptionGroup` --Alex + // and the inferred type will just be `Unbound | BaseExceptionGroup` --Alex // - // TODO more precise would be `ExceptionGroup[OSError]` --Alex - assert_public_ty(&db, "src/a.py", "e", "Unknown | BaseExceptionGroup"); + // TODO more precise would be `Unbound | ExceptionGroup[OSError]` --Alex + assert_public_ty( + &db, + "src/a.py", + "e", + "Unbound | Unknown | BaseExceptionGroup", + ); Ok(()) } @@ -4640,10 +4650,264 @@ mod tests { // TODO: once we support `sys.version_info` branches, // we can set `--target-version=py311` in this test - // and the inferred type will just be `BaseExceptionGroup` --Alex + // and the inferred type will just be `Unbound | BaseExceptionGroup` --Alex // - // TODO more precise would be `ExceptionGroup[TypeError | AttributeError]` --Alex - assert_public_ty(&db, "src/a.py", "e", "Unknown | BaseExceptionGroup"); + // TODO more precise would be `Unbound | ExceptionGroup[TypeError | AttributeError]` --Alex + assert_public_ty( + &db, + "src/a.py", + "e", + "Unbound | Unknown | BaseExceptionGroup", + ); + + Ok(()) + } + + #[test] + fn except_handler_definition_in_try_and_except_no_else() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + x = 1 + except: + x = 2 + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + + // Either the `try` block ran to completion (an implicit empty `else` block), + // or the `except` block ran; either way, `x` is defined: + assert_public_ty(&db, "src/a.py", "x", "Literal[1, 2]"); + + Ok(()) + } + + #[test] + fn except_handler_definition_in_try_and_multiple_excepts_no_else() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + x = 1 + except NameError: + x = 2 + except TypeError: + pass + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + + // `x` may be unbound if the `except TypeError` branch is taken; + // it is bound if the the `try` block runs to completion or the `except NameError` branch is taken: + assert_public_ty(&db, "src/a.py", "x", "Unbound | Literal[1, 2]"); + + Ok(()) + } + + #[test] + fn except_handler_definitions_with_pre_existing_definitions() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + e = 1 + f = 2 + + try: + x + except NameError + e = 42 + except TypeError: + f = 43 + except RuntimeError: + f = 44 + except OSError: + g = 45 + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + assert_public_ty(&db, "src/a.py", "e", "Literal[1, 42]"); + assert_public_ty(&db, "src/a.py", "f", "Literal[2, 43, 44]"); + assert_public_ty(&db, "src/a.py", "g", "Unbound | Literal[45]"); + + Ok(()) + } + + #[test] + fn except_handler_definitions_with_finally_branch() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + x + except NameError + e = 42 + finally: + e = 44 + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + + // The `finally` block is always executed: + assert_public_ty(&db, "src/a.py", "e", "Literal[44]"); + + Ok(()) + } + + #[test] + fn except_handler_definitions_with_else_branch() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + x = 42 + except NameError + x = 43 + except TypeError: + x = 44 + else: + x = 45 + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + + // Either one of the `except` branches or the `else` branch must have been taken; + // the `x` definition in the `try` block must have been overridden after completion + // of the entire `try`/`except` block: + assert_public_ty(&db, "src/a.py", "x", "Literal[43, 44, 45]"); + + Ok(()) + } + + #[test] + fn except_handler_multiple_definitions_in_try_block() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + x = 1 + y = 2 + z = 3 + except NameError + x = 11 + except TypeError: + x = 12 + y = 22 + else: + x = 13 + y = 23 + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + assert_public_ty(&db, "src/a.py", "x", "Literal[11, 12, 13]"); + + // `y` may be unbound if the `except NameError` branch is taken: + assert_public_ty(&db, "src/a.py", "y", "Unbound | Literal[2, 22, 23]"); + + // `z` may be unbound if the `except NameError` or the `except TypeError` branch is taken: + assert_public_ty(&db, "src/a.py", "z", "Unbound | Literal[3]"); + + Ok(()) + } + + #[test] + fn nested_try_except_definitions() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + try: + try: + x = 1 + except TypeError: + x = 2 + else: + x = 3 + except RuntimeError: + x = 4 + else: + x = 5 + except TypeError: + try: + try: + x = 6 + except RuntimeError: + x = 7 + else: + x = 8 + except OSError: + x = 9 + else: + x = 10 + else: + try: + try: + x = 11 + except RuntimeError: + x = 12 + else: + x = 13 + except OSError: + x = 14 + else: + x = 15 + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + + // Union the flows of the top-level `except TypeError` and `else` branches together. + // The first is `Literal[9, 10]` (union of that branch's inner `except` and `else` branches); + // the second is `Literal[14, 15]` (union of *that* branch's inner `except` and `else` branches). + assert_public_ty(&db, "src/a.py", "x", "Literal[9, 10, 14, 15]"); + + Ok(()) + } + + #[test] + fn except_handler_multiple_definitions_in_nested_try_block() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + try: + x = 1 + except: + x = 2 + y = 11 + except TypeError: + x = 101 + y = 111 + except RuntimeError: + y = 211 + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + assert_public_ty(&db, "src/a.py", "x", "Unbound | Literal[1, 2, 101]"); + assert_public_ty(&db, "src/a.py", "y", "Literal[11, 111, 211]"); Ok(()) } From 17320577518d0d87e2cad2ee883f577f43b1e81b Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 12 Sep 2024 14:09:36 -0400 Subject: [PATCH 02/12] Update benchmark assertions --- .../src/types/infer.rs | 2 +- crates/ruff_benchmark/benches/red_knot.rs | 21 +++++++++++++++++++ 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 5e2c02524662e..30d9d60216fc4 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -4723,7 +4723,7 @@ mod tests { try: x - except NameError + except NameError: e = 42 except TypeError: f = 43 diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs index 340000232be92..6827feaa8ab37 100644 --- a/crates/ruff_benchmark/benches/red_knot.rs +++ b/crates/ruff_benchmark/benches/red_knot.rs @@ -22,6 +22,9 @@ struct Case { const TOMLLIB_312_URL: &str = "https://raw.githubusercontent.com/python/cpython/8e8a4baf652f6e1cee7acde9d78c4b6154539748/Lib/tomllib"; // The failed import from 'collections.abc' is due to lack of support for 'import *'. +// +// The "possibly not defined" errors in `tomllib/_parser.py` are because +// we don't yet understand terminal statements (`return`/`raise`/etc.) in our control-flow analysis static EXPECTED_DIAGNOSTICS: &[&str] = &[ "/src/tomllib/_parser.py:5:24: Module '__future__' has no member 'annotations'", "/src/tomllib/_parser.py:7:29: Module 'collections.abc' has no member 'Iterable'", @@ -33,6 +36,24 @@ static EXPECTED_DIAGNOSTICS: &[&str] = &[ "Use double quotes for strings", "Use double quotes for strings", "Use double quotes for strings", + "/src/tomllib/_parser.py:66:18: Name 's' used when possibly not defined.", + "/src/tomllib/_parser.py:98:12: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:101:12: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:104:14: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:104:14: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:115:14: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:115:14: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:126:12: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:348:20: Name 'nest' used when possibly not defined.", + "/src/tomllib/_parser.py:353:5: Name 'nest' used when possibly not defined.", + "/src/tomllib/_parser.py:453:24: Name 'nest' used when possibly not defined.", + "/src/tomllib/_parser.py:455:9: Name 'nest' used when possibly not defined.", + "/src/tomllib/_parser.py:482:16: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:566:12: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:573:12: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:579:12: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:580:63: Name 'char' used when possibly not defined.", + "/src/tomllib/_parser.py:629:38: Name 'datetime_obj' used when possibly not defined.", ]; fn get_test_file(name: &str) -> TestFile { From 68ea26e97c6c7382b85f17bf88488c8a6d7429bb Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 12 Sep 2024 14:11:39 -0400 Subject: [PATCH 03/12] Apply suggestions from code review --- crates/red_knot_python_semantic/src/types/infer.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 30d9d60216fc4..2049ef5e6ce1a 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -4751,7 +4751,7 @@ mod tests { " try: x - except NameError + except NameError: e = 42 finally: e = 44 @@ -4775,7 +4775,7 @@ mod tests { " try: x = 42 - except NameError + except NameError: x = 43 except TypeError: x = 44 @@ -4805,7 +4805,7 @@ mod tests { x = 1 y = 2 z = 3 - except NameError + except NameError: x = 11 except TypeError: x = 12 From a53c020db8a1825492646618aa295affee88b3a3 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 12 Sep 2024 14:44:48 -0400 Subject: [PATCH 04/12] Reduce clones and add a TODO --- .../src/semantic_index/builder.rs | 13 ++++++++++--- crates/red_knot_python_semantic/src/types/infer.rs | 5 +++++ 2 files changed, 15 insertions(+), 3 deletions(-) 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 0921cc7c1fa35..a5ba631545ab0 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -780,15 +780,16 @@ where &mut self.try_block_definition_states, saved_definition_states, ); - self.flow_restore(pre_try_block_state.clone()); + self.flow_restore(pre_try_block_state); for state in visiting_try_block_states { self.flow_merge(state); } let pre_except_state = self.flow_snapshot(); let mut post_except_states = vec![]; + let num_handlers = handlers.len(); // Visit the `except` blocks! - for except_handler in handlers { + for (i, except_handler) in handlers.iter().enumerate() { let ast::ExceptHandler::ExceptHandler(except_handler) = except_handler; let ast::ExceptHandlerExceptHandler { name: symbol_name, @@ -820,7 +821,13 @@ where // Each `except` block is mutually exclusive with all other `except` blocks. post_except_states.push(self.flow_snapshot()); - self.flow_restore(pre_except_state.clone()); + + // It's unnecessary to do the `self.flow_restore()` call for the final except handler, + // 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()); + } } // If we get to the `else` block, we know that 0 of the `except` blocks can have been executed, diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 2049ef5e6ce1a..8f7245382d163 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -4817,6 +4817,11 @@ mod tests { )?; assert_file_diagnostics(&db, "src/a.py", &[]); + + // TODO: What would be great in these assertions would be + // if we could assert the types of values *within* the `else` block. + // (Not possible with our current testing infrastructure, sadly.) --Alex + assert_public_ty(&db, "src/a.py", "x", "Literal[11, 12, 13]"); // `y` may be unbound if the `except NameError` branch is taken: From 501090735e8129a6f6b85d0568f9f932aacd8b2f Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 12 Sep 2024 15:31:29 -0400 Subject: [PATCH 05/12] Add another test --- .../src/types/infer.rs | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 8f7245382d163..6670dfa49e4e3 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -4917,6 +4917,28 @@ mod tests { Ok(()) } + #[test] + fn multiple_definitions_of_same_symbol_in_try_block() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + x = 1 + x = 2 + x = 3 + except: + pass + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + assert_public_ty(&db, "src/a.py", "x", "Unbound | Literal[1, 2, 3]"); + + Ok(()) + } + #[test] fn basic_comprehension() -> anyhow::Result<()> { let mut db = setup_db(); From 121a1753695a1e0b1ca6fb3dd0485d61806bd79a Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 12 Sep 2024 15:50:08 -0400 Subject: [PATCH 06/12] Add another bugfix and test --- .../src/semantic_index/builder.rs | 8 +++++ .../src/types/infer.rs | 31 +++++++++++++++++++ 2 files changed, 39 insertions(+) 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 a5ba631545ab0..4dd184e28dfd4 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -785,6 +785,14 @@ where self.flow_merge(state); } let pre_except_state = self.flow_snapshot(); + + // Account for the fact that we could be visiting a nested `try` block, + // in which case the outer `try` block must be kept informed about all the possible + // between-definition states we could have encountered in the inner `try` block(!) + if self.visiting_try_block { + self.try_block_definition_states.push(self.flow_snapshot()); + } + let mut post_except_states = vec![]; let num_handlers = handlers.len(); diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 6670dfa49e4e3..84555cb34a94c 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -4939,6 +4939,37 @@ mod tests { Ok(()) } + #[test] + fn exception_raised_in_nested_try_block_caught_in_outer() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + try: + x = 1 + x = 2 + except TypeError: + x = 3 + else: + x = 4 + except: + pass + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + + // We might have left the inner `try` or the inner `except TypeError` + // block due to another exception, in which case the assignments in the inner `try` block + // would not necessarily have been overridden by the otherwise-exhaustive + // `except`/`else` branches in the inner block. + assert_public_ty(&db, "src/a.py", "x", "Unbound | Literal[1, 2, 3, 4]"); + + Ok(()) + } + #[test] fn basic_comprehension() -> anyhow::Result<()> { let mut db = setup_db(); From cc5a5c6b32c656e5a9faa6506bded713f5f8a377 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Thu, 12 Sep 2024 16:20:46 -0400 Subject: [PATCH 07/12] Fix a bug regarding nested scopes --- .../src/semantic_index/builder.rs | 5 ++ .../src/semantic_index/symbol.rs | 6 ++ .../src/types/infer.rs | 58 +++++++++++++++++++ 3 files changed, 69 insertions(+) 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 4dd184e28dfd4..f4c92946177d8 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -112,8 +112,12 @@ impl<'db> SemanticIndexBuilder<'db> { parent, kind: node.scope_kind(), descendents: children_start..children_start, + contained_inside_try_block: self.visiting_try_block, }; + // All scopes start off as not visiting a `try` block + self.visiting_try_block = false; + let file_scope_id = self.scopes.push(scope); self.symbol_tables.push(SymbolTableBuilder::new()); self.use_def_maps.push(UseDefMapBuilder::new()); @@ -142,6 +146,7 @@ impl<'db> SemanticIndexBuilder<'db> { let children_end = self.scopes.next_index(); let scope = &mut self.scopes[id]; scope.descendents = scope.descendents.start..children_end; + self.visiting_try_block = scope.contained_inside_try_block(); id } diff --git a/crates/red_knot_python_semantic/src/semantic_index/symbol.rs b/crates/red_knot_python_semantic/src/semantic_index/symbol.rs index eeab9bf16907d..38fa4dac0dbea 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/symbol.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/symbol.rs @@ -164,6 +164,7 @@ pub struct Scope { pub(super) parent: Option, pub(super) kind: ScopeKind, pub(super) descendents: Range, + pub(super) contained_inside_try_block: bool, } impl Scope { @@ -174,6 +175,11 @@ impl Scope { pub fn kind(&self) -> ScopeKind { self.kind } + + /// Whether this scope is a top-level expression/statement inside a `try` block + pub const fn contained_inside_try_block(&self) -> bool { + self.contained_inside_try_block + } } #[derive(Copy, Clone, Debug, PartialEq, Eq)] diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 84555cb34a94c..5e855d15f5066 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -4970,6 +4970,64 @@ mod tests { Ok(()) } + #[test] + fn inner_try_blocks_inside_separate_scopes_not_tracked_by_outer_block() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + x = 1 + + def foo(): + try: + x = 2 + x = 3 + except: + x = 4 + + except: + pass + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + assert_public_ty(&db, "src/a.py", "x", "Unbound | Literal[1]"); + assert_scope_ty(&db, "src/a.py", &["foo"], "x", "Literal[3, 4]"); + + Ok(()) + } + + #[test] + fn inner_try_blocks_inside_class_scopes_not_tracked_by_outer_block() -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + x = 1 + + class Foo: + try: + x = 2 + x = 3 + except: + x = 4 + + except: + pass + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + assert_public_ty(&db, "src/a.py", "x", "Unbound | Literal[1]"); + assert_scope_ty(&db, "src/a.py", &["Foo"], "x", "Literal[3, 4]"); + + Ok(()) + } + #[test] fn basic_comprehension() -> anyhow::Result<()> { let mut db = setup_db(); From 47b9f72e5a8a6ca53c55996461646b0c27f51087 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 13 Sep 2024 10:32:29 -0400 Subject: [PATCH 08/12] Complexify for the sake of perf --- .../src/semantic_index/builder.rs | 115 +++++++++++------- .../src/semantic_index/symbol.rs | 6 - 2 files changed, 68 insertions(+), 53 deletions(-) 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 f4c92946177d8..59850fe1fa877 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -49,7 +49,7 @@ pub(super) struct SemanticIndexBuilder<'db> { try_block_definition_states: Vec, /// Whether we are currently visiting a `try` branch /// of a `try`/`except` block - visiting_try_block: bool, + try_block_context: TryBlockContext, // Semantic Index fields scopes: IndexVec, @@ -74,7 +74,7 @@ impl<'db> SemanticIndexBuilder<'db> { current_match_case: None, loop_break_states: vec![], try_block_definition_states: vec![], - visiting_try_block: false, + try_block_context: TryBlockContext::default(), scopes: IndexVec::new(), symbol_tables: IndexVec::new(), @@ -112,11 +112,10 @@ impl<'db> SemanticIndexBuilder<'db> { parent, kind: node.scope_kind(), descendents: children_start..children_start, - contained_inside_try_block: self.visiting_try_block, }; - // All scopes start off as not visiting a `try` block - self.visiting_try_block = false; + // Reset the `try` block context before pushing a new scope + self.try_block_context = TryBlockContext::default(); let file_scope_id = self.scopes.push(scope); self.symbol_tables.push(SymbolTableBuilder::new()); @@ -141,15 +140,19 @@ impl<'db> SemanticIndexBuilder<'db> { self.scope_stack.push(file_scope_id); } - fn pop_scope(&mut self) -> FileScopeId { + fn pop_scope(&mut self, prior_try_block_context: TryBlockContext) -> FileScopeId { let id = self.scope_stack.pop().expect("Root scope to be present"); let children_end = self.scopes.next_index(); let scope = &mut self.scopes[id]; scope.descendents = scope.descendents.start..children_end; - self.visiting_try_block = scope.contained_inside_try_block(); + self.try_block_context = prior_try_block_context; id } + const fn visiting_try_block(&self) -> bool { + matches!(self.try_block_context, TryBlockContext::VisitingTryBlock) + } + fn current_symbol_table(&mut self) -> &mut SymbolTableBuilder { let scope_id = self.current_scope(); &mut self.symbol_tables[scope_id] @@ -235,7 +238,7 @@ impl<'db> SemanticIndexBuilder<'db> { DefinitionCategory::Binding => use_def.record_binding(symbol, definition), } - if self.visiting_try_block { + if self.visiting_try_block() { self.try_block_definition_states.push(self.flow_snapshot()); } @@ -299,45 +302,44 @@ impl<'db> SemanticIndexBuilder<'db> { type_params: Option<&'db ast::TypeParams>, nested: impl FnOnce(&mut Self) -> FileScopeId, ) -> FileScopeId { - if let Some(type_params) = type_params { - self.push_scope(with_scope); + let Some(type_params) = type_params else { + return nested(self); + }; - for type_param in &type_params.type_params { - let (name, bound, default) = match type_param { - ast::TypeParam::TypeVar(ast::TypeParamTypeVar { - range: _, - name, - bound, - default, - }) => (name, bound, default), - ast::TypeParam::ParamSpec(ast::TypeParamParamSpec { - name, default, .. - }) => (name, &None, default), - ast::TypeParam::TypeVarTuple(ast::TypeParamTypeVarTuple { - name, - default, - .. - }) => (name, &None, default), - }; - let symbol = self.add_symbol(name.id.clone()); - // TODO create Definition for PEP 695 typevars - // note that the "bound" on the typevar is a totally different thing than whether - // or not a name is "bound" by a typevar declaration; the latter is always true. - self.mark_symbol_bound(symbol); - if let Some(bounds) = bound { - self.visit_expr(bounds); - } - if let Some(default) = default { - self.visit_expr(default); + let try_block_context = self.try_block_context; + self.push_scope(with_scope); + + for type_param in &type_params.type_params { + let (name, bound, default) = match type_param { + ast::TypeParam::TypeVar(ast::TypeParamTypeVar { + range: _, + name, + bound, + default, + }) => (name, bound, default), + ast::TypeParam::ParamSpec(ast::TypeParamParamSpec { name, default, .. }) => { + (name, &None, default) } + ast::TypeParam::TypeVarTuple(ast::TypeParamTypeVarTuple { + name, default, .. + }) => (name, &None, default), + }; + let symbol = self.add_symbol(name.id.clone()); + // TODO create Definition for PEP 695 typevars + // note that the "bound" on the typevar is a totally different thing than whether + // or not a name is "bound" by a typevar declaration; the latter is always true. + self.mark_symbol_bound(symbol); + if let Some(bounds) = bound { + self.visit_expr(bounds); + } + if let Some(default) = default { + self.visit_expr(default); } } let nested_scope = nested(self); - if type_params.is_some() { - self.pop_scope(); - } + self.pop_scope(try_block_context); nested_scope } @@ -424,7 +426,7 @@ impl<'db> SemanticIndexBuilder<'db> { self.visit_body(module.suite()); // Pop the root scope - self.pop_scope(); + self.pop_scope(TryBlockContext::default()); assert!(self.scope_stack.is_empty()); assert!(self.current_assignment.is_none()); @@ -491,6 +493,8 @@ where builder.visit_annotation(expr); } + let prior_try_block_context = builder.try_block_context; + builder.push_scope(NodeWithScopeRef::Function(function_def)); // Add symbols and definitions for the parameters to the function scope. @@ -499,7 +503,7 @@ where } builder.visit_body(&function_def.body); - builder.pop_scope() + builder.pop_scope(prior_try_block_context) }, ); // The default value of the parameters needs to be evaluated in the @@ -533,10 +537,11 @@ where builder.visit_arguments(arguments); } + let prior_try_block_context = builder.try_block_context; builder.push_scope(NodeWithScopeRef::Class(class)); builder.visit_body(&class.body); - builder.pop_scope() + builder.pop_scope(prior_try_block_context) }, ); } @@ -762,15 +767,15 @@ where // states during the `try` block before visiting the `except` blocks. let pre_try_block_state = self.flow_snapshot(); let saved_definition_states = std::mem::take(&mut self.try_block_definition_states); - let visiting_nested_try_block = self.visiting_try_block; + let prior_try_block_context = self.try_block_context; // Visit the `try` block! // // If `visiting_try_block == true`, this informs `.add_definition()` to record the state // in `self.try_block_definition_states` after each new definition is recorded. - self.visiting_try_block = true; + self.try_block_context = TryBlockContext::VisitingTryBlock; self.visit_body(body); - self.visiting_try_block = visiting_nested_try_block; + self.try_block_context = prior_try_block_context; // Save the state immediately *after* visiting the `try` block // but *before* we prepare for visiting the `except` block(s). @@ -794,7 +799,7 @@ where // Account for the fact that we could be visiting a nested `try` block, // in which case the outer `try` block must be kept informed about all the possible // between-definition states we could have encountered in the inner `try` block(!) - if self.visiting_try_block { + if self.visiting_try_block() { self.try_block_definition_states.push(self.flow_snapshot()); } @@ -874,6 +879,7 @@ where self.scopes_by_expression .insert(expr.into(), self.current_scope()); self.current_ast_ids().record_expression(expr); + let try_block_context = self.try_block_context; match expr { ast::Expr::Name(name_node @ ast::ExprName { id, ctx, .. }) => { @@ -1118,6 +1124,21 @@ where } } +/// Whether we are or aren't visiting the `try` block of a `try`/`except` statement. +/// +/// Note that this flag is reset to `NotVisitingTryBlock` whenever we start visiting +/// a nested scope inside a `try` block. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +enum TryBlockContext { + /// The [`SemanticIndexBuilder`] is visiting statements in the `try` block + /// of a `try`/`except` statement. + VisitingTryBlock, + /// The [`SemanticIndexBuilder`] is not visiting statements in the `try` block + /// of a `try`/`except` statement. + #[default] + NotVisitingTryBlock, +} + #[derive(Copy, Clone, Debug)] enum CurrentAssignment<'a> { Assign(&'a ast::StmtAssign), diff --git a/crates/red_knot_python_semantic/src/semantic_index/symbol.rs b/crates/red_knot_python_semantic/src/semantic_index/symbol.rs index 38fa4dac0dbea..eeab9bf16907d 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/symbol.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/symbol.rs @@ -164,7 +164,6 @@ pub struct Scope { pub(super) parent: Option, pub(super) kind: ScopeKind, pub(super) descendents: Range, - pub(super) contained_inside_try_block: bool, } impl Scope { @@ -175,11 +174,6 @@ impl Scope { pub fn kind(&self) -> ScopeKind { self.kind } - - /// Whether this scope is a top-level expression/statement inside a `try` block - pub const fn contained_inside_try_block(&self) -> bool { - self.contained_inside_try_block - } } #[derive(Copy, Clone, Debug, PartialEq, Eq)] From 821da5c648f4c8d42bba493723c9a44fd1ccb6b1 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Fri, 13 Sep 2024 17:29:22 -0400 Subject: [PATCH 09/12] Fix bugs relating to exhaustive `try`/`except`s --- .../src/semantic_index/builder.rs | 174 +++++++++++------- .../src/types/infer.rs | 57 ++++-- 2 files changed, 153 insertions(+), 78 deletions(-) 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 59850fe1fa877..555c6e254fc7d 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -1,3 +1,4 @@ +use std::cell::{RefCell, RefMut}; use std::sync::Arc; use rustc_hash::FxHashMap; @@ -44,12 +45,9 @@ pub(super) struct SemanticIndexBuilder<'db> { current_match_case: Option>, /// Flow states at each `break` in the current loop. loop_break_states: Vec, - /// Flow states after each definition in the `try` branch - /// of the current `try`/`except` block. - try_block_definition_states: Vec, /// Whether we are currently visiting a `try` branch /// of a `try`/`except` block - try_block_context: TryBlockContext, + try_block_contexts: TryBlockContexts, // Semantic Index fields scopes: IndexVec, @@ -73,8 +71,7 @@ impl<'db> SemanticIndexBuilder<'db> { current_assignment: None, current_match_case: None, loop_break_states: vec![], - try_block_definition_states: vec![], - try_block_context: TryBlockContext::default(), + try_block_contexts: TryBlockContexts::default(), scopes: IndexVec::new(), symbol_tables: IndexVec::new(), @@ -100,12 +97,17 @@ impl<'db> SemanticIndexBuilder<'db> { .expect("Always to have a root scope") } - fn push_scope(&mut self, node: NodeWithScopeRef) { + #[must_use] + fn push_scope(&mut self, node: NodeWithScopeRef) -> TryBlockContexts { let parent = self.current_scope(); - self.push_scope_with_parent(node, Some(parent)); + self.push_scope_with_parent(node, Some(parent)) } - fn push_scope_with_parent(&mut self, node: NodeWithScopeRef, parent: Option) { + fn push_scope_with_parent( + &mut self, + node: NodeWithScopeRef, + parent: Option, + ) -> TryBlockContexts { let children_start = self.scopes.next_index() + 1; let scope = Scope { @@ -114,8 +116,8 @@ impl<'db> SemanticIndexBuilder<'db> { descendents: children_start..children_start, }; - // Reset the `try` block context before pushing a new scope - self.try_block_context = TryBlockContext::default(); + // Reset the `try` block contexts before pushing a new scope + let prior_try_context = std::mem::take(&mut self.try_block_contexts); let file_scope_id = self.scopes.push(scope); self.symbol_tables.push(SymbolTableBuilder::new()); @@ -138,21 +140,19 @@ impl<'db> SemanticIndexBuilder<'db> { debug_assert_eq!(ast_id_scope, file_scope_id); self.scope_stack.push(file_scope_id); + + prior_try_context } - fn pop_scope(&mut self, prior_try_block_context: TryBlockContext) -> FileScopeId { + fn pop_scope(&mut self, prior_try_block_contexts: TryBlockContexts) -> FileScopeId { let id = self.scope_stack.pop().expect("Root scope to be present"); let children_end = self.scopes.next_index(); let scope = &mut self.scopes[id]; scope.descendents = scope.descendents.start..children_end; - self.try_block_context = prior_try_block_context; + self.try_block_contexts = prior_try_block_contexts; id } - const fn visiting_try_block(&self) -> bool { - matches!(self.try_block_context, TryBlockContext::VisitingTryBlock) - } - fn current_symbol_table(&mut self) -> &mut SymbolTableBuilder { let scope_id = self.current_scope(); &mut self.symbol_tables[scope_id] @@ -238,8 +238,10 @@ impl<'db> SemanticIndexBuilder<'db> { DefinitionCategory::Binding => use_def.record_binding(symbol, definition), } - if self.visiting_try_block() { - self.try_block_definition_states.push(self.flow_snapshot()); + if let Some(current_try_block) = self.try_block_contexts.borrow_mut().current_try_block() { + if !current_try_block.visiting_nested_try_stmt { + current_try_block.push_snapshot(self.flow_snapshot()); + } } definition @@ -306,8 +308,7 @@ impl<'db> SemanticIndexBuilder<'db> { return nested(self); }; - let try_block_context = self.try_block_context; - self.push_scope(with_scope); + let prior_try_block_contexts = self.push_scope(with_scope); for type_param in &type_params.type_params { let (name, bound, default) = match type_param { @@ -338,9 +339,7 @@ impl<'db> SemanticIndexBuilder<'db> { } let nested_scope = nested(self); - - self.pop_scope(try_block_context); - + self.pop_scope(prior_try_block_contexts); nested_scope } @@ -371,7 +370,7 @@ impl<'db> SemanticIndexBuilder<'db> { // nodes are evaluated in the inner scope. self.add_standalone_expression(&generator.iter); self.visit_expr(&generator.iter); - self.push_scope(scope); + let prior_try_block_contexts = self.push_scope(scope); self.current_assignment = Some(CurrentAssignment::Comprehension { node: generator, @@ -426,7 +425,7 @@ impl<'db> SemanticIndexBuilder<'db> { self.visit_body(module.suite()); // Pop the root scope - self.pop_scope(TryBlockContext::default()); + self.pop_scope(TryBlockContexts::default()); assert!(self.scope_stack.is_empty()); assert!(self.current_assignment.is_none()); @@ -493,9 +492,8 @@ where builder.visit_annotation(expr); } - let prior_try_block_context = builder.try_block_context; - - builder.push_scope(NodeWithScopeRef::Function(function_def)); + let prior_try_block_contexts = + builder.push_scope(NodeWithScopeRef::Function(function_def)); // Add symbols and definitions for the parameters to the function scope. for parameter in &*function_def.parameters { @@ -503,7 +501,7 @@ where } builder.visit_body(&function_def.body); - builder.pop_scope(prior_try_block_context) + builder.pop_scope(prior_try_block_contexts) }, ); // The default value of the parameters needs to be evaluated in the @@ -537,11 +535,10 @@ where builder.visit_arguments(arguments); } - let prior_try_block_context = builder.try_block_context; - builder.push_scope(NodeWithScopeRef::Class(class)); + let prior_try_block_contexts = + builder.push_scope(NodeWithScopeRef::Class(class)); builder.visit_body(&class.body); - - builder.pop_scope(prior_try_block_context) + builder.pop_scope(prior_try_block_contexts) }, ); } @@ -766,16 +763,13 @@ where // the `except` block(s), so we will merge this state with all of the intermediate // states during the `try` block before visiting the `except` blocks. let pre_try_block_state = self.flow_snapshot(); - let saved_definition_states = std::mem::take(&mut self.try_block_definition_states); - let prior_try_block_context = self.try_block_context; + self.try_block_contexts.push_try_block(); // Visit the `try` block! // // If `visiting_try_block == true`, this informs `.add_definition()` to record the state // in `self.try_block_definition_states` after each new definition is recorded. - self.try_block_context = TryBlockContext::VisitingTryBlock; self.visit_body(body); - self.try_block_context = prior_try_block_context; // Save the state immediately *after* visiting the `try` block // but *before* we prepare for visiting the `except` block(s). @@ -786,22 +780,26 @@ where let post_try_block_state = self.flow_snapshot(); // Prepare for visiting the `except` block(s) - let visiting_try_block_states = std::mem::replace( - &mut self.try_block_definition_states, - saved_definition_states, - ); + + let visiting_try_block_states = self + .try_block_contexts + .pop_try_block() + .expect("A `try` block should have been pushed to the stack prior to visiting the `body` field"); + if let Some(parent_try_block) = + self.try_block_contexts.borrow_mut().current_try_block() + { + parent_try_block.visiting_nested_try_stmt = true; + } self.flow_restore(pre_try_block_state); - for state in visiting_try_block_states { - self.flow_merge(state); + for state in &visiting_try_block_states.snapshots { + self.flow_merge(state.clone()); } let pre_except_state = self.flow_snapshot(); - // Account for the fact that we could be visiting a nested `try` block, - // in which case the outer `try` block must be kept informed about all the possible - // between-definition states we could have encountered in the inner `try` block(!) - if self.visiting_try_block() { - self.try_block_definition_states.push(self.flow_snapshot()); - } + // TODO: we currently detect a `try`/`except` with a bare `except:` branch as being exhaustive, + // but not a `try`/`except` with `except BaseException:`. + // They should be treated the same way. --Alex. + let mut exhaustive_except_branches = false; let mut post_except_states = vec![]; let num_handlers = handlers.len(); @@ -818,6 +816,8 @@ where if let Some(handled_exceptions) = handled_exceptions { self.visit_expr(handled_exceptions); + } else { + exhaustive_except_branches = true; } // If `handled_exceptions` above was `None`, it's something like `except as e:`, @@ -856,7 +856,11 @@ where // Visiting the `finally` block! // // The state here could be that we visited exactly one `except` block, - // or that we visited the `else` block. + // or that we visited the `else` block, **or** (unless the `except` branches + // are exhaustive) that we visited none of them! + // (An exception **could** have been raised that wasn't handled by any + // of the except handlers! But it would still be caught by the `finally`, + // which **always** runs.) // // N.B. Not all `try`/`except` clauses have `else` and/or `finally`! // (In fact, some of them have `finally` but not `except`!) @@ -867,7 +871,23 @@ where for state in post_except_states { self.flow_merge(state); } + if !exhaustive_except_branches { + for state in visiting_try_block_states.snapshots { + self.flow_merge(state); + } + } + self.visit_body(finalbody); + + // Account for the fact that we could be visiting a nested `try` block, + // in which case the outer `try` block must be kept informed about all the possible + // between-definition states we could have encountered in the inner `try` block(!) + if let Some(current_try_block) = + self.try_block_contexts.borrow_mut().current_try_block() + { + current_try_block.push_snapshot(self.flow_snapshot()); + current_try_block.visiting_nested_try_stmt = false; + } } _ => { walk_stmt(self, stmt); @@ -879,7 +899,6 @@ where self.scopes_by_expression .insert(expr.into(), self.current_scope()); self.current_ast_ids().record_expression(expr); - let try_block_context = self.try_block_context; match expr { ast::Expr::Name(name_node @ ast::ExprName { id, ctx, .. }) => { @@ -979,7 +998,7 @@ where } self.visit_parameters(parameters); } - self.push_scope(NodeWithScopeRef::Lambda(lambda)); + let prior_try_block_contexts = self.push_scope(NodeWithScopeRef::Lambda(lambda)); // Add symbols and definitions for the parameters to the lambda scope. if let Some(parameters) = &lambda.parameters { @@ -1124,19 +1143,42 @@ where } } -/// Whether we are or aren't visiting the `try` block of a `try`/`except` statement. -/// -/// Note that this flag is reset to `NotVisitingTryBlock` whenever we start visiting -/// a nested scope inside a `try` block. -#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] -enum TryBlockContext { - /// The [`SemanticIndexBuilder`] is visiting statements in the `try` block - /// of a `try`/`except` statement. - VisitingTryBlock, - /// The [`SemanticIndexBuilder`] is not visiting statements in the `try` block - /// of a `try`/`except` statement. - #[default] - NotVisitingTryBlock, +#[derive(Debug, Default)] +struct TryBlockContext { + snapshots: Vec, + visiting_nested_try_stmt: bool, +} + +impl TryBlockContext { + fn push_snapshot(&mut self, snapshot: FlowSnapshot) { + self.snapshots.push(snapshot); + } +} + +#[derive(Debug)] +struct TryBlockContextsRefMut<'a>(RefMut<'a, Vec>); + +impl<'a> TryBlockContextsRefMut<'a> { + fn current_try_block(&mut self) -> Option<&mut TryBlockContext> { + self.0.last_mut() + } +} + +#[derive(Debug, Default)] +struct TryBlockContexts(RefCell>); + +impl TryBlockContexts { + fn push_try_block(&self) { + self.0.borrow_mut().push(TryBlockContext::default()); + } + + fn pop_try_block(&self) -> Option { + self.0.borrow_mut().pop() + } + + fn borrow_mut(&self) -> TryBlockContextsRefMut { + TryBlockContextsRefMut(self.0.borrow_mut()) + } } #[derive(Copy, Clone, Debug)] diff --git a/crates/red_knot_python_semantic/src/types/infer.rs b/crates/red_knot_python_semantic/src/types/infer.rs index 5e855d15f5066..3cbf38b854c42 100644 --- a/crates/red_knot_python_semantic/src/types/infer.rs +++ b/crates/red_knot_python_semantic/src/types/infer.rs @@ -4697,7 +4697,7 @@ mod tests { x = 1 except NameError: x = 2 - except TypeError: + except: pass ", )?; @@ -4767,7 +4767,7 @@ mod tests { } #[test] - fn except_handler_definitions_with_else_branch() -> anyhow::Result<()> { + fn exhaustive_except_handler_definitions_with_else_branch() -> anyhow::Result<()> { let mut db = setup_db(); db.write_dedented( @@ -4777,7 +4777,7 @@ mod tests { x = 42 except NameError: x = 43 - except TypeError: + except: x = 44 else: x = 45 @@ -4795,7 +4795,8 @@ mod tests { } #[test] - fn except_handler_multiple_definitions_in_try_block() -> anyhow::Result<()> { + fn except_handler_multiple_definitions_in_try_block_with_exhaustive_excepts( + ) -> anyhow::Result<()> { let mut db = setup_db(); db.write_dedented( @@ -4807,7 +4808,7 @@ mod tests { z = 3 except NameError: x = 11 - except TypeError: + except: x = 12 y = 22 else: @@ -4880,17 +4881,18 @@ mod tests { )?; assert_file_diagnostics(&db, "src/a.py", &[]); - - // Union the flows of the top-level `except TypeError` and `else` branches together. - // The first is `Literal[9, 10]` (union of that branch's inner `except` and `else` branches); - // the second is `Literal[14, 15]` (union of *that* branch's inner `except` and `else` branches). - assert_public_ty(&db, "src/a.py", "x", "Literal[9, 10, 14, 15]"); + assert_public_ty( + &db, + "src/a.py", + "x", + "Literal[1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]", + ); Ok(()) } #[test] - fn except_handler_multiple_definitions_in_nested_try_block() -> anyhow::Result<()> { + fn except_handler_multiple_definitions_in_exhaustive_nested_try_block() -> anyhow::Result<()> { let mut db = setup_db(); db.write_dedented( @@ -4905,7 +4907,7 @@ mod tests { except TypeError: x = 101 y = 111 - except RuntimeError: + except: y = 211 ", )?; @@ -4970,6 +4972,37 @@ mod tests { Ok(()) } + #[test] + fn exception_raised_in_nested_try_block_with_finally_clause_caught_in_outer( + ) -> anyhow::Result<()> { + let mut db = setup_db(); + + db.write_dedented( + "src/a.py", + " + try: + try: + try: + x = 1 + except: + x = 2 + else: + x = 3 + finally: + x = 4 + except: + x = 5 + except: + pass + ", + )?; + + assert_file_diagnostics(&db, "src/a.py", &[]); + assert_public_ty(&db, "src/a.py", "x", "Unbound | Literal[4, 5]"); + + Ok(()) + } + #[test] fn inner_try_blocks_inside_separate_scopes_not_tracked_by_outer_block() -> anyhow::Result<()> { let mut db = setup_db(); From f469cc186218e8406482d81b1f5d2f053d3b7343 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Sat, 14 Sep 2024 13:32:32 -0400 Subject: [PATCH 10/12] `push_scope` needs no knowledge of the `TryBlockContexts` --- .../src/semantic_index/builder.rs | 82 +++++++++++-------- 1 file changed, 47 insertions(+), 35 deletions(-) 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 555c6e254fc7d..26bf554ba7551 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -45,9 +45,8 @@ pub(super) struct SemanticIndexBuilder<'db> { current_match_case: Option>, /// Flow states at each `break` in the current loop. loop_break_states: Vec, - /// Whether we are currently visiting a `try` branch - /// of a `try`/`except` block - try_block_contexts: TryBlockContexts, + /// Per-scope contexts regarding nested `try`/`except` statements + try_block_contexts_stack: TryBlockContextsStack, // Semantic Index fields scopes: IndexVec, @@ -71,7 +70,7 @@ impl<'db> SemanticIndexBuilder<'db> { current_assignment: None, current_match_case: None, loop_break_states: vec![], - try_block_contexts: TryBlockContexts::default(), + try_block_contexts_stack: TryBlockContextsStack::default(), scopes: IndexVec::new(), symbol_tables: IndexVec::new(), @@ -97,17 +96,16 @@ impl<'db> SemanticIndexBuilder<'db> { .expect("Always to have a root scope") } - #[must_use] - fn push_scope(&mut self, node: NodeWithScopeRef) -> TryBlockContexts { + fn try_block_contexts(&self) -> &TryBlockContexts { + self.try_block_contexts_stack.current_try_block_context() + } + + fn push_scope(&mut self, node: NodeWithScopeRef) { let parent = self.current_scope(); - self.push_scope_with_parent(node, Some(parent)) + self.push_scope_with_parent(node, Some(parent)); } - fn push_scope_with_parent( - &mut self, - node: NodeWithScopeRef, - parent: Option, - ) -> TryBlockContexts { + fn push_scope_with_parent(&mut self, node: NodeWithScopeRef, parent: Option) { let children_start = self.scopes.next_index() + 1; let scope = Scope { @@ -115,9 +113,7 @@ impl<'db> SemanticIndexBuilder<'db> { kind: node.scope_kind(), descendents: children_start..children_start, }; - - // Reset the `try` block contexts before pushing a new scope - let prior_try_context = std::mem::take(&mut self.try_block_contexts); + self.try_block_contexts_stack.push_context(); let file_scope_id = self.scopes.push(scope); self.symbol_tables.push(SymbolTableBuilder::new()); @@ -140,16 +136,14 @@ impl<'db> SemanticIndexBuilder<'db> { debug_assert_eq!(ast_id_scope, file_scope_id); self.scope_stack.push(file_scope_id); - - prior_try_context } - fn pop_scope(&mut self, prior_try_block_contexts: TryBlockContexts) -> FileScopeId { + fn pop_scope(&mut self) -> FileScopeId { let id = self.scope_stack.pop().expect("Root scope to be present"); let children_end = self.scopes.next_index(); let scope = &mut self.scopes[id]; scope.descendents = scope.descendents.start..children_end; - self.try_block_contexts = prior_try_block_contexts; + self.try_block_contexts_stack.pop_context(); id } @@ -238,7 +232,8 @@ impl<'db> SemanticIndexBuilder<'db> { DefinitionCategory::Binding => use_def.record_binding(symbol, definition), } - if let Some(current_try_block) = self.try_block_contexts.borrow_mut().current_try_block() { + if let Some(current_try_block) = self.try_block_contexts().borrow_mut().current_try_block() + { if !current_try_block.visiting_nested_try_stmt { current_try_block.push_snapshot(self.flow_snapshot()); } @@ -308,7 +303,7 @@ impl<'db> SemanticIndexBuilder<'db> { return nested(self); }; - let prior_try_block_contexts = self.push_scope(with_scope); + self.push_scope(with_scope); for type_param in &type_params.type_params { let (name, bound, default) = match type_param { @@ -339,7 +334,7 @@ impl<'db> SemanticIndexBuilder<'db> { } let nested_scope = nested(self); - self.pop_scope(prior_try_block_contexts); + self.pop_scope(); nested_scope } @@ -370,7 +365,7 @@ impl<'db> SemanticIndexBuilder<'db> { // nodes are evaluated in the inner scope. self.add_standalone_expression(&generator.iter); self.visit_expr(&generator.iter); - let prior_try_block_contexts = self.push_scope(scope); + self.push_scope(scope); self.current_assignment = Some(CurrentAssignment::Comprehension { node: generator, @@ -425,7 +420,7 @@ impl<'db> SemanticIndexBuilder<'db> { self.visit_body(module.suite()); // Pop the root scope - self.pop_scope(TryBlockContexts::default()); + self.pop_scope(); assert!(self.scope_stack.is_empty()); assert!(self.current_assignment.is_none()); @@ -492,8 +487,7 @@ where builder.visit_annotation(expr); } - let prior_try_block_contexts = - builder.push_scope(NodeWithScopeRef::Function(function_def)); + builder.push_scope(NodeWithScopeRef::Function(function_def)); // Add symbols and definitions for the parameters to the function scope. for parameter in &*function_def.parameters { @@ -501,7 +495,7 @@ where } builder.visit_body(&function_def.body); - builder.pop_scope(prior_try_block_contexts) + builder.pop_scope() }, ); // The default value of the parameters needs to be evaluated in the @@ -535,10 +529,9 @@ where builder.visit_arguments(arguments); } - let prior_try_block_contexts = - builder.push_scope(NodeWithScopeRef::Class(class)); + builder.push_scope(NodeWithScopeRef::Class(class)); builder.visit_body(&class.body); - builder.pop_scope(prior_try_block_contexts) + builder.pop_scope() }, ); } @@ -763,7 +756,7 @@ where // the `except` block(s), so we will merge this state with all of the intermediate // states during the `try` block before visiting the `except` blocks. let pre_try_block_state = self.flow_snapshot(); - self.try_block_contexts.push_try_block(); + self.try_block_contexts().push_try_block(); // Visit the `try` block! // @@ -782,11 +775,11 @@ where // Prepare for visiting the `except` block(s) let visiting_try_block_states = self - .try_block_contexts + .try_block_contexts() .pop_try_block() .expect("A `try` block should have been pushed to the stack prior to visiting the `body` field"); if let Some(parent_try_block) = - self.try_block_contexts.borrow_mut().current_try_block() + self.try_block_contexts().borrow_mut().current_try_block() { parent_try_block.visiting_nested_try_stmt = true; } @@ -883,7 +876,7 @@ where // in which case the outer `try` block must be kept informed about all the possible // between-definition states we could have encountered in the inner `try` block(!) if let Some(current_try_block) = - self.try_block_contexts.borrow_mut().current_try_block() + self.try_block_contexts().borrow_mut().current_try_block() { current_try_block.push_snapshot(self.flow_snapshot()); current_try_block.visiting_nested_try_stmt = false; @@ -998,7 +991,7 @@ where } self.visit_parameters(parameters); } - let prior_try_block_contexts = self.push_scope(NodeWithScopeRef::Lambda(lambda)); + self.push_scope(NodeWithScopeRef::Lambda(lambda)); // Add symbols and definitions for the parameters to the lambda scope. if let Some(parameters) = &lambda.parameters { @@ -1143,6 +1136,25 @@ where } } +#[derive(Debug, Default)] +struct TryBlockContextsStack(Vec); + +impl TryBlockContextsStack { + fn push_context(&mut self) { + self.0.push(TryBlockContexts::default()); + } + + fn current_try_block_context(&self) -> &TryBlockContexts { + self.0 + .last() + .expect("There should always be at least one `TryBlockContexts` on the stack") + } + + fn pop_context(&mut self) { + self.0.pop(); + } +} + #[derive(Debug, Default)] struct TryBlockContext { snapshots: Vec, From 11a549965ec13bb3654e4496eaf9140a2517ddde Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Sat, 14 Sep 2024 13:57:26 -0400 Subject: [PATCH 11/12] Improve encapsulation --- .../src/semantic_index.rs | 1 + .../src/semantic_index/builder.rs | 139 ++++++------------ .../src/semantic_index/except_handlers.rs | 79 ++++++++++ 3 files changed, 122 insertions(+), 97 deletions(-) create mode 100644 crates/red_knot_python_semantic/src/semantic_index/except_handlers.rs diff --git a/crates/red_knot_python_semantic/src/semantic_index.rs b/crates/red_knot_python_semantic/src/semantic_index.rs index 1730ec1b74e75..7c0a8c5e2d326 100644 --- a/crates/red_knot_python_semantic/src/semantic_index.rs +++ b/crates/red_knot_python_semantic/src/semantic_index.rs @@ -23,6 +23,7 @@ pub mod ast_ids; mod builder; pub(crate) mod constraint; pub mod definition; +mod except_handlers; pub mod expression; pub mod symbol; mod use_def; 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 26bf554ba7551..c7e8a64281d04 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -1,4 +1,3 @@ -use std::cell::{RefCell, RefMut}; use std::sync::Arc; use rustc_hash::FxHashMap; @@ -32,6 +31,7 @@ use super::definition::{ DefinitionCategory, ExceptHandlerDefinitionNodeRef, MatchPatternDefinitionNodeRef, WithItemDefinitionNodeRef, }; +use super::except_handlers::{TryBlockContexts, TryBlockContextsStack}; pub(super) struct SemanticIndexBuilder<'db> { // Builder state @@ -167,7 +167,7 @@ impl<'db> SemanticIndexBuilder<'db> { &mut self.ast_ids[scope_id] } - fn flow_snapshot(&self) -> FlowSnapshot { + pub(super) fn flow_snapshot(&self) -> FlowSnapshot { self.current_use_def_map().snapshot() } @@ -234,9 +234,7 @@ impl<'db> SemanticIndexBuilder<'db> { if let Some(current_try_block) = self.try_block_contexts().borrow_mut().current_try_block() { - if !current_try_block.visiting_nested_try_stmt { - current_try_block.push_snapshot(self.flow_snapshot()); - } + current_try_block.record_definition(self); } definition @@ -299,42 +297,46 @@ impl<'db> SemanticIndexBuilder<'db> { type_params: Option<&'db ast::TypeParams>, nested: impl FnOnce(&mut Self) -> FileScopeId, ) -> FileScopeId { - let Some(type_params) = type_params else { - return nested(self); - }; - - self.push_scope(with_scope); + if let Some(type_params) = type_params { + self.push_scope(with_scope); - for type_param in &type_params.type_params { - let (name, bound, default) = match type_param { - ast::TypeParam::TypeVar(ast::TypeParamTypeVar { - range: _, - name, - bound, - default, - }) => (name, bound, default), - ast::TypeParam::ParamSpec(ast::TypeParamParamSpec { name, default, .. }) => { - (name, &None, default) + for type_param in &type_params.type_params { + let (name, bound, default) = match type_param { + ast::TypeParam::TypeVar(ast::TypeParamTypeVar { + range: _, + name, + bound, + default, + }) => (name, bound, default), + ast::TypeParam::ParamSpec(ast::TypeParamParamSpec { + name, default, .. + }) => (name, &None, default), + ast::TypeParam::TypeVarTuple(ast::TypeParamTypeVarTuple { + name, + default, + .. + }) => (name, &None, default), + }; + let symbol = self.add_symbol(name.id.clone()); + // TODO create Definition for PEP 695 typevars + // note that the "bound" on the typevar is a totally different thing than whether + // or not a name is "bound" by a typevar declaration; the latter is always true. + self.mark_symbol_bound(symbol); + if let Some(bounds) = bound { + self.visit_expr(bounds); + } + if let Some(default) = default { + self.visit_expr(default); } - ast::TypeParam::TypeVarTuple(ast::TypeParamTypeVarTuple { - name, default, .. - }) => (name, &None, default), - }; - let symbol = self.add_symbol(name.id.clone()); - // TODO create Definition for PEP 695 typevars - // note that the "bound" on the typevar is a totally different thing than whether - // or not a name is "bound" by a typevar declaration; the latter is always true. - self.mark_symbol_bound(symbol); - if let Some(bounds) = bound { - self.visit_expr(bounds); - } - if let Some(default) = default { - self.visit_expr(default); } } let nested_scope = nested(self); - self.pop_scope(); + + if type_params.is_some() { + self.pop_scope(); + } + nested_scope } @@ -781,10 +783,10 @@ where if let Some(parent_try_block) = self.try_block_contexts().borrow_mut().current_try_block() { - parent_try_block.visiting_nested_try_stmt = true; + parent_try_block.record_visiting_nested_try_stmt(); } self.flow_restore(pre_try_block_state); - for state in &visiting_try_block_states.snapshots { + for state in visiting_try_block_states.snapshots() { self.flow_merge(state.clone()); } let pre_except_state = self.flow_snapshot(); @@ -865,7 +867,7 @@ where self.flow_merge(state); } if !exhaustive_except_branches { - for state in visiting_try_block_states.snapshots { + for state in visiting_try_block_states.into_snapshots() { self.flow_merge(state); } } @@ -878,8 +880,8 @@ where if let Some(current_try_block) = self.try_block_contexts().borrow_mut().current_try_block() { - current_try_block.push_snapshot(self.flow_snapshot()); - current_try_block.visiting_nested_try_stmt = false; + current_try_block.record_exiting_nested_try_stmt(); + current_try_block.record_definition(self); } } _ => { @@ -1136,63 +1138,6 @@ where } } -#[derive(Debug, Default)] -struct TryBlockContextsStack(Vec); - -impl TryBlockContextsStack { - fn push_context(&mut self) { - self.0.push(TryBlockContexts::default()); - } - - fn current_try_block_context(&self) -> &TryBlockContexts { - self.0 - .last() - .expect("There should always be at least one `TryBlockContexts` on the stack") - } - - fn pop_context(&mut self) { - self.0.pop(); - } -} - -#[derive(Debug, Default)] -struct TryBlockContext { - snapshots: Vec, - visiting_nested_try_stmt: bool, -} - -impl TryBlockContext { - fn push_snapshot(&mut self, snapshot: FlowSnapshot) { - self.snapshots.push(snapshot); - } -} - -#[derive(Debug)] -struct TryBlockContextsRefMut<'a>(RefMut<'a, Vec>); - -impl<'a> TryBlockContextsRefMut<'a> { - fn current_try_block(&mut self) -> Option<&mut TryBlockContext> { - self.0.last_mut() - } -} - -#[derive(Debug, Default)] -struct TryBlockContexts(RefCell>); - -impl TryBlockContexts { - fn push_try_block(&self) { - self.0.borrow_mut().push(TryBlockContext::default()); - } - - fn pop_try_block(&self) -> Option { - self.0.borrow_mut().pop() - } - - fn borrow_mut(&self) -> TryBlockContextsRefMut { - TryBlockContextsRefMut(self.0.borrow_mut()) - } -} - #[derive(Copy, Clone, Debug)] enum CurrentAssignment<'a> { Assign(&'a ast::StmtAssign), diff --git a/crates/red_knot_python_semantic/src/semantic_index/except_handlers.rs b/crates/red_knot_python_semantic/src/semantic_index/except_handlers.rs new file mode 100644 index 0000000000000..2eded0089956d --- /dev/null +++ b/crates/red_knot_python_semantic/src/semantic_index/except_handlers.rs @@ -0,0 +1,79 @@ +use std::cell::{RefCell, RefMut}; + +use super::use_def::FlowSnapshot; +use super::SemanticIndexBuilder; + +#[derive(Debug, Default)] +pub(super) struct TryBlockContextsStack(Vec); + +impl TryBlockContextsStack { + pub(super) fn push_context(&mut self) { + self.0.push(TryBlockContexts::default()); + } + + pub(super) fn current_try_block_context(&self) -> &TryBlockContexts { + self.0 + .last() + .expect("There should always be at least one `TryBlockContexts` on the stack") + } + + pub(super) fn pop_context(&mut self) { + self.0.pop(); + } +} + +#[derive(Debug, Default)] +pub(super) struct TryBlockContext { + snapshots: Vec, + visiting_nested_try_stmt: bool, +} + +impl TryBlockContext { + pub(super) fn record_definition(&mut self, builder: &SemanticIndexBuilder) { + if !self.visiting_nested_try_stmt { + self.snapshots.push(builder.flow_snapshot()); + } + } + + pub(super) fn record_visiting_nested_try_stmt(&mut self) { + self.visiting_nested_try_stmt = true; + } + + pub(super) fn record_exiting_nested_try_stmt(&mut self) { + self.visiting_nested_try_stmt = false; + } + + pub(super) fn snapshots(&self) -> &[FlowSnapshot] { + &self.snapshots + } + + pub(super) fn into_snapshots(self) -> Vec { + self.snapshots + } +} + +#[derive(Debug)] +pub(super) struct TryBlockContextsRefMut<'a>(RefMut<'a, Vec>); + +impl<'a> TryBlockContextsRefMut<'a> { + pub(super) fn current_try_block(&mut self) -> Option<&mut TryBlockContext> { + self.0.last_mut() + } +} + +#[derive(Debug, Default)] +pub(super) struct TryBlockContexts(RefCell>); + +impl TryBlockContexts { + pub(super) fn push_try_block(&self) { + self.0.borrow_mut().push(TryBlockContext::default()); + } + + pub(super) fn pop_try_block(&self) -> Option { + self.0.borrow_mut().pop() + } + + pub(super) fn borrow_mut(&self) -> TryBlockContextsRefMut { + TryBlockContextsRefMut(self.0.borrow_mut()) + } +} From 89edfc9ff489de9f1265c299ce6ee6f27dfe2f1a Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 16 Sep 2024 11:27:11 -0400 Subject: [PATCH 12/12] misc review comments --- .../src/semantic_index.rs | 1 - .../src/semantic_index/builder.rs | 24 ++++--- .../{ => builder}/except_handlers.rs | 71 +++++++++++-------- .../src/semantic_index/use_def.rs | 2 +- 4 files changed, 56 insertions(+), 42 deletions(-) rename crates/red_knot_python_semantic/src/semantic_index/{ => builder}/except_handlers.rs (68%) diff --git a/crates/red_knot_python_semantic/src/semantic_index.rs b/crates/red_knot_python_semantic/src/semantic_index.rs index 7c0a8c5e2d326..1730ec1b74e75 100644 --- a/crates/red_knot_python_semantic/src/semantic_index.rs +++ b/crates/red_knot_python_semantic/src/semantic_index.rs @@ -23,7 +23,6 @@ pub mod ast_ids; mod builder; pub(crate) mod constraint; pub mod definition; -mod except_handlers; pub mod expression; pub mod symbol; mod use_def; 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 c7e8a64281d04..67594c7350d0d 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/builder.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder.rs @@ -31,7 +31,9 @@ use super::definition::{ DefinitionCategory, ExceptHandlerDefinitionNodeRef, MatchPatternDefinitionNodeRef, WithItemDefinitionNodeRef, }; -use super::except_handlers::{TryBlockContexts, TryBlockContextsStack}; +use except_handlers::{TryBlockContexts, TryBlockContextsRefMut, TryBlockContextsStack}; + +mod except_handlers; pub(super) struct SemanticIndexBuilder<'db> { // Builder state @@ -100,6 +102,12 @@ impl<'db> SemanticIndexBuilder<'db> { self.try_block_contexts_stack.current_try_block_context() } + fn try_block_contexts_mut(&self) -> TryBlockContextsRefMut { + self.try_block_contexts_stack + .current_try_block_context() + .borrow_mut() + } + fn push_scope(&mut self, node: NodeWithScopeRef) { let parent = self.current_scope(); self.push_scope_with_parent(node, Some(parent)); @@ -167,7 +175,7 @@ impl<'db> SemanticIndexBuilder<'db> { &mut self.ast_ids[scope_id] } - pub(super) fn flow_snapshot(&self) -> FlowSnapshot { + fn flow_snapshot(&self) -> FlowSnapshot { self.current_use_def_map().snapshot() } @@ -232,8 +240,7 @@ impl<'db> SemanticIndexBuilder<'db> { DefinitionCategory::Binding => use_def.record_binding(symbol, definition), } - if let Some(current_try_block) = self.try_block_contexts().borrow_mut().current_try_block() - { + if let Some(current_try_block) = self.try_block_contexts_mut().current_try_block() { current_try_block.record_definition(self); } @@ -533,6 +540,7 @@ where builder.push_scope(NodeWithScopeRef::Class(class)); builder.visit_body(&class.body); + builder.pop_scope() }, ); @@ -783,7 +791,7 @@ where if let Some(parent_try_block) = self.try_block_contexts().borrow_mut().current_try_block() { - parent_try_block.record_visiting_nested_try_stmt(); + parent_try_block.enter_nested_try_stmt(); } self.flow_restore(pre_try_block_state); for state in visiting_try_block_states.snapshots() { @@ -877,10 +885,8 @@ where // Account for the fact that we could be visiting a nested `try` block, // in which case the outer `try` block must be kept informed about all the possible // between-definition states we could have encountered in the inner `try` block(!) - if let Some(current_try_block) = - self.try_block_contexts().borrow_mut().current_try_block() - { - current_try_block.record_exiting_nested_try_stmt(); + if let Some(current_try_block) = self.try_block_contexts_mut().current_try_block() { + current_try_block.exit_nested_try_stmt(); current_try_block.record_definition(self); } } diff --git a/crates/red_knot_python_semantic/src/semantic_index/except_handlers.rs b/crates/red_knot_python_semantic/src/semantic_index/builder/except_handlers.rs similarity index 68% rename from crates/red_knot_python_semantic/src/semantic_index/except_handlers.rs rename to crates/red_knot_python_semantic/src/semantic_index/builder/except_handlers.rs index 2eded0089956d..f4dac8088e08a 100644 --- a/crates/red_knot_python_semantic/src/semantic_index/except_handlers.rs +++ b/crates/red_knot_python_semantic/src/semantic_index/builder/except_handlers.rs @@ -1,7 +1,7 @@ use std::cell::{RefCell, RefMut}; -use super::use_def::FlowSnapshot; use super::SemanticIndexBuilder; +use crate::semantic_index::use_def::FlowSnapshot; #[derive(Debug, Default)] pub(super) struct TryBlockContextsStack(Vec); @@ -18,11 +18,42 @@ impl TryBlockContextsStack { } pub(super) fn pop_context(&mut self) { - self.0.pop(); + let popped_context = self.0.pop(); + assert_ne!( + popped_context, None, + "pop_context() should never be called on an empty stack \ +(this indicates an unbalanced `push_context()`/`pop_context()` pair of calls)" + ); } } -#[derive(Debug, Default)] +#[derive(Debug, Default, PartialEq, Eq)] +pub(super) struct TryBlockContexts(RefCell>); + +impl TryBlockContexts { + pub(super) fn push_try_block(&self) { + self.0.borrow_mut().push(TryBlockContext::default()); + } + + pub(super) fn pop_try_block(&self) -> Option { + self.0.borrow_mut().pop() + } + + pub(super) fn borrow_mut(&self) -> TryBlockContextsRefMut { + TryBlockContextsRefMut(self.0.borrow_mut()) + } +} + +#[derive(Debug)] +pub(super) struct TryBlockContextsRefMut<'a>(RefMut<'a, Vec>); + +impl<'a> TryBlockContextsRefMut<'a> { + pub(super) fn current_try_block(&mut self) -> Option<&mut TryBlockContext> { + self.0.last_mut() + } +} + +#[derive(Debug, Default, PartialEq, Eq)] pub(super) struct TryBlockContext { snapshots: Vec, visiting_nested_try_stmt: bool, @@ -30,16 +61,20 @@ pub(super) struct TryBlockContext { impl TryBlockContext { pub(super) fn record_definition(&mut self, builder: &SemanticIndexBuilder) { + // The `if !self.visiting_nested_try_stmt` check here isn't necessary for correctness. + // It's a worthwhile optimisation, however: if we're visiting a nested `try/except/else/finally` + // block, we'll push a single snapshot on completion of visiting that block, + // meaning that pushing intermediate snapshots during visitation of that block if !self.visiting_nested_try_stmt { self.snapshots.push(builder.flow_snapshot()); } } - pub(super) fn record_visiting_nested_try_stmt(&mut self) { + pub(super) fn enter_nested_try_stmt(&mut self) { self.visiting_nested_try_stmt = true; } - pub(super) fn record_exiting_nested_try_stmt(&mut self) { + pub(super) fn exit_nested_try_stmt(&mut self) { self.visiting_nested_try_stmt = false; } @@ -51,29 +86,3 @@ impl TryBlockContext { self.snapshots } } - -#[derive(Debug)] -pub(super) struct TryBlockContextsRefMut<'a>(RefMut<'a, Vec>); - -impl<'a> TryBlockContextsRefMut<'a> { - pub(super) fn current_try_block(&mut self) -> Option<&mut TryBlockContext> { - self.0.last_mut() - } -} - -#[derive(Debug, Default)] -pub(super) struct TryBlockContexts(RefCell>); - -impl TryBlockContexts { - pub(super) fn push_try_block(&self) { - self.0.borrow_mut().push(TryBlockContext::default()); - } - - pub(super) fn pop_try_block(&self) -> Option { - self.0.borrow_mut().pop() - } - - pub(super) fn borrow_mut(&self) -> TryBlockContextsRefMut { - TryBlockContextsRefMut(self.0.borrow_mut()) - } -} 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 a4b2a3e3cc07f..b62a4a6ca0ddf 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 @@ -426,7 +426,7 @@ impl<'map, 'db> Iterator for DeclarationsIterator<'map, 'db> { impl std::iter::FusedIterator for DeclarationsIterator<'_, '_> {} /// A snapshot of the definitions and constraints state at a particular point in control flow. -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq, Eq)] pub(super) struct FlowSnapshot { symbol_states: IndexVec, }