Skip to content

Commit d64a8bc

Browse files
martindemellometa-codesync[bot]
authored andcommitted
Filter explicit lazy imports out from inferred side-effect-only imports
Summary: When a module is imported but not used, we infer that it is imported for its side effects, and treat it as unsafe. However, if it is explicitly imported via `lazy import ...` it cannot have side effects at runtime, and we therefore need to filter it out. Reviewed By: brittanyrey Differential Revision: D107947785 fbshipit-source-id: 8bf4f3230a2e06a817c62e288886715fc2b412af
1 parent cda2212 commit d64a8bc

6 files changed

Lines changed: 223 additions & 9 deletions

File tree

src/module_effects.rs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ pub struct ModuleEffects {
4343
// i.e if we have import A as C and C.foo() is called we should map C.foo() to A.foo()
4444
// we get the canonical value using the re_exports table
4545
pub indirectly_called_methods: AHashMap<ModuleName, ModuleName>,
46+
47+
// Modules imported via non-lazy import statements. Used to distinguish
48+
// side-effect imports from explicit lazy imports that have no effect when unused.
49+
pub eager_imports: AHashSet<ModuleName>,
4650
}
4751

4852
impl ModuleEffects {
@@ -56,6 +60,7 @@ impl ModuleEffects {
5660
all_called_import_names: AHashSet::new(),
5761
called_functions: AHashSet::new(),
5862
indirectly_called_methods: AHashMap::new(),
63+
eager_imports: AHashSet::new(),
5964
}
6065
}
6166

