Skip to content

Commit 3fdf26a

Browse files
Merge #7898
7898: generate_function assist: infer return type r=JoshMcguigan a=JoshMcguigan This PR makes two changes to the generate function assist: 1. Attempt to infer an appropriate return type for the generated function 2. If a return type is inferred, and that return type is not unit, don't render the snippet ```rust fn main() { let x: u32 = foo$0(); // ^^^ trigger the assist to generate this function } // BEFORE fn foo() ${0:-> ()} { todo!() } // AFTER (only change 1) fn foo() ${0:-> u32} { todo!() } // AFTER (change 1 and 2, note the lack of snippet around the return type) fn foo() -> u32 { todo!() } ``` These changes are made as two commits, in case we want to omit change 2. I personally feel like it is a nice change, but I could understand there being some opposition. #### Pros of change 2 If we are able to infer a return type, and especially if that return type is not the unit type, the return type is almost as likely to be correct as the argument names/types. I think this becomes even more true as people learn how this feature works. #### Cons of change 2 We could never be as confident about the return type as we are about the function argument types, so it is more likely a user will want to change that. Plus it is a confusing UX to sometimes have the cursor highlight the return type after triggering this assist and sometimes not have that happen. #### Why omit unit type? The assumption is that if we infer the return type as unit, it is likely just because of the current structure of the code rather than that actually being the desired return type. However, this is obviously just a heuristic and will sometimes be wrong. But being wrong here just means falling back to the exact behavior that existed before this PR. Co-authored-by: Josh Mcguigan <joshmcg88@gmail.com>
2 parents c484786 + b275e60 commit 3fdf26a

File tree

1 file changed

+67
-6
lines changed

1 file changed

+67
-6
lines changed

crates/ide_assists/src/handlers/generate_function.rs

Lines changed: 67 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,17 +83,18 @@ struct FunctionTemplate {
8383
leading_ws: String,
8484
fn_def: ast::Fn,
8585
ret_type: ast::RetType,
86+
should_render_snippet: bool,
8687
trailing_ws: String,
8788
file: FileId,
8889
}
8990

9091
impl FunctionTemplate {
9192
fn to_string(&self, cap: Option<SnippetCap>) -> String {
92-
let f = match cap {
93-
Some(cap) => {
93+
let f = match (cap, self.should_render_snippet) {
94+
(Some(cap), true) => {
9495
render_snippet(cap, self.fn_def.syntax(), Cursor::Replace(self.ret_type.syntax()))
9596
}
96-
None => self.fn_def.to_string(),
97+
_ => self.fn_def.to_string(),
9798
};
9899
format!("{}{}{}", self.leading_ws, f, self.trailing_ws)
99100
}
@@ -104,6 +105,8 @@ struct FunctionBuilder {
104105
fn_name: ast::Name,
105106
type_params: Option<ast::GenericParamList>,
106107
params: ast::ParamList,
108+
ret_type: ast::RetType,
109+
should_render_snippet: bool,
107110
file: FileId,
108111
needs_pub: bool,
109112
}
@@ -132,7 +135,43 @@ impl FunctionBuilder {
132135
let fn_name = fn_name(&path)?;
133136
let (type_params, params) = fn_args(ctx, target_module, &call)?;
134137

135-
Some(Self { target, fn_name, type_params, params, file, needs_pub })
138+
// should_render_snippet intends to express a rough level of confidence about
139+
// the correctness of the return type.
140+
//
141+
// If we are able to infer some return type, and that return type is not unit, we
142+
// don't want to render the snippet. The assumption here is in this situation the
143+
// return type is just as likely to be correct as any other part of the generated
144+
// function.
145+
//
146+
// In the case where the return type is inferred as unit it is likely that the
147+
// user does in fact intend for this generated function to return some non unit
148+
// type, but that the current state of their code doesn't allow that return type
149+
// to be accurately inferred.
150+
let (ret_ty, should_render_snippet) = {
151+
match ctx.sema.type_of_expr(&ast::Expr::CallExpr(call.clone())) {
152+
Some(ty) if ty.is_unknown() || ty.is_unit() => (make::ty_unit(), true),
153+
Some(ty) => {
154+
let rendered = ty.display_source_code(ctx.db(), target_module.into());
155+
match rendered {
156+
Ok(rendered) => (make::ty(&rendered), false),
157+
Err(_) => (make::ty_unit(), true),
158+
}
159+
}
160+
None => (make::ty_unit(), true),
161+
}
162+
};
163+
let ret_type = make::ret_type(ret_ty);
164+
165+
Some(Self {
166+
target,
167+
fn_name,
168+
type_params,
169+
params,
170+
ret_type,
171+
should_render_snippet,
172+
file,
173+
needs_pub,
174+
})
136175
}
137176

138177
fn render(self) -> FunctionTemplate {
@@ -145,7 +184,7 @@ impl FunctionBuilder {
145184
self.type_params,
146185
self.params,
147186
fn_body,
148-
Some(make::ret_type(make::ty_unit())),
187+
Some(self.ret_type),
149188
);
150189
let leading_ws;
151190
let trailing_ws;
@@ -171,6 +210,7 @@ impl FunctionBuilder {
171210
insert_offset,
172211
leading_ws,
173212
ret_type: fn_def.ret_type().unwrap(),
213+
should_render_snippet: self.should_render_snippet,
174214
fn_def,
175215
trailing_ws,
176216
file: self.file,
@@ -546,7 +586,7 @@ impl Baz {
546586
}
547587
}
548588
549-
fn bar(baz: Baz) ${0:-> ()} {
589+
fn bar(baz: Baz) -> Baz {
550590
todo!()
551591
}
552592
",
@@ -1059,6 +1099,27 @@ pub(crate) fn bar() ${0:-> ()} {
10591099
)
10601100
}
10611101

1102+
#[test]
1103+
fn add_function_with_return_type() {
1104+
check_assist(
1105+
generate_function,
1106+
r"
1107+
fn main() {
1108+
let x: u32 = foo$0();
1109+
}
1110+
",
1111+
r"
1112+
fn main() {
1113+
let x: u32 = foo();
1114+
}
1115+
1116+
fn foo() -> u32 {
1117+
todo!()
1118+
}
1119+
",
1120+
)
1121+
}
1122+
10621123
#[test]
10631124
fn add_function_not_applicable_if_function_already_exists() {
10641125
check_assist_not_applicable(

0 commit comments

Comments
 (0)