Skip to content
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.

Sign up for GitHub

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

internal: Migrate apply_demorgan to SyntaxEditor #19171

Merged
merged 3 commits into from
Feb 24, 2025
Merged

internal: Migrate apply_demorgan to SyntaxEditor #19171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f06f1b8
Migrate some leftovers in `add_missing_match_arms`
ShoyuVanilla Feb 17, 2025
bf0c394
Migrate `apply_demorgan` to `SyntaxEditor`
ShoyuVanilla Feb 17, 2025
a0b9931
Add `take()` method to `SyntaxEditor`
ShoyuVanilla Feb 18, 2025
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
49 changes: 27 additions & 22 deletions crates/ide-assists/src/handlers/add_missing_match_arms.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)

let cfg = ctx.config.import_path_config();

let make = SyntaxFactory::new();

let module = ctx.sema.scope(expr.syntax())?.module();
let (mut missing_pats, is_non_exhaustive, has_hidden_variants): (
Peekable<Box<dyn Iterator<Item = (ast::Pat, bool)>>>,
Expand All @@ -93,7 +95,7 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
.into_iter()
.filter_map(|variant| {
Some((
build_pat(ctx, module, variant, cfg)?,
build_pat(ctx, &make, module, variant, cfg)?,
variant.should_be_hidden(ctx.db(), module.krate()),
))
})
Expand Down Expand Up @@ -144,10 +146,11 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
let is_hidden = variants
.iter()
.any(|variant| variant.should_be_hidden(ctx.db(), module.krate()));
let patterns =
variants.into_iter().filter_map(|variant| build_pat(ctx, module, variant, cfg));
let patterns = variants
.into_iter()
.filter_map(|variant| build_pat(ctx, &make, module, variant, cfg));

(ast::Pat::from(make::tuple_pat(patterns)), is_hidden)
(ast::Pat::from(make.tuple_pat(patterns)), is_hidden)
})
.filter(|(variant_pat, _)| is_variant_missing(&top_lvl_pats, variant_pat));
(
Expand Down Expand Up @@ -176,9 +179,11 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
let is_hidden = variants
.iter()
.any(|variant| variant.should_be_hidden(ctx.db(), module.krate()));
let patterns =
variants.into_iter().filter_map(|variant| build_pat(ctx, module, variant, cfg));
(ast::Pat::from(make::slice_pat(patterns)), is_hidden)
let patterns = variants
.into_iter()
.filter_map(|variant| build_pat(ctx, &make, module, variant, cfg));

(ast::Pat::from(make.slice_pat(patterns)), is_hidden)
})
.filter(|(variant_pat, _)| is_variant_missing(&top_lvl_pats, variant_pat));
(
Expand All @@ -203,8 +208,6 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
"Fill match arms",
ctx.sema.original_range(match_expr.syntax()).range,
|builder| {
let make = SyntaxFactory::new();

// having any hidden variants means that we need a catch-all arm
needs_catch_all_arm |= has_hidden_variants;

Expand Down Expand Up @@ -243,7 +246,7 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)

if needs_catch_all_arm && !has_catch_all_arm {
cov_mark::hit!(added_wildcard_pattern);
let arm = make.match_arm(make::wildcard_pat().into(), None, make::ext::expr_todo());
let arm = make.match_arm(make.wildcard_pat().into(), None, make::ext::expr_todo());
arms.push(arm);
}

Expand Down Expand Up @@ -290,7 +293,7 @@ pub(crate) fn add_missing_match_arms(acc: &mut Assists, ctx: &AssistContext<'_>)
}
}

