Skip to content

Assist: create function stub #3639

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

Open
lnicola opened this issue Mar 18, 2020 · 2 comments
Open

Assist: create function stub #3639

lnicola opened this issue Mar 18, 2020 · 2 comments
Labels
A-assists E-hard fun A technically challenging issue with high impact S-actionable Someone could pick this issue up and work on it right now

Comments

@lnicola
Copy link
Member

lnicola commented Mar 18, 2020

E.g.

x.foo(bar, qux(), qux());
// ->
// impl X {
// ...
fn foo(&self, bar: Bar, qux_1: Qux, qux_2: Qux) {
    todo!();
}
// ...
// }
@matklad matklad added E-hard fun A technically challenging issue with high impact labels Mar 18, 2020
@lnicola lnicola changed the title Assists: create function stub Assist: create function stub Mar 18, 2020
bors bot added a commit that referenced this issue Apr 3, 2020
3746: Add create_function assist r=flodiebold a=TimoFreiberg

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

- [X] Function arguments
     - [X] Function call arguments
     - [x] Method call arguments
     - [x] Literal arguments
     - [x] Variable reference arguments
- [X] 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 #3746 (comment))
    Issues: see #3746 (comment). The unresolved call could be anywhere. But just offering this autocompletion for unresolved calls in the same module would already be cool.

Co-authored-by: Timo Freiberg <[email protected]>
@TimoFreiberg
Copy link
Contributor

TimoFreiberg commented Apr 3, 2020

Adding basic function (not method) stubs works now.

