Simplify extract_function assist

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<ControlFlow> {
     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 loop_depth = 0;
-    let mut nested_scope = None;
 
-    for e in syntax.preorder() {
+    body.preorder_expr(&mut |expr| {
-        let e = match e {
+        let expr = match expr {
             WalkEvent::Enter(e) => e,
-            WalkEvent::Leave(e) => {
+            WalkEvent::Leave(
-                if nested_loop.as_ref() == Some(&e) {
+                ast::Expr::LoopExpr(_) | ast::Expr::ForExpr(_) | ast::Expr::WhileExpr(_),
-                    nested_loop = None;
+            ) => {
-                }
+                loop_depth -= 1;
-                if nested_scope.as_ref() == Some(&e) {
+                return false;
-                    nested_scope = None;
-                }
-                continue;
             }
+            WalkEvent::Leave(_) => return false,
         };
-        if nested_scope.is_some() {
+        match expr {
-            continue;
+            ast::Expr::LoopExpr(_) | ast::Expr::ForExpr(_) | ast::Expr::WhileExpr(_) => {
+                loop_depth += 1;
             }
-        if !text_range.contains_range(e.text_range()) {
+            ast::Expr::ReturnExpr(it) => {
-            continue;
+                ret_expr = Some(it);
             }
-        match e.kind() {
+            ast::Expr::TryExpr(it) => {
-            SyntaxKind::LOOP_EXPR | SyntaxKind::WHILE_EXPR | SyntaxKind::FOR_EXPR => {
+                try_expr = Some(it);
-                if nested_loop.is_none() {
-                    nested_loop = Some(e);
             }
+            ast::Expr::BreakExpr(it) if loop_depth == 0 => {
+                break_expr = Some(it);
             }
-            SyntaxKind::FN
+            ast::Expr::ContinueExpr(it) if loop_depth == 0 => {
-            | SyntaxKind::CONST
+                continue_expr = Some(it);
-            | SyntaxKind::STATIC
-            | SyntaxKind::IMPL
-            | SyntaxKind::MODULE => {
-                if nested_scope.is_none() {
-                    nested_scope = Some(e);
-                }
-            }
-            SyntaxKind::RETURN_EXPR => {
-                ret_expr = Some(ast::ReturnExpr::cast(e).unwrap());
-            }
-            SyntaxKind::TRY_EXPR => {
-                try_expr = Some(ast::TryExpr::cast(e).unwrap());
-            }
-            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());
             }
             _ => {}
         }
-    }
+        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<FlowKind>,
-}
-
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 enum ParamKind {
     Value,
@@ -355,6 +326,11 @@ impl Param {
     }
 }
 
+#[derive(Debug)]
+struct ControlFlow {
+    kind: Option<FlowKind>,
+}
+
 /// 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()) {
+                text_range.contains_range(tail_expr.syntax().text_range()).then(|| tail_expr)
-                    Some(tail_expr)
-                } else {
-                    None
-                }
             }
         }
     }
@@ -520,6 +493,29 @@ impl FunctionBody {
         }
     }
 
+    fn preorder_expr(&self, cb: &mut dyn FnMut(WalkEvent<ast::Expr>) -> 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<Fu
 fn analyze_body(
     sema: &Semantics<RootDatabase>,
     body: &FunctionBody,
-) -> (FxIndexSet<Local>, bool, Option<(Local, ast::SelfParam)>) {
+) -> (FxIndexSet<Local>, bool, Option<ast::SelfParam>) {
     // 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<Item = &FileReference> + '_ {
-        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<Local> {
+fn locals_defined_in_body(
+    sema: &Semantics<RootDatabase>,
+    body: &FunctionBody,
+) -> FxIndexSet<Local> {
     // 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
+/// Local variables defined inside `body` that are accessed outside of it
-fn vars_defined_in_body_and_outlive(
+fn ret_values<'a>(
-    ctx: &AssistContext,
+    ctx: &'a AssistContext,
     body: &FunctionBody,
     parent: &SyntaxNode,
-) -> Vec<OutlivedLocal> {
+) -> impl Iterator<Item = OutlivedLocal> + 'a {
-    let vars_defined_in_body = locals_defined_in_body(body, ctx);
+    let parent = parent.clone();
-    vars_defined_in_body
+    let range = body.text_range();
+    locals_defined_in_body(&ctx.sema, body)
         .into_iter()
-        .filter_map(|var| var_outlives_body(ctx, body, var, parent))
+        .filter_map(move |local| local_outlives_body(ctx, range, local, &parent))
-        .collect()
+}
+
+/// 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<OutlivedLocal> {
+    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<ast::IdentPat, ast::SelfParam>) -> &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<OutlivedLocal> {
-    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<RetType> {
     match body.tail_expr() {
         Some(expr) => {
@@ -940,6 +945,7 @@ fn body_return_ty(ctx: &AssistContext, body: &FunctionBody) -> Option<RetType> {
         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<SyntaxNode> {
+fn node_to_insert_after(body: &FunctionBody, anchor: Anchor) -> Option<SyntaxNode> {
-    match body {
+    let node = match body {
-        FunctionBody::Expr(e) => scope_for_fn_insertion_node(e.syntax(), anchor),
+        FunctionBody::Expr(e) => e.syntax(),
-        FunctionBody::Span { parent, .. } => scope_for_fn_insertion_node(parent.syntax(), anchor),
+        FunctionBody::Span { parent, .. } => parent.syntax(),
-    }
+    };
-}
-
-fn scope_for_fn_insertion_node(node: &SyntaxNode, anchor: Anchor) -> Option<SyntaxNode> {
     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 => {
+            SyntaxKind::ASSOC_ITEM_LIST if !matches!(anchor, Anchor::Method) => {
-                if !matches!(anchor, Anchor::Method) {
                 continue;
             }
+            SyntaxKind::ASSOC_ITEM_LIST => {
                 if ancestors.peek().map(SyntaxNode::kind) == Some(SyntaxKind::IMPL) {
                     break;
                 }
             }
-            _ => {}
+            _ => (),
         }
         last_ancestor = Some(next_ancestor);
     }

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<ast::Expr>) -> 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)]

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)) {