editor.add_mappings(make.finish_with_mappings());
editor.add_mappings(make.take());
builder.add_file_edits(ctx.file_id(), editor);
},
)
Expand Down Expand Up @@ -445,6 +448,7 @@ fn resolve_array_of_enum_def(

fn build_pat(
ctx: &AssistContext<'_>,
make: &SyntaxFactory,
module: hir::Module,
var: ExtendedVariant,
cfg: ImportPathConfig,
Expand All @@ -455,31 +459,32 @@ fn build_pat(
let edition = module.krate().edition(db);
let path = mod_path_to_ast(&module.find_path(db, ModuleDef::from(var), cfg)?, edition);
let fields = var.fields(db);
let pat = match var.kind(db) {
let pat: ast::Pat = match var.kind(db) {
hir::StructKind::Tuple => {
let mut name_generator = suggest_name::NameGenerator::new();
let pats = fields.into_iter().map(|f| {
let name = name_generator.for_type(&f.ty(db), db, edition);
match name {
Some(name) => make::ext::simple_ident_pat(make::name(&name)).into(),
None => make::wildcard_pat().into(),
Some(name) => make::ext::simple_ident_pat(make.name(&name)).into(),
None => make.wildcard_pat().into(),
}
});
make::tuple_struct_pat(path, pats).into()
make.tuple_struct_pat(path, pats).into()
}
hir::StructKind::Record => {
let pats = fields
let fields = fields
.into_iter()
.map(|f| make::name(f.name(db).as_str()))
.map(|name| make::ext::simple_ident_pat(name).into());
make::record_pat(path, pats).into()
.map(|f| make.name_ref(f.name(db).as_str()))
.map(|name_ref| make.record_pat_field_shorthand(name_ref));
let fields = make.record_pat_field_list(fields, None);
make.record_pat_with_fields(path, fields).into()
}
hir::StructKind::Unit => make::path_pat(path),
hir::StructKind::Unit => make.path_pat(path),
};
Some(pat)
}
ExtendedVariant::True => Some(ast::Pat::from(make::literal_pat("true"))),
ExtendedVariant::False => Some(ast::Pat::from(make::literal_pat("false"))),
ExtendedVariant::True => Some(ast::Pat::from(make.literal_pat("true"))),
ExtendedVariant::False => Some(ast::Pat::from(make.literal_pat("false"))),
}
}

Expand Down
120 changes: 80 additions & 40 deletions crates/ide-assists/src/handlers/apply_demorgan.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ use std::collections::VecDeque;
use ide_db::{
assists::GroupLabel,
famous_defs::FamousDefs,
source_change::SourceChangeBuilder,
syntax_helpers::node_ext::{for_each_tail_expr, walk_expr},
};
use syntax::{
ast::{self, make, AstNode, Expr::BinExpr, HasArgList},
ted, SyntaxKind, T,
ast::{self, syntax_factory::SyntaxFactory, AstNode, Expr::BinExpr, HasArgList},
syntax_editor::{Position, SyntaxEditor},
SyntaxKind, SyntaxNode, T,
};

use crate::{utils::invert_boolean_expression, AssistContext, AssistId, AssistKind, Assists};
Expand Down Expand Up @@ -58,9 +58,12 @@ pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti
_ => return None,
};

let demorganed = bin_expr.clone_subtree().clone_for_update();
let make = SyntaxFactory::new();

let demorganed = bin_expr.clone_subtree();
let mut editor = SyntaxEditor::new(demorganed.syntax().clone());
editor.replace(demorganed.op_token()?, make.token(inv_token));

