From fb6329d18f16a8955aca4fdafca01481651c06ee Mon Sep 17 00:00:00 2001 From: stevencartavia Date: Mon, 23 Sep 2024 08:23:02 -0600 Subject: [PATCH 1/5] manual assert --- crates/cairo-lint-core/src/fix.rs | 41 +++++++++++++++++++ .../src/lints/manual/manual_assert.rs | 34 +++++++++++++++ .../cairo-lint-core/src/lints/manual/mod.rs | 1 + crates/cairo-lint-core/src/lints/mod.rs | 1 + crates/cairo-lint-core/src/plugin.rs | 20 ++++++--- .../test_files/manual_assert/manual_assert | 26 ++++++++++++ crates/cairo-lint-core/tests/tests.rs | 2 + 7 files changed, 120 insertions(+), 5 deletions(-) create mode 100644 crates/cairo-lint-core/src/lints/manual/manual_assert.rs create mode 100644 crates/cairo-lint-core/src/lints/manual/mod.rs create mode 100644 crates/cairo-lint-core/tests/test_files/manual_assert/manual_assert diff --git a/crates/cairo-lint-core/src/fix.rs b/crates/cairo-lint-core/src/fix.rs index 7cf99a24..5ff8d5e3 100644 --- a/crates/cairo-lint-core/src/fix.rs +++ b/crates/cairo-lint-core/src/fix.rs @@ -180,6 +180,7 @@ impl Fixer { CairoLintKind::LoopMatchPopFront => { self.fix_loop_match_pop_front(db, plugin_diag.stable_ptr.lookup(db.upcast())) } + CairoLintKind::ManualAssert => self.fix_if_then_panic(db, plugin_diag.stable_ptr.lookup(db.upcast())), _ => return None, }; Some((semantic_diag.stable_location.syntax_node(db.upcast()), new_text)) @@ -378,4 +379,44 @@ impl Fixer { expr.if_block(db).as_syntax_node().get_text(db), ) } + + pub fn fix_if_then_panic(&self, db: &dyn SyntaxGroup, node: SyntaxNode) -> String { + let if_expr = ExprIf::from_syntax_node(db, node.clone()); + let condition = if let Condition::Expr(expr) = if_expr.condition(db) { + expr + } else { + panic!("Unexpected condition type"); + }; + + let block_expr = if_expr.if_block(db); + let statements = block_expr.statements(db).elements(db); + + if let Some(Statement::Expr(statement_expr)) = statements.get(0) { + 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 condition_text = condition.as_syntax_node().get_text_without_trivia(db); + let assert_condition = condition_text.trim_start_matches('!'); + + let main_expression = if let Some(pos) = assert_condition.find(".is_empty()") { + &assert_condition[..pos] + } else { + assert_condition + }; + + let indentation = node.get_text(db).chars().take_while(|c| c.is_whitespace()).collect::(); + + let assert_macro = format!( + "{}assert!({}, \"{}: {{:?}}\", {});\n", + indentation, + assert_condition, + panic_message.trim_matches(&['(', ')'][..]).replace("\"", "").trim(), + main_expression, + ); + + return assert_macro; + } + } + + node.get_text(db).to_string() + } } 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 new file mode 100644 index 00000000..77d8308a --- /dev/null +++ b/crates/cairo-lint-core/src/lints/manual/mod.rs @@ -0,0 +1 @@ +pub mod manual_assert; diff --git a/crates/cairo-lint-core/src/lints/mod.rs b/crates/cairo-lint-core/src/lints/mod.rs index b1f08dc0..f17e8063 100644 --- a/crates/cairo-lint-core/src/lints/mod.rs +++ b/crates/cairo-lint-core/src/lints/mod.rs @@ -5,4 +5,5 @@ pub mod double_parens; pub mod duplicate_underscore_args; pub mod ifs; pub mod loops; +pub mod manual; pub mod single_match; diff --git a/crates/cairo-lint-core/src/plugin.rs b/crates/cairo-lint-core/src/plugin.rs index 570f0263..a56fbeb0 100644 --- a/crates/cairo-lint-core/src/plugin.rs +++ b/crates/cairo-lint-core/src/plugin.rs @@ -8,6 +8,7 @@ use cairo_lang_syntax::node::kind::SyntaxKind; use cairo_lang_syntax::node::{TypedStablePtr, TypedSyntaxNode}; use crate::lints::ifs::*; +use crate::lints::manual::manual_assert; use crate::lints::{ bool_comparison, breaks, double_comparison, double_parens, duplicate_underscore_args, loops, single_match, }; @@ -32,6 +33,7 @@ pub enum CairoLintKind { CollapsibleIfElse, DuplicateUnderscoreArgs, LoopMatchPopFront, + ManualAssert, Unknown, } @@ -49,6 +51,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, _ => CairoLintKind::Unknown, } } @@ -93,11 +96,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_assert/manual_assert b/crates/cairo-lint-core/tests/test_files/manual_assert/manual_assert new file mode 100644 index 00000000..63b55078 --- /dev/null +++ b/crates/cairo-lint-core/tests/test_files/manual_assert/manual_assert @@ -0,0 +1,26 @@ +//! > 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: `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); +} diff --git a/crates/cairo-lint-core/tests/tests.rs b/crates/cairo-lint-core/tests/tests.rs index 4cc35743..9234573d 100644 --- a/crates/cairo-lint-core/tests/tests.rs +++ b/crates/cairo-lint-core/tests/tests.rs @@ -153,3 +153,5 @@ test_file!( "Else if with multiple statements", "Else if inside loop" ); + +test_file!(manual_assert, manual_assert, "check-if-then"); From 54ecae4483905f79ac540e8d2ca8640d359c87c1 Mon Sep 17 00:00:00 2001 From: stevencartavia Date: Mon, 23 Sep 2024 08:29:23 -0600 Subject: [PATCH 2/5] fmt --- crates/cairo-lint-core/src/fix.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/cairo-lint-core/src/fix.rs b/crates/cairo-lint-core/src/fix.rs index 5ff8d5e3..dd585818 100644 --- a/crates/cairo-lint-core/src/fix.rs +++ b/crates/cairo-lint-core/src/fix.rs @@ -396,15 +396,15 @@ impl Fixer { let panic_message = inline_macro.arguments(db).as_syntax_node().get_text_without_trivia(db); let condition_text = condition.as_syntax_node().get_text_without_trivia(db); let assert_condition = condition_text.trim_start_matches('!'); - + let main_expression = if let Some(pos) = assert_condition.find(".is_empty()") { &assert_condition[..pos] } else { assert_condition }; - + let indentation = node.get_text(db).chars().take_while(|c| c.is_whitespace()).collect::(); - + let assert_macro = format!( "{}assert!({}, \"{}: {{:?}}\", {});\n", indentation, From 4cb2e0c6ad3fa7c53f9319432e823549b617a997 Mon Sep 17 00:00:00 2001 From: stevencartavia Date: Mon, 23 Sep 2024 08:34:51 -0600 Subject: [PATCH 3/5] fix fn update --- crates/cairo-lint-core/src/fix.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/cairo-lint-core/src/fix.rs b/crates/cairo-lint-core/src/fix.rs index dd585818..e9a8b8af 100644 --- a/crates/cairo-lint-core/src/fix.rs +++ b/crates/cairo-lint-core/src/fix.rs @@ -391,7 +391,7 @@ impl Fixer { let block_expr = if_expr.if_block(db); let statements = block_expr.statements(db).elements(db); - if let Some(Statement::Expr(statement_expr)) = statements.get(0) { + 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 condition_text = condition.as_syntax_node().get_text_without_trivia(db); From 569aa05521f3674f24d01f11f7f6187fbc83b72e Mon Sep 17 00:00:00 2001 From: stevencartavia Date: Wed, 25 Sep 2024 08:34:14 -0600 Subject: [PATCH 4/5] fix --- crates/cairo-lint-core/src/fix.rs | 4 +-- .../tests/test_files/manual/manual_assert | 26 ++++++++++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/crates/cairo-lint-core/src/fix.rs b/crates/cairo-lint-core/src/fix.rs index 03ad1ec3..d5a04ca9 100644 --- a/crates/cairo-lint-core/src/fix.rs +++ b/crates/cairo-lint-core/src/fix.rs @@ -513,8 +513,6 @@ impl Fixer { 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 assert_condition = condition_text.trim_start_matches('!'); - let main_expression = assert_condition.split('.').next().unwrap_or(assert_condition); let indentation = node.get_text(db).chars().take_while(|c| c.is_whitespace()).collect::(); @@ -523,7 +521,7 @@ impl Fixer { indentation, condition_text, panic_message.trim_matches(&['(', ')'][..]).replace("\"", "").trim(), - main_expression, + condition_text.split('.').next().unwrap_or(&condition_text), ); return assert_macro; diff --git a/crates/cairo-lint-core/tests/test_files/manual/manual_assert b/crates/cairo-lint-core/tests/test_files/manual/manual_assert index 2dcff79d..bae5c37e 100644 --- a/crates/cairo-lint-core/tests/test_files/manual/manual_assert +++ b/crates/cairo-lint-core/tests/test_files/manual/manual_assert @@ -9,6 +9,18 @@ fn main() { } //! > 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 | @@ -22,7 +34,7 @@ warning: Plugin diagnostic: `assert!` is simpler than `if`-then-`panic!` //! > fixed fn main() { let sad_people: Array = array![]; - assert!(!sad_people.is_empty(), "there are sad people: {:?}", sad_people); + assert!(!sad_people.is_empty(), "there are sad people: {:?}", !sad_people); } //! > ========================================================================== @@ -38,6 +50,18 @@ fn main() { } //! > 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 | From 35948a92fc379fcda7c3a240528ac2473a4d8d42 Mon Sep 17 00:00:00 2001 From: stevencartavia Date: Thu, 26 Sep 2024 19:35:24 -0600 Subject: [PATCH 5/5] more tests --- crates/cairo-lint-core/src/fix.rs | 59 +++++++++ .../tests/test_files/manual/manual_assert | 117 +++++++++++++++++- crates/cairo-lint-core/tests/tests.rs | 2 +- 3 files changed, 176 insertions(+), 2 deletions(-) diff --git a/crates/cairo-lint-core/src/fix.rs b/crates/cairo-lint-core/src/fix.rs index af470c40..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 @@ -557,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/tests/test_files/manual/manual_assert b/crates/cairo-lint-core/tests/test_files/manual/manual_assert index bae5c37e..f232aa20 100644 --- a/crates/cairo-lint-core/tests/test_files/manual/manual_assert +++ b/crates/cairo-lint-core/tests/test_files/manual/manual_assert @@ -1,3 +1,118 @@ +//! > 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 @@ -34,7 +149,7 @@ warning: Plugin diagnostic: `assert!` is simpler than `if`-then-`panic!` //! > fixed fn main() { let sad_people: Array = array![]; - assert!(!sad_people.is_empty(), "there are sad people: {:?}", !sad_people); + assert!(!sad_people.is_empty(), "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 0c589d37..ea01e30c 100644 --- a/crates/cairo-lint-core/tests/tests.rs +++ b/crates/cairo-lint-core/tests/tests.rs @@ -222,4 +222,4 @@ test_file!( "test with comment in None" ); -test_file!(manual, manual_assert, "check-if-then", "check-if-then2"); +test_file!(manual, manual_assert, "check if then else", "check if or if", "check-if-then", "check-if-then2");