Skip to content

Commit 15fc643

Browse files
committed
Fix ordering problem between qualifying paths and substituting params
1 parent 12905e5 commit 15fc643

File tree

7 files changed

+206
-126
lines changed

7 files changed

+206
-126
lines changed

crates/ra_assists/src/assists/add_missing_impl_members.rs

Lines changed: 8 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
1-
use std::collections::HashMap;
2-
3-
use hir::{db::HirDatabase, HasSource};
1+
use hir::{db::HirDatabase, HasSource, InFile};
42
use ra_syntax::{
53
ast::{self, edit, make, AstNode, NameOwner},
64
SmolStr,
75
};
86

9-
use crate::{Assist, AssistCtx, AssistId};
7+
use crate::{
8+
ast_transform::{self, AstTransform, QualifyPaths, SubstituteTypeParams},
9+
Assist, AssistCtx, AssistId,
10+
};
1011

1112
#[derive(PartialEq)]
1213
enum AddMissingImplMembersMode {
@@ -146,24 +147,11 @@ fn add_missing_impl_members_inner(
146147
None,
147148
)
148149
.module();
149-
let substs = get_syntactic_substs(impl_node).unwrap_or_default();
150-
let generic_def: hir::GenericDef = trait_.into();
151-
let substs_by_param: HashMap<_, _> = generic_def
152-
.params(db)
153-
.into_iter()
154-
// this is a trait impl, so we need to skip the first type parameter -- this is a bit hacky
155-
.skip(1)
156-
.zip(substs.into_iter())
157-
.collect();
150+
let ast_transform = QualifyPaths::new(db, module)
151+
.or(SubstituteTypeParams::for_trait_impl(db, trait_, impl_node));
158152
let items = missing_items
159153
.into_iter()
160-
.map(|it| {
161-
substitute_type_params(db, hir::InFile::new(trait_file_id, it), &substs_by_param)
162-
})
163-
.map(|it| match module {
164-
Some(module) => qualify_paths(db, hir::InFile::new(trait_file_id, it), module),
165-
None => it,
166-
})
154+
.map(|it| ast_transform::apply(&*ast_transform, InFile::new(trait_file_id, it)))
167155
.map(|it| match it {
168156
ast::ImplItem::FnDef(def) => ast::ImplItem::FnDef(add_body(def)),
169157
_ => it,
@@ -188,99 +176,6 @@ fn add_body(fn_def: ast::FnDef) -> ast::FnDef {
188176
}
189177
}
190178

191-
// FIXME: It would probably be nicer if we could get this via HIR (i.e. get the
192-
// trait ref, and then go from the types in the substs back to the syntax)
193-
// FIXME: This should be a general utility (not even just for assists)
194-
fn get_syntactic_substs(impl_block: ast::ImplBlock) -> Option<Vec<ast::TypeRef>> {
195-
let target_trait = impl_block.target_trait()?;
196-
let path_type = match target_trait {
197-
ast::TypeRef::PathType(path) => path,
198-
_ => return None,
199-
};
200-
let type_arg_list = path_type.path()?.segment()?.type_arg_list()?;
201-
let mut result = Vec::new();
202-
for type_arg in type_arg_list.type_args() {
203-
let type_arg: ast::TypeArg = type_arg;
204-
result.push(type_arg.type_ref()?);
205-
}
206-
Some(result)
207-
}
208-
209-
// FIXME: This should be a general utility (not even just for assists)
210-
fn substitute_type_params<N: AstNode + Clone>(
211-
db: &impl HirDatabase,
212-
node: hir::InFile<N>,
213-
substs: &HashMap<hir::TypeParam, ast::TypeRef>,
214-
) -> N {
215-
let type_param_replacements = node
216-
.clone()
217-
.descendants::<ast::TypeRef>()
218-
.filter_map(|n| {
219-
let path = match &n.value {
220-
ast::TypeRef::PathType(path_type) => path_type.path()?,
221-
_ => return None,
222-
};
223-
let analyzer = hir::SourceAnalyzer::new(db, n.syntax(), None);
224-
let resolution = analyzer.resolve_path(db, &path)?;
225-
match resolution {
226-
hir::PathResolution::TypeParam(tp) => Some((n.value, substs.get(&tp)?.clone())),
227-
_ => None,
228-
}
229-
})
230-
.collect::<Vec<_>>();
231-
232-
if type_param_replacements.is_empty() {
233-
node.value
234-
} else {
235-
edit::replace_descendants(&node.value, type_param_replacements.into_iter())
236-
}
237-
}
238-
239-
use hir::PathResolution;
240-
241-
// FIXME extract this to a general utility as well
242-
// FIXME handle value ns?
243-
// FIXME this doesn't 'commute' with `substitute_type_params`, since type params in newly generated type arg lists don't resolve. Currently we can avoid this problem, but it's worth thinking about a solution
244-
fn qualify_paths<N: AstNode>(db: &impl HirDatabase, node: hir::InFile<N>, from: hir::Module) -> N {
245-
let path_replacements = node
246-
.value
247-
.syntax()
248-
.descendants()
249-
.filter_map(ast::Path::cast)
250-
.filter_map(|p| {
251-
if p.segment().and_then(|s| s.param_list()).is_some() {
252-
// don't try to qualify `Fn(Foo) -> Bar` paths, they are in prelude anyway
253-
return None;
254-
}
255-
// FIXME check if some ancestor is already being replaced, if so skip this
256-
let analyzer = hir::SourceAnalyzer::new(db, node.with_value(p.syntax()), None);
257-
let resolution = analyzer.resolve_path(db, &p)?;
258-
match resolution {
259-
PathResolution::Def(def) => {
260-
let found_path = from.find_path(db, def)?;
261-
// TODO fix type arg replacements being qualified
262-
let args = p
263-
.segment()
264-
.and_then(|s| s.type_arg_list())
265-
.map(|arg_list| qualify_paths(db, node.with_value(arg_list), from));
266-
Some((p, make::path_with_type_arg_list(found_path.to_ast(), args)))
267-
}
268-
PathResolution::Local(_)
269-
| PathResolution::TypeParam(_)
270-
| PathResolution::SelfType(_) => None,
271-
PathResolution::Macro(_) => None,
272-
PathResolution::AssocItem(_) => None,
273-
}
274-
})
275-
.collect::<Vec<_>>();
276-
277-
if path_replacements.is_empty() {
278-
node.value
279-
} else {
280-
edit::replace_descendants(&node.value, path_replacements.into_iter())
281-
}
282-
}
283-
284179
/// Given an `ast::ImplBlock`, resolves the target trait (the one being
285180
/// implemented) to a `ast::TraitDef`.
286181
fn resolve_target_trait_def(
Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,178 @@
1+
//! `AstTransformer`s are functions that replace nodes in an AST and can be easily combined.
2+
use std::collections::HashMap;
3+
4+
use hir::{db::HirDatabase, InFile, PathResolution};
5+
use ra_syntax::ast::{self, make, AstNode};
6+
7+
pub trait AstTransform<'a> {
8+
fn get_substitution(
9+
&self,
10+
node: InFile<&ra_syntax::SyntaxNode>,
11+
) -> Option<ra_syntax::SyntaxNode>;
12+
13+
fn chain_before(self, other: Box<dyn AstTransform<'a> + 'a>) -> Box<dyn AstTransform<'a> + 'a>;
14+
fn or<T: AstTransform<'a> + 'a>(self, other: T) -> Box<dyn AstTransform<'a> + 'a>
15+
where
16+
Self: Sized + 'a,
17+
{
18+
self.chain_before(Box::new(other))
19+
}
20+
}
21+
22+
struct NullTransformer;
23+
24+
impl<'a> AstTransform<'a> for NullTransformer {
25+
fn get_substitution(
26+
&self,
27+
_node: InFile<&ra_syntax::SyntaxNode>,
28+
) -> Option<ra_syntax::SyntaxNode> {
29+
None
30+
}
31+
fn chain_before(self, other: Box<dyn AstTransform<'a> + 'a>) -> Box<dyn AstTransform<'a> + 'a> {
32+
other
33+
}
34+
}
35+
36+
pub struct SubstituteTypeParams<'a, DB: HirDatabase> {
37+
db: &'a DB,
38+
substs: HashMap<hir::TypeParam, ast::TypeRef>,
39+
previous: Box<dyn AstTransform<'a> + 'a>,
40+
}
41+
42+
impl<'a, DB: HirDatabase> SubstituteTypeParams<'a, DB> {
43+
pub fn for_trait_impl(
44+
db: &'a DB,
45+
trait_: hir::Trait,
46+
impl_block: ast::ImplBlock,
47+
) -> SubstituteTypeParams<'a, DB> {
48+
let substs = get_syntactic_substs(impl_block).unwrap_or_default();
49+
let generic_def: hir::GenericDef = trait_.into();
50+
let substs_by_param: HashMap<_, _> = generic_def
51+
.params(db)
52+
.into_iter()
53+
// this is a trait impl, so we need to skip the first type parameter -- this is a bit hacky
54+
.skip(1)
55+
.zip(substs.into_iter())
56+
.collect();
57+
return SubstituteTypeParams {
58+
db,
59+
substs: substs_by_param,
60+
previous: Box::new(NullTransformer),
61+
};
62+
63+
fn get_syntactic_substs(impl_block: ast::ImplBlock) -> Option<Vec<ast::TypeRef>> {
64+
let target_trait = impl_block.target_trait()?;
65+
let path_type = match target_trait {
66+
ast::TypeRef::PathType(path) => path,
67+
_ => return None,
68+
};
69+
let type_arg_list = path_type.path()?.segment()?.type_arg_list()?;
70+
let mut result = Vec::new();
71+
for type_arg in type_arg_list.type_args() {
72+
let type_arg: ast::TypeArg = type_arg;
73+
result.push(type_arg.type_ref()?);
74+
}
75+
Some(result)
76+
}
77+
}
78+
fn get_substitution_inner(
79+
&self,
80+
node: InFile<&ra_syntax::SyntaxNode>,
81+
) -> Option<ra_syntax::SyntaxNode> {
82+
let type_ref = ast::TypeRef::cast(node.value.clone())?;
83+
let path = match &type_ref {
84+
ast::TypeRef::PathType(path_type) => path_type.path()?,
85+
_ => return None,
86+
};
87+
let analyzer = hir::SourceAnalyzer::new(self.db, node, None);
88+
let resolution = analyzer.resolve_path(self.db, &path)?;
89+
match resolution {
90+
hir::PathResolution::TypeParam(tp) => Some(self.substs.get(&tp)?.syntax().clone()),
91+
_ => None,
92+
}
93+
}
94+
}
95+
96+
impl<'a, DB: HirDatabase> AstTransform<'a> for SubstituteTypeParams<'a, DB> {
97+
fn get_substitution(
98+
&self,
99+
node: InFile<&ra_syntax::SyntaxNode>,
100+
) -> Option<ra_syntax::SyntaxNode> {
101+
self.get_substitution_inner(node).or_else(|| self.previous.get_substitution(node))
102+
}
103+
fn chain_before(self, other: Box<dyn AstTransform<'a> + 'a>) -> Box<dyn AstTransform<'a> + 'a> {
104+
Box::new(SubstituteTypeParams { previous: other, ..self })
105+
}
106+
}
107+
108+
pub struct QualifyPaths<'a, DB: HirDatabase> {
109+
db: &'a DB,
110+
from: Option<hir::Module>,
111+
previous: Box<dyn AstTransform<'a> + 'a>,
112+
}
113+
114+
impl<'a, DB: HirDatabase> QualifyPaths<'a, DB> {
115+
pub fn new(db: &'a DB, from: Option<hir::Module>) -> Self {
116+
Self { db, from, previous: Box::new(NullTransformer) }
117+
}
118+
119+
fn get_substitution_inner(
120+
&self,
121+
node: InFile<&ra_syntax::SyntaxNode>,
122+
) -> Option<ra_syntax::SyntaxNode> {
123+
// FIXME handle value ns?
124+
let from = self.from?;
125+
let p = ast::Path::cast(node.value.clone())?;
126+
if p.segment().and_then(|s| s.param_list()).is_some() {
127+
// don't try to qualify `Fn(Foo) -> Bar` paths, they are in prelude anyway
128+
return None;
129+
}
130+
let analyzer = hir::SourceAnalyzer::new(self.db, node, None);
131+
let resolution = analyzer.resolve_path(self.db, &p)?;
132+
match resolution {
133+
PathResolution::Def(def) => {
134+
let found_path = from.find_path(self.db, def)?;
135+
let args = p
136+
.segment()
137+
.and_then(|s| s.type_arg_list())
138+
.map(|arg_list| apply(self, node.with_value(arg_list)));
139+
Some(make::path_with_type_arg_list(found_path.to_ast(), args).syntax().clone())
140+
}
141+
PathResolution::Local(_)
142+
| PathResolution::TypeParam(_)
143+
| PathResolution::SelfType(_) => None,
144+
PathResolution::Macro(_) => None,
145+
PathResolution::AssocItem(_) => None,
146+
}
147+
}
148+
}
149+
150+
pub fn apply<'a, N: AstNode>(transformer: &dyn AstTransform<'a>, node: InFile<N>) -> N {
151+
let syntax = node.value.syntax();
152+
let result = ra_syntax::algo::replace_descendants(syntax, &|element| match element {
153+
ra_syntax::SyntaxElement::Node(n) => {
154+
let replacement = transformer.get_substitution(node.with_value(&n))?;
155+
Some(replacement.into())
156+
}
157+
_ => None,
158+
});
159+
N::cast(result).unwrap()
160+
}
161+
162+
impl<'a, DB: HirDatabase> AstTransform<'a> for QualifyPaths<'a, DB> {
163+
fn get_substitution(
164+
&self,
165+
node: InFile<&ra_syntax::SyntaxNode>,
166+
) -> Option<ra_syntax::SyntaxNode> {
167+
self.get_substitution_inner(node).or_else(|| self.previous.get_substitution(node))
168+
}
169+
fn chain_before(self, other: Box<dyn AstTransform<'a> + 'a>) -> Box<dyn AstTransform<'a> + 'a> {
170+
Box::new(QualifyPaths { previous: other, ..self })
171+
}
172+
}
173+
174+
// FIXME: It would probably be nicer if we could get this via HIR (i.e. get the
175+
// trait ref, and then go from the types in the substs back to the syntax)
176+
// FIXME: This should be a general utility (not even just for assists)
177+
178+
// FIXME: This should be a general utility (not even just for assists)

