From dc2c02b638296709f06ead8a464e542d9c215a2f Mon Sep 17 00:00:00 2001 From: Pierre-Yves Le Borgne Date: Sun, 4 Oct 2026 13:37:27 +0100 Subject: [PATCH] Don't exclude imports in the else branch of TYPE_CHECKING guards With exclude_type_checking_imports, every import inside an `if TYPE_CHECKING:` statement was treated as type checking only, including the imports in its `elif` and `else` branches. Those branches run when TYPE_CHECKING is false, that is, at runtime, so their imports were wrongly left out of the graph, and any contracts they break were kept. Only treat the branches guarded by TYPE_CHECKING as type checking only. This also recognises `elif TYPE_CHECKING:` again, as the import scanner did before it was ported to Rust, and stops a nested `if TYPE_CHECKING:` from marking the rest of the enclosing guard as runtime code. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01GwUvhmzgfkiZ6vo4uGyesN --- AUTHORS.rst | 1 + CHANGELOG.rst | 2 + docs/usage.rst | 7 ++- rust/src/import_parsing.rs | 39 ++++++------ tests/unit/application/test_scanning.py | 81 +++++++++++++++++++++++++ 5 files changed, 109 insertions(+), 21 deletions(-) diff --git a/AUTHORS.rst b/AUTHORS.rst index 9f408c53..c669f9dd 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -13,3 +13,4 @@ Authors * Nathan McDougall - https://github.com/nathanjmcdougall * Oleksandr Zaiats - https://github.com/z4y4ts * Nikhil Dabas - https://github.com/ndabas +* Pierre-Yves Le Borgne - https://github.com/pylaterreur diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 8ee3b38a..61d6af78 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,6 +7,8 @@ latest * Fix missing macOS wheels for regular (non-freethreaded) Python 3.14+ (https://github.com/python-grimp/grimp/issues/317). +* Fix imports in the ``else`` (or non-type checking ``elif``) branch of an ``if TYPE_CHECKING:`` + statement being excluded by ``exclude_type_checking_imports``, even though they run at runtime. 3.17 (2026-09-04) ----------------- diff --git a/docs/usage.rst b/docs/usage.rst index 6419c03b..e658df63 100644 --- a/docs/usage.rst +++ b/docs/usage.rst @@ -76,10 +76,11 @@ Building the graph *Note: external packages are only analysed as modules that are imported; any imports they make themselves will not be included in the graph.* :param bool, optional exclude_type_checking_imports: Whether to exclude imports made in type checking guards. If this is ``True``, - any import made under an ``if TYPE_CHECKING:`` statement will not be added to the graph. + any import made under an ``if TYPE_CHECKING:`` statement will not be added to the graph. Imports in its ``else`` + branch are still added, as that is the branch that runs at runtime. See the `typing module documentation`_ for reference. (The type checking guard is detected purely by looking for - a statement in the form ``if TYPE_CHECKING`` or ``if {some_alias}.TYPE_CHECKING``. It does not check whether - ``TYPE_CHECKING`` is actually the attribute from the ``typing`` module.) + a statement in the form ``if TYPE_CHECKING`` or ``if {some_alias}.TYPE_CHECKING``, or the ``elif`` equivalents. + It does not check whether ``TYPE_CHECKING`` is actually the attribute from the ``typing`` module.) :param str, optional cache_dir: The directory to use for caching the graph. Defaults to ``.grimp_cache``. To disable caching, pass ``None``. See :doc:`caching`. :return: An import graph that you can use to analyse the package. diff --git a/rust/src/import_parsing.rs b/rust/src/import_parsing.rs index 2d2fe0ac..fe11fce6 100644 --- a/rust/src/import_parsing.rs +++ b/rust/src/import_parsing.rs @@ -122,25 +122,19 @@ impl<'a> StatementVisitor<'a> for Visitor<'a> { } walk_stmt(self, stmt); } - Stmt::If(if_stmt) => match if_stmt.test.as_ref() { - Expr::Name(expr) => { - if expr.id == "TYPE_CHECKING" { - self.typechecking_only = true; - walk_stmt(self, stmt); - self.typechecking_only = false; - } else { - walk_stmt(self, stmt); - } + Stmt::If(if_stmt) => { + // Only the branches guarded by `TYPE_CHECKING` are type checking only. The other + // branches (e.g. `else`) run when `TYPE_CHECKING` is false, that is, at runtime. + let outer_typechecking_only = self.typechecking_only; + self.typechecking_only = outer_typechecking_only || is_type_checking(&if_stmt.test); + self.visit_body(&if_stmt.body); + for clause in &if_stmt.elif_else_clauses { + self.typechecking_only = outer_typechecking_only + || clause.test.as_ref().is_some_and(is_type_checking); + self.visit_elif_else_clause(clause); } - Expr::Attribute(expr) if expr.attr.id == "TYPE_CHECKING" => { - self.typechecking_only = true; - walk_stmt(self, stmt); - self.typechecking_only = false; - } - _ => { - walk_stmt(self, stmt); - } - }, + self.typechecking_only = outer_typechecking_only; + } _ => { walk_stmt(self, stmt); } @@ -148,6 +142,15 @@ impl<'a> StatementVisitor<'a> for Visitor<'a> { } } +/// Whether the expression is `TYPE_CHECKING` or `.TYPE_CHECKING`. +fn is_type_checking(expr: &Expr) -> bool { + match expr { + Expr::Name(name) => name.id == "TYPE_CHECKING", + Expr::Attribute(attribute) => attribute.attr.id == "TYPE_CHECKING", + _ => false, + } +} + #[cfg(test)] mod tests { use super::parse_imports_from_code; diff --git a/tests/unit/application/test_scanning.py b/tests/unit/application/test_scanning.py index a510e0b3..7f209442 100644 --- a/tests/unit/application/test_scanning.py +++ b/tests/unit/application/test_scanning.py @@ -1079,6 +1079,87 @@ def test_exclude_type_checking_imports( assert {module_foo_one_file: expected_result} == result +@pytest.mark.parametrize( + "code, expected_imported_modules", + ( + pytest.param( + """ + if TYPE_CHECKING: + import foo.two + else: + import foo.three + """, + {"foo.three"}, + id="else", + ), + pytest.param( + """ + if typing.TYPE_CHECKING: + import foo.two + elif sys.version_info >= (3, 12): + import foo.three + else: + import foo.four + """, + {"foo.three", "foo.four"}, + id="elif-and-else", + ), + pytest.param( + """ + if sys.version_info >= (3, 12): + import foo.two + elif TYPE_CHECKING: + import foo.three + else: + import foo.four + """, + {"foo.two", "foo.four"}, + id="elif-type-checking", + ), + pytest.param( + """ + if TYPE_CHECKING: + if sys.version_info >= (3, 12): + import foo.two + else: + import foo.three + if TYPE_CHECKING: + import foo.four + import foo.five + """, + set(), + id="nested", + ), + ), +) +def test_exclude_type_checking_imports_only_excludes_type_checking_branches( + code, expected_imported_modules +): + all_modules = {Module(f"foo.{name}") for name in ("one", "two", "three", "four", "five")} + module_foo_one_file = _module_to_module_file(Module("foo.one")) + file_system = rust.FakeBasicFileSystem(content_map={"/path/to/foo/one.py": code}) + found_packages = { + FoundPackage( + name="foo", + directory="/path/to/foo", + module_files=_modules_to_module_files(all_modules), + ) + } + + with override_settings(FILE_SYSTEM=file_system): + result = scanning.scan_imports( + {module_foo_one_file}, + found_packages=found_packages, + include_external_packages=False, + exclude_type_checking_imports=True, + ) + + imported_modules = { + direct_import.imported.name for direct_import in result[module_foo_one_file] + } + assert imported_modules == expected_imported_modules + + def test_t_string_syntax(): module_file_to_scan = _module_to_module_file(Module("foo.one"))