Add new case for question_mark lint
#13772
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @y21 (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
clippy_lints/src/question_mark.rs
Outdated
| _: Span, | ||
| _: LocalDefId, | ||
| ) { | ||
| let return_expr; |
There was a problem hiding this comment.
Can we make it more compact something like this
let return_expr = match body.value.kind {
ExprKind::Block(block, _) => block.expr.or_else(|| {
block.stmts.iter().last().and_then(|stmt| {
if let StmtKind::Semi(expr) = &stmt.kind {
if let ExprKind::Ret(Some(ret)) = &expr.kind {
return Some(ret);
}
}
None
})
}),
_ => None,
};
if let Some(expr) = return_expr {
check_if_let_some_as_return_val(cx, expr);
}
There was a problem hiding this comment.
Thanks for the suggestion, this is much better
DylanBulfin
force-pushed
the
return-if-let-some-else
branch
from
f2d5663 to
5cc7eb6
Compare
| && if_else | ||
| .map(|e| eq_expr_value(cx, let_expr, peel_blocks(e))) | ||
| .filter(|e| *e) | ||
| .is_none() |
There was a problem hiding this comment.
What is this checking for? The is_early_return() checks that the else block is just None so this case seems unreachable to me? Unless you want to exclude specifically if let Some() = None but I'm not sure why
| && let ExprKind::Block(if_then_block, _) = if_then.kind | ||
| // Don't consider case where if-then branch has only one statement/expression | ||
| && (if_then_block.stmts.len() >= 2 || if_then_block.stmts.len() == 1 && if_then_block.expr.is_some()) | ||
| && (is_early_return(sym::Option, cx, &if_block)) |
There was a problem hiding this comment.
| && (is_early_return(sym::Option, cx, &if_block)) | |
| && is_early_return(sym::Option, cx, &if_block) |
|
|
||
| let body = snippet_with_applicability(cx, if_then.span, "..", &mut applicability); | ||
| let mut body_lines = body.lines().map(|line| &line[line.len().min(4)..]); | ||
| let (_, _) = (body_lines.next(), body_lines.next_back()); |
There was a problem hiding this comment.
This seems like it could lead to broken suggestions if the whole (or parts of the) if let are on the same line as the if let header. I guess it's a bit of an unusual case considering it lints only when there are at least two statements and rustfmt would split that up into separate lines, but still seems suboptimal especially for a machine applicable suggestion.
I wonder if this could use the inside span of the if block's braces and clippy_utils::source::reindent_multiline?
| _: Span, | ||
| _: LocalDefId, | ||
| ) { | ||
| let return_expr = match body.value.kind { |
There was a problem hiding this comment.
This also needs some of the checks that are in check_expr. Notably, a check that this isn't a const fn and that the question_mark_used lint isn't enabled
| }; | ||
|
|
||
| let body = snippet_with_applicability(cx, if_then.span, "..", &mut applicability); | ||
| let mut body_lines = body.lines().map(|line| &line[line.len().min(4)..]); |
There was a problem hiding this comment.
Instead of hardcoding 4, this might be a good use case for clippy_utils::source::indent_of(expr.span)
| if_then, | ||
| if_else, | ||
| ) | ||
| && let ExprKind::Block(if_then_block, _) = if_then.kind |
There was a problem hiding this comment.
There should be a check that this code isn't from an (external) macro expansion (can use in_external_macro)
| .is_none() | ||
| { | ||
| let mut applicability = Applicability::MachineApplicable; | ||
| let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability); |
There was a problem hiding this comment.
Should be snippet_with_context with expr.span.ctxt() for the syntax context, and a test case would be good where the if let scrutinee is a macro invocation:
macro_rules! m { () => { Some(()) } }
fn foo() -> Option<()> {
if let Some(v) = m!() {
Some(())
} else {
None
}
}currently the suggestion contains the macro expansion directly (if let Some(v) = Some(()))
| // Defining check_fn instead of using check_body to avoid accidentally triggering on | ||
| // const expressions, not sure if this is necessary |
changelog: [
question_mark]: suggests rewriting with question mark when returning an if-let-Some expressionfixes #13626
I think it's mostly done unless I've missed an edge case. I could use some pointers on formatting the suggestion though, I think my solution works but it's not very elegant.