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

feat: Introduce term search to rust-analyzer #16092

Merged
merged 8 commits into from
Feb 12, 2024

Conversation

kilpkonn
Copy link
Contributor

@kilpkonn kilpkonn commented Dec 11, 2023

Introduce term search to rust-analyzer

I've marked this as draft as there might be some shortcomings, please point them out so I can fix them. Otherwise I think it is kind of ready as I think I'll rather introduce extra functionality in follow up PRs.

Term search (or I guess expression search for rust) is a technique to generate code by basically making the types match.
Consider the following program

fn wrap(arg: i32) -> Option<i32> {
    todo!();
}

From the types of values in scope and constructors of Option, we can produce the expected result of wrapping the argument in Option

Dependently typed languages such as Idris2 and Agda have similar tools to help with proofs, but this can be also used in everyday development as a "auto-complete".

Demo videos

2023-12-11_16-28-17.mp4
2023-12-11_15-41-30.mp4

What does it currently do

  • It works well with locals, free functions, type constructors and non-static impl methods that take items by value.
  • Works with functions/methods that take shared references, but not with unique references (very conservative).
  • Can handle projections to struct fields (eg. foo.bar.baz) but this might me more conservative than it has to be to avoid conflicting with borrow checker
  • Should create only valid programs (no type / borrow checking errors). Tested with rust-analyzer analysis-stats /path/to/ripgrep/Cargo.toml --run-term-search --validate-term-search (basically running cargo check on all of the generated programs and only error seems to be due to type inference which is more of issue of testing method.

Performace / fitness

ripgrep (latest)
Tail Expr syntactic hits: 130/1692 (7%)
Tail Exprs found: 523/1692 (30%)
Term search avg time: 9ms
Term search:         15.64s, 97ginstr, 8mb

rust-analyzer (on this branch)
Tail Expr syntactic hits: 804/13860 (5%)
Tail Exprs found: 6757/13860 (48%)
Term search avg time: 78ms
Term search:         1088.23s, 6765ginstr, 98mb

Highly generic code seems to blow up the search space so currently the amount of generics allowed is functions/methods is limited down to 0 (1 didn't give much improvement and 2 is already like 0.5+s search time)

Plans for the future (not in this PR)

  • ``Add impl methods that do not take self type (should be quite straight forward) Done
  • Be smarter (aka less restrictive) about borrow checking - this seems quite hard but since the current approach is rather naive I think some easy improvement is available.
  • ``See if it works as a autocomplete while typing Done

Feel free to ask questions / point of shortcoming either here or on Zulip, I'll be happy to address them. I'm doing this as part of my MSc thesis so I'll be working on it till summer anyway 😄

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 11, 2023
@kilpkonn kilpkonn force-pushed the term_search_1 branch 3 times, most recently from dc56fa6 to 0a912d0 Compare December 11, 2023 16:53
@Young-Flash
Copy link
Member

interesting, it would be nicer if there give a bit more docs for crates/hir/src/term_search/*.rs to help understand the idea and the implementation approach under the hood.

@kilpkonn
Copy link
Contributor Author

interesting, it would be nicer if there give a bit more docs for crates/hir/src/term_search/*.rs to help understand the idea and the implementation approach under the hood.

I've added some more documentation there. If you find some caps / stuff that doesn't make sense etc let me know as it definitely can be improved further 😄

@Veykril
Copy link
Member

Veykril commented Dec 12, 2023

Just a headsup but it will take some time (I'm potentially speaking months) until this gets reviewed as there is a massive backlog + me having little time atm + holidays coming up.

@kilpkonn
Copy link
Contributor Author

Just a headsup but it will take some time (I'm potentially speaking months) until this gets reviewed as there is a massive backlog + me having little time atm + holidays coming up.

That's unfortunate :( But thanks for the heads-up, appreciate it!

@kilpkonn kilpkonn changed the title Draft: Introduce term search to rust-analyzer Feat: Introduce term search to rust-analyzer Dec 23, 2023
@kilpkonn kilpkonn marked this pull request as ready for review December 23, 2023 13:23
@kilpkonn kilpkonn changed the title Feat: Introduce term search to rust-analyzer feat: Introduce term search to rust-analyzer Dec 23, 2023
@kilpkonn
Copy link
Contributor Author

kilpkonn commented Jan 8, 2024

I added also autocomplete part to this PR.
Demo can be seen here:

2024-01-08_21-26-53.mp4

Snippet is a little misleading `auto completion kind for it, but couldn't find any better :p

@kilpkonn kilpkonn force-pushed the term_search_1 branch 2 times, most recently from 8c0be25 to e860e1a Compare January 9, 2024 09:53
@Veykril
Copy link
Member

Veykril commented Jan 17, 2024

CI fails because Generator stuff was renamed to Coroutine (i'll try giving this PR a review this or next week)

@kilpkonn
Copy link
Contributor Author

kilpkonn commented Jan 17, 2024

CI fails because Generator stuff was renamed to Coroutine (i'll try giving this PR a review this or next week)

Yep, rebasing onto master now.
Would be awesome to get some feedback next week!

@kilpkonn kilpkonn force-pushed the term_search_1 branch 3 times, most recently from 232bc6d to a2bdd37 Compare January 17, 2024 14:14
@bors
Copy link
Contributor

bors commented Jan 18, 2024

☔ The latest upstream changes (presumably #16395) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Jan 27, 2024

☔ The latest upstream changes (presumably #16434) made this pull request unmergeable. Please resolve the merge conflicts.

@Veykril
Copy link
Member

Veykril commented Jan 27, 2024

Sorry I haven't gotten around to this yet, been somewhat sick last week.

@kilpkonn
Copy link
Contributor Author

Sorry I haven't gotten around to this yet, been somewhat sick last week.

No worries, hope your well now 😄
I've done the rebase so everything should be ready for review again

@kilpkonn kilpkonn force-pushed the term_search_1 branch 2 times, most recently from 21adceb to 23f37ab Compare January 28, 2024 19:32
crates/hir-def/src/attr.rs Show resolved Hide resolved
crates/hir-ty/src/infer/unify.rs Outdated Show resolved Hide resolved
crates/hir-ty/src/infer/unify.rs Outdated Show resolved Hide resolved
crates/hir-ty/src/infer/unify.rs Show resolved Hide resolved
crates/hir-ty/src/mir/borrowck.rs Outdated Show resolved Hide resolved
crates/ide-assists/src/handlers/term_search.rs Outdated Show resolved Hide resolved
crates/ide-completion/src/render.rs Outdated Show resolved Hide resolved
crates/ide-diagnostics/src/tests.rs Show resolved Hide resolved
crates/rust-analyzer/src/cli/flags.rs Outdated Show resolved Hide resolved
crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
@Veykril
Copy link
Member

Veykril commented Feb 12, 2024

Then let's merge this I guess :) Excited to try this out!
@bors r+

@bors
Copy link
Contributor

bors commented Feb 12, 2024

📌 Commit 1257913 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 12, 2024

⌛ Testing commit 1257913 with merge 818c30c...

@bors
Copy link
Contributor

bors commented Feb 12, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 818c30c to master...

@bors bors merged commit 818c30c into rust-lang:master Feb 12, 2024
11 checks passed
bors added a commit that referenced this pull request Feb 27, 2024
feat: Add "make tuple" tactic to term search

Follow up to #16092

Now term search also supports tuples.
```rust
let a: i32 = 1;
let b: f64 = 0.0;
let c: (i32, (f64, i32)) = todo!(); // Finds (a, (b, a))
```
In addition to new tactic that handles tuples I changed how the generics are handled.
Previously it tried all possible options from types we had in scope but now it only tries useful ones that help us directly towards the goal or at least towards calling some other function.
This changes O(2^n) to O(n^2) where n is amount of rounds which in practice allows using types that take generics for multiple rounds (previously limited to 1). Average case that also used to be exponential is now roughly linear.
This means that deeply nested generics also work.
````rust
// Finds all valid combos, including `Some(Some(Some(...)))`
let a: Option<Option<Option<bool>>> = todo!();
````

_Note that although the complexity is smaller allowing more types with generics the search overall slows down considerably. I hope it's fine tho as the autocomplete is disabled by default and for code actions it's not super slow. Might have to tweak the depth hyper parameter tho_

This resulted in a huge increase of results found (benchmarks on `ripgrep` crate):
Before
````
Tail Expr syntactic hits: 149/1692 (8%)
Tail Exprs found: 749/1692 (44%)
Term search avg time: 18ms
```
After
```
Tail Expr syntactic hits: 291/1692 (17%)
Tail Exprs found: 1253/1692 (74%)
Term search avg time: 139ms
````

Most changes are local to term search except some tuple related stuff on `hir::Type`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants