Skip to content

Commit

Permalink
feat: lint check unused import in each file instead of global modules (…
Browse files Browse the repository at this point in the history 
…#724)

* feat: lint check unused import in each file instead of global modules

* test: fix ut
  • Loading branch information
@He1pa
He1pa authored Sep 20, 2023
1 parent edd9993 commit 9697d22
Show file tree
Hide file tree
Showing 14 changed files with 60 additions and 45 deletions.
4 changes: 2 additions & 2 deletions kclvm/sema/src/lint/lints_def.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ impl LintPass for UnusedImport {
for (_, scope_obj) in scope_objs {
let scope_obj = scope_obj.borrow();
if let ScopeObjectKind::Module(m) = &scope_obj.kind {
if !scope_obj.used {
for stmt in &m.import_stmts {
for (stmt, has_used) in &m.import_stmts {
if !has_used {
handler.add_warning(
WarningKind::UnusedImportWarning,
&[Message {
Expand Down
3 changes: 0 additions & 3 deletions kclvm/sema/src/resolver/config.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ impl<'ctx> Resolver<'ctx> {
end,
ty,
kind: ScopeObjectKind::Attribute,
used: false,
doc: None,
}
}
Expand Down Expand Up @@ -440,7 +439,6 @@ impl<'ctx> Resolver<'ctx> {
end: key.get_end_pos(),
ty: val_ty.clone(),
kind: ScopeObjectKind::Attribute,
used: false,
doc: None,
},
);
Expand Down Expand Up @@ -474,7 +472,6 @@ impl<'ctx> Resolver<'ctx> {
end: key.get_end_pos(),
ty: val_ty.clone(),
kind: ScopeObjectKind::Attribute,
used: false,
doc: None,
},
);
Expand Down
16 changes: 6 additions & 10 deletions kclvm/sema/src/resolver/global.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ impl<'ctx> Resolver<'ctx> {
end,
ty: Rc::new(Type::schema(schema_ty.clone())),
kind: ScopeObjectKind::Definition,
used: false,
doc: Some(parsed_doc.summary.clone()),
},
)
Expand Down Expand Up @@ -141,7 +140,6 @@ impl<'ctx> Resolver<'ctx> {
end,
ty: Rc::new(Type::schema(schema_ty.clone())),
kind: ScopeObjectKind::Definition,
used: false,
doc: Some(schema_ty.doc),
},
)
Expand Down Expand Up @@ -303,7 +301,6 @@ impl<'ctx> Resolver<'ctx> {
end,
ty,
kind: ScopeObjectKind::Variable,
used: false,
doc: None,
},
);
Expand Down Expand Up @@ -354,7 +351,7 @@ impl<'ctx> Resolver<'ctx> {
);
return;
}
let ty = self.walk_identifier(&unification_stmt.value.node.name.node);
let ty = self.walk_identifier_expr(&unification_stmt.value.node.name);
self.insert_object(
name,
ScopeObject {
Expand All @@ -363,7 +360,6 @@ impl<'ctx> Resolver<'ctx> {
end,
ty,
kind: ScopeObjectKind::Variable,
used: false,
doc: None,
},
);
Expand All @@ -374,7 +370,7 @@ impl<'ctx> Resolver<'ctx> {
rule_stmt: &'ctx ast::RuleStmt,
) -> Option<Box<SchemaType>> {
if let Some(host_name) = &rule_stmt.for_host_name {
let ty = self.walk_identifier(&host_name.node);
let ty = self.walk_identifier_expr(&host_name);
match &ty.kind {
TypeKind::Schema(schema_ty) if schema_ty.is_protocol && !schema_ty.is_instance => {
Some(Box::new(schema_ty.clone()))
Expand Down Expand Up @@ -418,7 +414,7 @@ impl<'ctx> Resolver<'ctx> {
return None;
}
// Mixin type check with protocol
let ty = self.walk_identifier(&host_name.node);
let ty = self.walk_identifier_expr(&host_name);
match &ty.kind {
TypeKind::Schema(schema_ty) if schema_ty.is_protocol && !schema_ty.is_instance => {
Some(Box::new(schema_ty.clone()))
Expand Down Expand Up @@ -449,7 +445,7 @@ impl<'ctx> Resolver<'ctx> {
schema_stmt: &'ctx ast::SchemaStmt,
) -> Option<Box<SchemaType>> {
if let Some(parent_name) = &schema_stmt.parent_name {
let ty = self.walk_identifier(&parent_name.node);
let ty = self.walk_identifier_expr(&parent_name);
match &ty.kind {
TypeKind::Schema(schema_ty)
if !schema_ty.is_protocol && !schema_ty.is_mixin && !schema_ty.is_instance =>
Expand Down Expand Up @@ -730,7 +726,7 @@ impl<'ctx> Resolver<'ctx> {
}],
);
}
let ty = self.walk_identifier(&mixin.node);
let ty = self.walk_identifier_expr(&mixin);
let mixin_ty = match &ty.kind {
TypeKind::Schema(schema_ty)
if !schema_ty.is_protocol && schema_ty.is_mixin && !schema_ty.is_instance =>
Expand Down Expand Up @@ -860,7 +856,7 @@ impl<'ctx> Resolver<'ctx> {
// Parent types
let mut parent_types: Vec<SchemaType> = vec![];
for rule in &rule_stmt.parent_rules {
let ty = self.walk_identifier(&rule.node);
let ty = self.walk_identifier_expr(&rule);
let parent_ty = match &ty.kind {
TypeKind::Schema(schema_ty) if schema_ty.is_rule && !schema_ty.is_instance => {
Some(schema_ty.clone())
Expand Down
8 changes: 3 additions & 5 deletions kclvm/sema/src/resolver/import.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ impl<'ctx> Resolver<'ctx> {
let mut obj = scope_obj.borrow_mut();
match &mut obj.kind {
ScopeObjectKind::Module(m) => {
m.import_stmts.push(stmt.clone())
m.import_stmts.push((stmt.clone(), false))
},
_ => bug!(
"invalid module type in the import check function {}",
Expand Down Expand Up @@ -170,9 +170,8 @@ impl<'ctx> Resolver<'ctx> {
end: end.clone(),
ty: Rc::new(ty.clone()),
kind: ScopeObjectKind::Module(Module {
import_stmts: vec![stmt.clone()],
import_stmts: vec![(stmt.clone(), true)],
}),
used: true,
doc: None,
})),
);
Expand All @@ -184,9 +183,8 @@ impl<'ctx> Resolver<'ctx> {
end,
ty: Rc::new(ty),
kind: ScopeObjectKind::Module(Module {
import_stmts: vec![stmt.clone()],
import_stmts: vec![(stmt.clone(), false)],
}),
used: false,
doc: None,
})),
);
Expand Down
6 changes: 0 additions & 6 deletions kclvm/sema/src/resolver/node.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,6 @@ impl<'ctx> MutSelfTypedResultWalker<'ctx> for Resolver<'ctx> {
end,
ty: ty.clone(),
kind: ScopeObjectKind::TypeAlias,
used: false,
doc: None,
},
);
Expand Down Expand Up @@ -328,7 +327,6 @@ impl<'ctx> MutSelfTypedResultWalker<'ctx> for Resolver<'ctx> {
end,
ty: self.any_ty(),
kind: ScopeObjectKind::Variable,
used: false,
doc: None,
},
);
Expand Down Expand Up @@ -383,7 +381,6 @@ impl<'ctx> MutSelfTypedResultWalker<'ctx> for Resolver<'ctx> {
end,
ty: expected_ty.clone(),
kind: ScopeObjectKind::Attribute,
used: false,
doc: doc_str,
},
);
Expand Down Expand Up @@ -516,7 +513,6 @@ impl<'ctx> MutSelfTypedResultWalker<'ctx> for Resolver<'ctx> {
end: selector_expr.attr.get_end_pos(),
ty: value_ty.clone(),
kind: ScopeObjectKind::FunctionCall,
used: false,
doc: Some(func.doc.clone()),
},
)
Expand Down Expand Up @@ -819,7 +815,6 @@ impl<'ctx> MutSelfTypedResultWalker<'ctx> for Resolver<'ctx> {
end,
ty: self.any_ty(),
kind: ScopeObjectKind::Variable,
used: false,
doc: None,
},
);
Expand Down Expand Up @@ -969,7 +964,6 @@ impl<'ctx> MutSelfTypedResultWalker<'ctx> for Resolver<'ctx> {
end: end.clone(),
ty: param.ty.clone(),
kind: ScopeObjectKind::Parameter,
used: false,
doc: None,
},
)
Expand Down
4 changes: 0 additions & 4 deletions kclvm/sema/src/resolver/schema.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ impl<'ctx> Resolver<'ctx> {
end: end.clone(),
ty: param.ty.clone(),
kind: ScopeObjectKind::Parameter,
used: false,
doc: None,
},
)
Expand All @@ -72,7 +71,6 @@ impl<'ctx> Resolver<'ctx> {
end,
ty: index_signature.key_ty.clone(),
kind: ScopeObjectKind::Variable,
used: false,
doc: None,
},
)
Expand All @@ -93,7 +91,6 @@ impl<'ctx> Resolver<'ctx> {
end: Position::dummy_pos(),
ty: self.any_ty(),
kind: ScopeObjectKind::Variable,
used: false,
doc: None,
},
);
Expand Down Expand Up @@ -144,7 +141,6 @@ impl<'ctx> Resolver<'ctx> {
end: end.clone(),
ty: param.ty.clone(),
kind: ScopeObjectKind::Parameter,
used: false,
doc: None,
},
)
Expand Down
8 changes: 3 additions & 5 deletions kclvm/sema/src/resolver/scope.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ pub struct ScopeObject {
pub ty: Rc<Type>,
/// The scope object kind.
pub kind: ScopeObjectKind,
/// Record whether has been used, for check unused imported module and var definition
pub used: bool,
/// The doc of the scope object, will be None unless the scope object represents a schema or schema attribute.
pub doc: Option<String>,
}
Expand Down Expand Up @@ -81,7 +79,8 @@ pub enum ScopeObjectKind {
/// is used to record information on the AST
#[derive(PartialEq, Clone, Debug)]
pub struct Module {
pub import_stmts: Vec<NodeRef<Stmt>>,
/// Record stmts which import this module and whether has been used, for check unused imported module and var definition
pub import_stmts: Vec<(NodeRef<Stmt>, bool)>,
}

/// A Scope maintains a set of objects and links to its containing
Expand Down Expand Up @@ -136,7 +135,7 @@ impl Scope {
match &obj.borrow().kind {
ScopeObjectKind::Module(module) => {
for stmt in &module.import_stmts {
if let Import(import_stmt) = &stmt.node {
if let Import(import_stmt) = &stmt.0.node {
res.insert(import_stmt.name.clone(), obj.clone());
}
}
Expand Down Expand Up @@ -345,7 +344,6 @@ pub(crate) fn builtin_scope() -> Scope {
end: Position::dummy_pos(),
ty: Rc::new(builtin_func.clone()),
kind: ScopeObjectKind::Definition,
used: false,
doc: None,
})),
);
Expand Down
12 changes: 7 additions & 5 deletions kclvm/sema/src/resolver/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,13 @@ fn test_record_used_module() {
.clone();
for (_, obj) in main_scope.elems {
let obj = obj.borrow_mut().clone();
if let ScopeObjectKind::Module(_) = obj.kind {
if obj.name == "math" {
assert!(!obj.used);
} else {
assert!(obj.used);
if let ScopeObjectKind::Module(m) = obj.kind {
for (_, used) in m.import_stmts {
if obj.name == "math" {
assert!(!used);
} else {
assert!(used);
}
}
}
}
Expand Down
11 changes: 8 additions & 3 deletions kclvm/sema/src/resolver/var.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::resolver::Resolver;
use crate::ty::TypeKind;
use indexmap::IndexMap;
use kclvm_ast::pos::GetPos;
use kclvm_error::diagnostic::Range;
use kclvm_error::*;

Expand Down Expand Up @@ -45,7 +46,6 @@ impl<'ctx> Resolver<'ctx> {
end: range.1.clone(),
ty: self.any_ty(),
kind: ScopeObjectKind::Variable,
used: false,
doc: None,
},
);
Expand Down Expand Up @@ -73,7 +73,6 @@ impl<'ctx> Resolver<'ctx> {
end: range.1.clone(),
ty: self.any_ty(),
kind: ScopeObjectKind::Variable,
used: false,
doc: None,
},
);
Expand All @@ -87,7 +86,13 @@ impl<'ctx> Resolver<'ctx> {
// It should be recursively search whole scope to lookup scope object, not the current scope.element.
if !pkgpath.is_empty() {
if let Some(obj) = self.scope.borrow().lookup(pkgpath) {
obj.borrow_mut().used = true;
if let ScopeObjectKind::Module(m) = &mut obj.borrow_mut().kind {
for (stmt, used) in m.import_stmts.iter_mut() {
if stmt.get_pos().filename == range.0.filename {
*used = true;
}
}
}
}
}
// Load type
Expand Down
2 changes: 1 addition & 1 deletion kclvm/tools/src/LSP/src/completion.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ fn completion_variable(pos: &KCLPos, prog_scope: &ProgramScope) -> IndexSet<KCLC
match &obj.borrow().kind {
kclvm_sema::resolver::scope::ScopeObjectKind::Module(module) => {
for stmt in &module.import_stmts {
match &stmt.node {
match &stmt.0.node {
Stmt::Import(import_stmtt) => {
completions.insert(KCLCompletionItem {
label: import_stmtt.name.clone(),
Expand Down
1 change: 0 additions & 1 deletion kclvm/tools/src/LSP/src/goto_def.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,6 @@ pub(crate) fn resolve_var(
end: func_name_node.get_end_pos(),
ty: ty.clone(),
kind: ScopeObjectKind::FunctionCall,
used: false,
doc: Some(func_ty.doc.clone()),
}))
}
Expand Down
1 change: 1 addition & 0 deletions kclvm/tools/src/lint/test_data/unused_check_for_each_file/a.k
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import math
2 changes: 2 additions & 0 deletions kclvm/tools/src/lint/test_data/unused_check_for_each_file/b.k
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import math
a = math.pow(1, 1)
27 changes: 27 additions & 0 deletions kclvm/tools/src/lint/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,30 @@ fn test_lint() {
assert_eq!(diag.messages[0].message, m.to_string());
}
}

#[test]
fn test_unused_check_for_each_file() {
let (errs, warnings) = lint_files(
&[
"./src/lint/test_data/unused_check_for_each_file/a.k",
"./src/lint/test_data/unused_check_for_each_file/b.k",
],
None,
);
assert_eq!(errs.len(), 0);
assert_eq!(warnings.len(), 1);
assert_eq!(
warnings[0].messages[0].message,
"Module 'math' imported but unused".to_string()
);
let mut path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
path.push("src");
path.push("lint");
path.push("test_data");
path.push("unused_check_for_each_file");
path.push("a.k");
assert_eq!(
warnings[0].messages[0].range.0.filename,
path.to_str().unwrap().to_string()
);
}

0 comments on commit 9697d22

Please sign in to comment.