Skip to content

Commit fe0dd85

Browse files
stepanchegfacebook-github-bot
authored andcommitted
remove_unused_loads
Summary: The plan is to replace `arc f` buildifer with: * load removal implemented in starlark * code formatting with black This diff implements unused load removal. As a function in `starlark` crate for now. It does not work perfectly yet. One known issue is this: For the program: ``` load("foo", "x", "y") print(x) ``` It produces: ``` load("foo", "x", ) print(x) ``` and black for some mysterious reason reformats this code as: ``` load( "foo", "x", ) print(x) ``` instead of placing load on one line. This can be addressed later if we proceed with black. # libCST Note initially I planned to use libCST, but I think load removal is easy enough to implement it just by text manipulation, while implementing with libCST would require less-trivial communication of unused imports between rust and python code. Reviewed By: ianlevesque Differential Revision: D48941531 fbshipit-source-id: b4ba870300b8c32828a95f2cf800b10555e4a1cd
1 parent 771d2ba commit fe0dd85

23 files changed

+682
-4
lines changed

starlark/src/analysis/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ pub use lint_message::LintMessage;
2323
pub use types::EvalMessage;
2424
pub use types::EvalSeverity;
2525
pub use types::Lint;
26+
pub use unused_loads::remove::remove_unused_loads;
2627

2728
use crate::analysis::types::LintT;
2829
use crate::syntax::AstModule;
@@ -36,6 +37,7 @@ mod names;
3637
mod performance;
3738
mod types;
3839
mod underscore;
40+
mod unused_loads;
3941