Next steps (in a rough cost/benefit priority order, off the top of my head):

  • [Done! Make add_function generate functions in other modules via qualified path #3998] 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.

  • [Done Introduce HirDisplay method for rendering source code & use it in add_function assist #4175] refer to arguments of the generated function with a qualified path if the types aren't imported yet

  • make sure the generated names are always snake_case
    Should be quite easy, important to make it not feel janky.

  • function reference/closure arguments: Their type isn't printed properly right now.
    The "insert explicit type" assist has the same issue.
    Not sure how hard, important for completeness.

  • add type arguments of the arguments to the generated function
    (generating foo via foo(bar()) with fn bar<T>(...) -> T should generate fn foo<T>(bar: T). Right now it generates fn foo(bar: T))
    Not sure how hard, important for completeness.

  • 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
    Medium difficulty I guess, important for completeness, .

  • 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.
    Should be quite easy (may be a slog though), medium importance to make it not feel janky.

  • (fixed via Support snippet text edit #4494) select the generated placeholder body so it's a bit easier to overwrite it
    The WIP snippets implementation in coc-rust-analyzer properly selects the generated todo!()

  • Refine placement of the generated function.
    Currently it's placed behind the last element of the surrounding module/file.
    That means it's after test modules etc, which is probably not what the developer wants.
    We would need a "most sensible place for a new function" heuristic.
    Not sure how hard, probably not easy. In very big files this feature might make a big ergonomics difference.

  • 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).

  • When using the name of the passed argument, nested expressions should probably be ignored
    Example: when generating foo out of foo(Some('x').map(|it| it.toUpper())), the arg name should rather be map than to_upper.

  • Generate only the minimum necessary whitespace around the function.
    In the initial version, the generated function is inserted with newlines before (and in Make add_function generate functions in other modules via qualified path #3998, sometimes also newlines after). If there is already enough whitespace at the placement spot, it would be cleaner to do something so the generated function doesn't end with 3 empty lines behind it.

  • Offer the create_function assist for other function references than calls.
    Example: some_iterator.map(foo<|>) should offer to create foo if it doesn't exist and infer the expected parameters.

  • 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.

@lnicola
Copy link
Member Author

lnicola commented Apr 3, 2020

generating proper names for all kinds of argument expressions (if, loop, ...?)

I think naming them based on the type would work better, but there still are some corner cases (e.g. tuples and unnameable types).

bors bot added a commit that referenced this issue Apr 24, 2020
3998: Make add_function generate functions in other modules via qualified path r=matklad a=TimoFreiberg

Additional feature for #3639 

- [x] Add tests for paths with more segments
- [x] Make generating the function in another file work
- [x] Add `pub` or `pub(crate)` to the generated function if it's generated in a different module
- [x] Make the assist jump to the edited file
- [x] Enable file support in the `check_assist` helper

4006: Syntax highlighting for format strings r=matklad a=ltentrup

I have an implementation for syntax highlighting for format string modifiers `{}`.
The first commit refactors the changes in #3826 into a separate struct.
The second commit implements the highlighting: first we check in a macro call whether the macro is a format macro from `std`. In this case, we remember the format string node. If we encounter this node during syntax highlighting, we check for the format modifiers `{}` using regular expressions.

There are a few places which I am not quite sure:
- Is the way I extract the macro names correct?
- Is the `HighlightTag::Attribute` suitable for highlighting the `{}`?

Let me know what you think, any feedback is welcome!

Co-authored-by: Timo Freiberg <[email protected]>
Co-authored-by: Leander Tentrup <[email protected]>
Co-authored-by: Leander Tentrup <[email protected]>
bors bot added a commit that referenced this issue May 9, 2020
4175: Introduce HirDisplay method for rendering source code & use it in add_function assist r=flodiebold a=TimoFreiberg

Next feature for #3639.

So far the only change in the new `HirDisplay` method is that paths are qualified, but more changes will be necessary (omitting the function name from function types, returning an error instead of printing `"{unknown}"`, probably more).

Is that approach okay?

Co-authored-by: Timo Freiberg <[email protected]>
@lnicola lnicola added the S-actionable Someone could pick this issue up and work on it right now label Jan 22, 2021
bors bot added a commit that referenced this issue Mar 6, 2021
7894: generate_function assist: convert arg names to lower snake case r=Veykril a=JoshMcguigan

This PR fixes one of the points listed by @TimoFreiberg in #3639. Specifically that all generated argument names should be converted to lower snake case. 

```rust
struct BazBaz;

fn foo() {
    bar$0(BazBaz);
    // ^ when triggering the assist here, you get the output below
}

// BEFORE
fn bar(BazBaz: BazBaz) ${0:-> ()} {
    todo!()
}

// AFTER
fn bar(baz_baz: BazBaz) ${0:-> ()} {
    todo!()
}
```

Co-authored-by: Josh Mcguigan <[email protected]>
bors added a commit that referenced this issue Feb 2, 2023
Support generic function in `generate_function` assist

Part of #3639

This PR adds support for generic function generation in `generate_function` assist. Now the assist looks for generic parameters and trait bounds in scope, filters out irrelevant ones, and generates new function with them.

See `fn_generic_params()` for the outline of the procedure, and see comments on `filter_unnecessary_bounds()` for criteria for filtering. I think it's good criteria for most cases, but I'm open to opinions and suggestions.

The diff is pretty big, but it should run in linear time w.r.t. the number of nodes we operate on and should be fast enough.

Some notes:
- When we generate function in an existing impl, generic parameters may cause name conflict. While we can detect the conflict and rename conflicting params, I didn't find it worthwhile mainly because it's really easy to resolve on IDE: use Rename functionality.
- I've implemented graph structure myself, because we don't have graph library as a dependency and we only need the simplest one.
  - Although `petgraph` is in our dependency graph and I was initially looking to use it, we don't actually depend on it AFAICT since it's only used in chalk's specialization graph handling, which we don't use. I'd be happy to replace my implementation with `petgraph` if it's okay to use it though.
- There are some caveats that I consider out of scope of this PR. See FIXME notes on added tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-assists E-hard fun A technically challenging issue with high impact S-actionable Someone could pick this issue up and work on it right now
Projects
None yet
Development

No branches or pull requests

4 participants