feat: semantics implementation of locals used for extract function

Author: dfireBird

crates/hir-expand/src/name.rs

@@ -197,6 +197,10 @@ impl Name {
     pub fn symbol(&self) -> &Symbol {
         &self.symbol
     }
+
+    pub fn is_generated(&self) -> bool {
+        self.as_str().starts_with("<ra@gennew>")
+    }
 }
 
 struct Display<'a> {

crates/hir/src/lib.rs

@@ -3908,6 +3908,31 @@ impl Local {
                 .unwrap(),
         }
     }
+
+    /// The leftmost definition for this local if present.
+    pub fn try_primary_source(self, db: &dyn HirDatabase) -> Option<LocalSource> {
+        let (body, source_map) = db.body_with_source_map(self.parent);
+        match body.self_param.zip(source_map.self_param_syntax()) {
+            Some((param, source)) if param == self.binding_id => {
+                let root = source.file_syntax(db);
+                Some(LocalSource {
+                    local: self,
+                    source: source.map(|ast| Either::Right(ast.to_node(&root))),
+                })
+            }
+            _ => source_map.patterns_for_binding(self.binding_id).first().and_then(|&definition| {
+                let src = source_map.pat_syntax(definition).ok()?;
+                let root = src.file_syntax(db);
+                Some(LocalSource {
+                    local: self,
+                    source: src.map(|ast| match ast.to_node(&root) {
+                        Either::Right(ast::Pat::IdentPat(it)) => Either::Left(it),
+                        _ => unreachable!("local with non ident-pattern"),
+                    }),
+                })
+            }),
+        }
+    }
 }
 
 impl PartialOrd for Local {

crates/hir/src/semantics.rs

@@ -16,7 +16,7 @@ use hir_def::{
     expr_store::{Body, ExprOrPatSource, HygieneId, path::Path},
     hir::{BindingId, Expr, ExprId, ExprOrPatId, Pat},
     nameres::{ModuleOrigin, crate_def_map},
-    resolver::{self, HasResolver, Resolver, TypeNs},
+    resolver::{self, HasResolver, Resolver, TypeNs, ValueNs},
     type_ref::Mutability,
 };
 use hir_expand::{
@@ -2192,6 +2192,88 @@ impl<'db> SemanticsImpl<'db> {
         self.cache(adt_source.value.syntax().ancestors().last().unwrap(), adt_source.file_id);
         ToDef::to_def(self, adt_source.as_ref())
     }
+
+    pub fn locals_used(
+        &self,
+        element: Either<&ast::Expr, &ast::StmtList>,
+        text_range: TextRange,
+    ) -> Option<Vec<Local>> {
+        let sa = self.analyze(element.either(|e| e.syntax(), |s| s.syntax()))?;
+        let store = sa.store()?;
+        let mut resolver = sa.resolver.clone();
+        let def = resolver.body_owner()?;
+
+        let is_not_generated = |path: &Path| {
+            !path.mod_path().and_then(|path| path.as_ident()).is_some_and(Name::is_generated)
+        };
+
+        let exprs = element.either(
+            |e| vec![e.clone()],
+            |stmts| {
+                let mut exprs: Vec<_> = stmts
+                    .statements()
+                    .filter(|stmt| text_range.contains_range(stmt.syntax().text_range()))
+                    .filter_map(|stmt| match stmt {
+                        ast::Stmt::ExprStmt(expr_stmt) => expr_stmt.expr().map(|e| vec![e]),
+                        ast::Stmt::Item(_) => None,
+                        ast::Stmt::LetStmt(let_stmt) => {
+                            let init = let_stmt.initializer();
+                            let let_else = let_stmt
+                                .let_else()
+                                .and_then(|le| le.block_expr())
+                                .map(ast::Expr::BlockExpr);
+
+                            match (init, let_else) {
+                                (Some(i), Some(le)) => Some(vec![i, le]),
+                                (Some(i), _) => Some(vec![i]),
+                                (_, Some(le)) => Some(vec![le]),
+                                _ => None,
+                            }
+                        }
+                    })
+                    .flatten()
+                    .collect();
+
+                if let Some(tail_expr) = stmts.tail_expr()
+                    && text_range.contains_range(tail_expr.syntax().text_range())
+                {
+                    exprs.push(tail_expr);
+                }
+                exprs
+            },
+        );
+        let mut exprs: Vec<_> =
+            exprs.into_iter().filter_map(|e| sa.expr_id(e).and_then(|e| e.as_expr())).collect();
+
+        let mut locals: Vec<Local> = Vec::new();
+        let mut add_to_locals_used = |expr_id| {
+            if let Expr::Path(path) = &store[expr_id]
+                && is_not_generated(path)
+            {
+                let _ = resolver.update_to_inner_scope(self.db, def, expr_id);
+                resolver
+                    .resolve_path_in_value_ns_fully(self.db, path, store.expr_path_hygiene(expr_id))
+                    .inspect(|value| {
+                        if let ValueNs::LocalBinding(id) = value {
+                            locals.push((def, *id).into());
+                        }
+                    });
+            }
+        };
+
+        while let Some(expr_id) = exprs.pop() {
+            let mut has_child = false;
+            store.walk_child_exprs(expr_id, |id| {
+                has_child = true;
+                exprs.push(id);
+            });
+            if !has_child {
+                add_to_locals_used(expr_id)
+            }
+        }
+
+        Some(locals)
+    }
 }
 
 // FIXME This can't be the best way to do this

crates/hir/src/source_analyzer.rs

@@ -240,7 +240,7 @@ impl<'db> SourceAnalyzer<'db> {
         )
     }
 