src/project.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,7 @@ impl GlobalAnalysisState {
327327

328328
/// Compute side-effect imports: module-level imports never accessed in any scope.
329329
/// These are imports that exist solely for their side effects (e.g., decorator registration).
330+
/// Lazy imports that are never accessed have no side effects, so they are excluded.
330331
fn compute_side_effect_imports(analysis_map: &AnalysisMap) -> SideEffectMap {
331332
let results: Vec<_> = analysis_map
332333
.par_iter()
@@ -338,6 +339,7 @@ fn compute_side_effect_imports(analysis_map: &AnalysisMap) -> SideEffectMap {
338339

339340
let side_effects: AHashSet<ModuleName> = module_pending
340341
.difference(&output.module_effects.all_called_import_names)
342+
.filter(|m| output.module_effects.eager_imports.contains(m))
341343
.copied()
342344
.collect();
343345

src/source_analyzer.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1249,6 +1249,9 @@ impl<'a> SourceAnalyzer<'a> {
12491249
fn add_pending_import(&self, x: &StmtImport, output: &mut ModuleEffects) {
12501250
for name in &x.names {
12511251
let to = ModuleName::from_name(&name.name.id);
1252+
if !x.is_lazy {
1253+
output.eager_imports.insert(to);
1254+
}
12521255
match &name.asname {
12531256
None => output.add_pending_import(to, &self.cursor.scope()),
12541257
Some(asname) => {
@@ -1280,6 +1283,10 @@ impl<'a> SourceAnalyzer<'a> {
12801283
) {
12811284
let scope = self.cursor.scope();
12821285

1286+
if !x.is_lazy {
1287+
output.eager_imports.insert(m);
1288+
}
1289+
12831290
// `from x import y` adds `x` as a dependency
12841291
if m.as_str() != "" {
12851292
if is_called {

src/test_lib.rs

Lines changed: 34 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,13 @@ use crate::format::ErrorString;
2626
use crate::imports::ImportGraph;
2727
use crate::module_effects::ModuleEffects;
2828
use crate::module_parser::ParsedModule;
29-
use crate::module_parser::parse_pyi;
30-
use crate::module_parser::parse_source;
29+
use crate::module_parser::parse_pyi_with_version;
30+
use crate::module_parser::parse_source_with_version;
3131
use crate::module_safety::ModuleSafety;
3232
use crate::project;
3333
use crate::project::AnalysisMap;
3434
use crate::project::SafetyMap;
35+
use crate::pyrefly::sys_info::PythonVersion;
3536
use crate::source_map::AstResult;
3637
use crate::source_map::ModuleProvider;
3738
use crate::stubs::Stubs;
@@ -50,18 +51,27 @@ pub struct TestSources {
5051
parse_errors: AHashSet<ModuleName>,
5152
stubs: Stubs,
5253
names: Vec<ModuleName>,
54+
python_version: PythonVersion,
5355
}
5456

5557
impl TestSources {
5658
pub fn new(modules: &[(&str, &str)]) -> Self {
57-
Self::new_impl(modules, &[])
59+
Self::new_impl(modules, &[], PythonVersion::default())
60+
}
61+
62+
pub fn new_with_version(modules: &[(&str, &str)], python_version: PythonVersion) -> Self {
63+
Self::new_impl(modules, &[], python_version)
5864
}
5965

6066
pub fn new_with_stubs(modules: &[(&str, &str)], stub_names: &[&str]) -> Self {
61-
Self::new_impl(modules, stub_names)
67+
Self::new_impl(modules, stub_names, PythonVersion::default())
6268
}
6369

64-
fn new_impl(modules: &[(&str, &str)], stub_names: &[&str]) -> Self {
70+
fn new_impl(
71+
modules: &[(&str, &str)],
72+
stub_names: &[&str],
73+
python_version: PythonVersion,
74+
) -> Self {
6575
let stubs = Stubs::new();
6676

6777
// Collect all names: stubs first, then test modules (test modules override stubs)
@@ -86,6 +96,7 @@ impl TestSources {
8696
parse_errors: AHashSet::new(),
8797
stubs,
8898
names,
99+
python_version,
89100
}
90101
}
91102

@@ -126,20 +137,35 @@ impl ModuleProvider for TestSources {
126137
// Test modules take priority over stubs
127138
if let Some(code) = self.modules.get(name) {
128139
if self.stub_modules.contains(name) {
129-
return Some(AstResult::Ok(parse_pyi(code, *name, false)));
140+
return Some(AstResult::Ok(parse_pyi_with_version(
141+
code,
142+
*name,
143+
false,
144+
self.python_version,
145+
)));
130146
}
131147
// A module is an __init__.py (package) if any other module is a child of it
132148
let name_prefix = format!("{}.", name.as_str());
133149
let is_init = self
134150
.names
135151
.iter()
136152
.any(|n| n.as_str().starts_with(&name_prefix));
137-
return Some(AstResult::Ok(parse_source(code, *name, is_init)));
153+
return Some(AstResult::Ok(parse_source_with_version(
154+
code,
155+
*name,
156+
is_init,
157+
self.python_version,
158+
)));
138159
}
139160

140161
// Fall back to stubs
141162
if let Some(src) = self.stubs.get_raw_source(name) {
142-
return Some(AstResult::Ok(parse_pyi(src, *name, false)));
163+
return Some(AstResult::Ok(parse_pyi_with_version(
164+
src,
165+
*name,
166+
false,
167+
self.python_version,
168+
)));
143169
}
144170

145171
None

tests/Cargo.toml

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
# @generated by autocargo from //safer_lazy_imports/lifeguard/tests:[assign,attrs,basic,blocks,cache,calls,constructors,decorators,effects,encoding,exceptions,find_sources,gen_source_db,imports,nested_functions,nested_imports,output,param_mutation,port_test_analyzer,port_test_catch_implicit_imports,port_test_catch_import_cycles,port_test_finalization,port_test_interpreter,port_test_ownership,run_tree,source_analyzer,star_imports,stubs]
1+
# @generated by autocargo from //safer_lazy_imports/lifeguard/tests:[assign,attrs,basic,blocks,cache,calls,constructors,decorators,effects,encoding,exceptions,find_sources,gen_source_db,imports,lazy_imports,nested_functions,nested_imports,output,param_mutation,port_test_analyzer,port_test_catch_implicit_imports,port_test_catch_import_cycles,port_test_finalization,port_test_interpreter,port_test_ownership,run_tree,source_analyzer,star_imports,stubs]
22

33
[package]
44
name = "safer_lazy_imports_lifeguard_tests"
@@ -64,6 +64,10 @@ path = "gen_source_db.rs"
6464
name = "imports"
6565
path = "imports.rs"
6666

67+
[[test]]
68+
name = "lazy_imports"
69+
path = "lazy_imports.rs"
70+
6771
[[test]]
6872
name = "nested_functions"
6973
path = "nested_functions.rs"
@@ -121,6 +125,7 @@ name = "stubs"
121125
path = "stubs.rs"
122126

123127
[dev-dependencies]
128+
ahash = { version = "0.8.11", features = ["runtime-rng"] }
124129
anyhow = "1.0.102"
125130
clap = { version = "4.6.0", features = ["derive", "env", "string", "unicode", "wrap_help"] }
126131
lifeguard = { path = "../src" }

tests/lazy_imports.rs

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
/*
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*/
7+
8+
#[cfg(test)]
9+
mod tests {
10+
use ahash::AHashSet;
11+
use lifeguard::config::AnalysisConfig;
12+
use lifeguard::imports::ImportGraph;
13+
use lifeguard::project;
14+
use lifeguard::project::CachingMode;
15+
use lifeguard::pyrefly::module_name::ModuleName;
16+
use lifeguard::pyrefly::sys_info::PythonVersion;
17+
use lifeguard::test_lib::TestSources;
18+
19+
fn py315() -> PythonVersion {
20+
PythonVersion::new(3, 15, 0)
21+
}
22+
23+
fn side_effect_imports(modules: Vec<(&str, &str)>) -> Vec<(ModuleName, AHashSet<ModuleName>)> {
24+
let sources = TestSources::new_with_version(&modules, py315());
25+
let config = AnalysisConfig::with_python_version(py315(), None);
26+
let (import_graph, exports) = ImportGraph::make_with_exports(&sources, &config);
27+
let output = project::run_analysis(
28+
&sources,
29+
&exports,
30+
&import_graph,
31+
&config,
32+
CachingMode::Disabled,
33+
);
34+
35+
output
36+
.side_effect_imports
37+
.into_iter()
38+
.filter(|(_, v)| !v.is_empty())
39+
.collect()
40+
}
41+
42+
fn side_effects_for(modules: Vec<(&str, &str)>, module_name: &str) -> AHashSet<ModuleName> {
43+
let all = side_effect_imports(modules);
44+
let name = ModuleName::from_str(module_name);
45+
all.into_iter()
46+
.find(|(k, _)| *k == name)
47+
.map(|(_, v)| v)
48+
.unwrap_or_default()
49+
}
50+
51+
fn has_side_effect(modules: Vec<(&str, &str)>, module_name: &str, import: &str) -> bool {
52+
side_effects_for(modules, module_name).contains(&ModuleName::from_str(import))
53+
}
54+
55+
// --- Basic lazy import behavior ---
56+
57+
#[test]
58+
fn test_eager_import_unused_is_side_effect() {
59+
assert!(has_side_effect(
60+
vec![("main", "import a"), ("a", "x = 1")],
61+
"main",
62+
"a",
63+
));
64+
}
65+
66+
#[test]
67+
fn test_lazy_import_unused_is_not_side_effect() {
68+
assert!(!has_side_effect(
69+
vec![("main", "lazy import a"), ("a", "x = 1")],
70+
"main",
71+
"a",
72+
));
73+
}
74+
75+
#[test]
76+
fn test_lazy_import_used_is_not_side_effect() {
77+
let code = "lazy import a\nx = a.foo";
78+
assert!(!has_side_effect(
79+
vec![("main", code), ("a", "foo = 1")],
80+
"main",
81+
"a",
82+
));
83+
}
84+
85+
#[test]
86+
fn test_eager_import_used_is_not_side_effect() {
87+
let code = "import a\nx = a.foo";
88+
assert!(!has_side_effect(
89+
vec![("main", code), ("a", "foo = 1")],
90+
"main",
91+
"a",
92+
));
93+
}
94+
95+
// --- Lazy + eager interaction (order independent) ---
96+
97+
#[test]
98+
fn test_lazy_then_eager_import_unused_is_side_effect() {
99+
let code = "lazy import a\nimport a";
100+
assert!(has_side_effect(
101+
vec![("main", code), ("a", "x = 1")],
102+
"main",
103+
"a",
104+
));
105+
}
106+
107+
#[test]
108+
fn test_eager_then_lazy_import_unused_is_side_effect() {
109+
let code = "import a\nlazy import a";
110+
assert!(has_side_effect(
111+
vec![("main", code), ("a", "x = 1")],
112+
"main",
113+
"a",
114+
));
115+
}
116+
117+
// --- from-import variants ---
118+
119+
#[test]
120+
fn test_lazy_from_import_unused_is_not_side_effect() {
121+
assert!(!has_side_effect(
122+
vec![("main", "lazy from a import b"), ("a", "b = 1")],
123+
"main",
124+
"a",
125+
));
126+
}
127+
128+
#[test]
129+
fn test_eager_from_import_unused_is_side_effect() {
130+
assert!(has_side_effect(
131+
vec![("main", "from a import b"), ("a", "b = 1")],
132+
"main",
133+
"a",
134+
));
135+
}
136+
137+
// --- Submodule interaction: lazy import a; import a.b ---
138+
139+
#[test]
140+
fn test_lazy_parent_with_eager_submodule_import() {
141+
let code = "lazy import a\nimport a.b";
142+
let se = side_effects_for(
143+
vec![("main", code), ("a", "x = 1"), ("a.b", "y = 2")],
144+
"main",
145+
);
146+
// `a` is only imported lazily at module level, so not a side-effect import.
147+
// `a.b` is imported eagerly and unused, so it IS a side-effect import.
148+
// (a's side effects flow through the a.b import chain in the import graph)
149+
assert!(!se.contains(&ModuleName::from_str("a")));
150+
assert!(se.contains(&ModuleName::from_str("a.b")));
151+
}
152+
153+
// --- Multiple lazy imports ---
154+
155+
#[test]
156+
fn test_all_lazy_imports_unused_no_side_effects() {
157+
let code = "lazy import a\nlazy import b";
158+
let se = side_effects_for(vec![("main", code), ("a", "x = 1"), ("b", "y = 2")], "main");
159+
assert!(se.is_empty());
160+
}
161+
162+
#[test]
163+
fn test_mixed_lazy_and_eager_unused() {
164+
let code = "lazy import a\nimport b";
165+
let se = side_effects_for(vec![("main", code), ("a", "x = 1"), ("b", "y = 2")], "main");
166+
assert!(!se.contains(&ModuleName::from_str("a")));
167+
assert!(se.contains(&ModuleName::from_str("b")));
168+
}
169+
}

0 commit comments

Comments
 (0)