-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add create_function assist #3746
Conversation
cc516e6
to
c9148c1
Compare
It just occured to me that another plausible UI here is completion:
|
That seems a bit non-local. Instead of adding the function under the cursor to its proper scope, you need to look for unresolved function calls in all other scopes when typing a new function. It seems harder. |
3ce6e67
to
b630410
Compare
1ce72a7
to
da2ea6c
Compare
da2ea6c
to
5c618b6
Compare
I'd really like to get qualified paths and generics working in this PR, but I'm not sure how much is missing. For what it's worth, I find the assist in its existing state to be very helpful already, so I'd be easily convinced to handle those two issues in another PR as well ;) |
I think it's always better to keep PRs small. These two issues won't be trivial to solve completely. |
5c618b6
to
a4598bb
Compare
About the Trying to use it instead of building the string directly made it a bit harder for me to place the cursor in the function body though. The only way I see is to search through the AST descendants of the Update: I had issues with prepending 2 newlines to the |
// We take the offset here to put the cursor in front of the `todo` body | ||
let offset = TextUnit::of_str(&fn_buf); | ||
|
||
fn_buf.push_str("todo!()\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo!()
here breaks the tidy test. i can just use unimplemented!()
instead, but todo!()
seems slightly more fitting...
I switched to unimplemented!()
for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm questioning the placeholder now. Intellij leaves the body empty.
If you just want to start typing something, the assist will always put something in your way first, which might get annoying.
Should I just leave the body empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be asking for too much, but can we insert and select the placeholder, so that the user can overwrite it directly? 😅.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like that, I haven't found a way to do that yet though. I'm looking at the extend_selection
code for inspiration and I'm not finding anything I can use in the assist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would need some changes in the top-level assist handler, so far it looks like it only handles inserts and deletes.
Maybe we should move this to another PR as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo!() here breaks the tidy test.
- fn_buf.push_str("todo!()\n");
+ fn_buf.push_str("tod");
+ fn_buf.push_str("o!()\n");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately, we can't do this workaround in the doctest
a4598bb
to
1066775
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
bors r+
let call = path_expr.syntax().parent().and_then(ast::CallExpr::cast)?; | ||
let path = path_expr.path()?; | ||
|
||
if path.qualifier().is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could allow adding functions to other modules in the future as well 🤔
Build succeeded |
The function part of #3639, creating methods will come later
ast::make
APIDone, but there are some ugly spots.
Issues to handle in another PR:
function reference arguments: Their type isn't printed properly right now.
The "insert explicit type" assist has the same issue and this is probably a relatively rare usecase.
generating proper names for all kinds of argument expressions (if, loop, ...?)
Without this, it's totally possible for the assist to generate invalid argument names.
I think the assist it's already helpful enough to be shipped as it is, at least for me the main usecase involves passing in named references.
Besides, the Rust tooling ecosystem is immature enough that some janky behaviour in a new assist probably won't scare anyone off.
select the generated placeholder body so it's a bit easier to overwrite it
create method (
self.foo<|>(..)
orsome_foo.foo<|>(..)
) instead of create_function.The main difference would be finding (or creating) the impl block and inserting the
self
argument correctlymore specific default arg names for literals.
So far, every generated argument whose name can't be taken from the call site is called
arg
(with a number suffix if necessary).creating functions in another module of the same crate.
E.g. when typing
some_mod::foo<|>(...)
when inlib.rs
, I'd want to havefoo
generated insome_mod.rs
and jump there.Issues: the mod could exist in
some_mod.rs
, inlib.rs
asmod some_mod
, or inside another mod but be imported viause other_mod::some_mod
.refer to arguments of the generated function with a qualified path if the types aren't imported yet
(alternative: run autoimport. i think starting with a qualified path is cleaner and there's already an assist to replace a qualified path with an import and an unqualified path)
add type arguments of the arguments to the generated function
Autocomplete functions with information from unresolved calls (see Add create_function assist #3746 (comment))
Issues: see Add create_function assist #3746 (comment). The unresolved call could be anywhere. But just offering this autocompletion for unresolved calls in the same module would already be cool.