crates/ra_assists/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ mod marks;
1111
mod doc_tests;
1212
#[cfg(test)]
1313
mod test_db;
14+
pub mod ast_transform;
1415

1516
use hir::db::HirDatabase;
1617
use ra_db::FileRange;

crates/ra_ide/src/expand_macro.rs

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,7 @@ use rustc_hash::FxHashMap;
77

88
use ra_syntax::{
99
algo::{find_node_at_offset, replace_descendants},
10-
ast::{self},
11-
AstNode, NodeOrToken, SyntaxKind, SyntaxNode, WalkEvent, T,
10+
ast, AstNode, NodeOrToken, SyntaxElement, SyntaxKind, SyntaxNode, WalkEvent, T,
1211
};
1312

1413
pub struct ExpandedMacro {
@@ -43,7 +42,7 @@ fn expand_macro_recur(
4342
let mut expanded: SyntaxNode = db.parse_or_expand(macro_file_id)?;
4443

4544
let children = expanded.descendants().filter_map(ast::MacroCall::cast);
46-
let mut replaces = FxHashMap::default();
45+
let mut replaces: FxHashMap<SyntaxElement, SyntaxElement> = FxHashMap::default();
4746

4847
for child in children.into_iter() {
4948
let node = hir::InFile::new(macro_file_id, &child);
@@ -59,7 +58,7 @@ fn expand_macro_recur(
5958
}
6059
}
6160

62-
Some(replace_descendants(&expanded, &replaces))
61+
Some(replace_descendants(&expanded, &|n| replaces.get(n).cloned()))
6362
}
6463

6564
// FIXME: It would also be cool to share logic here and in the mbe tests,

crates/ra_syntax/src/algo.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -184,17 +184,17 @@ pub fn replace_children(
184184
/// to create a type-safe abstraction on top of it instead.
185185
pub fn replace_descendants(
186186
parent: &SyntaxNode,
187-
map: &FxHashMap<SyntaxElement, SyntaxElement>,
187+
map: &impl Fn(&SyntaxElement) -> Option<SyntaxElement>,
188188
) -> SyntaxNode {
189189
// FIXME: this could be made much faster.
190190
let new_children = parent.children_with_tokens().map(|it| go(map, it)).collect::<Vec<_>>();
191191
return with_children(parent, new_children);
192192

193193
fn go(
194-
map: &FxHashMap<SyntaxElement, SyntaxElement>,
194+
map: &impl Fn(&SyntaxElement) -> Option<SyntaxElement>,
195195
element: SyntaxElement,
196196
) -> NodeOrToken<rowan::GreenNode, rowan::GreenToken> {
197-
if let Some(replacement) = map.get(&element) {
197+
if let Some(replacement) = map(&element) {
198198
return match replacement {
199199
NodeOrToken::Node(it) => NodeOrToken::Node(it.green().clone()),
200200
NodeOrToken::Token(it) => NodeOrToken::Token(it.green().clone()),

0 commit comments

Comments
 (0)