-    fn expr_id(&self, expr: ast::Expr) -> Option<ExprOrPatId> {
+    pub(crate) fn expr_id(&self, expr: ast::Expr) -> Option<ExprOrPatId> {
         let src = InFile { file_id: self.file_id, value: expr };
         self.store_sm()?.node_expr(src.as_ref())
     }

crates/ide-assists/src/handlers/extract_function.rs

@@ -9,14 +9,14 @@ use hir::{
 use ide_db::{
     FxIndexSet, RootDatabase,
     assists::GroupLabel,
-    defs::{Definition, NameRefClass},
+    defs::Definition,
     famous_defs::FamousDefs,
     helpers::mod_path_to_ast,
     imports::insert_use::{ImportScope, insert_use},
     search::{FileReference, ReferenceCategory, SearchScope},
     source_change::SourceChangeBuilder,
     syntax_helpers::node_ext::{
-        for_each_tail_expr, preorder_expr, walk_expr, walk_pat, walk_patterns_in_expr,
+        for_each_tail_expr, preorder_expr, walk_pat, walk_patterns_in_expr,
     },
 };
 use itertools::Itertools;
@@ -687,29 +687,6 @@ impl FunctionBody {
         }
     }
 
-    fn walk_expr(&self, cb: &mut dyn FnMut(ast::Expr)) {
-        match self {
-            FunctionBody::Expr(expr) => walk_expr(expr, 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| walk_expr(&expr, cb));
-                if let Some(expr) = parent
-                    .tail_expr()
-                    .filter(|it| text_range.contains_range(it.syntax().text_range()))
-                {
-                    walk_expr(&expr, cb);
-                }
-            }
-        }
-    }
-
     fn preorder_expr(&self, cb: &mut dyn FnMut(WalkEvent<ast::Expr>) -> bool) {
         match self {
             FunctionBody::Expr(expr) => preorder_expr(expr, cb),
@@ -799,23 +776,17 @@ impl FunctionBody {
         let mut self_param = None;
         let mut res = FxIndexSet::default();
 
-        fn local_from_name_ref(
-            sema: &Semantics<'_, RootDatabase>,
-            name_ref: ast::NameRef,
-        ) -> Option<hir::Local> {
-            match NameRefClass::classify(sema, &name_ref) {
-                Some(
-                    NameRefClass::Definition(Definition::Local(local_ref), _)
-                    | NameRefClass::FieldShorthand { local_ref, field_ref: _, adt_subst: _ },
-                ) => Some(local_ref),
-                _ => None,
-            }
-        }
+        let (text_range, element) = match self {
+            FunctionBody::Expr(expr) => (expr.syntax().text_range(), Either::Left(expr)),
+            FunctionBody::Span { parent, text_range, .. } => (*text_range, Either::Right(parent)),
+        };
 
         let mut add_name_if_local = |local_ref: Local| {
-            let InFile { file_id, value } = local_ref.primary_source(sema.db).source;
             // locals defined inside macros are not relevant to us
-            if !file_id.is_macro() {
+            if let Some(local_source) = local_ref.try_primary_source(sema.db)
+                && let InFile { file_id, value } = local_source.source
+                && !file_id.is_macro()
+            {
                 match value {
                     Either::Right(it) => {
                         self_param.replace(it);
@@ -826,59 +797,11 @@ impl FunctionBody {
                 }
             }
         };
-        self.walk_expr(&mut |expr| match expr {
-            ast::Expr::PathExpr(path_expr) => {
-                if let Some(local) = path_expr
-                    .path()
-                    .and_then(|it| it.as_single_name_ref())
-                    .and_then(|name_ref| local_from_name_ref(sema, name_ref))
-                {
-                    add_name_if_local(local);
-                }
-            }
-            ast::Expr::ClosureExpr(closure_expr) => {
-                if let Some(body) = closure_expr.body() {
-                    body.syntax()
-                        .descendants()
-                        .filter_map(ast::NameRef::cast)
-                        .filter_map(|name_ref| local_from_name_ref(sema, name_ref))
-                        .for_each(&mut add_name_if_local);
-                }
-            }
-            ast::Expr::MacroExpr(expr) => {
-                if let Some(tt) = expr.macro_call().and_then(|call| call.token_tree()) {
-                    tt.syntax()
-                        .descendants_with_tokens()
-                        .filter_map(SyntaxElement::into_token)
-                        .filter(|it| {
-                            matches!(it.kind(), SyntaxKind::STRING | SyntaxKind::IDENT | T![self])
-                        })
-                        .for_each(|t| {
-                            if ast::String::can_cast(t.kind()) {
-                                if let Some(parts) =
-                                    ast::String::cast(t).and_then(|s| sema.as_format_args_parts(&s))
-                                {
-                                    parts
-                                        .into_iter()
-                                        .filter_map(|(_, value)| value.and_then(|it| it.left()))
-                                        .filter_map(|path| match path {
-                                            PathResolution::Local(local) => Some(local),
-                                            _ => None,
-                                        })
-                                        .for_each(&mut add_name_if_local);
-                                }
-                            } else {
-                                sema.descend_into_macros_exact(t)
-                                    .into_iter()
-                                    .filter_map(|t| t.parent().and_then(ast::NameRef::cast))
-                                    .filter_map(|name_ref| local_from_name_ref(sema, name_ref))
-                                    .for_each(&mut add_name_if_local);
-                            }
-                        });
-                }
-            }
-            _ => (),
-        });
+
+        if let Some(locals) = sema.locals_used(element, text_range) {
+            locals.into_iter().for_each(&mut add_name_if_local);
+        }
+
         (res, self_param)
     }
 
@@ -6291,6 +6214,90 @@ fn foo() {
 
 fn $0fun_name(v: i32) {
     print!("{v:?}{}", v == 123);
+}"#,
+        );
+    }
+
+    #[test]
+    fn no_parameter_for_variable_used_only_let_else() {
+        check_assist(
+            extract_function,
+            r#"
+fn foo() -> u32 {
+    let x = 5;
+
+    $0let Some(y) = Some(1) else {
+        return x * 2;
+    };$0
+
+    y
+}"#,
+            r#"
+fn foo() -> u32 {
+    let x = 5;
+
+    let y = match fun_name(x) {
+        Ok(value) => value,
+        Err(value) => return value,
+    };
+
+    y
+}
+
+fn $0fun_name(x: u32) -> Result<_, u32> {
+    let Some(y) = Some(1) else {
+        return Err(x * 2);
+    };
+    Ok(y)
+}"#,
+        );
+    }
+
+    #[test]
+    fn deeply_nested_macros() {
+        check_assist(
+            extract_function,
+            r#"
+macro_rules! m {
+    ($val:ident) => { $val };
+}
+
+macro_rules! n {
+    ($v1:ident, $v2:ident) => { m!($v1) + $v2 };
+}
+
+macro_rules! o {
+    ($v1:ident, $v2:ident, $v3:ident) => { n!($v1, $v2) + $v3 };
+}
+
+fn foo() -> u32 {
+    let v1 = 1;
+    let v2 = 2;
+    $0let v3 = 3;
+    o!(v1, v2, v3)$0
+}"#,
+            r#"
+macro_rules! m {
+    ($val:ident) => { $val };
+}
+
+macro_rules! n {
+    ($v1:ident, $v2:ident) => { m!($v1) + $v2 };
+}
+
+macro_rules! o {
+    ($v1:ident, $v2:ident, $v3:ident) => { n!($v1, $v2) + $v3 };
+}
+
+fn foo() -> u32 {
+    let v1 = 1;
+    let v2 = 2;
+    fun_name(v1, v2)
+}
+
+fn $0fun_name(v1: u32, v2: u32) -> u32 {
+    let v3 = 3;
+    o!(v1, v2, v3)
 }"#,
         );
     }