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

Find usages #1622

Closed
matklad opened this issue Jul 29, 2019 · 12 comments · Fixed by #1892
Closed

Find usages #1622

matklad opened this issue Jul 29, 2019 · 12 comments · Fixed by #1892
Labels
E-hard fun A technically challenging issue with high impact

Comments

@matklad
Copy link
Member

matklad commented Jul 29, 2019

We should support find usages (aka reference search).

This is a feature where IDE and compiler work differently. Compiler typically eagerly walks all code in top-down manner, and can collect usages along the way.

IDE should use a lazy bottom-up approach. So, to handle find usages, we need to implement classic trick of dong text search to find a superset of usages, and than pruning them with precise resolve.

Here's how find usages should work:

  1. First, given an element, we should compute SearchScope: an overopraximating filter on files that might contain references for this element. For things like local variables the search scope is just a function. for pub(crate) things it's a crate, and for pub things it's crate + dependant crates.

  2. Then, we should process all files from the search scopes and look for text occurrences of the identifier. For a first implementation, just going through each file will be good enough. Ideally, this should be speed up with a use of a trigram index.

  3. For each of the potential matches, we invoke "goto definition" to check if it is indeed a usage and not a random match

@matklad matklad added E-hard fun A technically challenging issue with high impact labels Jul 29, 2019
@umanwizard
Copy link
Contributor

How do you know for sure a text search will find all uses?

E.g. https://doc.rust-lang.org/std/macro.concat_idents.html

@matklad
Copy link
Member Author

matklad commented Jul 30, 2019

Two options:

  • search in macro expanded code as well
  • don't treat as a usage

It's unclear what would be the best for the user. Ideally, for macros, we should find usages in definition of a macro, but that's impossible because macro definitions are more or less opaque for IDEs.

@umanwizard
Copy link
Contributor

@matklad One more question - wouldn't it also work to build an index during (top-down) compilation/analysis ? I.e., every time a name is resolved, add the current source location as a "use" of that name. Then when the action is invoked, you could return that list of locations, doing no work.

I am not sure, but I think this is how cquery works.

@lnicola
Copy link
Member

lnicola commented Aug 4, 2019

I think that's basically what happens, but RA is lazy and only parses the files when it needs to. See #1650 for a discussion related to that.

@matklad
Copy link
Member Author

matklad commented Aug 5, 2019

@umanwizard top-down processing is O(total size of all files), while bottom up approach is O(number of usages). The latter is much more efficient if you search only for some names, and that's what IDEs typically do (all IntelliJ based ones, and TypeScript as well). Note that, even if you have a ton of usages (like for something like From::from), you typically show only the first n in the UI, so bottom up wins even in this case.

I'd be curious to learn more about how cquery does this. Is there any document that describes how it works?

@umanwizard
Copy link
Contributor

umanwizard commented Aug 5, 2019

Yes, building the initial index is O(total size of all files) and can take hours on a large enough codebase. Once the index is built however, looking things up in it is instant.

Showing only the first n references is not useful, IMO -- there should at least be a flag to turn on showing all possible references. Not sure about cquery, but clangd at least supports this (with an optional flag), and even things with thousands of references work fine and instantly.

@matklad
Copy link
Member Author

matklad commented Aug 5, 2019

The main problem with such an index is not that it takes long to build it, but that it is hard to keep it up-to-date as the code changes. I think it is somewhat easier to do in C++, where there is no dependencies between .cc files, but even there, chaining an .h file will require quite a bit of work.

Showing only the first n references is not useful, IMO -- there should at least be a flag to turn on showing all possible references.

I mean that the UI that shows the results should be paginated anyway: you can't show more references than there are pixels on the screen :) For a typical IDE usage, I find such streamed/paginated results are quite handy.

It's true that for purely code-browsing purposes, building a top-down index is a better approach, but I do feel that for IDE being able to react to code changes swiftly is more important than getting instant search results for references with huge number of usages.

@umanwizard
Copy link
Contributor

Thanks, I think I understand the difference in perspective better now. For me, the main value is in a tool for browsing static code -- I spend more time reading code and trying to understand it than I do editing. But I do understand your point of why a top-down approach is not ideal for the editing use-case.

@matklad
Copy link
Member Author

matklad commented Aug 5, 2019

@umanwizard for reading, specifically, there's a cool https://github.com/rust-dev-tools/cargo-src project.

@Geobert
Copy link
Contributor

Geobert commented Sep 6, 2019

Does this issue has any link with #907 ?

@matklad
Copy link
Member Author

matklad commented Sep 6, 2019

#907 is subset of this, yeah, but it should work close to how find usages for local variables works now, and not like the generic search infra described here.

@viorina
Copy link
Contributor

viorina commented Sep 6, 2019

I'd like to give it a go.

bors bot added a commit that referenced this issue Sep 19, 2019
1853: Introduce FromSource trait r=matklad a=viorina

The idea is to provide an ability to get HIR from AST in a more general way than it's possible using `source_binder`. 
It also could help with #1622 fixing.

Co-authored-by: Ekaterina Babshukova <ekaterina.babshukova@yandex.ru>
@viorina viorina mentioned this issue Sep 21, 2019
@bors bors bot closed this as completed in 4f4fe14 Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-hard fun A technically challenging issue with high impact
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants