Skip to content

Commit 8241ec6

Browse files
ShoyuVanillaShoyuVanilla
authored
Merge pull request #20387 from ChayimFriedman2/rename-macro
fix: Do not remove the original token when descending into derives
2 parents 7543653 + ea140ef commit 8241ec6

File tree

2 files changed

+81
-63
lines changed

2 files changed

+81
-63
lines changed

‎crates/hir/src/semantics.rs‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1241,29 +1241,27 @@ impl<'db> SemanticsImpl<'db> {
12411241
adt,
12421242
))
12431243
})?;
1244-
let mut res = None;
12451244
for (_, derive_attr, derives) in derives {
12461245
// as there may be multiple derives registering the same helper
12471246
// name, we gotta make sure to call this for all of them!
12481247
// FIXME: We need to call `f` for all of them as well though!
1249-
res = res.or(process_expansion_for_token(
1250-
ctx,
1251-
&mut stack,
1252-
derive_attr,
1253-
));
1248+
process_expansion_for_token(ctx, &mut stack, derive_attr);
12541249
for derive in derives.into_iter().flatten() {
1255-
res = res
1256-
.or(process_expansion_for_token(ctx, &mut stack, derive));
1250+
process_expansion_for_token(ctx, &mut stack, derive);
12571251
}
12581252
}
12591253
// remove all tokens that are within the derives expansion
12601254
filter_duplicates(tokens, adt.syntax().text_range());
1261-
Some(res)
1255+
Some(())
12621256
});
12631257
// if we found derives, we can early exit. There is no way we can be in any
12641258
// macro call at this point given we are not in a token tree
1265-
if let Some(res) = res {
1266-
return res;
1259+
if let Some(()) = res {
1260+
// Note: derives do not remap the original token. Furthermore, we want
1261+
// the original token to be before the derives in the list, because if they
1262+
// upmap to the same token and we deduplicate them (e.g. in rename), we
1263+
// want the original token to remain, not the derive.
1264+
return None;
12671265
}
12681266
}
12691267
// Then check for token trees, that means we are either in a function-like macro or

‎crates/ide/src/rename.rs‎

Lines changed: 72 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,27 @@ pub use ide_db::rename::RenameError;
2727

2828
type RenameResult<T> = Result<T, RenameError>;
2929

30+
/// This is similar to `collect::<Result<Vec<_>, _>>`, but unlike it, it succeeds if there is *any* `Ok` item.
31+
fn ok_if_any<T, E>(iter: impl Iterator<Item = Result<T, E>>) -> Result<Vec<T>, E> {
32+
let mut err = None;
33+
let oks = iter
34+
.filter_map(|item| match item {
35+
Ok(it) => Some(it),
36+
Err(it) => {
37+
err = Some(it);
38+
None
39+
}
40+
})
41+
.collect::<Vec<_>>();
42+
if !oks.is_empty() {
43+
Ok(oks)
44+
} else if let Some(err) = err {
45+
Err(err)
46+
} else {
47+
Ok(Vec::new())
48+
}
49+
}
50+
3051
/// Prepares a rename. The sole job of this function is to return the TextRange of the thing that is
3152
/// being targeted for a rename.
3253
pub(crate) fn prepare_rename(
@@ -95,58 +116,57 @@ pub(crate) fn rename(
95116
alias_fallback(syntax, position, &new_name.display(db, edition).to_string());
96117

97118
let ops: RenameResult<Vec<SourceChange>> = match alias_fallback {
98-
Some(_) => defs
99-
// FIXME: This can use the `ide_db::rename_reference` (or def.rename) method once we can
100-
// properly find "direct" usages/references.
101-
.map(|(.., def, new_name, _)| {
102-
match kind {
103-
IdentifierKind::Ident => (),
104-
IdentifierKind::Lifetime => {
105-
bail!("Cannot alias reference to a lifetime identifier")
106-
}
107-
IdentifierKind::Underscore => bail!("Cannot alias reference to `_`"),
108-
IdentifierKind::LowercaseSelf => {
109-
bail!("Cannot rename alias reference to `self`")
110-
}
111-
};
112-
let mut usages = def.usages(&sema).all();
113-
114-
// FIXME: hack - removes the usage that triggered this rename operation.
115-
match usages.references.get_mut(&file_id).and_then(|refs| {
116-
refs.iter()
117-
.position(|ref_| ref_.range.contains_inclusive(position.offset))
118-
.map(|idx| refs.remove(idx))
119-
}) {
120-
Some(_) => (),
121-
None => never!(),
122-
};
123-
124-
let mut source_change = SourceChange::default();
125-
source_change.extend(usages.references.get_mut(&file_id).iter().map(|refs| {
126-
(
127-
position.file_id,
128-
source_edit_from_references(db, refs, def, &new_name, edition),
129-
)
130-
}));
131-
132-
Ok(source_change)
133-
})
134-
.collect(),
135-
None => defs
136-
.map(|(.., def, new_name, rename_def)| {
137-
if let Definition::Local(local) = def {
138-
if let Some(self_param) = local.as_self_param(sema.db) {
139-
cov_mark::hit!(rename_self_to_param);
140-
return rename_self_to_param(&sema, local, self_param, &new_name, kind);
141-
}
142-
if kind == IdentifierKind::LowercaseSelf {
143-
cov_mark::hit!(rename_to_self);
144-
return rename_to_self(&sema, local);
145-
}
119+
Some(_) => ok_if_any(
120+
defs
121+
// FIXME: This can use the `ide_db::rename_reference` (or def.rename) method once we can
122+
// properly find "direct" usages/references.
123+
.map(|(.., def, new_name, _)| {
124+
match kind {
125+
IdentifierKind::Ident => (),
126+
IdentifierKind::Lifetime => {
127+
bail!("Cannot alias reference to a lifetime identifier")
128+
}
129+
IdentifierKind::Underscore => bail!("Cannot alias reference to `_`"),
130+
IdentifierKind::LowercaseSelf => {
131+
bail!("Cannot rename alias reference to `self`")
132+
}
133+
};
134+
let mut usages = def.usages(&sema).all();
135+
136+
// FIXME: hack - removes the usage that triggered this rename operation.
137+
match usages.references.get_mut(&file_id).and_then(|refs| {
138+
refs.iter()
139+
.position(|ref_| ref_.range.contains_inclusive(position.offset))
140+
.map(|idx| refs.remove(idx))
141+
}) {
142+
Some(_) => (),
143+
None => never!(),
144+
};
145+
146+
let mut source_change = SourceChange::default();
147+
source_change.extend(usages.references.get_mut(&file_id).iter().map(|refs| {
148+
(
149+
position.file_id,
150+
source_edit_from_references(db, refs, def, &new_name, edition),
151+
)
152+
}));
153+
154+
Ok(source_change)
155+
}),
156+
),
157+
None => ok_if_any(defs.map(|(.., def, new_name, rename_def)| {
158+
if let Definition::Local(local) = def {
159+
if let Some(self_param) = local.as_self_param(sema.db) {
160+
cov_mark::hit!(rename_self_to_param);
161+
return rename_self_to_param(&sema, local, self_param, &new_name, kind);
146162
}
147-
def.rename(&sema, new_name.as_str(), rename_def)
148-
})
149-
.collect(),
163+
if kind == IdentifierKind::LowercaseSelf {
164+
cov_mark::hit!(rename_to_self);
165+
return rename_to_self(&sema, local);
166+
}
167+
}
168+
def.rename(&sema, new_name.as_str(), rename_def)
169+
})),
150170
};
151171

152172
ops?.into_iter()
@@ -320,7 +340,7 @@ fn find_definitions(
320340
})
321341
});
322342

323-
let res: RenameResult<Vec<_>> = symbols.filter_map(Result::transpose).collect();
343+
let res: RenameResult<Vec<_>> = ok_if_any(symbols.filter_map(Result::transpose));
324344
match res {
325345
Ok(v) => {
326346
// remove duplicates, comparing `Definition`s

0 commit comments

Comments
 (0)