diff --git a/crates/ide_assists/src/handlers/extract_function.rs b/crates/ide_assists/src/handlers/extract_function.rs index 350e204a16..493cac6e21 100644 --- a/crates/ide_assists/src/handlers/extract_function.rs +++ b/crates/ide_assists/src/handlers/extract_function.rs @@ -74,20 +74,12 @@ pub(crate) fn extract_function(acc: &mut Assists, ctx: &AssistContext) -> Option let (locals_used, has_await, self_param) = analyze_body(&ctx.sema, &body); let anchor = if self_param.is_some() { Anchor::Method } else { Anchor::Freestanding }; - let insert_after = scope_for_fn_insertion(&body, anchor)?; + let insert_after = node_to_insert_after(&body, anchor)?; let module = ctx.sema.scope(&insert_after).module()?; - let vars_defined_in_body_and_outlive = - vars_defined_in_body_and_outlive(ctx, &body, node.parent().as_ref().unwrap_or(&node)); let ret_ty = body_return_ty(ctx, &body)?; + let ret_values = ret_values(ctx, &body, node.parent().as_ref().unwrap_or(&node)); - // FIXME: we compute variables that outlive here just to check `never!` condition - // this requires traversing whole `body` (cheap) and finding all references (expensive) - // maybe we can move this check to `edit` closure somehow? - if stdx::never!(!vars_defined_in_body_and_outlive.is_empty() && !ret_ty.is_unit()) { - // We should not have variables that outlive body if we have expression block - return None; - } let control_flow = external_control_flow(ctx, &body)?; let target_range = body.text_range(); @@ -97,16 +89,22 @@ pub(crate) fn extract_function(acc: &mut Assists, ctx: &AssistContext) -> Option "Extract into function", target_range, move |builder| { + let ret_values: Vec<_> = ret_values.collect(); + if stdx::never!(!ret_values.is_empty() && !ret_ty.is_unit()) { + // We should not have variables that outlive body if we have expression block + return; + } + let params = extracted_function_params(ctx, &body, locals_used.iter().copied()); let fun = Function { name: "fun_name".to_string(), - self_param: self_param.map(|(_, pat)| pat), + self_param, params, control_flow, ret_ty, body, - vars_defined_in_body_and_outlive, + vars_defined_in_body_and_outlive: ret_values, }; let new_indent = IndentLevel::from_node(&insert_after); @@ -124,68 +122,46 @@ pub(crate) fn extract_function(acc: &mut Assists, ctx: &AssistContext) -> Option ) } +/// Analyses the function body for external control flow. fn external_control_flow(ctx: &AssistContext, body: &FunctionBody) -> Option { let mut ret_expr = None; let mut try_expr = None; let mut break_expr = None; let mut continue_expr = None; - let (syntax, text_range) = match body { - FunctionBody::Expr(expr) => (expr.syntax(), expr.syntax().text_range()), - FunctionBody::Span { parent, text_range } => (parent.syntax(), *text_range), - }; - let mut nested_loop = None; - let mut nested_scope = None; + let mut loop_depth = 0; - for e in syntax.preorder() { - let e = match e { + body.preorder_expr(&mut |expr| { + let expr = match expr { WalkEvent::Enter(e) => e, - WalkEvent::Leave(e) => { - if nested_loop.as_ref() == Some(&e) { - nested_loop = None; - } - if nested_scope.as_ref() == Some(&e) { - nested_scope = None; - } - continue; + WalkEvent::Leave( + ast::Expr::LoopExpr(_) | ast::Expr::ForExpr(_) | ast::Expr::WhileExpr(_), + ) => { + loop_depth -= 1; + return false; } + WalkEvent::Leave(_) => return false, }; - if nested_scope.is_some() { - continue; - } - if !text_range.contains_range(e.text_range()) { - continue; - } - match e.kind() { - SyntaxKind::LOOP_EXPR | SyntaxKind::WHILE_EXPR | SyntaxKind::FOR_EXPR => { - if nested_loop.is_none() { - nested_loop = Some(e); - } + match expr { + ast::Expr::LoopExpr(_) | ast::Expr::ForExpr(_) | ast::Expr::WhileExpr(_) => { + loop_depth += 1; } - SyntaxKind::FN - | SyntaxKind::CONST - | SyntaxKind::STATIC - | SyntaxKind::IMPL - | SyntaxKind::MODULE => { - if nested_scope.is_none() { - nested_scope = Some(e); - } + ast::Expr::ReturnExpr(it) => { + ret_expr = Some(it); } - SyntaxKind::RETURN_EXPR => { - ret_expr = Some(ast::ReturnExpr::cast(e).unwrap()); + ast::Expr::TryExpr(it) => { + try_expr = Some(it); } - SyntaxKind::TRY_EXPR => { - try_expr = Some(ast::TryExpr::cast(e).unwrap()); + ast::Expr::BreakExpr(it) if loop_depth == 0 => { + break_expr = Some(it); } - SyntaxKind::BREAK_EXPR if nested_loop.is_none() => { - break_expr = Some(ast::BreakExpr::cast(e).unwrap()); - } - SyntaxKind::CONTINUE_EXPR if nested_loop.is_none() => { - continue_expr = Some(ast::ContinueExpr::cast(e).unwrap()); + ast::Expr::ContinueExpr(it) if loop_depth == 0 => { + continue_expr = Some(it); } _ => {} } - } + false + }); let kind = match (try_expr, ret_expr, break_expr, continue_expr) { (Some(e), None, None, None) => { @@ -272,11 +248,6 @@ struct Param { is_copy: bool, } -#[derive(Debug)] -struct ControlFlow { - kind: Option, -} - #[derive(Debug, Clone, Copy, PartialEq, Eq)] enum ParamKind { Value, @@ -355,6 +326,11 @@ impl Param { } } +#[derive(Debug)] +struct ControlFlow { + kind: Option, +} + /// Control flow that is exported from extracted function /// /// E.g.: @@ -456,6 +432,7 @@ impl RetType { } /// Semantically same as `ast::Expr`, but preserves identity when using only part of the Block +/// This is the future function body, the part that is being extracted. #[derive(Debug)] enum FunctionBody { Expr(ast::Expr), @@ -488,11 +465,7 @@ impl FunctionBody { FunctionBody::Expr(expr) => Some(expr.clone()), FunctionBody::Span { parent, text_range } => { let tail_expr = parent.tail_expr()?; - if text_range.contains_range(tail_expr.syntax().text_range()) { - Some(tail_expr) - } else { - None - } + text_range.contains_range(tail_expr.syntax().text_range()).then(|| tail_expr) } } } @@ -520,6 +493,29 @@ impl FunctionBody { } } + fn preorder_expr(&self, cb: &mut dyn FnMut(WalkEvent) -> bool) { + match self { + FunctionBody::Expr(expr) => expr.preorder(cb), + FunctionBody::Span { parent, text_range } => { + parent + .statements() + .filter(|stmt| text_range.contains_range(stmt.syntax().text_range())) + .filter_map(|stmt| match stmt { + ast::Stmt::ExprStmt(expr_stmt) => expr_stmt.expr(), + ast::Stmt::Item(_) => None, + ast::Stmt::LetStmt(stmt) => stmt.initializer(), + }) + .for_each(|expr| expr.preorder(cb)); + if let Some(expr) = parent + .tail_expr() + .filter(|it| text_range.contains_range(it.syntax().text_range())) + { + expr.preorder(cb); + } + } + } + } + fn walk_pat(&self, cb: &mut dyn FnMut(ast::Pat)) { match self { FunctionBody::Expr(expr) => expr.walk_patterns(cb), @@ -556,7 +552,7 @@ impl FunctionBody { fn text_range(&self) -> TextRange { match self { FunctionBody::Expr(expr) => expr.syntax().text_range(), - FunctionBody::Span { parent: _, text_range } => *text_range, + &FunctionBody::Span { text_range, .. } => text_range, } } @@ -564,7 +560,7 @@ impl FunctionBody { self.text_range().contains_range(range) } - fn preceedes_range(&self, range: TextRange) -> bool { + fn precedes_range(&self, range: TextRange) -> bool { self.text_range().end() <= range.start() } @@ -668,7 +664,7 @@ fn extraction_target(node: &SyntaxNode, selection_range: TextRange) -> Option, body: &FunctionBody, -) -> (FxIndexSet, bool, Option<(Local, ast::SelfParam)>) { +) -> (FxIndexSet, bool, Option) { // FIXME: currently usages inside macros are not found let mut has_await = false; let mut self_param = None; @@ -692,7 +688,7 @@ fn analyze_body( match local_ref.source(sema.db).value { Either::Right(it) => { stdx::always!( - self_param.replace((local_ref, it)).is_none(), + self_param.replace(it).is_none(), "body references two different self params" ); } @@ -743,7 +739,7 @@ fn extracted_function_params( } fn has_usages_after_body(usages: &LocalUsages, body: &FunctionBody) -> bool { - usages.iter().any(|reference| body.preceedes_range(reference.range)) + usages.iter().any(|reference| body.precedes_range(reference.range)) } /// checks if relevant var is used with `&mut` access inside body @@ -828,7 +824,7 @@ impl LocalUsages { } fn iter(&self) -> impl Iterator + '_ { - self.0.iter().flat_map(|(_, rs)| rs.iter()) + self.0.iter().flat_map(|(_, rs)| rs) } } @@ -868,13 +864,16 @@ fn path_element_of_reference( } /// list local variables defined inside `body` -fn locals_defined_in_body(body: &FunctionBody, ctx: &AssistContext) -> FxIndexSet { +fn locals_defined_in_body( + sema: &Semantics, + body: &FunctionBody, +) -> FxIndexSet { // FIXME: this doesn't work well with macros // see https://github.com/rust-analyzer/rust-analyzer/pull/7535#discussion_r570048550 let mut res = FxIndexSet::default(); body.walk_pat(&mut |pat| { if let ast::Pat::IdentPat(pat) = pat { - if let Some(local) = ctx.sema.to_def(&pat) { + if let Some(local) = sema.to_def(&pat) { res.insert(local); } } @@ -882,17 +881,42 @@ fn locals_defined_in_body(body: &FunctionBody, ctx: &AssistContext) -> FxIndexSe res } -/// list local variables defined inside `body` that should be returned from extracted function -fn vars_defined_in_body_and_outlive( - ctx: &AssistContext, +/// Local variables defined inside `body` that are accessed outside of it +fn ret_values<'a>( + ctx: &'a AssistContext, body: &FunctionBody, parent: &SyntaxNode, -) -> Vec { - let vars_defined_in_body = locals_defined_in_body(body, ctx); - vars_defined_in_body +) -> impl Iterator + 'a { + let parent = parent.clone(); + let range = body.text_range(); + locals_defined_in_body(&ctx.sema, body) .into_iter() - .filter_map(|var| var_outlives_body(ctx, body, var, parent)) - .collect() + .filter_map(move |local| local_outlives_body(ctx, range, local, &parent)) +} + +/// Returns usage details if local variable is used after(outside of) body +fn local_outlives_body( + ctx: &AssistContext, + body_range: TextRange, + local: Local, + parent: &SyntaxNode, +) -> Option { + let usages = LocalUsages::find(ctx, local); + let mut has_mut_usages = false; + let mut any_outlives = false; + for usage in usages.iter() { + if body_range.end() <= usage.range.start() { + has_mut_usages |= reference_is_exclusive(usage, parent, ctx); + any_outlives |= true; + if has_mut_usages { + break; // no need to check more elements we have all the info we wanted + } + } + } + if !any_outlives { + return None; + } + Some(OutlivedLocal { local, mut_usage_outside_body: has_mut_usages }) } /// checks if the relevant local was defined before(outside of) body @@ -912,25 +936,6 @@ fn either_syntax(value: &Either) -> &SyntaxNode { } } -/// returns usage details if local variable is used after(outside of) body -fn var_outlives_body( - ctx: &AssistContext, - body: &FunctionBody, - var: Local, - parent: &SyntaxNode, -) -> Option { - let usages = LocalUsages::find(ctx, var); - let has_usages = usages.iter().any(|reference| body.preceedes_range(reference.range)); - if !has_usages { - return None; - } - let has_mut_usages = usages - .iter() - .filter(|reference| body.preceedes_range(reference.range)) - .any(|reference| reference_is_exclusive(reference, parent, ctx)); - Some(OutlivedLocal { local: var, mut_usage_outside_body: has_mut_usages }) -} - fn body_return_ty(ctx: &AssistContext, body: &FunctionBody) -> Option { match body.tail_expr() { Some(expr) => { @@ -940,6 +945,7 @@ fn body_return_ty(ctx: &AssistContext, body: &FunctionBody) -> Option { None => Some(RetType::Stmt), } } + /// Where to put extracted function definition #[derive(Debug)] enum Anchor { @@ -952,36 +958,31 @@ enum Anchor { /// find where to put extracted function definition /// /// Function should be put right after returned node -fn scope_for_fn_insertion(body: &FunctionBody, anchor: Anchor) -> Option { - match body { - FunctionBody::Expr(e) => scope_for_fn_insertion_node(e.syntax(), anchor), - FunctionBody::Span { parent, .. } => scope_for_fn_insertion_node(parent.syntax(), anchor), - } -} - -fn scope_for_fn_insertion_node(node: &SyntaxNode, anchor: Anchor) -> Option { +fn node_to_insert_after(body: &FunctionBody, anchor: Anchor) -> Option { + let node = match body { + FunctionBody::Expr(e) => e.syntax(), + FunctionBody::Span { parent, .. } => parent.syntax(), + }; let mut ancestors = node.ancestors().peekable(); let mut last_ancestor = None; while let Some(next_ancestor) = ancestors.next() { match next_ancestor.kind() { SyntaxKind::SOURCE_FILE => break, + SyntaxKind::ITEM_LIST if !matches!(anchor, Anchor::Freestanding) => continue, SyntaxKind::ITEM_LIST => { - if !matches!(anchor, Anchor::Freestanding) { - continue; - } if ancestors.peek().map(SyntaxNode::kind) == Some(SyntaxKind::MODULE) { break; } } + SyntaxKind::ASSOC_ITEM_LIST if !matches!(anchor, Anchor::Method) => { + continue; + } SyntaxKind::ASSOC_ITEM_LIST => { - if !matches!(anchor, Anchor::Method) { - continue; - } if ancestors.peek().map(SyntaxNode::kind) == Some(SyntaxKind::IMPL) { break; } } - _ => {} + _ => (), } last_ancestor = Some(next_ancestor); } diff --git a/crates/syntax/src/ast/expr_ext.rs b/crates/syntax/src/ast/expr_ext.rs index 6317d84ba3..5307516ecb 100644 --- a/crates/syntax/src/ast/expr_ext.rs +++ b/crates/syntax/src/ast/expr_ext.rs @@ -1,5 +1,7 @@ //! Various extension methods to ast Expr Nodes, which are hard to code-generate. +use rowan::WalkEvent; + use crate::{ ast::{self, support, AstChildren, AstNode}, AstToken, @@ -34,6 +36,122 @@ impl ast::Expr { } None } + + /// Preorder walk all the expression's child expressions. + pub fn walk(&self, cb: &mut dyn FnMut(ast::Expr)) { + self.preorder(&mut |ev| { + if let WalkEvent::Enter(expr) = ev { + cb(expr); + } + false + }) + } + + /// Preorder walk all the expression's child expressions preserving events. + /// If the callback returns true the subtree of the expression will be skipped. + /// Note that the subtree may already be skipped due to the context analysis this function does. + pub fn preorder(&self, cb: &mut dyn FnMut(WalkEvent) -> bool) { + let mut preorder = self.syntax().preorder(); + while let Some(event) = preorder.next() { + let node = match event { + WalkEvent::Enter(node) => node, + WalkEvent::Leave(node) => { + if let Some(expr) = ast::Expr::cast(node) { + cb(WalkEvent::Leave(expr)); + } + continue; + } + }; + match ast::Stmt::cast(node.clone()) { + // recursively walk the initializer, skipping potential const pat expressions + // let statements aren't usually nested too deeply so this is fine to recurse on + Some(ast::Stmt::LetStmt(l)) => { + if let Some(expr) = l.initializer() { + expr.preorder(cb); + } + preorder.skip_subtree(); + } + // Don't skip subtree since we want to process the expression child next + Some(ast::Stmt::ExprStmt(_)) => (), + // skip inner items which might have their own expressions + Some(ast::Stmt::Item(_)) => preorder.skip_subtree(), + None => { + // skip const args, those expressions are a different context + if ast::GenericArg::can_cast(node.kind()) { + preorder.skip_subtree(); + } else if let Some(expr) = ast::Expr::cast(node) { + let is_different_context = match &expr { + ast::Expr::EffectExpr(effect) => { + matches!( + effect.effect(), + ast::Effect::Async(_) + | ast::Effect::Try(_) + | ast::Effect::Const(_) + ) + } + ast::Expr::ClosureExpr(_) => true, + _ => false, + }; + let skip = cb(WalkEvent::Enter(expr)); + if skip || is_different_context { + preorder.skip_subtree(); + } + } + } + } + } + } + + /// Preorder walk all the expression's child patterns. + pub fn walk_patterns(&self, cb: &mut dyn FnMut(ast::Pat)) { + let mut preorder = self.syntax().preorder(); + while let Some(event) = preorder.next() { + let node = match event { + WalkEvent::Enter(node) => node, + WalkEvent::Leave(_) => continue, + }; + match ast::Stmt::cast(node.clone()) { + Some(ast::Stmt::LetStmt(l)) => { + if let Some(pat) = l.pat() { + pat.walk(cb); + } + if let Some(expr) = l.initializer() { + expr.walk_patterns(cb); + } + preorder.skip_subtree(); + } + // Don't skip subtree since we want to process the expression child next + Some(ast::Stmt::ExprStmt(_)) => (), + // skip inner items which might have their own patterns + Some(ast::Stmt::Item(_)) => preorder.skip_subtree(), + None => { + // skip const args, those are a different context + if ast::GenericArg::can_cast(node.kind()) { + preorder.skip_subtree(); + } else if let Some(expr) = ast::Expr::cast(node.clone()) { + let is_different_context = match &expr { + ast::Expr::EffectExpr(effect) => { + matches!( + effect.effect(), + ast::Effect::Async(_) + | ast::Effect::Try(_) + | ast::Effect::Const(_) + ) + } + ast::Expr::ClosureExpr(_) => true, + _ => false, + }; + if is_different_context { + preorder.skip_subtree(); + } + } else if let Some(pat) = ast::Pat::cast(node) { + preorder.skip_subtree(); + pat.walk(cb); + } + } + } + } + } } #[derive(Debug, Clone, PartialEq, Eq)] diff --git a/crates/syntax/src/ast/node_ext.rs b/crates/syntax/src/ast/node_ext.rs index e9465536c8..ea156087ff 100644 --- a/crates/syntax/src/ast/node_ext.rs +++ b/crates/syntax/src/ast/node_ext.rs @@ -55,107 +55,6 @@ impl ast::BlockExpr { } } -impl ast::Expr { - /// Preorder walk all the expression's child expressions. - pub fn walk(&self, cb: &mut dyn FnMut(ast::Expr)) { - let mut preorder = self.syntax().preorder(); - while let Some(event) = preorder.next() { - let node = match event { - WalkEvent::Enter(node) => node, - WalkEvent::Leave(_) => continue, - }; - match ast::Stmt::cast(node.clone()) { - // recursively walk the initializer, skipping potential const pat expressions - // let statements aren't usually nested too deeply so this is fine to recurse on - Some(ast::Stmt::LetStmt(l)) => { - if let Some(expr) = l.initializer() { - expr.walk(cb); - } - preorder.skip_subtree(); - } - // Don't skip subtree since we want to process the expression child next - Some(ast::Stmt::ExprStmt(_)) => (), - // skip inner items which might have their own expressions - Some(ast::Stmt::Item(_)) => preorder.skip_subtree(), - None => { - // skip const args, those expressions are a different context - if ast::GenericArg::can_cast(node.kind()) { - preorder.skip_subtree(); - } else if let Some(expr) = ast::Expr::cast(node) { - let is_different_context = match &expr { - ast::Expr::EffectExpr(effect) => { - matches!( - effect.effect(), - ast::Effect::Async(_) - | ast::Effect::Try(_) - | ast::Effect::Const(_) - ) - } - ast::Expr::ClosureExpr(_) => true, - _ => false, - }; - cb(expr); - if is_different_context { - preorder.skip_subtree(); - } - } - } - } - } - } - - /// Preorder walk all the expression's child patterns. - pub fn walk_patterns(&self, cb: &mut dyn FnMut(ast::Pat)) { - let mut preorder = self.syntax().preorder(); - while let Some(event) = preorder.next() { - let node = match event { - WalkEvent::Enter(node) => node, - WalkEvent::Leave(_) => continue, - }; - match ast::Stmt::cast(node.clone()) { - Some(ast::Stmt::LetStmt(l)) => { - if let Some(pat) = l.pat() { - pat.walk(cb); - } - if let Some(expr) = l.initializer() { - expr.walk_patterns(cb); - } - preorder.skip_subtree(); - } - // Don't skip subtree since we want to process the expression child next - Some(ast::Stmt::ExprStmt(_)) => (), - // skip inner items which might have their own patterns - Some(ast::Stmt::Item(_)) => preorder.skip_subtree(), - None => { - // skip const args, those are a different context - if ast::GenericArg::can_cast(node.kind()) { - preorder.skip_subtree(); - } else if let Some(expr) = ast::Expr::cast(node.clone()) { - let is_different_context = match &expr { - ast::Expr::EffectExpr(effect) => { - matches!( - effect.effect(), - ast::Effect::Async(_) - | ast::Effect::Try(_) - | ast::Effect::Const(_) - ) - } - ast::Expr::ClosureExpr(_) => true, - _ => false, - }; - if is_different_context { - preorder.skip_subtree(); - } - } else if let Some(pat) = ast::Pat::cast(node) { - preorder.skip_subtree(); - pat.walk(cb); - } - } - } - } - } -} - impl ast::Pat { /// Preorder walk all the pattern's sub patterns. pub fn walk(&self, cb: &mut dyn FnMut(ast::Pat)) {