From f2a50a265e477a9d9cacd0aeaf4e2791e4df518d Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 10:43:02 +0100 Subject: [PATCH 01/20] test: add coverage for dynamic import with importlib --- tests/data/some_dyn_imports.py | 3 +++ tests/unit/imports/test_extract.py | 9 +++++++++ 2 files changed, 12 insertions(+) create mode 100644 tests/data/some_dyn_imports.py diff --git a/tests/data/some_dyn_imports.py b/tests/data/some_dyn_imports.py new file mode 100644 index 00000000..953641fc --- /dev/null +++ b/tests/data/some_dyn_imports.py @@ -0,0 +1,3 @@ +from importlib import import_module + +import_module("polars") diff --git a/tests/unit/imports/test_extract.py b/tests/unit/imports/test_extract.py index 142250e6..4667eeb1 100644 --- a/tests/unit/imports/test_extract.py +++ b/tests/unit/imports/test_extract.py @@ -17,6 +17,15 @@ from _pytest.logging import LogCaptureFixture +def test_dyn_import_parser_py() -> None: + some_dyn_imports_path = Path("tests/data/some_dyn_imports.py") + + assert get_imported_modules_from_list_of_files([some_dyn_imports_path]) == { + "importlib": [Location(some_dyn_imports_path, line=1, column=1)] + "polars": [Location(some_dyn_imports_path, line=3, column=1)] + } + + def test_import_parser_py() -> None: some_imports_path = Path("tests/data/some_imports.py") From 735b43fead48706cee42dbc90fb2e7df0e4b9884 Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 10:44:08 +0100 Subject: [PATCH 02/20] style: lint --- tests/unit/imports/test_extract.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/imports/test_extract.py b/tests/unit/imports/test_extract.py index 4667eeb1..90b5f11f 100644 --- a/tests/unit/imports/test_extract.py +++ b/tests/unit/imports/test_extract.py @@ -21,8 +21,8 @@ def test_dyn_import_parser_py() -> None: some_dyn_imports_path = Path("tests/data/some_dyn_imports.py") assert get_imported_modules_from_list_of_files([some_dyn_imports_path]) == { - "importlib": [Location(some_dyn_imports_path, line=1, column=1)] - "polars": [Location(some_dyn_imports_path, line=3, column=1)] + "importlib": [Location(some_dyn_imports_path, line=1, column=1)], + "polars": [Location(some_dyn_imports_path, line=3, column=1)], } From 709acb8eccde17e802290dad01916e439ee3914b Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 11:40:57 +0100 Subject: [PATCH 03/20] feat(WIP): attempt to get importlib imports --- src/visitor.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/visitor.rs b/src/visitor.rs index e6ed0e97..b0aba233 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -45,6 +45,25 @@ impl<'a> Visitor<'a> for ImportVisitor { Stmt::If(if_stmt) if is_guarded_by_type_checking(if_stmt) => { // Avoid parsing imports that are only evaluated by type checkers. } + Stmt::Expr(expr_stmt) => { + if let Expr::Call(call_expr) = expr_stmt.value.as_ref() { + if let Expr::Attribute(attr_expr) = call_expr.func.as_ref() { + if let Expr::Name(name) = attr_expr.value.as_ref() { + if name.id.as_str() == "importlib" && attr_expr.attr.as_str() == "import_module" { + if let Some(arg) = call_expr.arguments.args.first() { + if let Expr::StringLiteral(string_literal) = arg { + let top_level_module = get_top_level_module_name(&string_literal.value.to_string()); + self.imports + .entry(top_level_module) + .or_default() + .push(expr_stmt.range); + } + } + } + } + } + } + } _ => walk_stmt(self, stmt), // Delegate other statements to walk_stmt } } From 025df9734f4699b6025303079f81e917118bd19c Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 11:45:29 +0100 Subject: [PATCH 04/20] test: add test coverage for both importlib formats --- tests/data/some_dyn_imports.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/data/some_dyn_imports.py b/tests/data/some_dyn_imports.py index 953641fc..651589e0 100644 --- a/tests/data/some_dyn_imports.py +++ b/tests/data/some_dyn_imports.py @@ -1,3 +1,5 @@ from importlib import import_module +import importlib import_module("polars") +importlib.import_module("patito") From 73c790a033666ae7601562cce29626937d7791fe Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 11:59:27 +0100 Subject: [PATCH 05/20] feat: also handle case where importlib.import_module is aliased --- src/visitor.rs | 58 ++++++++++++++++++++++-------- tests/data/some_dyn_imports.py | 2 ++ tests/unit/imports/test_extract.py | 13 +++++-- 3 files changed, 57 insertions(+), 16 deletions(-) diff --git a/src/visitor.rs b/src/visitor.rs index b0aba233..76fc7fda 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -6,12 +6,14 @@ use std::collections::HashMap; #[derive(Debug, Clone)] pub struct ImportVisitor { imports: HashMap>, + import_module_name: Option, } impl ImportVisitor { pub fn new() -> Self { Self { imports: HashMap::new(), + import_module_name: None, } } @@ -25,20 +27,39 @@ impl<'a> Visitor<'a> for ImportVisitor { match stmt { Stmt::Import(import_stmt) => { for alias in &import_stmt.names { - let top_level_module = get_top_level_module_name(&alias.name); + let top_level_module = get_top_level_module_name(alias.name.as_str()); self.imports .entry(top_level_module) .or_default() .push(alias.range); + + if alias.name.as_str() == "importlib" { + self.import_module_name = Some("import_module".to_string()); + } } } Stmt::ImportFrom(import_from_stmt) => { if let Some(module) = &import_from_stmt.module { if import_from_stmt.level == 0 { + let module_name = module.as_str(); self.imports - .entry(get_top_level_module_name(module.as_str())) + .entry(get_top_level_module_name(module_name)) .or_default() .push(import_from_stmt.range); + + if module_name == "importlib" { + for alias in &import_from_stmt.names { + if alias.name.as_str() == "import_module" { + self.import_module_name = Some( + alias.asname + .as_ref() + .map(|id| id.as_str().to_string()) + .unwrap_or_else(|| "import_module".to_string()) + ); + break; + } + } + } } } } @@ -47,18 +68,27 @@ impl<'a> Visitor<'a> for ImportVisitor { } Stmt::Expr(expr_stmt) => { if let Expr::Call(call_expr) = expr_stmt.value.as_ref() { - if let Expr::Attribute(attr_expr) = call_expr.func.as_ref() { - if let Expr::Name(name) = attr_expr.value.as_ref() { - if name.id.as_str() == "importlib" && attr_expr.attr.as_str() == "import_module" { - if let Some(arg) = call_expr.arguments.args.first() { - if let Expr::StringLiteral(string_literal) = arg { - let top_level_module = get_top_level_module_name(&string_literal.value.to_string()); - self.imports - .entry(top_level_module) - .or_default() - .push(expr_stmt.range); - } - } + let is_import_module = match call_expr.func.as_ref() { + Expr::Attribute(attr_expr) => { + // Case: importlib.import_module(...) + matches!(attr_expr.value.as_ref(), Expr::Name(name) if name.id.as_str() == "importlib") + && attr_expr.attr.as_str() == "import_module" + } + Expr::Name(name) => { + // Case: import_module(...) or aliased version + self.import_module_name.as_ref().map_or(false, |im_name| name.id.as_str() == im_name) + } + _ => false, + }; + + if is_import_module { + if let Some(arg) = call_expr.arguments.args.first() { + if let Expr::StringLiteral(string_literal) = arg { + let top_level_module = get_top_level_module_name(&string_literal.value.to_string()); + self.imports + .entry(top_level_module) + .or_default() + .push(expr_stmt.range); } } } diff --git a/tests/data/some_dyn_imports.py b/tests/data/some_dyn_imports.py index 651589e0..2cea55a8 100644 --- a/tests/data/some_dyn_imports.py +++ b/tests/data/some_dyn_imports.py @@ -1,5 +1,7 @@ from importlib import import_module +from importlib import import_module as im import importlib import_module("polars") importlib.import_module("patito") +im("uvicorn") diff --git a/tests/unit/imports/test_extract.py b/tests/unit/imports/test_extract.py index 90b5f11f..60c8745f 100644 --- a/tests/unit/imports/test_extract.py +++ b/tests/unit/imports/test_extract.py @@ -21,8 +21,17 @@ def test_dyn_import_parser_py() -> None: some_dyn_imports_path = Path("tests/data/some_dyn_imports.py") assert get_imported_modules_from_list_of_files([some_dyn_imports_path]) == { - "importlib": [Location(some_dyn_imports_path, line=1, column=1)], - "polars": [Location(some_dyn_imports_path, line=3, column=1)], + "importlib": [ + Location(some_dyn_imports_path, line=1, column=1), + Location(some_dyn_imports_path, line=2, column=1), + Location(some_dyn_imports_path, line=3, column=8), + ], + "patito": [ + Location(some_dyn_imports_path, line=6, column=1), + ], + "polars": [ + Location(some_dyn_imports_path, line=5, column=1), + ], } From 88b10b465f0fb5bf8a4b0a076abb7759705ff836 Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 13:56:01 +0100 Subject: [PATCH 06/20] refactor: consolidate proof of concept test into existing test --- src/visitor.rs | 12 +++++--- tests/data/some_dyn_imports.py | 7 ----- tests/data/some_imports.py | 7 +++++ tests/unit/imports/test_extract.py | 49 ++++++++++++------------------ 4 files changed, 34 insertions(+), 41 deletions(-) delete mode 100644 tests/data/some_dyn_imports.py diff --git a/src/visitor.rs b/src/visitor.rs index 76fc7fda..faf40b67 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -51,10 +51,11 @@ impl<'a> Visitor<'a> for ImportVisitor { for alias in &import_from_stmt.names { if alias.name.as_str() == "import_module" { self.import_module_name = Some( - alias.asname + alias + .asname .as_ref() .map(|id| id.as_str().to_string()) - .unwrap_or_else(|| "import_module".to_string()) + .unwrap_or_else(|| "import_module".to_string()), ); break; } @@ -76,7 +77,9 @@ impl<'a> Visitor<'a> for ImportVisitor { } Expr::Name(name) => { // Case: import_module(...) or aliased version - self.import_module_name.as_ref().map_or(false, |im_name| name.id.as_str() == im_name) + self.import_module_name + .as_ref() + .map_or(false, |im_name| name.id.as_str() == im_name) } _ => false, }; @@ -84,7 +87,8 @@ impl<'a> Visitor<'a> for ImportVisitor { if is_import_module { if let Some(arg) = call_expr.arguments.args.first() { if let Expr::StringLiteral(string_literal) = arg { - let top_level_module = get_top_level_module_name(&string_literal.value.to_string()); + let top_level_module = + get_top_level_module_name(&string_literal.value.to_string()); self.imports .entry(top_level_module) .or_default() diff --git a/tests/data/some_dyn_imports.py b/tests/data/some_dyn_imports.py deleted file mode 100644 index 2cea55a8..00000000 --- a/tests/data/some_dyn_imports.py +++ /dev/null @@ -1,7 +0,0 @@ -from importlib import import_module -from importlib import import_module as im -import importlib - -import_module("polars") -importlib.import_module("patito") -im("uvicorn") diff --git a/tests/data/some_imports.py b/tests/data/some_imports.py index fb68b678..b225dd00 100644 --- a/tests/data/some_imports.py +++ b/tests/data/some_imports.py @@ -2,6 +2,9 @@ from os import chdir, walk from pathlib import Path from typing import List, TYPE_CHECKING +from importlib import import_module +from importlib import import_module as im +import importlib import numpy as np import pandas @@ -40,3 +43,7 @@ def func(): class MyClass: def __init__(self): import module_in_class + +import_module("polars") +importlib.import_module("patito") +im("uvicorn") diff --git a/tests/unit/imports/test_extract.py b/tests/unit/imports/test_extract.py index 60c8745f..9107c2e9 100644 --- a/tests/unit/imports/test_extract.py +++ b/tests/unit/imports/test_extract.py @@ -17,44 +17,33 @@ from _pytest.logging import LogCaptureFixture -def test_dyn_import_parser_py() -> None: - some_dyn_imports_path = Path("tests/data/some_dyn_imports.py") - - assert get_imported_modules_from_list_of_files([some_dyn_imports_path]) == { - "importlib": [ - Location(some_dyn_imports_path, line=1, column=1), - Location(some_dyn_imports_path, line=2, column=1), - Location(some_dyn_imports_path, line=3, column=8), - ], - "patito": [ - Location(some_dyn_imports_path, line=6, column=1), - ], - "polars": [ - Location(some_dyn_imports_path, line=5, column=1), - ], - } - - def test_import_parser_py() -> None: some_imports_path = Path("tests/data/some_imports.py") assert get_imported_modules_from_list_of_files([some_imports_path]) == { - "barfoo": [Location(some_imports_path, 21, 8)], - "baz": [Location(some_imports_path, 17, 5)], - "click": [Location(some_imports_path, 31, 12)], - "foobar": [Location(some_imports_path, 19, 12)], - "httpx": [Location(some_imports_path, 15, 12)], - "module_in_class": [Location(some_imports_path, 42, 16)], - "module_in_func": [Location(some_imports_path, 37, 12)], - "not_click": [Location(some_imports_path, 33, 12)], + "barfoo": [Location(some_imports_path, 24, 8)], + "baz": [Location(some_imports_path, 20, 5)], + "click": [Location(some_imports_path, 34, 12)], + "foobar": [Location(some_imports_path, 22, 12)], + "httpx": [Location(some_imports_path, 18, 12)], + "importlib": [ + Location(some_imports_path, line=5, column=1), + Location(some_imports_path, line=6, column=1), + Location(some_imports_path, line=7, column=8), + ], + "module_in_class": [Location(some_imports_path, 45, 16)], + "module_in_func": [Location(some_imports_path, 40, 12)], + "not_click": [Location(some_imports_path, 36, 12)], "numpy": [ - Location(some_imports_path, 6, 8), - Location(some_imports_path, 8, 1), + Location(some_imports_path, 9, 8), + Location(some_imports_path, 11, 1), ], "os": [Location(some_imports_path, 2, 1)], - "pandas": [Location(some_imports_path, 7, 8)], + "pandas": [Location(some_imports_path, 10, 8)], "pathlib": [Location(some_imports_path, 3, 1)], - "randomizer": [Location(some_imports_path, 22, 1)], + "patito": [Location(some_imports_path, line=48, column=1)], + "polars": [Location(some_imports_path, line=47, column=1)], + "randomizer": [Location(some_imports_path, 25, 1)], "typing": [ Location(some_imports_path, 1, 8), Location(some_imports_path, 4, 1), From 05cf8ea161e2d43504485adf84060bb21167535c Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 14:00:36 +0100 Subject: [PATCH 07/20] refactor(clippy): collapse nested `if let` statements in `Stmt::Expr` match arm --- src/visitor.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/visitor.rs b/src/visitor.rs index faf40b67..90f723d1 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -85,15 +85,15 @@ impl<'a> Visitor<'a> for ImportVisitor { }; if is_import_module { - if let Some(arg) = call_expr.arguments.args.first() { - if let Expr::StringLiteral(string_literal) = arg { - let top_level_module = - get_top_level_module_name(&string_literal.value.to_string()); - self.imports - .entry(top_level_module) - .or_default() - .push(expr_stmt.range); - } + if let Some(Expr::StringLiteral(string_literal)) = + call_expr.arguments.args.first() + { + let top_level_module = + get_top_level_module_name(&string_literal.value.to_string()); + self.imports + .entry(top_level_module) + .or_default() + .push(expr_stmt.range); } } } From 0a0245aa0bd8123d7657b9e16351bf0220c9aaa3 Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 14:13:02 +0100 Subject: [PATCH 08/20] docs: note which sorts of `importlib` import are supported --- docs/usage.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/usage.md b/docs/usage.md index a1b1d9eb..343d434b 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -70,6 +70,9 @@ if TYPE_CHECKING: import mypy_boto3_s3 ``` +There is some support for imports created with `importlib`: namely any usage with a string literal +(not where the argument is provided dynamically from a variable, attribute, etc.). + ## Excluding files and directories To determine issues with imported modules and dependencies, _deptry_ will scan the working directory and its subdirectories recursively for `.py` and `.ipynb` files, so it can From eb2116be44550697f2ebd7791706a5eb3b641884 Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 14:33:09 +0100 Subject: [PATCH 09/20] test: check nested packages with import_module resolve to the top level package (e.g. `http.server` -> `http`) --- tests/data/some_imports.py | 5 +++-- tests/unit/imports/test_extract.py | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/data/some_imports.py b/tests/data/some_imports.py index b225dd00..a702b064 100644 --- a/tests/data/some_imports.py +++ b/tests/data/some_imports.py @@ -44,6 +44,7 @@ class MyClass: def __init__(self): import module_in_class -import_module("polars") -importlib.import_module("patito") +import_module("patito") +importlib.import_module("polars") im("uvicorn") +import_module("http.server") diff --git a/tests/unit/imports/test_extract.py b/tests/unit/imports/test_extract.py index 9107c2e9..b1ddc1f5 100644 --- a/tests/unit/imports/test_extract.py +++ b/tests/unit/imports/test_extract.py @@ -25,6 +25,7 @@ def test_import_parser_py() -> None: "baz": [Location(some_imports_path, 20, 5)], "click": [Location(some_imports_path, 34, 12)], "foobar": [Location(some_imports_path, 22, 12)], + "http": [Location(some_imports_path, line=50, column=1)], "httpx": [Location(some_imports_path, 18, 12)], "importlib": [ Location(some_imports_path, line=5, column=1), @@ -41,8 +42,8 @@ def test_import_parser_py() -> None: "os": [Location(some_imports_path, 2, 1)], "pandas": [Location(some_imports_path, 10, 8)], "pathlib": [Location(some_imports_path, 3, 1)], - "patito": [Location(some_imports_path, line=48, column=1)], - "polars": [Location(some_imports_path, line=47, column=1)], + "patito": [Location(some_imports_path, line=47, column=1)], + "polars": [Location(some_imports_path, line=48, column=1)], "randomizer": [Location(some_imports_path, 25, 1)], "typing": [ Location(some_imports_path, 1, 8), From 2c5075958db1641bb8803929118c9804a13bbfdc Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 14:34:54 +0100 Subject: [PATCH 10/20] test: test coverage for aliased import (not being collected) --- tests/unit/imports/test_extract.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/unit/imports/test_extract.py b/tests/unit/imports/test_extract.py index b1ddc1f5..ca123eed 100644 --- a/tests/unit/imports/test_extract.py +++ b/tests/unit/imports/test_extract.py @@ -49,6 +49,7 @@ def test_import_parser_py() -> None: Location(some_imports_path, 1, 8), Location(some_imports_path, 4, 1), ], + "uvicorn": [Location(some_imports_path, line=49, column=1)], } From 1aabac6bf4922dacb16aa1302edb9d426c50e2d2 Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 14:53:12 +0100 Subject: [PATCH 11/20] fix: working import alias coverage --- src/visitor.rs | 43 ++++++++++++++++--------------------------- 1 file changed, 16 insertions(+), 27 deletions(-) diff --git a/src/visitor.rs b/src/visitor.rs index 90f723d1..0f9ae8f5 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -1,19 +1,19 @@ use ruff_python_ast::visitor::{walk_stmt, Visitor}; use ruff_python_ast::{self, Expr, ExprAttribute, ExprName, Stmt, StmtIf}; use ruff_text_size::TextRange; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; #[derive(Debug, Clone)] pub struct ImportVisitor { imports: HashMap>, - import_module_name: Option, + import_module_names: HashSet, } impl ImportVisitor { pub fn new() -> Self { Self { imports: HashMap::new(), - import_module_name: None, + import_module_names: HashSet::new(), } } @@ -34,7 +34,7 @@ impl<'a> Visitor<'a> for ImportVisitor { .push(alias.range); if alias.name.as_str() == "importlib" { - self.import_module_name = Some("import_module".to_string()); + self.import_module_names.insert("import_module".to_string()); } } } @@ -50,46 +50,32 @@ impl<'a> Visitor<'a> for ImportVisitor { if module_name == "importlib" { for alias in &import_from_stmt.names { if alias.name.as_str() == "import_module" { - self.import_module_name = Some( - alias - .asname - .as_ref() - .map(|id| id.as_str().to_string()) - .unwrap_or_else(|| "import_module".to_string()), - ); - break; + let name = alias + .asname + .as_ref() + .map(|id| id.as_str().to_string()) + .unwrap_or_else(|| "import_module".to_string()); + self.import_module_names.insert(name); } } } } } } - Stmt::If(if_stmt) if is_guarded_by_type_checking(if_stmt) => { - // Avoid parsing imports that are only evaluated by type checkers. - } Stmt::Expr(expr_stmt) => { if let Expr::Call(call_expr) = expr_stmt.value.as_ref() { let is_import_module = match call_expr.func.as_ref() { Expr::Attribute(attr_expr) => { - // Case: importlib.import_module(...) matches!(attr_expr.value.as_ref(), Expr::Name(name) if name.id.as_str() == "importlib") && attr_expr.attr.as_str() == "import_module" } - Expr::Name(name) => { - // Case: import_module(...) or aliased version - self.import_module_name - .as_ref() - .map_or(false, |im_name| name.id.as_str() == im_name) - } + Expr::Name(name) => self.import_module_names.contains(name.id.as_str()), _ => false, }; if is_import_module { - if let Some(Expr::StringLiteral(string_literal)) = - call_expr.arguments.args.first() - { - let top_level_module = - get_top_level_module_name(&string_literal.value.to_string()); + if let Some(Expr::StringLiteral(string_literal)) = call_expr.arguments.args.first() { + let top_level_module = get_top_level_module_name(&string_literal.value.to_string()); self.imports .entry(top_level_module) .or_default() @@ -98,6 +84,9 @@ impl<'a> Visitor<'a> for ImportVisitor { } } } + Stmt::If(if_stmt) if is_guarded_by_type_checking(if_stmt) => { + // Avoid parsing imports that are only evaluated by type checkers. + } _ => walk_stmt(self, stmt), // Delegate other statements to walk_stmt } } From 1b60cf10b1f3e170b445e747600cd96894a2bdaf Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 15:06:43 +0100 Subject: [PATCH 12/20] feat: full import path so importlib aliases are acknowledged too --- src/visitor.rs | 29 ++++++++++++++++++------- tests/data/some_imports.py | 2 ++ tests/unit/imports/test_extract.py | 34 ++++++++++++++++-------------- 3 files changed, 41 insertions(+), 24 deletions(-) diff --git a/src/visitor.rs b/src/visitor.rs index 0f9ae8f5..36735380 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -34,7 +34,13 @@ impl<'a> Visitor<'a> for ImportVisitor { .push(alias.range); if alias.name.as_str() == "importlib" { - self.import_module_names.insert("import_module".to_string()); + let name = alias + .asname + .as_ref() + .map(|id| id.as_str()) + .unwrap_or("importlib"); + self.import_module_names + .insert(format!("{}.import_module", name)); } } } @@ -53,9 +59,9 @@ impl<'a> Visitor<'a> for ImportVisitor { let name = alias .asname .as_ref() - .map(|id| id.as_str().to_string()) - .unwrap_or_else(|| "import_module".to_string()); - self.import_module_names.insert(name); + .map(|id| id.as_str()) + .unwrap_or("import_module"); + self.import_module_names.insert(name.to_string()); } } } @@ -66,16 +72,23 @@ impl<'a> Visitor<'a> for ImportVisitor { if let Expr::Call(call_expr) = expr_stmt.value.as_ref() { let is_import_module = match call_expr.func.as_ref() { Expr::Attribute(attr_expr) => { - matches!(attr_expr.value.as_ref(), Expr::Name(name) if name.id.as_str() == "importlib") - && attr_expr.attr.as_str() == "import_module" + if let Expr::Name(name) = attr_expr.value.as_ref() { + self.import_module_names + .contains(&format!("{}.import_module", name.id.as_str())) + } else { + false + } } Expr::Name(name) => self.import_module_names.contains(name.id.as_str()), _ => false, }; if is_import_module { - if let Some(Expr::StringLiteral(string_literal)) = call_expr.arguments.args.first() { - let top_level_module = get_top_level_module_name(&string_literal.value.to_string()); + if let Some(Expr::StringLiteral(string_literal)) = + call_expr.arguments.args.first() + { + let top_level_module = + get_top_level_module_name(&string_literal.value.to_string()); self.imports .entry(top_level_module) .or_default() diff --git a/tests/data/some_imports.py b/tests/data/some_imports.py index a702b064..bcbcb57e 100644 --- a/tests/data/some_imports.py +++ b/tests/data/some_imports.py @@ -5,6 +5,7 @@ from importlib import import_module from importlib import import_module as im import importlib +import importlib as il import numpy as np import pandas @@ -48,3 +49,4 @@ def __init__(self): importlib.import_module("polars") im("uvicorn") import_module("http.server") +il.import_module("xml.etree.ElementTree") diff --git a/tests/unit/imports/test_extract.py b/tests/unit/imports/test_extract.py index ca123eed..997e33ac 100644 --- a/tests/unit/imports/test_extract.py +++ b/tests/unit/imports/test_extract.py @@ -21,35 +21,37 @@ def test_import_parser_py() -> None: some_imports_path = Path("tests/data/some_imports.py") assert get_imported_modules_from_list_of_files([some_imports_path]) == { - "barfoo": [Location(some_imports_path, 24, 8)], - "baz": [Location(some_imports_path, 20, 5)], - "click": [Location(some_imports_path, 34, 12)], - "foobar": [Location(some_imports_path, 22, 12)], - "http": [Location(some_imports_path, line=50, column=1)], - "httpx": [Location(some_imports_path, 18, 12)], + "barfoo": [Location(some_imports_path, 25, 8)], + "baz": [Location(some_imports_path, 21, 5)], + "click": [Location(some_imports_path, 35, 12)], + "foobar": [Location(some_imports_path, 23, 12)], + "http": [Location(some_imports_path, line=51, column=1)], + "httpx": [Location(some_imports_path, 19, 12)], "importlib": [ Location(some_imports_path, line=5, column=1), Location(some_imports_path, line=6, column=1), Location(some_imports_path, line=7, column=8), + Location(some_imports_path, line=8, column=8), ], - "module_in_class": [Location(some_imports_path, 45, 16)], - "module_in_func": [Location(some_imports_path, 40, 12)], - "not_click": [Location(some_imports_path, 36, 12)], + "module_in_class": [Location(some_imports_path, 46, 16)], + "module_in_func": [Location(some_imports_path, 41, 12)], + "not_click": [Location(some_imports_path, 37, 12)], "numpy": [ - Location(some_imports_path, 9, 8), - Location(some_imports_path, 11, 1), + Location(some_imports_path, 10, 8), + Location(some_imports_path, 12, 1), ], "os": [Location(some_imports_path, 2, 1)], - "pandas": [Location(some_imports_path, 10, 8)], + "pandas": [Location(some_imports_path, 11, 8)], "pathlib": [Location(some_imports_path, 3, 1)], - "patito": [Location(some_imports_path, line=47, column=1)], - "polars": [Location(some_imports_path, line=48, column=1)], - "randomizer": [Location(some_imports_path, 25, 1)], + "patito": [Location(some_imports_path, line=48, column=1)], + "polars": [Location(some_imports_path, line=49, column=1)], + "randomizer": [Location(some_imports_path, 26, 1)], "typing": [ Location(some_imports_path, 1, 8), Location(some_imports_path, 4, 1), ], - "uvicorn": [Location(some_imports_path, line=49, column=1)], + "uvicorn": [Location(some_imports_path, line=50, column=1)], + "xml": [Location(some_imports_path, line=52, column=1)], } From 9008d49d4c5cb5c390ec0a2cd521b1a6e8e13cfa Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 17:24:07 +0100 Subject: [PATCH 13/20] docs: Clarify cases where `importlib.import_module` usage is/is not detected --- docs/usage.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index 343d434b..3d0c81c4 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -70,8 +70,20 @@ if TYPE_CHECKING: import mypy_boto3_s3 ``` -There is some support for imports created with `importlib`: namely any usage with a string literal -(not where the argument is provided dynamically from a variable, attribute, etc.). +There is some support for imports created with `importlib`: namely any usage with a string literal: + +``` +import importlib + +importlib.import_module("foo") # package 'foo' imported +``` + +But not where the argument is provided dynamically from a variable, attribute, etc. + +```py +bar = "foo" +importlib.import_module(bar) # Not detected +``` ## Excluding files and directories From 09439b7a95d3ec6d78613a615e87877db557aced Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 17:35:30 +0100 Subject: [PATCH 14/20] docs: docstring for `ImportVisitor` explaining what `import_module_names` is --- src/visitor.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/visitor.rs b/src/visitor.rs index 36735380..9860cc6c 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -9,6 +9,13 @@ pub struct ImportVisitor { import_module_names: HashSet, } +/// Visitor for tracking Python imports in AST. +/// +/// `imports`: Maps top-level module names to their import locations. +/// +/// `import_module_names`: Tracks import names (including aliases) that +/// refer to the `importlib.import_module` function itself, i.e. the import +/// alias for `importlib` package and/or the `importlib.import_module` function. impl ImportVisitor { pub fn new() -> Self { Self { From 3c27b8fcb5e8bf5954bd8691bb914056597ef69a Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 18:03:50 +0100 Subject: [PATCH 15/20] refactor: split long match statement arms into new functions with docstrings (`handle_import`, `handle_import_from`, `handle_expr`) --- src/visitor.rs | 157 +++++++++++++++++++++++++++++-------------------- 1 file changed, 92 insertions(+), 65 deletions(-) diff --git a/src/visitor.rs b/src/visitor.rs index 9860cc6c..c0aa216e 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -27,83 +27,110 @@ impl ImportVisitor { pub fn get_imports(self) -> HashMap> { self.imports } -} -impl<'a> Visitor<'a> for ImportVisitor { - fn visit_stmt(&mut self, stmt: &'a Stmt) { - match stmt { - Stmt::Import(import_stmt) => { - for alias in &import_stmt.names { - let top_level_module = get_top_level_module_name(alias.name.as_str()); - self.imports - .entry(top_level_module) - .or_default() - .push(alias.range); + /// Handles regular import statements (e.g., `import foo` or `import foo as bar`). + /// + /// Processes each name in the import statement (dealiasing if needed), and adds the + /// top-level module to the `imports` map. + /// + /// Imports of the `importlib` package are handled as a special case: its name (or alias), + /// suffixed with ".import_module", is stored in `import_module_names`. This is done to + /// indicate how we should look out for dynamic imports in the code that follows. + fn handle_import(&mut self, import_stmt: &ruff_python_ast::Import) { + for alias in &import_stmt.names { + let top_level_module = get_top_level_module_name(alias.name.as_str()); + self.imports + .entry(top_level_module) + .or_default() + .push(alias.range); - if alias.name.as_str() == "importlib" { - let name = alias - .asname - .as_ref() - .map(|id| id.as_str()) - .unwrap_or("importlib"); - self.import_module_names - .insert(format!("{}.import_module", name)); - } - } + if alias.name.as_str() == "importlib" { + let name = alias + .asname + .as_ref() + .map(|id| id.as_str()) + .unwrap_or("importlib"); + self.import_module_names + .insert(format!("{}.import_module", name)); } - Stmt::ImportFrom(import_from_stmt) => { - if let Some(module) = &import_from_stmt.module { - if import_from_stmt.level == 0 { - let module_name = module.as_str(); - self.imports - .entry(get_top_level_module_name(module_name)) - .or_default() - .push(import_from_stmt.range); + } + } - if module_name == "importlib" { - for alias in &import_from_stmt.names { - if alias.name.as_str() == "import_module" { - let name = alias - .asname - .as_ref() - .map(|id| id.as_str()) - .unwrap_or("import_module"); - self.import_module_names.insert(name.to_string()); - } - } + /// Handles "from" import statements (e.g., `from foo import bar` or `from foo import bar as baz`). + /// + /// Processes the module which the names are being imported from, adds it to the `imports` map + /// if it's a top-level import. + /// + /// Imports of `import_module` from the `importlib` package are handled as a special case: its + /// name (or alias), prefixed with "importlib.", is stored in `import_module_names`. This is done to + /// indicate how we should look out for dynamic imports in the code that follows. + fn handle_import_from(&mut self, import_from_stmt: &ruff_python_ast::ImportFrom) { + if let Some(module) = &import_from_stmt.module { + if import_from_stmt.level == 0 { + let module_name = module.as_str(); + self.imports + .entry(get_top_level_module_name(module_name)) + .or_default() + .push(import_from_stmt.range); + + if module_name == "importlib" { + for alias in &import_from_stmt.names { + if alias.name.as_str() == "import_module" { + let name = alias + .asname + .as_ref() + .map(|id| id.as_str()) + .unwrap_or("import_module"); + self.import_module_names.insert(name.to_string()); } } } } - Stmt::Expr(expr_stmt) => { - if let Expr::Call(call_expr) = expr_stmt.value.as_ref() { - let is_import_module = match call_expr.func.as_ref() { - Expr::Attribute(attr_expr) => { - if let Expr::Name(name) = attr_expr.value.as_ref() { - self.import_module_names - .contains(&format!("{}.import_module", name.id.as_str())) - } else { - false - } - } - Expr::Name(name) => self.import_module_names.contains(name.id.as_str()), - _ => false, - }; + } + } - if is_import_module { - if let Some(Expr::StringLiteral(string_literal)) = - call_expr.arguments.args.first() - { - let top_level_module = - get_top_level_module_name(&string_literal.value.to_string()); - self.imports - .entry(top_level_module) - .or_default() - .push(expr_stmt.range); - } + /// Handles expression statements, looking for dynamic imports using `importlib.import_module`. + /// + /// This function checks if the expression is a call to a function that might be `importlib.import_module` + /// (which could be import aliased at either the package or function name). If so, processes the imported + /// module name (which must be given as a string literal, not a variable) and adds it to the `imports` map. + fn handle_expr(&mut self, expr_stmt: &ruff_python_ast::ExprStmt) { + // Check for dynamic imports using `importlib.import_module` + if let Expr::Call(call_expr) = expr_stmt.value.as_ref() { + let is_import_module = match call_expr.func.as_ref() { + Expr::Attribute(attr_expr) => { + if let Expr::Name(name) = attr_expr.value.as_ref() { + self.import_module_names + .contains(&format!("{}.import_module", name.id.as_str())) + } else { + false } } + Expr::Name(name) => self.import_module_names.contains(name.id.as_str()), + _ => false, + }; + + if is_import_module { + if let Some(Expr::StringLiteral(string_literal)) = call_expr.arguments.args.first() + { + let top_level_module = + get_top_level_module_name(&string_literal.value.to_string()); + self.imports + .entry(top_level_module) + .or_default() + .push(expr_stmt.range); + } } + } + } +} + +impl<'a> Visitor<'a> for ImportVisitor { + fn visit_stmt(&mut self, stmt: &'a Stmt) { + match stmt { + Stmt::Import(import_stmt) => self.handle_import(import_stmt), + Stmt::ImportFrom(import_from_stmt) => self.handle_import_from(import_from_stmt), + Stmt::Expr(expr_stmt) => self.handle_expr(expr_stmt), Stmt::If(if_stmt) if is_guarded_by_type_checking(if_stmt) => { // Avoid parsing imports that are only evaluated by type checkers. } From 541a29e52b152ddcdb5440a31040e16d59c20a5c Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 18:15:53 +0100 Subject: [PATCH 16/20] style(consistency): Add language hint to code block in docs/usage.md Co-authored-by: Mathieu Kniewallner --- docs/usage.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/usage.md b/docs/usage.md index 3d0c81c4..8724bced 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -72,7 +72,7 @@ if TYPE_CHECKING: There is some support for imports created with `importlib`: namely any usage with a string literal: -``` +```python import importlib importlib.import_module("foo") # package 'foo' imported From bacb82d06738b2122dac285307a3d78fa49b78f6 Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 18:16:27 +0100 Subject: [PATCH 17/20] docs: Link to importlib stdlib docs URL in docs/usage.md Co-authored-by: Mathieu Kniewallner --- docs/usage.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/usage.md b/docs/usage.md index 8724bced..a59a0a6a 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -70,7 +70,7 @@ if TYPE_CHECKING: import mypy_boto3_s3 ``` -There is some support for imports created with `importlib`: namely any usage with a string literal: +There is some support for imports created with [`importlib.import_module`](https://docs.python.org/3/library/importlib.html#importlib.import_module) that use a string literal: ```python import importlib From 80062c8dcb919705d72e411d7200ae4839895089 Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 18:17:00 +0100 Subject: [PATCH 18/20] fix(consistency): Use full language hint on code block in docs/usage.md Co-authored-by: Mathieu Kniewallner --- docs/usage.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/usage.md b/docs/usage.md index a59a0a6a..55a54ddc 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -80,7 +80,7 @@ importlib.import_module("foo") # package 'foo' imported But not where the argument is provided dynamically from a variable, attribute, etc. -```py +```python bar = "foo" importlib.import_module(bar) # Not detected ``` From 5060df7a879f41072c8f3355701492ff37583daa Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 18:17:41 +0100 Subject: [PATCH 19/20] fix(typo): No capitalisation in sentence punctuated by code block in docs/usage.md Co-authored-by: Mathieu Kniewallner --- docs/usage.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/usage.md b/docs/usage.md index 55a54ddc..1d1fe138 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -78,7 +78,7 @@ import importlib importlib.import_module("foo") # package 'foo' imported ``` -But not where the argument is provided dynamically from a variable, attribute, etc. +but not where the argument is provided dynamically from a variable, attribute, etc., e.g.: ```python bar = "foo" From cb79798507c659d7a661f75868423bb87c8d1373 Mon Sep 17 00:00:00 2001 From: Louis Maddox Date: Sat, 27 Jul 2024 18:31:56 +0100 Subject: [PATCH 20/20] fix(typo): wrong type names used in refactored function --- src/visitor.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/visitor.rs b/src/visitor.rs index c0aa216e..2d28d9e0 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -1,5 +1,7 @@ use ruff_python_ast::visitor::{walk_stmt, Visitor}; -use ruff_python_ast::{self, Expr, ExprAttribute, ExprName, Stmt, StmtIf}; +use ruff_python_ast::{ + self, Expr, ExprAttribute, ExprName, Stmt, StmtExpr, StmtIf, StmtImport, StmtImportFrom, +}; use ruff_text_size::TextRange; use std::collections::{HashMap, HashSet}; @@ -36,7 +38,7 @@ impl ImportVisitor { /// Imports of the `importlib` package are handled as a special case: its name (or alias), /// suffixed with ".import_module", is stored in `import_module_names`. This is done to /// indicate how we should look out for dynamic imports in the code that follows. - fn handle_import(&mut self, import_stmt: &ruff_python_ast::Import) { + fn handle_import(&mut self, import_stmt: &StmtImport) { for alias in &import_stmt.names { let top_level_module = get_top_level_module_name(alias.name.as_str()); self.imports @@ -64,7 +66,7 @@ impl ImportVisitor { /// Imports of `import_module` from the `importlib` package are handled as a special case: its /// name (or alias), prefixed with "importlib.", is stored in `import_module_names`. This is done to /// indicate how we should look out for dynamic imports in the code that follows. - fn handle_import_from(&mut self, import_from_stmt: &ruff_python_ast::ImportFrom) { + fn handle_import_from(&mut self, import_from_stmt: &StmtImportFrom) { if let Some(module) = &import_from_stmt.module { if import_from_stmt.level == 0 { let module_name = module.as_str(); @@ -94,7 +96,7 @@ impl ImportVisitor { /// This function checks if the expression is a call to a function that might be `importlib.import_module` /// (which could be import aliased at either the package or function name). If so, processes the imported /// module name (which must be given as a string literal, not a variable) and adds it to the `imports` map. - fn handle_expr(&mut self, expr_stmt: &ruff_python_ast::ExprStmt) { + fn handle_expr(&mut self, expr_stmt: &StmtExpr) { // Check for dynamic imports using `importlib.import_module` if let Expr::Call(call_expr) = expr_stmt.value.as_ref() { let is_import_module = match call_expr.func.as_ref() {