diff --git a/kclvm/Cargo.lock b/kclvm/Cargo.lock index 2bbd087bf..ccfda5632 100644 --- a/kclvm/Cargo.lock +++ b/kclvm/Cargo.lock @@ -772,6 +772,7 @@ dependencies = [ "once_cell", "petgraph", "phf", + "regex", "unicode_names2", ] diff --git a/kclvm/ast/src/walker.rs b/kclvm/ast/src/walker.rs index 365825eb6..23b5d430c 100644 --- a/kclvm/ast/src/walker.rs +++ b/kclvm/ast/src/walker.rs @@ -990,3 +990,267 @@ pub fn walk_comment<'ctx, V: Walker<'ctx>>(walker: &mut V, comment: &'ctx ast::C pub fn walk_module<'ctx, V: Walker<'ctx>>(walker: &mut V, module: &'ctx ast::Module) { walk_list!(walker, walk_stmt, module.body) } +/// Each method of the `MutSelfWalker` trait returns void type and does not need to modify the AST. +/// We can use it to traverse the AST and do some check at the same time, For example, in the process +/// of lint checking, we can use it to check each AST node and generate diagnostcs. +pub trait MutSelfWalker<'ctx> { + fn walk_expr_stmt(&mut self, expr_stmt: &'ctx ast::ExprStmt) { + for expr in &expr_stmt.exprs { + self.walk_expr(&expr.node) + } + } + + fn walk_type_alias_stmt(&mut self, type_alias_stmt: &'ctx ast::TypeAliasStmt) { + self.walk_identifier(&type_alias_stmt.type_name.node); + } + fn walk_unification_stmt(&mut self, unification_stmt: &'ctx ast::UnificationStmt) { + self.walk_identifier(&unification_stmt.target.node); + self.walk_schema_expr(&unification_stmt.value.node); + } + fn walk_assign_stmt(&mut self, assign_stmt: &'ctx ast::AssignStmt) { + for target in &assign_stmt.targets { + self.walk_identifier(&target.node) + } + self.walk_expr(&assign_stmt.value.node); + } + fn walk_aug_assign_stmt(&mut self, aug_assign_stmt: &'ctx ast::AugAssignStmt) { + self.walk_identifier(&aug_assign_stmt.target.node); + self.walk_expr(&aug_assign_stmt.value.node); + } + fn walk_assert_stmt(&mut self, assert_stmt: &'ctx ast::AssertStmt) { + self.walk_expr(&assert_stmt.test.node); + walk_if!(self, walk_expr, assert_stmt.if_cond); + walk_if!(self, walk_expr, assert_stmt.msg); + } + fn walk_if_stmt(&mut self, if_stmt: &'ctx ast::IfStmt) { + self.walk_expr(&if_stmt.cond.node); + walk_list!(self, walk_stmt, if_stmt.body); + walk_list!(self, walk_stmt, if_stmt.orelse); + } + fn walk_import_stmt(&mut self, _import_stmt: &'ctx ast::ImportStmt) { + // Nothing to do + } + fn walk_schema_attr(&mut self, schema_attr: &'ctx ast::SchemaAttr) { + walk_list!(self, walk_call_expr, schema_attr.decorators); + walk_if!(self, walk_expr, schema_attr.value); + } + fn walk_schema_stmt(&mut self, schema_stmt: &'ctx ast::SchemaStmt) { + walk_if!(self, walk_identifier, schema_stmt.parent_name); + walk_if!(self, walk_identifier, schema_stmt.for_host_name); + walk_if!(self, walk_arguments, schema_stmt.args); + if let Some(schema_index_signature) = &schema_stmt.index_signature { + let value = &schema_index_signature.node.value; + walk_if!(self, walk_expr, value); + } + walk_list!(self, walk_identifier, schema_stmt.mixins); + walk_list!(self, walk_call_expr, schema_stmt.decorators); + walk_list!(self, walk_check_expr, schema_stmt.checks); + walk_list!(self, walk_stmt, schema_stmt.body); + } + fn walk_rule_stmt(&mut self, rule_stmt: &'ctx ast::RuleStmt) { + walk_list!(self, walk_identifier, rule_stmt.parent_rules); + walk_list!(self, walk_call_expr, rule_stmt.decorators); + walk_list!(self, walk_check_expr, rule_stmt.checks); + walk_if!(self, walk_arguments, rule_stmt.args); + walk_if!(self, walk_identifier, rule_stmt.for_host_name); + } + fn walk_quant_expr(&mut self, quant_expr: &'ctx ast::QuantExpr) { + self.walk_expr(&quant_expr.target.node); + walk_list!(self, walk_identifier, quant_expr.variables); + self.walk_expr(&quant_expr.test.node); + walk_if!(self, walk_expr, quant_expr.if_cond); + } + fn walk_if_expr(&mut self, if_expr: &'ctx ast::IfExpr) { + self.walk_expr(&if_expr.cond.node); + self.walk_expr(&if_expr.body.node); + self.walk_expr(&if_expr.orelse.node); + } + fn walk_unary_expr(&mut self, unary_expr: &'ctx ast::UnaryExpr) { + self.walk_expr(&unary_expr.operand.node); + } + fn walk_binary_expr(&mut self, binary_expr: &'ctx ast::BinaryExpr) { + self.walk_expr(&binary_expr.left.node); + self.walk_expr(&binary_expr.right.node); + } + fn walk_selector_expr(&mut self, selector_expr: &'ctx ast::SelectorExpr) { + self.walk_expr(&selector_expr.value.node); + self.walk_identifier(&selector_expr.attr.node); + } + fn walk_call_expr(&mut self, call_expr: &'ctx ast::CallExpr) { + self.walk_expr(&call_expr.func.node); + walk_list!(self, walk_expr, call_expr.args); + walk_list!(self, walk_keyword, call_expr.keywords); + } + fn walk_subscript(&mut self, subscript: &'ctx ast::Subscript) { + self.walk_expr(&subscript.value.node); + walk_if!(self, walk_expr, subscript.index); + walk_if!(self, walk_expr, subscript.lower); + walk_if!(self, walk_expr, subscript.upper); + walk_if!(self, walk_expr, subscript.step); + } + fn walk_paren_expr(&mut self, paren_expr: &'ctx ast::ParenExpr) { + self.walk_expr(&paren_expr.expr.node); + } + fn walk_list_expr(&mut self, list_expr: &'ctx ast::ListExpr) { + walk_list!(self, walk_expr, list_expr.elts); + } + fn walk_list_comp(&mut self, list_comp: &'ctx ast::ListComp) { + self.walk_expr(&list_comp.elt.node); + walk_list!(self, walk_comp_clause, list_comp.generators); + } + fn walk_list_if_item_expr(&mut self, list_if_item_expr: &'ctx ast::ListIfItemExpr) { + self.walk_expr(&list_if_item_expr.if_cond.node); + walk_list!(self, walk_expr, list_if_item_expr.exprs); + walk_if!(self, walk_expr, list_if_item_expr.orelse); + } + fn walk_starred_expr(&mut self, starred_expr: &'ctx ast::StarredExpr) { + self.walk_expr(&starred_expr.value.node); + } + fn walk_dict_comp(&mut self, dict_comp: &'ctx ast::DictComp) { + if let Some(key) = &dict_comp.entry.key { + self.walk_expr(&key.node); + } + self.walk_expr(&dict_comp.entry.value.node); + walk_list!(self, walk_comp_clause, dict_comp.generators); + } + fn walk_config_if_entry_expr( + &mut self, + config_if_entry_expr: &'ctx ast::ConfigIfEntryExpr, + ) { + self.walk_expr(&config_if_entry_expr.if_cond.node); + for config_entry in &config_if_entry_expr.items { + walk_if!(self, walk_expr, config_entry.node.key); + self.walk_expr(&config_entry.node.value.node); + } + walk_if!(self, walk_expr, config_if_entry_expr.orelse); + } + fn walk_comp_clause(&mut self, comp_clause: &'ctx ast::CompClause) { + walk_list!(self, walk_identifier, comp_clause.targets); + self.walk_expr(&comp_clause.iter.node); + walk_list!(self, walk_expr, comp_clause.ifs); + } + fn walk_schema_expr(&mut self, schema_expr: &'ctx ast::SchemaExpr) { + self.walk_identifier(&schema_expr.name.node); + walk_list!(self, walk_expr, schema_expr.args); + walk_list!(self, walk_keyword, schema_expr.kwargs); + self.walk_expr(&schema_expr.config.node); + } + fn walk_config_expr(&mut self, config_expr: &'ctx ast::ConfigExpr) { + for config_entry in &config_expr.items { + walk_if!(self, walk_expr, config_entry.node.key); + self.walk_expr(&config_entry.node.value.node); + } + } + fn walk_check_expr(&mut self, check_expr: &'ctx ast::CheckExpr) { + self.walk_expr(&check_expr.test.node); + walk_if!(self, walk_expr, check_expr.if_cond); + walk_if!(self, walk_expr, check_expr.msg); + } + fn walk_lambda_expr(&mut self, lambda_expr: &'ctx ast::LambdaExpr) { + walk_if!(self, walk_arguments, lambda_expr.args); + walk_list!(self, walk_stmt, lambda_expr.body); + } + fn walk_keyword(&mut self, keyword: &'ctx ast::Keyword) { + self.walk_identifier(&keyword.arg.node); + if let Some(v) = &keyword.value { + self.walk_expr(&v.node) + } + } + fn walk_arguments(&mut self, arguments: &'ctx ast::Arguments) { + walk_list!(self, walk_identifier, arguments.args); + for default in &arguments.defaults { + if let Some(d) = default { + self.walk_expr(&d.node) + } + } + } + fn walk_compare(&mut self, compare: &'ctx ast::Compare) { + self.walk_expr(&compare.left.node); + walk_list!(self, walk_expr, compare.comparators); + } + fn walk_identifier(&mut self, identifier: &'ctx ast::Identifier) { + // Nothing to do. + let _ = identifier; + } + fn walk_number_lit(&mut self, number_lit: &'ctx ast::NumberLit) { + let _ = number_lit; + } + fn walk_string_lit(&mut self, string_lit: &'ctx ast::StringLit) { + // Nothing to do. + let _ = string_lit; + } + fn walk_name_constant_lit(&mut self, name_constant_lit: &'ctx ast::NameConstantLit) { + // Nothing to do. + let _ = name_constant_lit; + } + fn walk_joined_string(&mut self, joined_string: &'ctx ast::JoinedString) { + walk_list!(self, walk_expr, joined_string.values); + } + fn walk_formatted_value(&mut self, formatted_value: &'ctx ast::FormattedValue) { + self.walk_expr(&formatted_value.value.node); + } + fn walk_comment(&mut self, comment: &'ctx ast::Comment) { + // Nothing to do. + let _ = comment; + } + fn walk_module(&mut self, module: &'ctx ast::Module) { + walk_list!(self, walk_stmt, module.body) + } + fn walk_stmt(&mut self, stmt: &'ctx ast::Stmt) { + match stmt { + ast::Stmt::TypeAlias(type_alias) => self.walk_type_alias_stmt(type_alias), + ast::Stmt::Expr(expr_stmt) => self.walk_expr_stmt(expr_stmt), + ast::Stmt::Unification(unification_stmt) => { + self.walk_unification_stmt(unification_stmt) + } + ast::Stmt::Assign(assign_stmt) => self.walk_assign_stmt(assign_stmt), + ast::Stmt::AugAssign(aug_assign_stmt) => self.walk_aug_assign_stmt(aug_assign_stmt), + ast::Stmt::Assert(assert_stmt) => self.walk_assert_stmt(assert_stmt), + ast::Stmt::If(if_stmt) => self.walk_if_stmt(if_stmt), + ast::Stmt::Import(import_stmt) => self.walk_import_stmt(import_stmt), + ast::Stmt::SchemaAttr(schema_attr) => self.walk_schema_attr(schema_attr), + ast::Stmt::Schema(schema_stmt) => self.walk_schema_stmt(schema_stmt), + ast::Stmt::Rule(rule_stmt) => self.walk_rule_stmt(rule_stmt), + } + } + fn walk_expr(&mut self, expr: &'ctx ast::Expr) { + match expr { + ast::Expr::Identifier(identifier) => self.walk_identifier(identifier), + ast::Expr::Unary(unary_expr) => self.walk_unary_expr(unary_expr), + ast::Expr::Binary(binary_expr) => self.walk_binary_expr(binary_expr), + ast::Expr::If(if_expr) => self.walk_if_expr(if_expr), + ast::Expr::Selector(selector_expr) => self.walk_selector_expr(selector_expr), + ast::Expr::Call(call_expr) => self.walk_call_expr(call_expr), + ast::Expr::Paren(paren_expr) => self.walk_paren_expr(paren_expr), + ast::Expr::Quant(quant_expr) => self.walk_quant_expr(quant_expr), + ast::Expr::List(list_expr) => self.walk_list_expr(list_expr), + ast::Expr::ListIfItem(list_if_item_expr) => { + self.walk_list_if_item_expr(list_if_item_expr) + } + ast::Expr::ListComp(list_comp) => self.walk_list_comp(list_comp), + ast::Expr::Starred(starred_expr) => self.walk_starred_expr(starred_expr), + ast::Expr::DictComp(dict_comp) => self.walk_dict_comp(dict_comp), + ast::Expr::ConfigIfEntry(config_if_entry_expr) => { + self.walk_config_if_entry_expr(config_if_entry_expr) + } + ast::Expr::CompClause(comp_clause) => self.walk_comp_clause(comp_clause), + ast::Expr::Schema(schema_expr) => self.walk_schema_expr(schema_expr), + ast::Expr::Config(config_expr) => self.walk_config_expr(config_expr), + ast::Expr::Check(check) => self.walk_check_expr(check), + ast::Expr::Lambda(lambda) => self.walk_lambda_expr(lambda), + ast::Expr::Subscript(subscript) => self.walk_subscript(subscript), + ast::Expr::Keyword(keyword) => self.walk_keyword(keyword), + ast::Expr::Arguments(arguments) => self.walk_arguments(arguments), + ast::Expr::Compare(compare) => self.walk_compare(compare), + ast::Expr::NumberLit(number_lit) => self.walk_number_lit(number_lit), + ast::Expr::StringLit(string_lit) => self.walk_string_lit(string_lit), + ast::Expr::NameConstantLit(name_constant_lit) => { + self.walk_name_constant_lit(name_constant_lit) + } + ast::Expr::JoinedString(joined_string) => self.walk_joined_string(joined_string), + ast::Expr::FormattedValue(formatted_value) => { + self.walk_formatted_value(formatted_value) + } + } + } +} \ No newline at end of file diff --git a/kclvm/error/src/diagnostic.rs b/kclvm/error/src/diagnostic.rs index f88b6cc05..b118cfe94 100644 --- a/kclvm/error/src/diagnostic.rs +++ b/kclvm/error/src/diagnostic.rs @@ -5,7 +5,7 @@ use kclvm_span::Loc; use rustc_span::Pos; use termcolor::{Color, ColorSpec}; -use crate::ErrorKind; +use crate::{ErrorKind, WarningKind}; /// Diagnostic structure. #[derive(Clone, Debug, PartialEq, Eq, Hash)] @@ -134,7 +134,7 @@ pub struct Message { #[derive(Clone, Debug, PartialEq, Eq, Hash)] pub enum DiagnosticId { Error(ErrorKind), - Warning(String), + Warning(WarningKind), } #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] diff --git a/kclvm/error/src/error.rs b/kclvm/error/src/error.rs index b10c73d17..9bf92380e 100644 --- a/kclvm/error/src/error.rs +++ b/kclvm/error/src/error.rs @@ -86,3 +86,29 @@ impl ErrorKind { return format!("{:?}", self); } } + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub struct Warning { + pub code: &'static str, + pub kind: ErrorKind, + pub message: Option<&'static str>, +} + +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +pub enum WarningKind { + UnusedImportWarning, + ReimportWarning, + ImportPositionWarning, +} + +impl std::fmt::Display for WarningKind { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + write!(f, "{:?}", self) + } +} + +impl WarningKind { + pub fn name(&self) -> String { + return format!("{:?}", self); + } +} diff --git a/kclvm/error/src/lib.rs b/kclvm/error/src/lib.rs index 6cb2060cc..c70fe9bdd 100644 --- a/kclvm/error/src/lib.rs +++ b/kclvm/error/src/lib.rs @@ -216,6 +216,30 @@ impl Handler { self } + /// Add an warning into the handler + /// ``` + /// use kclvm_error::*; + /// let mut handler = Handler::default(); + /// handler.add_warning(WarningKind::UnusedImportWarning, &[ + /// Message { + /// pos: Position::dummy_pos(), + /// style: Style::LineAndColumn, + /// message: "Module 'a' imported but unused.".to_string(), + /// note: None, + /// }], + /// ); + /// ``` + pub fn add_warning(&mut self, warning: WarningKind, msgs: &[Message]) -> &mut Self { + let diag = Diagnostic { + level: Level::Warning, + messages: msgs.to_owned(), + code: Some(DiagnosticId::Warning(warning)), + }; + self.add_diagnostic(diag); + + self + } + /// Store a diagnostics #[inline] fn add_diagnostic(&mut self, diagnostic: Diagnostic) -> &mut Self { diff --git a/kclvm/sema/Cargo.toml b/kclvm/sema/Cargo.toml index 4eb251eba..efccd1bb7 100644 --- a/kclvm/sema/Cargo.toml +++ b/kclvm/sema/Cargo.toml @@ -15,6 +15,7 @@ once_cell = "1.5.2" fancy-regex = "0.7.1" unicode_names2 = "0.4" petgraph = "0.6.0" +regex = "1.5" kclvm-ast = {path = "../ast", version = "0.1.0"} kclvm-runtime = {path = "../runtime", version = "0.1.0"} kclvm-error = {path = "../error", version = "0.1.0"} @@ -28,3 +29,5 @@ criterion = "0.3" name = "my_benchmark" harness = false +[lib] +bench = false diff --git a/kclvm/sema/benches/my_benchmark.rs b/kclvm/sema/benches/my_benchmark.rs index 5b0bc6fbf..767dbc4d6 100644 --- a/kclvm/sema/benches/my_benchmark.rs +++ b/kclvm/sema/benches/my_benchmark.rs @@ -1,4 +1,7 @@ -use criterion::{black_box, criterion_group, criterion_main, Criterion}; +use criterion::{criterion_group, criterion_main, Criterion}; +use kclvm_parser::load_program; +use kclvm_sema::pre_process::pre_process_program; +use kclvm_sema::resolver::*; use kclvm_sema::ty::*; pub fn criterion_benchmark(c: &mut Criterion) { @@ -15,5 +18,10 @@ pub fn criterion_benchmark(c: &mut Criterion) { }); } -criterion_group!(benches, criterion_benchmark); +pub fn criterion_benchmark_resolver(c: &mut Criterion) { + let mut program = load_program(&["./src/resolver/test_data/import.k"], None).unwrap(); + c.bench_function("resolver", |b| b.iter(|| resolve_program(&mut program))); +} + +criterion_group!(benches, criterion_benchmark, criterion_benchmark_resolver); criterion_main!(benches); diff --git a/kclvm/sema/src/resolver/import.rs b/kclvm/sema/src/resolver/import.rs index ca83dd70a..91c2cebd3 100644 --- a/kclvm/sema/src/resolver/import.rs +++ b/kclvm/sema/src/resolver/import.rs @@ -39,7 +39,7 @@ impl<'ctx> Resolver<'ctx> { pos: Position { filename: m.filename.clone(), line: stmt.line, - column: Some(1), + column: None, }, style: Style::Line, message: format!( @@ -59,7 +59,7 @@ impl<'ctx> Resolver<'ctx> { pos: Position { filename: self.ctx.filename.clone(), line: stmt.line, - column: Some(1), + column: None, }, style: Style::Line, message: format!( @@ -98,10 +98,29 @@ impl<'ctx> Resolver<'ctx> { { match self.ctx.import_names.get_mut(&self.ctx.filename) { Some(mapping) => { - mapping.insert( - import_stmt.name.to_string(), - import_stmt.path.to_string(), - ); + if mapping.contains_key(&import_stmt.name.to_string()) { + self.handler.add_warning( + WarningKind::ReimportWarning, + &[Message { + pos: Position { + filename: module.filename.clone(), + line: stmt.line, + column: None, + }, + style: Style::Line, + message: format!( + "Module '{}' is reimported multiple times.", + &import_stmt.name + ), + note: None, + }], + ); + } else { + mapping.insert( + import_stmt.name.to_string(), + import_stmt.path.to_string(), + ); + } } None => { let mut mapping = IndexMap::default(); diff --git a/kclvm/sema/src/resolver/lint.rs b/kclvm/sema/src/resolver/lint.rs new file mode 100644 index 000000000..6882fbdc7 --- /dev/null +++ b/kclvm/sema/src/resolver/lint.rs @@ -0,0 +1,549 @@ +//! This file is the implementation of KCLLint, which is used to perform some additional checks on KCL code. +//! The main structures of the file are Lint, LintPass, CombinedLintPass and Linter. +//! For details see the: https://github.com/KusionStack/KCLVM/issues/109 +//! +//! Steps to define a new lint: +//! 1. Define a static instance of the `Lint` structure,e.g., +//! +//! ```rust,no_run +//! pub static IMPORT_Position: &Lint = &Lint { +//! ... +//! } +//! ``` +//! +//! 2. Define a lintpass, which is used to implement the checking process,e.g., +//! +//! ```rust,no_run +//! declare_lint_pass!(ImportPosition => [IMPORT_POSITION]); +//! ``` +//! +//! The `ImportPosition` is the defined LintPass structure and the `IMPORT_POSITION` is the `Lint` structure +//! defined in step 1. Here is a `LintArray`, which means that multiple lint checks can be implemented +//! in a single lintpass. +//! +//! 3. Implement the lintpass check process, e.g., +//! +//! ```rust,no_run +//! impl LintPass for ImportPosition{ +//! fn check_module(&mut self, diags: &mut IndexSet, ctx: &mut LintContext,module: &ast::Module){ +//! ... +//! } +//! } +//! ``` +//! +//! 4. Add the `check_*` methods in lintpass to the macro `lint_methods`, or skip it if it exists +//! +//! ```rust,no_run +//! macro_rules! lint_methods { +//! ($macro:path, $args:tt) => ( +//! $macro!($args, [ +//! fn check_module(module: &ast::Module); +//! ]); +//! ) +//! } +//! ``` +//! +//! 5. Add the new lintpass to the macro `default_lint_passes`, noting that `:` is preceded and followed by +//! the name of the lintpass. e.g., +//! +//! ```rust,no_run +//! macro_rules! default_lint_passes { +//! ($macro:path, $args:tt) => { +//! $macro!( +//! $args, +//! [ +//! ImportPosition: ImportPosition, +//! ] +//! ); +//! }; +//! } +//! ``` +//! +//! 6. If new `check_*` method was added in step 4, it needs to override the walk_* method in Linter. +//! In addition to calling the self.pass.check_* function, the original walk method in MutSelfWalker +//! should be copied here so that it can continue to traverse the child nodes. +//! + +use super::Resolver; +use indexmap::{IndexMap, IndexSet}; +use kclvm_ast::ast; +use kclvm_ast::walker::MutSelfWalker; +use kclvm_ast::{walk_if, walk_list}; +use kclvm_error::{Diagnostic, DiagnosticId, Level, Message, Position, Style, WarningKind}; +use regex::Regex; + +/// A summary of the methods that need to be implemented in lintpass, to be added when constructing new lint +/// lint and lintpass. When defining lintpass, the default implementation of these methods is provided: null +/// check (see macro `expand_default_lint_pass_methods`). So what need to do is to override the specific +/// `check_*` function. +#[macro_export] +macro_rules! lint_methods { + ($macro:path, $args:tt) => ( + $macro!($args, [ + fn check_module(module: &ast::Module); + fn check_module_post(module: &ast::Module); + /* + * Stmt + */ + + // fn check_stmt(stmt: ast::Node); + // fn check_expr_stmt(expr_stmt: ast::ExprStmt); + // fn check_unification_stmt(unification_stmt: ast::UnificationStmt); + // fn check_type_alias_stmt(type_alias_stmt: ast::TypeAliasStmt); + // fn check_assign_stmt(assign_stmt: ast::AssignStmt); + // fn check_aug_assign_stmt(aug_assign_stmt: ast::AugAssignStmt); + // fn check_assert_stmt(assert_stmt: ast::AssertStmt); + // fn check_if_stmt(if_stmt: ast::IfStmt); + // fn check_import_stmt(import_stmt: ast::ImportStmt); + // fn check_schema_stmt(schema_stmt: ast::SchemaStmt); + // fn check_rule_stmt(rule_stmt: ast::RuleStmt); + + /* + * Expr + */ + + // fn check_expr(expr: ast::Node); + // fn check_quant_expr(quant_expr: ast::QuantExpr); + fn check_schema_attr(schema_attr: &ast::SchemaAttr); + // fn check_if_expr(if_expr: ast::IfExpr); + // fn check_unary_expr(unary_expr: ast::UnaryExpr); + // fn check_binary_expr(binary_expr: ast::BinaryExpr); + // fn check_selector_expr(selector_expr: ast::SelectorExpr); + // fn check_call_expr(call_expr: ast::CallExpr); + // fn check_subscript(subscript: ast::Subscript); + // fn check_paren_expr(paren_expr: ast::ParenExpr); + // fn check_list_expr(list_expr: ast::ListExpr); + // fn check_list_comp(list_comp: ast::ListComp); + // fn check_list_if_item_expr(list_if_item_expr: ast::ListIfItemExpr); + // fn check_starred_expr(starred_expr: ast::StarredExpr); + // fn check_dict_comp(dict_comp: ast::DictComp); + // fn check_config_if_entry_expr(config_if_entry_expr: ast::ConfigIfEntryExpr, + // ); + // fn check_comp_clause(comp_clause: ast::CompClause); + // fn check_schema_expr(schema_expr: ast::SchemaExpr); + // fn check_config_expr(config_expr: ast::ConfigExpr); + // fn check_check_expr(check_expr: ast::CheckExpr); + // fn check_lambda_expr(lambda_expr: ast::LambdaExpr); + // fn check_keyword(keyword: ast::Keyword); + // fn check_arguments(arguments: ast::Arguments); + // fn check_compare(compare: ast::Compare); + fn check_identifier(id: &ast::Identifier); + // fn check_number_lit(number_lit: ast::NumberLit); + // fn check_string_lit(string_lit: ast::StringLit); + // fn check_name_constant_lit(name_constant_lit: ast::NameConstantLit); + // fn check_joined_string(joined_string: ast::JoinedString); + // fn check_formatted_value(formatted_value: ast::FormattedValue); + // fn check_comment(comment: ast::Comment); + ]); + ) +} + +/// Definition of `Lint` struct +/// Note that Lint declarations don't carry any "state" - they are merely global identifiers and descriptions of lints. +pub struct Lint { + /// A string identifier for the lint. + pub name: &'static str, + + /// Level for the lint. + pub level: Level, + + /// Description of the lint or the issue it detects. + /// e.g., "imports that are never used" + pub desc: &'static str, + + // Error/Warning code + pub code: &'static str, + + // Suggest methods to fix this problem + pub note: Option<&'static str>, +} + +pub type LintArray = Vec<&'static Lint>; + +/// Declares a static `LintArray` and return it as an expression. +#[macro_export] +macro_rules! lint_array { + ($( $lint:expr ),* ,) => { lint_array!( $($lint),* ) }; + ($( $lint:expr ),*) => {{ + vec![$($lint),*] + }} +} + +/// Provide a default implementation of the methods in lint_methods for each lintpass: null checking +macro_rules! expand_default_lint_pass_methods { + ($diag:ty, $ctx:ty, [$($(#[$attr:meta])* fn $name:ident($($param:ident: $arg:ty),*);)*]) => ( + $(#[inline(always)] fn $name(&mut self, diags: &mut $diag, ctx: &mut $ctx, $($param: $arg),*) {})* + ) +} + +/// Definition of `LintPass` trait +macro_rules! declare_default_lint_pass_impl { + ([], [$($methods:tt)*]) => ( + pub trait LintPass { + expand_default_lint_pass_methods!(IndexSet, LintContext, [$($methods)*]); + } + ) +} + +lint_methods!(declare_default_lint_pass_impl, []); + +/// The macro to define the LintPass and bind a set of corresponding Lint. +/// +/// Here is a `LintArray`, which means that multiple lint checks can be implemented in a single lintpass. +#[macro_export] +macro_rules! declare_lint_pass { + ($(#[$m:meta])* $name:ident => [$($lint:expr),* $(,)?]) => { + $(#[$m])* #[derive(Copy, Clone)] pub struct $name; + $crate::impl_lint_pass!($name => [$($lint),*]); + }; +} + +/// Implements `LintPass for $ty` with the given list of `Lint` statics. +#[macro_export] +macro_rules! impl_lint_pass { + ($ty:ty => [$($lint:expr),* $(,)?]) => { + impl $ty { + pub fn get_lints() -> LintArray { $crate::lint_array!($($lint),*) } + } + }; +} + +/// Call the `check_*` method of each lintpass in CombinedLintLass.check_*. +/// +/// ```rust,no_run +/// fn check_ident(&mut self, diags: &mut IndexSet, ctx: &mut LintContext, id: &ast::Identifier, ){ +/// self.LintPassA.check_ident(diags, ctx, id); +/// self.LintPassB.check_ident(diags, ctx, id); +/// ... +/// } +/// ``` +#[macro_export] +macro_rules! expand_combined_lint_pass_method { + ([$($passes:ident),*], $self: ident, $name: ident, $params:tt) => ({ + $($self.$passes.$name $params;)* + }) +} + +/// Expand all methods defined in macro `lint_methods` in the `CombinedLintLass`. +/// +/// ```rust,no_run +/// fn check_ident(&mut self, diags: &mut IndexSet, ctx: &mut LintContext, id: &ast::Identifier){}; +/// fn check_stmt(&mut self, diags: &mut IndexSet, ctx: &mut LintContext, module: &ast::Module){}; +/// ... +/// ``` +#[macro_export] +macro_rules! expand_combined_lint_pass_methods { + ($diags:ty, $ctx:ty, $passes:tt, [$($(#[$attr:meta])* fn $name:ident($($param:ident: $arg:ty),*);)*]) => ( + $(fn $name(&mut self, diags: &mut $diags, ctx: &mut $ctx, $($param: $arg),*) { + expand_combined_lint_pass_method!($passes, self, $name, (diags, ctx, $($param),*)); + })* + ) +} + +/// Expand all definitions of `CombinedLintPass`. The results are as follows: +/// +/// ```rust,no_run +/// pub struct CombinedLintPass { +/// LintPassA: LintPassA; +/// LintPassB: LintPassB; +/// ... +/// } +/// +/// impl CombinedLintPass{ +/// pub fn new() -> Self { +/// Self { +/// LintPassA: LintPassA, +/// LintPassB: LintPassB, +/// ... +/// } +/// } +/// pub fn get_lints() -> LintArray { +/// let mut lints = Vec::new(); +/// lints.extend_from_slice(&LintPassA::get_lints()); +/// lints.extend_from_slice(&LintPassB::get_lints()); +/// ... +/// lints +/// } +/// } +/// +/// impl LintPass for CombinedLintPass { +/// fn check_ident(&mut self, diags: &mut IndexSet, ctx: &mut LintContext, id: &ast::Identifier, ){ +/// self.LintPassA.check_ident(diags, ctx, id); +/// self.LintPassB.check_ident(diags, ctx, id); +/// ... +/// } +/// fn check_stmt(&mut self, diags: &mut IndexSet, ctx: &mut LintContext, module: &ast::Module){ +/// self.LintPassA.check_stmt(diags, ctx, stmt); +/// self.LintPassB.check_stmt(diags, ctx, stmt); +/// ... +/// } +/// ... +/// } +/// ``` +#[macro_export] +macro_rules! declare_combined_lint_pass { + ([$v:vis $name:ident, [$($passes:ident: $constructor:expr,)*]], $methods:tt) => ( + #[allow(non_snake_case)] + $v struct $name { + $($passes: $passes,)* + } + + impl $name { + $v fn new() -> Self { + Self { + $($passes: $constructor,)* + } + } + + $v fn get_lints() -> LintArray { + let mut lints = Vec::new(); + $(lints.extend_from_slice(&$passes::get_lints());)* + lints + } + } + + impl LintPass for $name { + expand_combined_lint_pass_methods!(IndexSet, LintContext,[$($passes),*], $methods); + } + ) +} + +macro_rules! default_lint_passes { + ($macro:path, $args:tt) => { + $macro!( + $args, + [ImportPosition: ImportPosition, UnusedImport: UnusedImport,] + ); + }; +} + +macro_rules! declare_combined_default_pass { + ([$name:ident], $passes:tt) => ( + lint_methods!(declare_combined_lint_pass, [pub $name, $passes]); + ) +} + +// Define `CombinedLintPass`. +default_lint_passes!(declare_combined_default_pass, [CombinedLintPass]); + +/// The struct `Linter` is used to traverse the AST and call the `check_*` method defined in `CombinedLintPass`. +pub struct Linter<'l, T: LintPass> { + pass: T, + diags: &'l mut IndexSet, + ctx: LintContext, +} + +/// Record the information at `LintContext` when traversing the AST for analysis across AST nodes, e.g., record +/// used importstmt(used_import_names) when traversing `ast::Identifier` and `ast::SchemaAttr`, and detect unused +/// importstmt after traversing the entire module. +pub struct LintContext { + /// What source file are we in. + pub filename: String, + /// Import pkgpath and name + pub import_names: IndexMap>, + /// Used import nams + pub used_import_names: IndexMap>, +} + +impl<'l> Linter<'l, CombinedLintPass> { + pub fn new(diags: &'l mut IndexSet, ctx: LintContext) -> Self { + Linter::<'l, CombinedLintPass> { + pass: CombinedLintPass::new(), + diags, + ctx, + } + } +} + +impl<'ctx> Resolver<'ctx> { + pub fn lint_check_module(&mut self, module: &ast::Module) { + let ctx = LintContext { + filename: self.ctx.filename.clone(), + import_names: self.ctx.import_names.clone(), + used_import_names: IndexMap::new(), + }; + let mut linter = Linter::::new(&mut self.handler.diagnostics, ctx); + linter.walk_module(module) + } +} + +impl<'ctx> MutSelfWalker<'ctx> for Linter<'_, CombinedLintPass> { + fn walk_module(&mut self, module: &'ctx ast::Module) { + self.pass + .check_module(&mut self.diags, &mut self.ctx, module); + walk_list!(self, walk_stmt, module.body); + self.pass + .check_module_post(&mut self.diags, &mut self.ctx, module); + } + + fn walk_identifier(&mut self, id: &'ctx ast::Identifier) { + self.pass + .check_identifier(&mut self.diags, &mut self.ctx, id); + } + + fn walk_schema_attr(&mut self, schema_attr: &'ctx ast::SchemaAttr) { + self.pass + .check_schema_attr(&mut self.diags, &mut self.ctx, schema_attr); + walk_list!(self, walk_call_expr, schema_attr.decorators); + walk_if!(self, walk_expr, schema_attr.value); + } +} + +pub static IMPORT_POSITION: &Lint = &Lint { + name: stringify!("IMPORT_POSITION"), + level: Level::Warning, + desc: "Check for importstmt that are not defined at the top of file", + code: "W0413", + note: Some("Consider moving tihs statement to the top of the file"), +}; + +declare_lint_pass!(ImportPosition => [IMPORT_POSITION]); + +impl LintPass for ImportPosition { + fn check_module( + &mut self, + diags: &mut IndexSet, + ctx: &mut LintContext, + module: &ast::Module, + ) { + let mut first_non_importstmt = std::u64::MAX; + for stmt in &module.body { + match &stmt.node { + ast::Stmt::Import(_import_stmt) => {} + _ => { + if stmt.line < first_non_importstmt { + first_non_importstmt = stmt.line + } + } + } + } + for stmt in &module.body { + if let ast::Stmt::Import(_import_stmt) = &stmt.node { + if stmt.line > first_non_importstmt { + diags.insert(Diagnostic { + level: Level::Warning, + messages: (&[Message { + pos: Position { + filename: module.filename.clone(), + line: stmt.line, + column: None, + }, + style: Style::Line, + message: format!( + "Importstmt should be placed at the top of the module" + ), + note: Some( + ImportPosition::get_lints()[0] + .note + .unwrap() + .clone() + .to_string(), + ), + }]) + .to_vec(), + code: Some(DiagnosticId::Warning(WarningKind::ImportPositionWarning)), + }); + } + } + } + } +} + +pub static UNUSED_IMPORT: &Lint = &Lint { + name: stringify!("UNUSED_IMPORT"), + level: Level::Warning, + desc: "Check for unused importstmt", + code: "W0411", + note: Some("Consider removing this statement"), +}; + +declare_lint_pass!(UnusedImport => [UNUSED_IMPORT]); + +fn record_use(name: &String, ctx: &mut LintContext) { + let re = Regex::new(r"[|:\[\]\{\}]").unwrap(); + // # SchemaAttr.types, A|B, [A|B], {A|B:C} + let types: Vec<&str> = re.split(name).collect(); + for t in types { + let t = t.to_string(); + // name: a.b.c + let name: Vec<&str> = t.split(".").collect(); + let firstname = name[0]; + if let Some(import_names) = ctx.import_names.get(&ctx.filename) { + if import_names.contains_key(firstname) { + ctx.used_import_names + .get_mut(&ctx.filename) + .unwrap() + .insert(firstname.to_string()); + } + } + } +} + +impl LintPass for UnusedImport { + fn check_module( + &mut self, + diags: &mut IndexSet, + ctx: &mut LintContext, + module: &ast::Module, + ) { + ctx.used_import_names + .insert(ctx.filename.clone(), IndexSet::default()); + } + + fn check_module_post( + &mut self, + diags: &mut IndexSet, + ctx: &mut LintContext, + module: &ast::Module, + ) { + let used_import_names = ctx.used_import_names.get(&ctx.filename).unwrap(); + for stmt in &module.body { + if let ast::Stmt::Import(import_stmt) = &stmt.node { + if !used_import_names.contains(&import_stmt.name) { + diags.insert(Diagnostic { + level: Level::Warning, + messages: (&[Message { + pos: Position { + filename: module.filename.clone(), + line: stmt.line, + column: None, + }, + style: Style::Line, + message: format!("Module '{}' imported but unused.", import_stmt.name), + note: Some( + UnusedImport::get_lints()[0] + .note + .unwrap() + .clone() + .to_string(), + ), + }]) + .to_vec(), + code: Some(DiagnosticId::Warning(WarningKind::UnusedImportWarning)), + }); + } + } + } + } + + fn check_identifier( + &mut self, + diags: &mut IndexSet, + ctx: &mut LintContext, + id: &ast::Identifier, + ) { + if id.names.len() >= 2 { + let id_firstname = &id.names[0]; + record_use(&id_firstname, ctx); + } + } + + fn check_schema_attr( + &mut self, + diags: &mut IndexSet, + ctx: &mut LintContext, + schema_attr: &ast::SchemaAttr, + ) { + record_use(&schema_attr.type_str.node, ctx); + } +} diff --git a/kclvm/sema/src/resolver/mod.rs b/kclvm/sema/src/resolver/mod.rs index 400c76b13..cae2db845 100644 --- a/kclvm/sema/src/resolver/mod.rs +++ b/kclvm/sema/src/resolver/mod.rs @@ -5,6 +5,7 @@ mod config; mod format; pub mod global; mod import; +mod lint; mod r#loop; mod node; mod para; @@ -18,7 +19,7 @@ mod var; #[cfg(test)] mod tests; -use indexmap::IndexMap; +use indexmap::{IndexMap, IndexSet}; use std::{cell::RefCell, rc::Rc}; use crate::pre_process::pre_process_program; @@ -31,6 +32,7 @@ use kclvm_error::*; use crate::ty::TypeContext; +use self::lint::{ImportPosition, Linter}; use self::scope::{builtin_scope, ProgramScope}; /// Resolver is responsible for program semantic checking, mainly @@ -71,6 +73,7 @@ impl<'ctx> Resolver<'ctx> { for stmt in &module.body { self.walk_stmt(&stmt.node); } + self.lint_check_module(&module) } } None => {} diff --git a/kclvm/sema/src/resolver/test_data/a.k b/kclvm/sema/src/resolver/test_data/a.k new file mode 100644 index 000000000..8b6a81409 --- /dev/null +++ b/kclvm/sema/src/resolver/test_data/a.k @@ -0,0 +1,4 @@ +_a = 1 +schema Person: + name: str + age: int diff --git a/kclvm/sema/src/resolver/test_data/b.k b/kclvm/sema/src/resolver/test_data/b.k new file mode 100644 index 000000000..3ffa2426b --- /dev/null +++ b/kclvm/sema/src/resolver/test_data/b.k @@ -0,0 +1 @@ +_b = 1 \ No newline at end of file diff --git a/kclvm/sema/src/resolver/test_data/c.k b/kclvm/sema/src/resolver/test_data/c.k new file mode 100644 index 000000000..f4832d10f --- /dev/null +++ b/kclvm/sema/src/resolver/test_data/c.k @@ -0,0 +1,2 @@ +schema TestOfMixin: + age?: int diff --git a/kclvm/sema/src/resolver/test_data/d.k b/kclvm/sema/src/resolver/test_data/d.k new file mode 100644 index 000000000..78dcd8b21 --- /dev/null +++ b/kclvm/sema/src/resolver/test_data/d.k @@ -0,0 +1,2 @@ +schema Parent: + age1?: int diff --git a/kclvm/sema/src/resolver/test_data/e.k b/kclvm/sema/src/resolver/test_data/e.k new file mode 100644 index 000000000..98fabf8f4 --- /dev/null +++ b/kclvm/sema/src/resolver/test_data/e.k @@ -0,0 +1,2 @@ +schema UnionType: + a?: int diff --git a/kclvm/sema/src/resolver/test_data/f.k b/kclvm/sema/src/resolver/test_data/f.k new file mode 100644 index 000000000..65a0fa043 --- /dev/null +++ b/kclvm/sema/src/resolver/test_data/f.k @@ -0,0 +1,2 @@ +schema UnionType: + b?: int diff --git a/kclvm/sema/src/resolver/test_data/import.k b/kclvm/sema/src/resolver/test_data/import.k new file mode 100644 index 000000000..b5ccd5571 --- /dev/null +++ b/kclvm/sema/src/resolver/test_data/import.k @@ -0,0 +1,27 @@ +import abc # unable to import +import .a +import .a # reimport +import .c # test unused import for mixin name +import .d # test unused import for parent name +import .e # test unused import for union type +import .f as g # test unused import for union type + +schema Main(d.Parent): + mixin [c.TestOfMixin] + age?: int = 18 + person?: a.Person + list_union_type: [e.UnionType|int] + dict_union_type: {g.UnionType|int:float} + +import .b + +if a._a > 1: + _c = 1 +elif a._a == 1: + _c = 2 +else: + _c = 3 + +p = Main{ + age = b._b +} diff --git a/kclvm/sema/src/resolver/tests.rs b/kclvm/sema/src/resolver/tests.rs index 762a572c8..f491f503f 100644 --- a/kclvm/sema/src/resolver/tests.rs +++ b/kclvm/sema/src/resolver/tests.rs @@ -1,7 +1,12 @@ +use super::Options; +use super::Resolver; use crate::builtin::BUILTIN_FUNCTION_NAMES; +use crate::pre_process::pre_process_program; +use crate::resolver::lint; use crate::resolver::resolve_program; use crate::resolver::scope::*; use crate::ty::Type; +use indexmap::IndexSet; use kclvm_ast::ast; use kclvm_error::*; use kclvm_parser::{load_program, parse_program}; @@ -79,3 +84,84 @@ fn test_resolve_program_fail() { assert_eq!(diag.messages.len(), 1); assert_eq!(diag.messages[0].message, "expect int, got {str:int(1)}"); } + +#[test] +fn test_lint() { + let mut program = load_program(&["./src/resolver/test_data/import.k"], None).unwrap(); + pre_process_program(&mut program); + let mut resolver = Resolver::new( + &program, + Options { + raise_err: true, + config_auto_fix: false, + }, + ); + resolver.resolve_import(); + resolver.check(kclvm_ast::MAIN_PKG); + + let root = &program.root.clone(); + let filename = root.clone() + "/import.k"; + + let mut diagnostics: IndexSet = IndexSet::default(); + diagnostics.insert(Diagnostic { + level: Level::Error, + messages: vec![Message { + pos: Position { + filename: filename.clone(), + line: 1, + column: None, + }, + style: Style::Line, + message: format!( + "Cannot find the module {} from {}", + "abc", + root.clone() + "/abc" + ), + note: None, + }], + code: Some(DiagnosticId::Error(ErrorKind::CannotFindModule)), + }); + diagnostics.insert(Diagnostic { + level: Level::Warning, + messages: vec![Message { + pos: Position { + filename: filename.clone(), + line: 3, + column: None, + }, + style: Style::Line, + message: format!("Module '{}' is reimported multiple times.", "a",), + note: None, + }], + code: Some(DiagnosticId::Warning(WarningKind::ReimportWarning)), + }); + diagnostics.insert(Diagnostic { + level: Level::Warning, + messages: vec![Message { + pos: Position { + filename: filename.clone(), + line: 16, + column: None, + }, + style: Style::Line, + message: format!("Importstmt should be placed at the top of the module"), + note: Some("Consider moving tihs statement to the top of the file".to_string()), + }], + code: Some(DiagnosticId::Warning(WarningKind::ImportPositionWarning)), + }); + diagnostics.insert(Diagnostic { + level: Level::Warning, + messages: vec![Message { + pos: Position { + filename: filename.clone(), + line: 1, + column: None, + }, + style: Style::Line, + message: format!("Module '{}' imported but unused.", "abc",), + note: Some("Consider removing this statement".to_string()), + }], + code: Some(DiagnosticId::Warning(WarningKind::UnusedImportWarning)), + }); + assert_eq!(diagnostics, resolver.handler.diagnostics); +}