4042
/// Run the linter.
4143
pub trait AstModuleLint {
Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
/*
2+
* Copyright 2019 The Starlark in Rust Authors.
3+
* Copyright (c) Facebook, Inc. and its affiliates.
4+
*
5+
* Licensed under the Apache License, Version 2.0 (the "License");
6+
* you may not use this file except in compliance with the License.
7+
* You may obtain a copy of the License at
8+
*
9+
* https://www.apache.org/licenses/LICENSE-2.0
10+
*
11+
* Unless required by applicable law or agreed to in writing, software
12+
* distributed under the License is distributed on an "AS IS" BASIS,
13+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
* See the License for the specific language governing permissions and
15+
* limitations under the License.
16+
*/
17+
18+
use std::collections::HashMap;
19+
20+
use anyhow::Context;
21+
use dupe::Dupe;
22+
use starlark_syntax::codemap::CodeMap;
23+
use starlark_syntax::codemap::FileSpanRef;
24+
use starlark_syntax::codemap::Spanned;
25+
use starlark_syntax::slice_vec_ext::SliceExt;
26+
use starlark_syntax::syntax::ast::LoadArgP;
27+
use starlark_syntax::syntax::ast::LoadP;
28+
use starlark_syntax::syntax::ast::StmtP;
29+
use starlark_syntax::syntax::top_level_stmts::top_level_stmts;
30+
31+
use crate::environment::names::MutableNames;
32+
use crate::eval::compiler::scope::payload::CstPayload;
33+
use crate::eval::compiler::scope::scope_resolver_globals::ScopeResolverGlobals;
34+
use crate::eval::compiler::scope::BindingId;
35+
use crate::eval::compiler::scope::ModuleScopes;
36+
use crate::eval::compiler::scope::ResolvedIdent;
37+
use crate::eval::compiler::scope::Slot;
38+
use crate::syntax::AstModule;
39+
use crate::syntax::Dialect;
40+
use crate::values::FrozenHeap;
41+
42+
/// Unused load statement.
43+
pub(crate) struct UnusedLoad {
44+
/// Location of the statement (i.e. position of `load` keyword).
45+
pub(crate) load: Spanned<LoadP<CstPayload>>,
46+
/// Unused local names, e. g. `x` in `load("foo", x="y")`.
47+
pub(crate) unused_args: Vec<LoadArgP<CstPayload>>,
48+
}
49+
50+
impl UnusedLoad {
51+
/// If the whole `load` statement is unused.
52+
pub(crate) fn all_unused(&self) -> bool {
53+
self.unused_args.len() == self.load.args.len()
54+
}
55+
}
56+
57+
/// Check if there are `@unused` markers on the lines with the given span.
58+
fn has_unused_marker_in_range(span: FileSpanRef) -> bool {
59+
let begin_line = span.file.find_line(span.span.begin());
60+
let end_line = span.file.find_line(span.span.end());
61+
for line_no in begin_line..=end_line {
62+
let line = span.file.source_line(line_no);
63+
if line.ends_with("@unused") || line.contains("@unused ") {
64+
return true;
65+
}
66+
}
67+
false
68+
}
69+
70+
/// Parse the module and find unused loads.
71+
pub(crate) fn find_unused_loads(
72+
name: &str,
73+
program: &str,
74+
) -> anyhow::Result<(CodeMap, Vec<UnusedLoad>)> {
75+
let module = AstModule::parse(name, program.to_owned(), &Dialect::Extended)?;
76+
let names = MutableNames::new();
77+
let heap = FrozenHeap::new();
78+
let codemap = heap.alloc_any_display_from_type_name(module.codemap);
79+
let module_scopes = ModuleScopes::check_module_err(
80+
&names,
81+
&heap,
82+
&HashMap::new(),
83+
module.statement,
84+
ScopeResolverGlobals::unknown(),
85+
codemap,
86+
&module.dialect,
87+
)?;
88+
89+
let mut loads = Vec::new();
90+
91+
struct LoadSymbol<'a> {
92+
arg: &'a LoadArgP<CstPayload>,
93+
binding_id: BindingId,
94+
used: bool,
95+
}
96+
97+
struct LoadWip<'a> {
98+
load: Spanned<&'a LoadP<CstPayload>>,
99+
args: Vec<LoadSymbol<'a>>,
100+
}
101+
102+
impl<'a> LoadWip<'a> {
103+
fn any_unused(&self) -> bool {
104+
self.args.iter().any(|arg| !arg.used)
105+
}
106+
}
107+
108+
for top in top_level_stmts(&module_scopes.cst) {
109+
if let StmtP::Load(load) = &**top {
110+
let args = load.args.try_map(|arg| {
111+
anyhow::Ok(LoadSymbol {
112+
arg,
113+
binding_id: arg.local.payload.context("payload is not set")?,
114+
used: false,
115+
})
116+
})?;
117+
loads.push(LoadWip {
118+
load: Spanned {
119+
span: top.span,
120+
node: load,
121+
},
122+
args,
123+
});
124+
}
125+
}
126+
127+
for top in top_level_stmts(&module_scopes.cst) {
128+
top.visit_ident(|ident| {
129+
println!("visit ident: {:?}", ident);
130+
let ResolvedIdent::Slot(Slot::Module(_), binding_id) = ident
131+
.payload
132+
.context("ident is not resolved (internal error)")?
133+
else {
134+
return Ok(());
135+
};
136+
for load in &mut loads {
137+
for arg in &mut load.args {
138+
if arg.binding_id == binding_id {
139+
arg.used = true;
140+
}
141+
}
142+
}
143+
anyhow::Ok(())
144+
})?;
145+
}
146+
147+
let mut unused = Vec::new();
148+
149+
for load in loads {
150+
if !load.any_unused() {
151+
continue;
152+
}
153+
let unused_args: Vec<_> = load
154+
.args
155+
.into_iter()
156+
.filter_map(|arg| {
157+
if arg.used {
158+
None
159+
} else if has_unused_marker_in_range(FileSpanRef {
160+
file: &codemap,
161+
span: arg.arg.span_with_trailing_comma(),
162+
}) {
163+
None
164+
} else {
165+
Some(arg.arg.clone())
166+
}
167+
})
168+
.collect();
169+
if unused_args.is_empty() {
170+
continue;
171+
}
172+
unused.push(UnusedLoad {
173+
load: load.load.map(|load| load.clone()),
174+
unused_args,
175+
});
176+
}
177+
178+
Ok(((*codemap).dupe(), unused))
179+
}
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# @generated
2+
# To regenerate, run:
3+
# ```
4+
# STARLARK_RUST_REGENERATE_GOLDEN_TESTS=1 cargo test -p starlark --lib tests
5+
# ```
6+
7+
Program:
8+
load("foo", "x", "y")
9+
print(x)
10+
11+
Unused loads:
12+
13+
error: Unused load
14+
--> one_of_two_unused:1:18
15+
|
16+
1 | load("foo", "x", "y")
17+
| ^^^
18+
|
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
# @generated
2+
# To regenerate, run:
3+
# ```
4+
# STARLARK_RUST_REGENERATE_GOLDEN_TESTS=1 cargo test -p starlark --lib tests
5+
# ```
6+
7+
Program:
8+
load("foo", "x")
9+
10+
Unused loads:
11+
12+
error: Unused load
13+
--> simple:1:1
14+
|
15+
1 | load("foo", "x")
16+
| ^^^^^^^^^^^^^^^^
17+
|
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
# @generated
2+
# To regenerate, run:
3+
# ```
4+
# STARLARK_RUST_REGENERATE_GOLDEN_TESTS=1 cargo test -p starlark --lib tests
5+
# ```
6+
7+
Program:
8+
load("foo",
9+
"T", # @unused
10+
"U",
11+
)
12+
13+
Unused loads:
14+
15+
error: Unused load
16+
--> unused_annotation_on_arg:3:5
17+
|
18+
3 | "U",
19+
| ^^^
20+
|
Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
# @generated
2+
# To regenerate, run:
3+
# ```
4+
# STARLARK_RUST_REGENERATE_GOLDEN_TESTS=1 cargo test -p starlark --lib tests
5+
# ```
6+
7+
Program:
8+
load("foo", "x")
9+
y = x
10+
11+
No unused loads
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
# @generated
2+
# To regenerate, run:
3+
# ```
4+
# STARLARK_RUST_REGENERATE_GOLDEN_TESTS=1 cargo test -p starlark --lib tests
5+
# ```
6+
7+
Program:
8+
load("foo", "T")
9+
load("bar", "U")
10+
11+
y: T = 1
12+
13+
def f(x: U):
14+
pass
15+
16+
No unused loads
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
# @generated
2+
# To regenerate, run:
3+
# ```
4+
# STARLARK_RUST_REGENERATE_GOLDEN_TESTS=1 cargo test -p starlark --lib tests
5+
# ```
6+
7+
Program:
8+
load("foo", x="y", z="w")
9+
y = z
10+
11+
Unused loads:
12+
13+
error: Unused load
14+
--> with_rename:1:13
15+
|
16+
1 | load("foo", x="y", z="w")
17+
| ^^^^^
18+
|

0 commit comments

Comments
 (0)