Skip to content

fix: Wrap inner tail expressions in MissingOkOrSomeInTailExpr #9747

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 31, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/hir/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ use hir_def::path::ModPath;
use hir_expand::{name::Name, HirFileId, InFile};
use syntax::{ast, AstPtr, SyntaxNodePtr, TextRange};

use crate::Type;

macro_rules! diagnostics {
($($diag:ident,)*) => {
pub enum AnyDiagnostic {$(
Expand Down Expand Up @@ -142,6 +144,7 @@ pub struct MissingOkOrSomeInTailExpr {
pub expr: InFile<AstPtr<ast::Expr>>,
// `Some` or `Ok` depending on whether the return type is Result or Option
pub required: String,
pub expected: Type,
}

#[derive(Debug)]
Expand Down
9 changes: 8 additions & 1 deletion crates/hir/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1216,7 +1216,14 @@ impl Function {
}
BodyValidationDiagnostic::MissingOkOrSomeInTailExpr { expr, required } => {
match source_map.expr_syntax(expr) {
Ok(expr) => acc.push(MissingOkOrSomeInTailExpr { expr, required }.into()),
Ok(expr) => acc.push(
MissingOkOrSomeInTailExpr {
expr,
required,
expected: self.ret_type(db),
}
.into(),
),
Err(SyntheticSyntax) => (),
}
}
Expand Down
1 change: 1 addition & 0 deletions crates/hir_ty/src/diagnostics/match_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

mod deconstruct_pat;
mod pat_util;

pub(crate) mod usefulness;

use hir_def::{body::Body, EnumVariantId, LocalFieldId, VariantId};
Expand Down
3 changes: 3 additions & 0 deletions crates/ide/src/highlight_related.rs
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,9 @@ fn foo() ->$0 u32 {
5
// ^
}
} else if false {
0
// ^
} else {
match 5 {
6 => 100,
Expand Down
6 changes: 1 addition & 5 deletions crates/ide_completion/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -624,11 +624,7 @@ impl<'a> CompletionContext<'a> {
fn classify_name(&mut self, name: ast::Name) {
if let Some(bind_pat) = name.syntax().parent().and_then(ast::IdentPat::cast) {
self.is_pat_or_const = Some(PatternRefutability::Refutable);
// if any of these is here our bind pat can't be a const pat anymore
let complex_ident_pat = bind_pat.at_token().is_some()
|| bind_pat.ref_token().is_some()
|| bind_pat.mut_token().is_some();
if complex_ident_pat {
if !bind_pat.is_simple_ident() {
self.is_pat_or_const = None;
} else {
let irrefutable_pat = bind_pat.syntax().ancestors().find_map(|node| {
Expand Down
15 changes: 14 additions & 1 deletion crates/ide_db/src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,20 @@ pub fn for_each_tail_expr(expr: &ast::Expr, cb: &mut dyn FnMut(&ast::Expr)) {
ast::Effect::Async(_) | ast::Effect::Try(_) | ast::Effect::Const(_) => cb(expr),
},
ast::Expr::IfExpr(if_) => {
if_.blocks().for_each(|block| for_each_tail_expr(&ast::Expr::BlockExpr(block), cb))
let mut if_ = if_.clone();
loop {
if let Some(block) = if_.then_branch() {
for_each_tail_expr(&ast::Expr::BlockExpr(block), cb);
}
match if_.else_branch() {
Some(ast::ElseBranch::IfExpr(it)) => if_ = it,
Some(ast::ElseBranch::Block(block)) => {
for_each_tail_expr(&ast::Expr::BlockExpr(block), cb);
break;
}
None => break,
}
}
}
ast::Expr::LoopExpr(l) => {
for_each_break_expr(l.label(), l.loop_body(), &mut |b| cb(&ast::Expr::BreakExpr(b)))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use hir::db::AstDatabase;
use ide_db::{assists::Assist, source_change::SourceChange};
use ide_db::{assists::Assist, helpers::for_each_tail_expr, source_change::SourceChange};
use syntax::AstNode;
use text_edit::TextEdit;

Expand Down Expand Up @@ -33,10 +33,15 @@ fn fixes(ctx: &DiagnosticsContext<'_>, d: &hir::MissingOkOrSomeInTailExpr) -> Op
let root = ctx.sema.db.parse_or_expand(d.expr.file_id)?;
let tail_expr = d.expr.value.to_node(&root);
let tail_expr_range = tail_expr.syntax().text_range();
let replacement = format!("{}({})", d.required, tail_expr.syntax());
let edit = TextEdit::replace(tail_expr_range, replacement);
let mut builder = TextEdit::builder();
for_each_tail_expr(&tail_expr, &mut |expr| {
if ctx.sema.type_of_expr(expr).as_ref() != Some(&d.expected) {
builder.insert(expr.syntax().text_range().start(), format!("{}(", d.required));
builder.insert(expr.syntax().text_range().end(), ")".to_string());
}
});
let source_change =
SourceChange::from_text_edit(d.expr.file_id.original_file(ctx.sema.db), edit);
SourceChange::from_text_edit(d.expr.file_id.original_file(ctx.sema.db), builder.finish());
let name = if d.required == "Ok" { "Wrap with Ok" } else { "Wrap with Some" };
Some(vec![fix("wrap_tail_expr", name, source_change, tail_expr_range)])
}
Expand Down Expand Up @@ -68,6 +73,35 @@ fn div(x: i32, y: i32) -> Option<i32> {
);
}

#[test]
fn test_wrap_return_type_option_tails() {
check_fix(
r#"
//- minicore: option, result
fn div(x: i32, y: i32) -> Option<i32> {
if y == 0 {
0
} else if true {
100
} else {
None
}$0
}
"#,
r#"
fn div(x: i32, y: i32) -> Option<i32> {
if y == 0 {
Some(0)
} else if true {
Some(100)
} else {
None
}
}
"#,
);
}

#[test]
fn test_wrap_return_type() {
check_fix(
Expand Down
1 change: 1 addition & 0 deletions crates/syntax/src/ast/expr_ext.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ impl ast::IfExpr {
pub fn then_branch(&self) -> Option<ast::BlockExpr> {
self.blocks().next()
}

pub fn else_branch(&self) -> Option<ElseBranch> {
let res = match self.blocks().nth(1) {
Some(block) => ElseBranch::Block(block),
Expand Down