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

Filter imports on find-all-references #13186

Merged
merged 12 commits into from
Sep 12, 2022
Merged

Filter imports on find-all-references #13186

merged 12 commits into from
Sep 12, 2022

Conversation

enomado
Copy link
Contributor

@enomado enomado commented Sep 4, 2022

Attempt to #13184

@lnicola
Copy link
Member

lnicola commented Sep 4, 2022

I'm not sure we want this by default. For example, "Find usages" on a trait should probably include the use statements, since our trait solving is still quite imprecise (though I'm not sure how well that works at the moment).

Co-authored-by: Laurențiu Nicola <lnicola@users.noreply.github.com>
@enomado
Copy link
Contributor Author

enomado commented Sep 4, 2022

I'm not sure we want this by default. For example, "Find usages" on a trait should probably include the use statements, since our trait solving is still quite imprecise (though I'm not sure how well that works at the moment).

Its okay. Its my first attempt. It may be configurable. I'll use my own fork, at the worst.

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should definitely have a config option that is disabled by default (ideally it wouldn't be a config but a client side filter for the results, but I don't think thats possible yet)

See https://github.com/rust-lang/rust-analyzer/blob/master/crates/rust-analyzer/src/config.rs, as for the config key, something like references_include_use should be good I suppose

crates/ide/src/references.rs Outdated Show resolved Hide resolved
crates/ide/src/references.rs Outdated Show resolved Hide resolved
@enomado
Copy link
Contributor Author

enomado commented Sep 7, 2022

So. I have added config and tested on a few cases.

@enomado enomado changed the title Retain imports on find-all-references Filter imports on find-all-references Sep 8, 2022
@enomado
Copy link
Contributor Author

enomado commented Sep 8, 2022

Could somebody assist me? my cargo test panics locally on some ABI stuff. but CI wants me to commit generated files

    NOTE: run `cargo test` locally and commit the updated files

thread 'config::tests::generate_config_documentation' panicked at 'Some files were not up-to-date', crates/rust-analyzer/src/config.rs:2035:9

@lnicola
Copy link
Member

lnicola commented Sep 8, 2022

Try cargo test -p rust-analyzer, then commit the modified files.

@enomado
Copy link
Contributor Author

enomado commented Sep 9, 2022

ready

crates/ide/src/references.rs Outdated Show resolved Hide resolved
crates/rust-analyzer/src/config.rs Outdated Show resolved Hide resolved
@enomado
Copy link
Contributor Author

enomado commented Sep 9, 2022

ready №2

@Veykril
Copy link
Member

Veykril commented Sep 12, 2022

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Sep 12, 2022

📌 Commit f7f4792 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 12, 2022

⌛ Testing commit f7f4792 with merge e38dfe5...

@bors
Copy link
Contributor

bors commented Sep 12, 2022

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

@bors bors merged commit e38dfe5 into rust-lang:master Sep 12, 2022
@matklad
Copy link
Member

matklad commented Sep 12, 2022

@Veykril note that the original intended design here is a bit different: filtering should be done in 2proto module (that is, we always return all usages, and filter out imports near the edge)

This comes from perfect client thinking, which client would just classify usages in its UI (eg, it’ll show “9001 usages in imports” which is collapsed by default).

don’t remember exactly, but there should be some kind of Class filed on a usage which should classify it into ctor/dtor/read/write/use/comment.

@Veykril
Copy link
Member

Veykril commented Sep 12, 2022

Yes, I realized that the filtering should (and could) happen in the proto layer instead after merging as well. Meant to change that but didn't find the time today. There is a classification for this of some sort (which is very bare bones still I believe)

bors added a commit that referenced this pull request Sep 13, 2022
Move reference imports filtering into to_proto layer

Follow up to #13186
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.

5 participants