Skip to content

Commit 8434be8

Browse files
dfireBirddfireBird
committed
feat: semantics implementation of locals used for extract function
fix: consider let-else expr for return control type
1 parent ec884b3 commit 8434be8

File tree

4 files changed

+200
-95
lines changed

4 files changed

+200
-95
lines changed

‎crates/hir-expand/src/name.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,10 @@ impl Name {
197197
pub fn symbol(&self) -> &Symbol {
198198
&self.symbol
199199
}
200+
201+
pub fn is_generated(&self) -> bool {
202+
self.as_str().starts_with("<ra@gennew>")
203+
}
200204
}
201205

202206
struct Display<'a> {

‎crates/hir/src/semantics.rs‎

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use hir_def::{
1616
expr_store::{Body, ExprOrPatSource, HygieneId, path::Path},
1717
hir::{BindingId, Expr, ExprId, ExprOrPatId, Pat},
1818
nameres::{ModuleOrigin, crate_def_map},
19-
resolver::{self, HasResolver, Resolver, TypeNs},
19+
resolver::{self, HasResolver, Resolver, TypeNs, ValueNs},
2020
type_ref::Mutability,
2121
};
2222
use hir_expand::{
@@ -2192,6 +2192,88 @@ impl<'db> SemanticsImpl<'db> {
21922192
self.cache(adt_source.value.syntax().ancestors().last().unwrap(), adt_source.file_id);
21932193
ToDef::to_def(self, adt_source.as_ref())
21942194
}
2195+
2196+
pub fn locals_used(
2197+
&self,
2198+
element: Either<&ast::Expr, &ast::StmtList>,
2199+
text_range: TextRange,
2200+
) -> Option<Vec<Local>> {
2201+
let sa = self.analyze(element.either(|e| e.syntax(), |s| s.syntax()))?;
2202+
let store = sa.store()?;
2203+
let mut resolver = sa.resolver.clone();
2204+
let def = resolver.body_owner()?;
2205+
2206+
let is_not_generated = |path: &Path| {
2207+
!path.mod_path().and_then(|path| path.as_ident()).is_some_and(Name::is_generated)
2208+
};
2209+
2210+
let exprs = element.either(
2211+
|e| vec![e.clone()],
2212+
|stmts| {
2213+
let mut exprs: Vec<_> = stmts
2214+
.statements()
2215+
.filter(|stmt| text_range.contains_range(stmt.syntax().text_range()))
2216+
.filter_map(|stmt| match stmt {
2217+
ast::Stmt::ExprStmt(expr_stmt) => expr_stmt.expr().map(|e| vec![e]),
2218+
ast::Stmt::Item(_) => None,
2219+
ast::Stmt::LetStmt(let_stmt) => {
2220+
let init = let_stmt.initializer();
2221+
let let_else = let_stmt
2222+
.let_else()
2223+
.and_then(|le| le.block_expr())
2224+
.map(ast::Expr::BlockExpr);
2225+
2226+
match (init, let_else) {
2227+
(Some(i), Some(le)) => Some(vec![i, le]),
2228+
(Some(i), _) => Some(vec![i]),
2229+
(_, Some(le)) => Some(vec![le]),
2230+
_ => None,
2231+
}
2232+
}
2233+
})
2234+
.flatten()
2235+
.collect();
2236+
2237+
if let Some(tail_expr) = stmts.tail_expr()
2238+
&& text_range.contains_range(tail_expr.syntax().text_range())
2239+
{
2240+
exprs.push(tail_expr);
2241+
}
2242+
exprs
2243+
},
2244+
);
2245+
let mut exprs: Vec<_> =
2246+
exprs.into_iter().filter_map(|e| sa.expr_id(e).and_then(|e| e.as_expr())).collect();
2247+
2248+
let mut locals: Vec<Local> = Vec::new();
2249+
let mut add_to_locals_used = |expr_id| {
2250+
if let Expr::Path(path) = &store[expr_id]
2251+
&& is_not_generated(path)
2252+
{
2253+
let _ = resolver.update_to_inner_scope(self.db, def, expr_id);
2254+
resolver
2255+
.resolve_path_in_value_ns_fully(self.db, path, store.expr_path_hygiene(expr_id))
2256+
.inspect(|value| {
2257+
if let ValueNs::LocalBinding(id) = value {
2258+
locals.push((def, *id).into());
2259+
}
2260+
});
2261+
}
2262+
};
2263+
2264+
while let Some(expr_id) = exprs.pop() {
2265+
let mut has_child = false;
2266+
store.walk_child_exprs(expr_id, |id| {
2267+
has_child = true;
2268+
exprs.push(id);
2269+
});
2270+
if !has_child {
2271+
add_to_locals_used(expr_id)
2272+
}
2273+
}
2274+
2275+
Some(locals)
2276+
}
21952277
}
21962278

21972279
// FIXME This can't be the best way to do this

‎crates/hir/src/source_analyzer.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ impl<'db> SourceAnalyzer<'db> {
240240
)
241241
}
242242

