diff --git a/crates/cairo-lint-core/src/fix.rs b/crates/cairo-lint-core/src/fix.rs index 667df806..680694e1 100644 --- a/crates/cairo-lint-core/src/fix.rs +++ b/crates/cairo-lint-core/src/fix.rs @@ -20,6 +20,7 @@ use crate::plugin::{diagnostic_kind_from_message, CairoLintKind}; mod import_fixes; pub use import_fixes::{apply_import_fixes, collect_unused_imports, ImportFix}; mod helper; +use cairo_lang_syntax::node::ast::ExprBlock; use helper::{invert_condition, remove_break_from_block, remove_break_from_else_clause}; /// Represents a fix for a diagnostic, containing the span of code to be replaced @@ -184,6 +185,7 @@ impl Fixer { } CairoLintKind::LoopForWhile => self.fix_loop_break(db.upcast(), plugin_diag.stable_ptr.lookup(db.upcast())), CairoLintKind::ManualOkOr => self.fix_manual_ok_or(db, plugin_diag.stable_ptr.lookup(db.upcast())), + CairoLintKind::ManualAssert => self.fix_if_then_panic(db, plugin_diag.stable_ptr.lookup(db.upcast())), CairoLintKind::ManualIsSome => self.fix_manual_is_some(db, plugin_diag.stable_ptr.lookup(db.upcast())), CairoLintKind::ManualExpect => self.fix_manual_expect(db, plugin_diag.stable_ptr.lookup(db.upcast())), CairoLintKind::ManualIsNone => self.fix_manual_is_none(db, plugin_diag.stable_ptr.lookup(db.upcast())), @@ -556,4 +558,62 @@ impl Fixer { format!("{option_var_name}.expect({none_arm_err})") } + + pub fn fix_if_then_panic(&self, db: &dyn SyntaxGroup, node: SyntaxNode) -> String { + let if_expr = ExprIf::from_syntax_node(db, node.clone()); + let mut result = String::new(); + + // process each conditional block + let process_block = |condition_text: String, block_expr: ExprBlock, indentation: &str| { + let mut output = String::new(); + let statements = block_expr.statements(db).elements(db); + if let Some(Statement::Expr(statement_expr)) = statements.first() { + if let Expr::InlineMacro(inline_macro) = &statement_expr.expr(db) { + let panic_message = inline_macro.arguments(db).as_syntax_node().get_text_without_trivia(db); + + let variable_name = condition_text + .split(|c: char| !c.is_alphanumeric() && c != '_') + .find(|part| !part.is_empty()) + .unwrap_or(&condition_text); + + let assert_macro = format!( + "{}assert!({}, \"{}: {{:?}}\", {});\n", + indentation, + condition_text, + panic_message.trim_matches(&['(', ')'][..]).replace("\"", "").trim(), + variable_name, + ); + output.push_str(&assert_macro); + } + } + output + }; + + // process first 'if' condition + let condition_text = if_expr.condition(db).as_syntax_node().get_text_without_trivia(db); + let block_expr = if_expr.if_block(db); + let indentation = node + .get_text(db) + .lines() + .next() + .unwrap_or("") + .chars() + .take_while(|c| c.is_whitespace()) + .collect::(); + result.push_str(&process_block(condition_text, block_expr, &indentation)); + + // process 'else if' block + let mut else_clause = if_expr.else_clause(db); + while let OptionElseClause::ElseClause(else_if_clause) = else_clause { + if let BlockOrIf::If(else_if_expr) = else_if_clause.else_block_or_if(db) { + let condition_text = else_if_expr.condition(db).as_syntax_node().get_text_without_trivia(db); + let block_expr = else_if_expr.if_block(db); + result.push_str(&process_block(condition_text, block_expr, &indentation)); + else_clause = else_if_expr.else_clause(db); + } else { + break; + } + } + result + } } diff --git a/crates/cairo-lint-core/src/lints/manual/manual_assert.rs b/crates/cairo-lint-core/src/lints/manual/manual_assert.rs new file mode 100644 index 00000000..af18ac5d --- /dev/null +++ b/crates/cairo-lint-core/src/lints/manual/manual_assert.rs @@ -0,0 +1,34 @@ +use cairo_lang_defs::plugin::PluginDiagnostic; +use cairo_lang_diagnostics::Severity; +use cairo_lang_syntax::node::ast::{Condition, Expr, ExprBlock, ExprIf, Statement}; +use cairo_lang_syntax::node::db::SyntaxGroup; +use cairo_lang_syntax::node::{TypedStablePtr, TypedSyntaxNode}; + +pub const MANUAL_ASSERT: &str = "`assert!` is simpler than `if`-then-`panic!`"; + +pub fn contains_panic(db: &dyn SyntaxGroup, block_expr: &ExprBlock) -> bool { + let statements = block_expr.statements(db).elements(db); + if let Statement::Expr(statement_expr) = &statements[0] { + if let Expr::InlineMacro(inline_macro) = &statement_expr.expr(db) { + let macro_name: String = inline_macro.path(db).node.clone().get_text_without_trivia(db); + if macro_name == "panic" { + return true; + } + } + } + false +} + +pub fn check_if_then_panic(db: &dyn SyntaxGroup, expr: &ExprIf, diagnostics: &mut Vec) { + let condition = expr.condition(db); + let block_expr = expr.if_block(db); + if let Condition::Expr(_condition_expr) = condition + && contains_panic(db, &block_expr) + { + diagnostics.push(PluginDiagnostic { + stable_ptr: expr.stable_ptr().untyped(), + message: MANUAL_ASSERT.to_string(), + severity: Severity::Warning, + }); + } +} diff --git a/crates/cairo-lint-core/src/lints/manual/mod.rs b/crates/cairo-lint-core/src/lints/manual/mod.rs index 486606fb..add5318b 100644 --- a/crates/cairo-lint-core/src/lints/manual/mod.rs +++ b/crates/cairo-lint-core/src/lints/manual/mod.rs @@ -1,3 +1,4 @@ +pub mod manual_assert; pub mod manual_expect; pub mod manual_is_none; pub mod manual_is_some; diff --git a/crates/cairo-lint-core/src/plugin.rs b/crates/cairo-lint-core/src/plugin.rs index 1d25a44c..84599941 100644 --- a/crates/cairo-lint-core/src/plugin.rs +++ b/crates/cairo-lint-core/src/plugin.rs @@ -34,6 +34,7 @@ pub enum CairoLintKind { CollapsibleIfElse, DuplicateUnderscoreArgs, LoopMatchPopFront, + ManualAssert, BitwiseForParityCheck, LoopForWhile, Unknown, @@ -59,6 +60,7 @@ pub fn diagnostic_kind_from_message(message: &str) -> CairoLintKind { collapsible_if_else::COLLAPSIBLE_IF_ELSE => CairoLintKind::CollapsibleIfElse, duplicate_underscore_args::DUPLICATE_UNDERSCORE_ARGS => CairoLintKind::DuplicateUnderscoreArgs, loops::LOOP_MATCH_POP_FRONT => CairoLintKind::LoopMatchPopFront, + manual_assert::MANUAL_ASSERT => CairoLintKind::ManualAssert, panic::PANIC_IN_CODE => CairoLintKind::Panic, loop_for_while::LOOP_FOR_WHILE => CairoLintKind::LoopForWhile, erasing_op::ERASING_OPERATION => CairoLintKind::ErasingOperation, @@ -111,11 +113,18 @@ impl AnalyzerPlugin for CairoLint { &mut diags, ), SyntaxKind::StatementBreak => breaks::check_break(db.upcast(), node, &mut diags), - SyntaxKind::ExprIf => equatable_if_let::check_equatable_if_let( - db.upcast(), - &ExprIf::from_syntax_node(db.upcast(), node), - &mut diags, - ), + SyntaxKind::ExprIf => { + equatable_if_let::check_equatable_if_let( + db.upcast(), + &ExprIf::from_syntax_node(db.upcast(), node.clone()), + &mut diags, + ); + manual_assert::check_if_then_panic( + db.upcast(), + &ExprIf::from_syntax_node(db.upcast(), node.clone()), + &mut diags, + ); + } SyntaxKind::ExprBinary => { let expr_binary = ExprBinary::from_syntax_node(db.upcast(), node); bool_comparison::check_bool_comparison(db.upcast(), &expr_binary, &mut diags); diff --git a/crates/cairo-lint-core/tests/test_files/manual/manual_assert b/crates/cairo-lint-core/tests/test_files/manual/manual_assert new file mode 100644 index 00000000..f232aa20 --- /dev/null +++ b/crates/cairo-lint-core/tests/test_files/manual/manual_assert @@ -0,0 +1,194 @@ +//! > check if or if + +//! > cairo_code +fn main() { + let sad_people: Array = array![]; + let happy_people: Array = array![]; + if !sad_people.is_empty() || !happy_people.is_empty() { + panic!("panic"); + } +} + +//! > diagnostics +warning: Plugin diagnostic: Leaving `panic!` in the code is discouraged. + --> lib.cairo:8:9 + | +8 | panic!("panic"); + | ----- + | +warning: Plugin diagnostic: Leaving `panic!` in the code is discouraged. + --> lib.cairo:8:9 + | +8 | panic!("panic"); + | ----- + | +warning: Plugin diagnostic: `assert!` is simpler than `if`-then-`panic!` + --> lib.cairo:6:5 + | +6 | if !sad_people.is_empty() || !happy_people.is_empty() { + | _____- +7 | | panic!("panic"); +8 | | } + | |_____- + | + +//! > fixed +fn main() { + let sad_people: Array = array![]; + let happy_people: Array = array![]; + assert!(!sad_people.is_empty() || !happy_people.is_empty(), "panic: {:?}", sad_people); +} + +//! > ========================================================================== + +//! > check if then else + +//! > cairo_code +fn main() { + let sad_people: Array = array![1, 2, 3]; + if !sad_people.is_empty() { + let _ = 0; + } else if sad_people.len() > 1 { + panic!("there are sad people"); + } else if sad_people.len() > 2 { + panic!("there are more sad people"); + } +} + +//! > diagnostics +warning: Plugin diagnostic: Leaving `panic!` in the code is discouraged. + --> lib.cairo:10:9 + | +10 | panic!("there are sad people"); + | ----- + | +warning: Plugin diagnostic: Leaving `panic!` in the code is discouraged. + --> lib.cairo:10:9 + | +10 | panic!("there are sad people"); + | ----- + | +warning: Plugin diagnostic: Leaving `panic!` in the code is discouraged. + --> lib.cairo:14:9 + | +14 | panic!("there are more sad people"); + | ----- + | +warning: Plugin diagnostic: Leaving `panic!` in the code is discouraged. + --> lib.cairo:14:9 + | +14 | panic!("there are more sad people"); + | ----- + | +warning: Plugin diagnostic: `assert!` is simpler than `if`-then-`panic!` + --> lib.cairo:8:12 + | + 8 | } else if sad_people.len() > 1 { + | ____________- + 9 | | panic!("there are sad people"); +10 | | } else if sad_people.len() > 2 { +11 | | panic!("there are more sad people"); +12 | | } + | |_____- + | +warning: Plugin diagnostic: `assert!` is simpler than `if`-then-`panic!` + --> lib.cairo:12:12 + | +12 | } else if sad_people.len() > 2 { + | ____________- +13 | | panic!("there are more sad people"); +14 | | } + | |_____- + | + +//! > fixed +fn main() { + let sad_people: Array = array![1, 2, 3]; + if !sad_people.is_empty() { + let _ = 0; + } else assert!(sad_people.len() > 1, "there are sad people: {:?}", sad_people); +assert!(sad_people.len() > 2, "there are more sad people: {:?}", sad_people); + +} + +//! > ========================================================================== + +//! > check-if-then + +//! > cairo_code +fn main() { + let sad_people: Array = array![]; + if !sad_people.is_empty() { + panic!("there are sad people"); + } +} + +//! > diagnostics +warning: Plugin diagnostic: Leaving `panic!` in the code is discouraged. + --> lib.cairo:6:9 + | +6 | panic!("there are sad people"); + | ----- + | +warning: Plugin diagnostic: Leaving `panic!` in the code is discouraged. + --> lib.cairo:6:9 + | +6 | panic!("there are sad people"); + | ----- + | +warning: Plugin diagnostic: `assert!` is simpler than `if`-then-`panic!` + --> lib.cairo:4:5 + | +4 | if !sad_people.is_empty() { + | _____- +5 | | panic!("there are sad people"); +6 | | } + | |_____- + | + +//! > fixed +fn main() { + let sad_people: Array = array![]; + assert!(!sad_people.is_empty(), "there are sad people: {:?}", sad_people); +} + +//! > ========================================================================== + +//! > check-if-then2 + +//! > cairo_code +fn main() { + let sad_people: Array = array![]; + if sad_people.len() > 2 { + panic!("there are sad people"); + } +} + +//! > diagnostics +warning: Plugin diagnostic: Leaving `panic!` in the code is discouraged. + --> lib.cairo:6:9 + | +6 | panic!("there are sad people"); + | ----- + | +warning: Plugin diagnostic: Leaving `panic!` in the code is discouraged. + --> lib.cairo:6:9 + | +6 | panic!("there are sad people"); + | ----- + | +warning: Plugin diagnostic: `assert!` is simpler than `if`-then-`panic!` + --> lib.cairo:4:5 + | +4 | if sad_people.len() > 2 { + | _____- +5 | | panic!("there are sad people"); +6 | | } + | |_____- + | + +//! > fixed +fn main() { + let sad_people: Array = array![]; + assert!(sad_people.len() > 2, "there are sad people: {:?}", sad_people); +} diff --git a/crates/cairo-lint-core/tests/tests.rs b/crates/cairo-lint-core/tests/tests.rs index c0e8c399..ea01e30c 100644 --- a/crates/cairo-lint-core/tests/tests.rs +++ b/crates/cairo-lint-core/tests/tests.rs @@ -221,3 +221,5 @@ test_file!( "test with comment in Some", "test with comment in None" ); + +test_file!(manual, manual_assert, "check if then else", "check if or if", "check-if-then", "check-if-then2");