ted::replace(demorganed.op_token()?, ast::make::token(inv_token));
let mut exprs = VecDeque::from([
(bin_expr.lhs()?, demorganed.lhs()?),
(bin_expr.rhs()?, demorganed.rhs()?),
Expand All @@ -70,35 +73,39 @@ pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti
if let BinExpr(bin_expr) = &expr {
if let BinExpr(cbin_expr) = &dm {
if op == bin_expr.op_kind()? {
ted::replace(cbin_expr.op_token()?, ast::make::token(inv_token));
editor.replace(cbin_expr.op_token()?, make.token(inv_token));
exprs.push_back((bin_expr.lhs()?, cbin_expr.lhs()?));
exprs.push_back((bin_expr.rhs()?, cbin_expr.rhs()?));
} else {
let mut inv = invert_boolean_expression(expr);
if inv.needs_parens_in(dm.syntax().parent()?) {
inv = ast::make::expr_paren(inv).clone_for_update();
let mut inv = invert_boolean_expression(&make, expr);
if needs_parens_in_place_of(&inv, &dm.syntax().parent()?, &dm) {
inv = make.expr_paren(inv).into();
}
ted::replace(dm.syntax(), inv.syntax());
editor.replace(dm.syntax(), inv.syntax());
}
} else {
return None;
}
} else {
let mut inv = invert_boolean_expression(dm.clone_subtree()).clone_for_update();
if inv.needs_parens_in(dm.syntax().parent()?) {
inv = ast::make::expr_paren(inv).clone_for_update();
let mut inv = invert_boolean_expression(&make, dm.clone());
if needs_parens_in_place_of(&inv, &dm.syntax().parent()?, &dm) {
inv = make.expr_paren(inv).into();
}
ted::replace(dm.syntax(), inv.syntax());
editor.replace(dm.syntax(), inv.syntax());
}
}

editor.add_mappings(make.finish_with_mappings());
let edit = editor.finish();
let demorganed = ast::Expr::cast(edit.new_root().clone())?;

acc.add_group(
&GroupLabel("Apply De Morgan's law".to_owned()),
AssistId("apply_demorgan", AssistKind::RefactorRewrite),
"Apply De Morgan's law",
op_range,
|edit| {
let demorganed = ast::Expr::BinExpr(demorganed);
|builder| {
let make = SyntaxFactory::new();
let paren_expr = bin_expr.syntax().parent().and_then(ast::ParenExpr::cast);
let neg_expr = paren_expr
.clone()
Expand All @@ -107,24 +114,32 @@ pub(crate) fn apply_demorgan(acc: &mut Assists, ctx: &AssistContext<'_>) -> Opti
.filter(|prefix_expr| matches!(prefix_expr.op_kind(), Some(ast::UnaryOp::Not)))
.map(ast::Expr::PrefixExpr);

let mut editor;
if let Some(paren_expr) = paren_expr {
if let Some(neg_expr) = neg_expr {
cov_mark::hit!(demorgan_double_negation);
let parent = neg_expr.syntax().parent();
editor = builder.make_editor(neg_expr.syntax());

if parent.is_some_and(|parent| demorganed.needs_parens_in(parent)) {
cov_mark::hit!(demorgan_keep_parens_for_op_precedence2);
edit.replace_ast(neg_expr, make::expr_paren(demorganed));
editor.replace(neg_expr.syntax(), make.expr_paren(demorganed).syntax());
} else {
edit.replace_ast(neg_expr, demorganed);
editor.replace(neg_expr.syntax(), demorganed.syntax());
};
} else {
cov_mark::hit!(demorgan_double_parens);
edit.replace_ast(paren_expr.into(), add_bang_paren(demorganed));
editor = builder.make_editor(paren_expr.syntax());

editor.replace(paren_expr.syntax(), add_bang_paren(&make, demorganed).syntax());
}
} else {
edit.replace_ast(bin_expr.into(), add_bang_paren(demorganed));
editor = builder.make_editor(bin_expr.syntax());
editor.replace(bin_expr.syntax(), add_bang_paren(&make, demorganed).syntax());
}

editor.add_mappings(make.finish_with_mappings());
builder.add_file_edits(ctx.file_id(), editor);
},
)
}
Expand Down Expand Up @@ -161,7 +176,7 @@ pub(crate) fn apply_demorgan_iterator(acc: &mut Assists, ctx: &AssistContext<'_>
let (name, arg_expr) = validate_method_call_expr(ctx, &method_call)?;

let ast::Expr::ClosureExpr(closure_expr) = arg_expr else { return None };
let closure_body = closure_expr.body()?;
let closure_body = closure_expr.body()?.clone_for_update();

let op_range = method_call.syntax().text_range();
let label = format!("Apply De Morgan's law to `Iterator::{}`", name.text().as_str());
Expand All @@ -170,18 +185,19 @@ pub(crate) fn apply_demorgan_iterator(acc: &mut Assists, ctx: &AssistContext<'_>
AssistId("apply_demorgan_iterator", AssistKind::RefactorRewrite),
label,
op_range,
|edit| {
|builder| {
let make = SyntaxFactory::new();
let mut editor = builder.make_editor(method_call.syntax());
// replace the method name
let new_name = match name.text().as_str() {
"all" => make::name_ref("any"),
"any" => make::name_ref("all"),
"all" => make.name_ref("any"),
"any" => make.name_ref("all"),
_ => unreachable!(),
}
.clone_for_update();
edit.replace_ast(name, new_name);
};
editor.replace(name.syntax(), new_name.syntax());

// negate all tail expressions in the closure body
let tail_cb = &mut |e: &_| tail_cb_impl(edit, e);
let tail_cb = &mut |e: &_| tail_cb_impl(&mut editor, &make, e);
walk_expr(&closure_body, &mut |expr| {
if let ast::Expr::ReturnExpr(ret_expr) = expr {
if let Some(ret_expr_arg) = &ret_expr.expr() {
Expand All @@ -198,15 +214,15 @@ pub(crate) fn apply_demorgan_iterator(acc: &mut Assists, ctx: &AssistContext<'_>
.and_then(ast::PrefixExpr::cast)
.filter(|prefix_expr| matches!(prefix_expr.op_kind(), Some(ast::UnaryOp::Not)))
{
edit.delete(
prefix_expr
.op_token()
.expect("prefix expression always has an operator")
.text_range(),
editor.delete(
prefix_expr.op_token().expect("prefix expression always has an operator"),
);
} else {
edit.insert(method_call.syntax().text_range().start(), "!");
editor.insert(Position::before(method_call.syntax()), make.token(SyntaxKind::BANG));
}

editor.add_mappings(make.finish_with_mappings());
builder.add_file_edits(ctx.file_id(), editor);
},
)
}
Expand All @@ -233,26 +249,50 @@ fn validate_method_call_expr(
it_type.impls_trait(sema.db, iter_trait, &[]).then_some((name_ref, arg_expr))
}

fn tail_cb_impl(edit: &mut SourceChangeBuilder, e: &ast::Expr) {
fn tail_cb_impl(editor: &mut SyntaxEditor, make: &SyntaxFactory, e: &ast::Expr) {
match e {
ast::Expr::BreakExpr(break_expr) => {
if let Some(break_expr_arg) = break_expr.expr() {
for_each_tail_expr(&break_expr_arg, &mut |e| tail_cb_impl(edit, e))
for_each_tail_expr(&break_expr_arg, &mut |e| tail_cb_impl(editor, make, e))
}
}
ast::Expr::ReturnExpr(_) => {
// all return expressions have already been handled by the walk loop
}
e => {
let inverted_body = invert_boolean_expression(e.clone());
edit.replace(e.syntax().text_range(), inverted_body.syntax().text());
let inverted_body = invert_boolean_expression(make, e.clone());
editor.replace(e.syntax(), inverted_body.syntax());
}
}
}

/// Add bang and parentheses to the expression.
fn add_bang_paren(expr: ast::Expr) -> ast::Expr {
make::expr_prefix(T![!], make::expr_paren(expr)).into()
fn add_bang_paren(make: &SyntaxFactory, expr: ast::Expr) -> ast::Expr {
make.expr_prefix(T![!], make.expr_paren(expr).into()).into()
}

fn needs_parens_in_place_of(
this: &ast::Expr,
parent: &SyntaxNode,
in_place_of: &ast::Expr,
) -> bool {
assert_eq!(Some(parent), in_place_of.syntax().parent().as_ref());

let child_idx = parent
.children()
.enumerate()
.find_map(|(i, it)| if &it == in_place_of.syntax() { Some(i) } else { None })
.unwrap();
let parent = parent.clone_subtree();
let subtree_place = parent.children().nth(child_idx).unwrap();

let mut editor = SyntaxEditor::new(parent);
editor.replace(subtree_place, this.syntax());
let edit = editor.finish();

let replaced = edit.new_root().children().nth(child_idx).unwrap();
let replaced = ast::Expr::cast(replaced).unwrap();
replaced.needs_parens_in(edit.new_root().clone())
}

#[cfg(test)]
Expand Down
4 changes: 2 additions & 2 deletions crates/ide-assists/src/handlers/convert_bool_then.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use syntax::{
};

use crate::{
utils::{invert_boolean_expression, unwrap_trivial_block},
utils::{invert_boolean_expression_legacy, unwrap_trivial_block},
AssistContext, AssistId, AssistKind, Assists,
};

Expand Down Expand Up @@ -119,7 +119,7 @@ pub(crate) fn convert_if_to_bool_then(acc: &mut Assists, ctx: &AssistContext<'_>
| ast::Expr::WhileExpr(_)
| ast::Expr::YieldExpr(_)
);
let cond = if invert_cond { invert_boolean_expression(cond) } else { cond };
let cond = if invert_cond { invert_boolean_expression_legacy(cond) } else { cond };
let cond = if parenthesize { make::expr_paren(cond) } else { cond };
let arg_list = make::arg_list(Some(make::expr_closure(None, closure_body)));
let mcall = make::expr_method_call(cond, make::name_ref("then"), arg_list);
Expand Down
4 changes: 2 additions & 2 deletions crates/ide-assists/src/handlers/convert_to_guarded_return.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use syntax::{

use crate::{
assist_context::{AssistContext, Assists},
utils::invert_boolean_expression,
utils::invert_boolean_expression_legacy,
AssistId, AssistKind,
};

Expand Down Expand Up @@ -139,7 +139,7 @@ fn if_expr_to_guarded_return(
let new_expr = {
let then_branch =
make::block_expr(once(make::expr_stmt(early_expression).into()), None);
let cond = invert_boolean_expression(cond_expr);
let cond = invert_boolean_expression_legacy(cond_expr);
make::expr_if(cond, then_branch, None).indent(if_indent_level)
};
new_expr.syntax().clone_for_update()
Expand Down
Loading