Skip to content

Commit 8dd3a71

Browse files
bors[bot]Veykril
bors[bot]
and
Veykril
authored
Merge #9955
9955: fix: Rename fails on renaming definitions created by macros instead of renaming the macro invocation r=Veykril a=Veykril bors r+ Co-authored-by: Lukas Wirth <[email protected]>
2 parents 59aa091 + c67ecbe commit 8dd3a71

File tree

5 files changed

+62
-66
lines changed

5 files changed

+62
-66
lines changed

‎crates/hir_expand/src/lib.rs

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,7 @@ use either::Either;
2222
pub use mbe::{ExpandError, ExpandResult};
2323
pub use parser::FragmentKind;
2424

25-
use std::hash::Hash;
26-
use std::sync::Arc;
25+
use std::{hash::Hash, sync::Arc};
2726

2827
use base_db::{impl_intern_key, salsa, CrateId, FileId, FileRange};
2928
use syntax::{
@@ -32,11 +31,13 @@ use syntax::{
3231
Direction, SyntaxNode, SyntaxToken, TextRange, TextSize,
3332
};
3433

35-
use crate::ast_id_map::FileAstId;
36-
use crate::builtin_attr::BuiltinAttrExpander;
37-
use crate::builtin_derive::BuiltinDeriveExpander;
38-
use crate::builtin_macro::{BuiltinFnLikeExpander, EagerExpander};
39-
use crate::proc_macro::ProcMacroExpander;
34+
use crate::{
35+
ast_id_map::FileAstId,
36+
builtin_attr::BuiltinAttrExpander,
37+
builtin_derive::BuiltinDeriveExpander,
38+
builtin_macro::{BuiltinFnLikeExpander, EagerExpander},
39+
proc_macro::ProcMacroExpander,
40+
};
4041

4142
#[cfg(test)]
4243
mod test_db;
@@ -210,12 +211,12 @@ impl MacroDefId {
210211

211212
pub fn ast_id(&self) -> Either<AstId<ast::Macro>, AstId<ast::Fn>> {
212213
let id = match &self.kind {
213-
MacroDefKind::Declarative(id) => id,
214-
MacroDefKind::BuiltIn(_, id) => id,
215-
MacroDefKind::BuiltInAttr(_, id) => id,
216-
MacroDefKind::BuiltInDerive(_, id) => id,
217-
MacroDefKind::BuiltInEager(_, id) => id,
218214
MacroDefKind::ProcMacro(.., id) => return Either::Right(*id),
215+
MacroDefKind::Declarative(id)
216+
| MacroDefKind::BuiltIn(_, id)
217+
| MacroDefKind::BuiltInAttr(_, id)
218+
| MacroDefKind::BuiltInDerive(_, id)
219+
| MacroDefKind::BuiltInEager(_, id) => id,
219220
};
220221
Either::Left(*id)
221222
}
@@ -464,15 +465,10 @@ impl InFile<SyntaxNode> {
464465
}
465466

466467
impl<'a> InFile<&'a SyntaxNode> {
468+
/// Falls back to the macro call range if the node cannot be mapped up fully.
467469
pub fn original_file_range(self, db: &dyn db::AstDatabase) -> FileRange {
468-
if let Some(range) = original_range_opt(db, self) {
469-
let original_file = range.file_id.original_file(db);
470-
if range.file_id == original_file.into() {
471-
return FileRange { file_id: original_file, range: range.value };
472-
}
473-
474-
log::error!("Fail to mapping up more for {:?}", range);
475-
return FileRange { file_id: range.file_id.original_file(db), range: range.value };
470+
if let Some(res) = self.original_file_range_opt(db) {
471+
return res;
476472
}
477473

478474
// Fall back to whole macro call.
@@ -483,8 +479,27 @@ impl<'a> InFile<&'a SyntaxNode> {
483479

484480
let orig_file = node.file_id.original_file(db);
485481
assert_eq!(node.file_id, orig_file.into());
482+
486483
FileRange { file_id: orig_file, range: node.value.text_range() }
487484
}
485+
486+
/// Attempts to map the syntax node back up its macro calls.
487+
pub fn original_file_range_opt(self, db: &dyn db::AstDatabase) -> Option<FileRange> {
488+
match original_range_opt(db, self) {
489+
Some(range) => {
490+
let original_file = range.file_id.original_file(db);
491+
if range.file_id != original_file.into() {
492+
log::error!("Failed mapping up more for {:?}", range);
493+
}
494+
Some(FileRange { file_id: original_file, range: range.value })
495+
}
496+
_ if !self.file_id.is_macro() => Some(FileRange {
497+
file_id: self.file_id.original_file(db),
498+
range: self.value.text_range(),
499+
}),
500+
_ => None,
501+
}
502+
}
488503
}
489504

490505
fn original_range_opt(

‎crates/ide/src/display/navigation_target.rs

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,11 @@ impl NavigationTarget {
143143
kind: SymbolKind,
144144
) -> NavigationTarget {
145145
let name = node.value.name().map(|it| it.text().into()).unwrap_or_else(|| "_".into());
146-
let focus_range =
147-
node.value.name().map(|it| node.with_value(it.syntax()).original_file_range(db).range);
146+
let focus_range = node
147+
.value
148+
.name()
149+
.and_then(|it| node.with_value(it.syntax()).original_file_range_opt(db))
150+
.map(|it| it.range);
148151
let frange = node.map(|it| it.syntax()).original_file_range(db);
149152

150153
NavigationTarget::from_syntax(frange.file_id, name, focus_range, frange.range, kind)

‎crates/ide/src/rename.rs

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1869,20 +1869,15 @@ fn f() { <()>::BAR$0; }"#,
18691869
}
18701870

18711871
#[test]
1872-
fn macros_are_broken_lol() {
1873-
cov_mark::check!(macros_are_broken_lol);
1872+
fn defs_from_macros_arent_renamed() {
18741873
check(
18751874
"lol",
18761875
r#"
18771876
macro_rules! m { () => { fn f() {} } }
18781877
m!();
18791878
fn main() { f$0() }
18801879
"#,
1881-
r#"
1882-
macro_rules! m { () => { fn f() {} } }
1883-
lol
1884-
fn main() { lol() }
1885-
"#,
1880+
"error: No identifier available to rename",
18861881
)
18871882
}
18881883
}

‎crates/ide/src/runnables.rs

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1446,7 +1446,6 @@ gen2!();
14461446
0,
14471447
),
14481448
full_range: 228..236,
1449-
focus_range: 228..236,
14501449
name: "foo_test2",
14511450
kind: Function,
14521451
},
@@ -1467,7 +1466,6 @@ gen2!();
14671466
0,
14681467
),
14691468
full_range: 218..225,
1470-
focus_range: 218..225,
14711469
name: "foo_test",
14721470
kind: Function,
14731471
},
@@ -1533,7 +1531,6 @@ foo!();
15331531
0,
15341532
),
15351533
full_range: 210..217,
1536-
focus_range: 210..217,
15371534
name: "foo0",
15381535
kind: Function,
15391536
},
@@ -1554,7 +1551,6 @@ foo!();
15541551
0,
15551552
),
15561553
full_range: 210..217,
1557-
focus_range: 210..217,
15581554
name: "foo1",
15591555
kind: Function,
15601556
},
@@ -1575,7 +1571,6 @@ foo!();
15751571
0,
15761572
),
15771573
full_range: 210..217,
1578-
focus_range: 210..217,
15791574
name: "foo2",
15801575
kind: Function,
15811576
},

‎crates/ide_db/src/rename.rs

Lines changed: 20 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -81,53 +81,42 @@ impl Definition {
8181
/// `Definition`. Note that some definitions, like buitin types, can't be
8282
/// renamed.
8383
pub fn range_for_rename(self, sema: &Semantics<RootDatabase>) -> Option<FileRange> {
84-
// FIXME: the `original_file_range` calls here are wrong -- they never fail,
85-
// and _fall back_ to the entirety of the macro call. Such fall back is
86-
// incorrect for renames. The safe behavior would be to return an error for
87-
// such cases. The correct behavior would be to return an auxiliary list of
88-
// "can't rename these occurrences in macros" items, and then show some kind
89-
// of a dialog to the user. See:
90-
cov_mark::hit!(macros_are_broken_lol);
91-
9284
let res = match self {
9385
Definition::Macro(mac) => {
9486
let src = mac.source(sema.db)?;
9587
let name = match &src.value {
9688
Either::Left(it) => it.name()?,
9789
Either::Right(it) => it.name()?,
9890
};
99-
src.with_value(name.syntax()).original_file_range(sema.db)
91+
src.with_value(name.syntax()).original_file_range_opt(sema.db)
10092
}
10193
Definition::Field(field) => {
10294
let src = field.source(sema.db)?;
103-
10495
match &src.value {
10596
FieldSource::Named(record_field) => {
10697
let name = record_field.name()?;
107-
src.with_value(name.syntax()).original_file_range(sema.db)
108-
}
109-
FieldSource::Pos(_) => {
110-
return None;
98+
src.with_value(name.syntax()).original_file_range_opt(sema.db)
11199
}
100+
FieldSource::Pos(_) => None,
112101
}
113102
}
114103
Definition::ModuleDef(module_def) => match module_def {
115104
hir::ModuleDef::Module(module) => {
116105
let src = module.declaration_source(sema.db)?;
117106
let name = src.value.name()?;
118-
src.with_value(name.syntax()).original_file_range(sema.db)
107+
src.with_value(name.syntax()).original_file_range_opt(sema.db)
119108
}
120-
hir::ModuleDef::Function(it) => name_range(it, sema)?,
109+
hir::ModuleDef::Function(it) => name_range(it, sema),
121110
hir::ModuleDef::Adt(adt) => match adt {
122-
hir::Adt::Struct(it) => name_range(it, sema)?,
123-
hir::Adt::Union(it) => name_range(it, sema)?,
124-
hir::Adt::Enum(it) => name_range(it, sema)?,
111+
hir::Adt::Struct(it) => name_range(it, sema),
112+
hir::Adt::Union(it) => name_range(it, sema),
113+
hir::Adt::Enum(it) => name_range(it, sema),
125114
},
126-
hir::ModuleDef::Variant(it) => name_range(it, sema)?,
127-
hir::ModuleDef::Const(it) => name_range(it, sema)?,
128-
hir::ModuleDef::Static(it) => name_range(it, sema)?,
129-
hir::ModuleDef::Trait(it) => name_range(it, sema)?,
130-
hir::ModuleDef::TypeAlias(it) => name_range(it, sema)?,
115+
hir::ModuleDef::Variant(it) => name_range(it, sema),
116+
hir::ModuleDef::Const(it) => name_range(it, sema),
117+
hir::ModuleDef::Static(it) => name_range(it, sema),
118+
hir::ModuleDef::Trait(it) => name_range(it, sema),
119+
hir::ModuleDef::TypeAlias(it) => name_range(it, sema),
131120
hir::ModuleDef::BuiltinType(_) => return None,
132121
},
133122
Definition::SelfType(_) => return None,
@@ -137,7 +126,7 @@ impl Definition {
137126
Either::Left(bind_pat) => bind_pat.name()?,
138127
Either::Right(_) => return None,
139128
};
140-
src.with_value(name.syntax()).original_file_range(sema.db)
129+
src.with_value(name.syntax()).original_file_range_opt(sema.db)
141130
}
142131
Definition::GenericParam(generic_param) => match generic_param {
143132
hir::GenericParam::TypeParam(type_param) => {
@@ -146,22 +135,22 @@ impl Definition {
146135
Either::Left(type_param) => type_param.name()?,
147136
Either::Right(_trait) => return None,
148137
};
149-
src.with_value(name.syntax()).original_file_range(sema.db)
138+
src.with_value(name.syntax()).original_file_range_opt(sema.db)
150139
}
151140
hir::GenericParam::LifetimeParam(lifetime_param) => {
152141
let src = lifetime_param.source(sema.db)?;
153142
let lifetime = src.value.lifetime()?;
154-
src.with_value(lifetime.syntax()).original_file_range(sema.db)
143+
src.with_value(lifetime.syntax()).original_file_range_opt(sema.db)
155144
}
156-
hir::GenericParam::ConstParam(it) => name_range(it, sema)?,
145+
hir::GenericParam::ConstParam(it) => name_range(it, sema),
157146
},
158147
Definition::Label(label) => {
159148
let src = label.source(sema.db);
160149
let lifetime = src.value.lifetime()?;
161-
src.with_value(lifetime.syntax()).original_file_range(sema.db)
150+
src.with_value(lifetime.syntax()).original_file_range_opt(sema.db)
162151
}
163152
};
164-
return Some(res);
153+
return res;
165154

166155
fn name_range<D>(def: D, sema: &Semantics<RootDatabase>) -> Option<FileRange>
167156
where
@@ -170,8 +159,7 @@ impl Definition {
170159
{
171160
let src = def.source(sema.db)?;
172161
let name = src.value.name()?;
173-
let res = src.with_value(name.syntax()).original_file_range(sema.db);
174-
Some(res)
162+
src.with_value(name.syntax()).original_file_range_opt(sema.db)
175163
}
176164
}
177165
}

0 commit comments

Comments
 (0)