243-
fn expr_id(&self, expr: ast::Expr) -> Option<ExprOrPatId> {
243+
pub(crate) fn expr_id(&self, expr: ast::Expr) -> Option<ExprOrPatId> {
244244
let src = InFile { file_id: self.file_id, value: expr };
245245
self.store_sm()?.node_expr(src.as_ref())
246246
}

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

Lines changed: 112 additions & 93 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,14 @@ use hir::{
99
use ide_db::{
1010
FxIndexSet, RootDatabase,
1111
assists::GroupLabel,
12-
defs::{Definition, NameRefClass},
12+
defs::Definition,
1313
famous_defs::FamousDefs,
1414
helpers::mod_path_to_ast,
1515
imports::insert_use::{ImportScope, insert_use},
1616
search::{FileReference, ReferenceCategory, SearchScope},
1717
source_change::SourceChangeBuilder,
1818
syntax_helpers::node_ext::{
19-
for_each_tail_expr, preorder_expr, walk_expr, walk_pat, walk_patterns_in_expr,
19+
for_each_tail_expr, preorder_expr, walk_pat, walk_patterns_in_expr,
2020
},
2121
};
2222
use itertools::Itertools;
@@ -687,29 +687,6 @@ impl FunctionBody {
687687
}
688688
}
689689

690-
fn walk_expr(&self, cb: &mut dyn FnMut(ast::Expr)) {
691-
match self {
692-
FunctionBody::Expr(expr) => walk_expr(expr, cb),
693-
FunctionBody::Span { parent, text_range, .. } => {
694-
parent
695-
.statements()
696-
.filter(|stmt| text_range.contains_range(stmt.syntax().text_range()))
697-
.filter_map(|stmt| match stmt {
698-
ast::Stmt::ExprStmt(expr_stmt) => expr_stmt.expr(),
699-
ast::Stmt::Item(_) => None,
700-
ast::Stmt::LetStmt(stmt) => stmt.initializer(),
701-
})
702-
.for_each(|expr| walk_expr(&expr, cb));
703-
if let Some(expr) = parent
704-
.tail_expr()
705-
.filter(|it| text_range.contains_range(it.syntax().text_range()))
706-
{
707-
walk_expr(&expr, cb);
708-
}
709-
}
710-
}
711-
}
712-
713690
fn preorder_expr(&self, cb: &mut dyn FnMut(WalkEvent<ast::Expr>) -> bool) {
714691
match self {
715692
FunctionBody::Expr(expr) => preorder_expr(expr, cb),
@@ -718,10 +695,24 @@ impl FunctionBody {
718695
.statements()
719696
.filter(|stmt| text_range.contains_range(stmt.syntax().text_range()))
720697
.filter_map(|stmt| match stmt {
721-
ast::Stmt::ExprStmt(expr_stmt) => expr_stmt.expr(),
698+
ast::Stmt::ExprStmt(expr_stmt) => expr_stmt.expr().map(|e| vec![e]),
722699
ast::Stmt::Item(_) => None,
723-
ast::Stmt::LetStmt(stmt) => stmt.initializer(),
700+
ast::Stmt::LetStmt(stmt) => {
701+
let init = stmt.initializer();
702+
let let_else = stmt
703+
.let_else()
704+
.and_then(|le| le.block_expr())
705+
.map(ast::Expr::BlockExpr);
706+
707+
match (init, let_else) {
708+
(Some(i), Some(le)) => Some(vec![i, le]),
709+
(Some(i), _) => Some(vec![i]),
710+
(_, Some(le)) => Some(vec![le]),
711+
_ => None,
712+
}
713+
}
724714
})
715+
.flatten()
725716
.for_each(|expr| preorder_expr(&expr, cb));
726717
if let Some(expr) = parent
727718
.tail_expr()
@@ -799,22 +790,14 @@ impl FunctionBody {
799790
let mut self_param = None;
800791
let mut res = FxIndexSet::default();
801792

802-
fn local_from_name_ref(
803-
sema: &Semantics<'_, RootDatabase>,
804-
name_ref: ast::NameRef,
805-
) -> Option<hir::Local> {
806-
match NameRefClass::classify(sema, &name_ref) {
807-
Some(
808-
NameRefClass::Definition(Definition::Local(local_ref), _)
809-
| NameRefClass::FieldShorthand { local_ref, field_ref: _, adt_subst: _ },
810-
) => Some(local_ref),
811-
_ => None,
812-
}
813-
}
793+
let (text_range, element) = match self {
794+
FunctionBody::Expr(expr) => (expr.syntax().text_range(), Either::Left(expr)),
795+
FunctionBody::Span { parent, text_range, .. } => (*text_range, Either::Right(parent)),
796+
};
814797

815798
let mut add_name_if_local = |local_ref: Local| {
816-
let InFile { file_id, value } = local_ref.primary_source(sema.db).source;
817799
// locals defined inside macros are not relevant to us
800+
let InFile { file_id, value } = local_ref.primary_source(sema.db).source;
818801
if !file_id.is_macro() {
819802
match value {
820803
Either::Right(it) => {
@@ -826,59 +809,11 @@ impl FunctionBody {
826809
}
827810
}
828811
};
829-
self.walk_expr(&mut |expr| match expr {
830-
ast::Expr::PathExpr(path_expr) => {
831-
if let Some(local) = path_expr
832-
.path()
833-
.and_then(|it| it.as_single_name_ref())
834-
.and_then(|name_ref| local_from_name_ref(sema, name_ref))
835-
{
836-
add_name_if_local(local);
837-
}
838-
}
839-
ast::Expr::ClosureExpr(closure_expr) => {
840-
if let Some(body) = closure_expr.body() {
841-
body.syntax()
842-
.descendants()
843-
.filter_map(ast::NameRef::cast)
844-
.filter_map(|name_ref| local_from_name_ref(sema, name_ref))
845-
.for_each(&mut add_name_if_local);
846-
}
847-
}
848-
ast::Expr::MacroExpr(expr) => {
849-
if let Some(tt) = expr.macro_call().and_then(|call| call.token_tree()) {
850-
tt.syntax()
851-
.descendants_with_tokens()
852-
.filter_map(SyntaxElement::into_token)
853-
.filter(|it| {
854-
matches!(it.kind(), SyntaxKind::STRING | SyntaxKind::IDENT | T![self])
855-
})
856-
.for_each(|t| {
857-
if ast::String::can_cast(t.kind()) {
858-
if let Some(parts) =
859-
ast::String::cast(t).and_then(|s| sema.as_format_args_parts(&s))
860-
{
861-
parts
862-
.into_iter()
863-
.filter_map(|(_, value)| value.and_then(|it| it.left()))
864-
.filter_map(|path| match path {
865-
PathResolution::Local(local) => Some(local),
866-
_ => None,
867-
})
868-
.for_each(&mut add_name_if_local);
869-
}
870-
} else {
871-
sema.descend_into_macros_exact(t)
872-
.into_iter()
873-
.filter_map(|t| t.parent().and_then(ast::NameRef::cast))
874-
.filter_map(|name_ref| local_from_name_ref(sema, name_ref))
875-
.for_each(&mut add_name_if_local);
876-
}
877-
});
878-
}
879-
}
880-
_ => (),
881-
});
812+
813+
if let Some(locals) = sema.locals_used(element, text_range) {
814+
locals.into_iter().for_each(&mut add_name_if_local);
815+
}
816+
882817
(res, self_param)
883818
}
884819

@@ -6291,6 +6226,90 @@ fn foo() {
62916226
62926227
fn $0fun_name(v: i32) {
62936228
print!("{v:?}{}", v == 123);
6229+
}"#,
6230+
);
6231+
}
6232+
6233+
#[test]
6234+
fn no_parameter_for_variable_used_only_let_else() {
6235+
check_assist(
6236+
extract_function,
6237+
r#"
6238+
fn foo() -> u32 {
6239+
let x = 5;
6240+
6241+
$0let Some(y) = Some(1) else {
6242+
return x * 2;
6243+
};$0
6244+
6245+
y
6246+
}"#,
6247+
r#"
6248+
fn foo() -> u32 {
6249+
let x = 5;
6250+
6251+
let y = match fun_name(x) {
6252+
Ok(value) => value,
6253+
Err(value) => return value,
6254+
};
6255+
6256+
y
6257+
}
6258+
6259+
fn $0fun_name(x: u32) -> Result<_, u32> {
6260+
let Some(y) = Some(1) else {
6261+
return Err(x * 2);
6262+
};
6263+
Ok(y)
6264+
}"#,
6265+
);
6266+
}
6267+
6268+
#[test]
6269+
fn deeply_nested_macros() {
6270+
check_assist(
6271+
extract_function,
6272+
r#"
6273+
macro_rules! m {
6274+
($val:ident) => { $val };
6275+
}
6276+
6277+
macro_rules! n {
6278+
($v1:ident, $v2:ident) => { m!($v1) + $v2 };
6279+
}
6280+
6281+
macro_rules! o {
6282+
($v1:ident, $v2:ident, $v3:ident) => { n!($v1, $v2) + $v3 };
6283+
}
6284+
6285+
fn foo() -> u32 {
6286+
let v1 = 1;
6287+
let v2 = 2;
6288+
$0let v3 = 3;
6289+
o!(v1, v2, v3)$0
6290+
}"#,
6291+
r#"
6292+
macro_rules! m {
6293+
($val:ident) => { $val };
6294+
}
6295+
6296+
macro_rules! n {
6297+
($v1:ident, $v2:ident) => { m!($v1) + $v2 };
6298+
}
6299+
6300+
macro_rules! o {
6301+
($v1:ident, $v2:ident, $v3:ident) => { n!($v1, $v2) + $v3 };
6302+
}
6303+
6304+
fn foo() -> u32 {
6305+
let v1 = 1;
6306+
let v2 = 2;
6307+
fun_name(v1, v2)
6308+
}
6309+
6310+
fn $0fun_name(v1: u32, v2: u32) -> u32 {
6311+
let v3 = 3;
6312+
o!(v1, v2, v3)
62946313
}"#,
62956314
);
62966315
}

0 commit comments

Comments
 (0)