Implement manual_unwrap_or_else lint
#473
Conversation
| /// let foo: Option<Struct> = None; | ||
| /// match foo { | ||
| /// Some(v) => v, | ||
| /// None => Struct { x: 0x0 }, | ||
| /// }; | ||
| /// ``` | ||
| /// | ||
| /// Can be simplified to: | ||
| /// | ||
| /// ```cairo | ||
| /// let foo: Option<i32> = None; | ||
| /// foo.unwrap_or_else(|| Struct { x: 0x0 }); |
There was a problem hiding this comment.
This case should be simplified to unwrap_or, do we have issue/follow-up for this?
There was a problem hiding this comment.
The constructor here is called in the closure not because of performance but because Struct doesn't implement Drop. It's not that case we discussed recently
There was a problem hiding this comment.
Can we add this context as comment?
| let is_droppable = db.droppable(matched_expr.ty()).is_ok(); | ||
| let is_manual_unwrap_or_else = | ||
| check_manual(db, match_expr, arenas, ManualLint::ManualUnwrapOrElse); | ||
| is_manual_unwrap_or_else && !is_droppable |
There was a problem hiding this comment.
| let is_droppable = db.droppable(matched_expr.ty()).is_ok(); | |
| let is_manual_unwrap_or_else = | |
| check_manual(db, match_expr, arenas, ManualLint::ManualUnwrapOrElse); | |
| is_manual_unwrap_or_else && !is_droppable | |
| let is_droppable = db.droppable(matched_expr.ty()).is_ok(); | |
| !is_droppable && check_manual(db, match_expr, arenas, ManualLint::ManualUnwrapOrElse) |
Nit: this way we don't execute check_manual if it is not necessary, a little bit faster
| arenas: &Arenas<'db>, | ||
| ) -> bool { | ||
| let condition_expr = db.expr_semantic(function_id, if_expr.if_block); | ||
| let is_droppable = db.droppable(condition_expr.ty()).is_ok(); |
| db: &'db dyn Database, | ||
| node: SyntaxNode<'db>, | ||
| ) -> Option<InternalFix<'db>> { | ||
| let expr = ast::Expr::from_syntax_node(db, node); |
There was a problem hiding this comment.
| let expr = ast::Expr::from_syntax_node(db, node); | |
| let expr = ast::Expr::cast(db, node)?; |
Are you sure it will always success?
There was a problem hiding this comment.
In fixer we should assume, that the given syntaxnode is correct. I would go for
let expr = ast::Expr::cast(db, node).expect("This should be valid as it's pointed by the diagnostics in the code above");or something like that
| ast::Expr::Match(expr_match) => { | ||
| let mut arms = expr_match.arms(db).elements(db); | ||
| let matched_expr = expr_match.expr(db).as_syntax_node(); | ||
| let arm = arms.nth(1).expect("Expected a `match` with second arm."); |
There was a problem hiding this comment.
| let arm = arms.nth(1).expect("Expected a `match` with second arm."); | |
| let arm = arms.nth(1)?; |
We didn't check this earlier (correct me if I'm wrong), so there is no guarantee it won't panic
There was a problem hiding this comment.
Also, the match arms order is no longer guaranteed, and here we make an assumption that .nth(1) branch is wrong
There was a problem hiding this comment.
Actually we should expect the match expr to have a second arm, as it's checked here. The overall concept of fixers, is that we can assume a lot of things, as the syntax node in the args, is coming based on the diagnostics that we sent from the code above.
tl;dr
We should expect certain type/form of data in the fixer. If it panics, it means that the diagnostics above are doing something wrong
| .expect("Expected an `if` with a condition."); | ||
| let condition = match matched_expr { | ||
| ast::Condition::Let(condition_let) => condition_let.expr(db).as_syntax_node(), | ||
| _ => panic!("Expected an `if let` expression."), |
There was a problem hiding this comment.
Same as above, i assume it's expected here as it is, as the fixer should only fire when we get the diagnostics from the checker. We can "expect" things here
| }; | ||
|
|
||
| let ast::OptionElseClause::ElseClause(else_clause) = expr_if.else_clause(db) else { | ||
| panic!("No else clause found."); |
| let a: Option<Struct> = Option::Some(Struct { x: 0x0 }); | ||
|
|
||
| match a { | ||
| Option::Some(v) => v, |
There was a problem hiding this comment.
Add test with reversed match arms order
| fn main() { | ||
| let a: Option<Struct> = Option::Some(Struct { x: 0x0 }); | ||
|
|
||
| if let Option::Some(v) = a { |
There was a problem hiding this comment.
add test for if true && let Some(v) if possible, otherwise make issue
| let or_body = if let ast::Expr::Block(block) = arm.expression(db) { | ||
| let block_statements = block.statements(db).node.get_text(db); | ||
|
|
||
| // If the block has more than one line or a comment, we need to adjust the indentation. |
Closes #393
Closes #405