Skip to content
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

Merged
merged 2 commits into from Apr 3, 2020

Conversation

TimoFreiberg
Copy link
Contributor

@TimoFreiberg TimoFreiberg commented Mar 27, 2020

The function part of #3639, creating methods will come later

  • Function arguments
    • Function call arguments
    • Method call arguments
    • Literal arguments
    • Variable reference arguments
  • Migrate to ast::make API
    Done, 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<|>(..) or some_foo.foo<|>(..)) instead of create_function.
    The main difference would be finding (or creating) the impl block and inserting the self argument correctly

  • more 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 in lib.rs, I'd want to have foo generated in some_mod.rs and jump there.
    Issues: the mod could exist in some_mod.rs, in lib.rs as mod some_mod, or inside another mod but be imported via use 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.

@matklad
Copy link
Member

matklad commented Mar 27, 2020

It just occured to me that another plausible UI here is completion:

fn foo() {
     bar("", baz());
}

fn ba<|> // completes to bar(_: &str, _: ())

@lnicola
Copy link
Member

lnicola commented Mar 27, 2020

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.

@TimoFreiberg TimoFreiberg force-pushed the create_function_assist branch 3 times, most recently from 3ce6e67 to b630410 Compare March 28, 2020 14:31
@TimoFreiberg TimoFreiberg force-pushed the create_function_assist branch 3 times, most recently from 1ce72a7 to da2ea6c Compare March 29, 2020 15:19
@TimoFreiberg
Copy link
Contributor Author

TimoFreiberg commented Mar 30, 2020

I'd really like to get qualified paths and generics working in this PR, but I'm not sure how much is missing.
I might need some help (or another few days of digging through the codebase)

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 ;)

@flodiebold
Copy link
Member

I think it's always better to keep PRs small. These two issues won't be trivial to solve completely.

@TimoFreiberg
Copy link
Contributor Author

TimoFreiberg commented Mar 31, 2020

About the ast::make API: do I understand it correctly that it uses a roundtrip through the parsers to ensure that no invalid syntax is generated?

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 FnDef again, which seems quite convoluted.

Update: I had issues with prepending 2 newlines to the FnDef and indenting the first line.
I finally read the increase_indent method and of course, to make it indent the whole FnDef, it needs to actually have some whitespace in front of the first line. I don't see a way to add that to a FnDef, sooo... I'm storing it as a SourceFile now. I feel dirty.

// 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");
Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

Copy link
Member

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? 😅.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Member

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");

Copy link
Contributor Author

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

@TimoFreiberg TimoFreiberg changed the title WIP: Add create_function assist Add create_function assist Apr 2, 2020
Copy link
Member

@flodiebold flodiebold left a 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() {
Copy link
Member

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 🤔

@bors
Copy link
Contributor

bors bot commented Apr 3, 2020

@bors bors bot merged commit 77462bb into rust-lang:master Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants