8000 Add support for excluding imports from symbol search by LHolten · Pull Request #19996 · rust-lang/rust-analyzer · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add support for excluding imports from symbol search #19996

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

Merged
merged 1 commit into from
Jun 15, 2025

Conversation

LHolten
Copy link
Contributor
@LHolten LHolten commented Jun 13, 2025

This implements the feature proposed in #19976

@LHolten LHolten marked this pull request as ready for review June 13, 2025 16:03
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 13, 2025
@LHolten
Copy link
Contributor Author
LHolten commented Jun 13, 2025

Should I add a test somewhere?

@ChayimFriedman2
Copy link
Contributor

Yes, please add a test.

The place to add the test will be crates/ide-db/src/symbol_index.rs. Call world_symbols() with exclude_imports set to true, and assert the result is equal to a file.

@LHolten
Copy link
Contributor Author
LHolten commented Jun 14, 2025

I added a test using world_symbols.
Not sure why RootDatabase::with_many_files doesn't have local roots by default, maybe I should have used something else?

@ChayimFriedman2
Copy link
Contributor

What do you mean by "local roots"?

@ChayimFriedman2
Copy link
Contributor

Ah, I see now. Probably because it wasn't needed. You can leave it as-is or set Query::libs.

@LHolten
Copy link
Contributor Author
LHolten commented Jun 15, 2025

Ah, I see now. Probably because it wasn't needed. You can leave it as-is or set Query::libs.

Query::libs doesn't work, it still doesn't find anything with that.

@ChayimFriedman2
Copy link
Contributor

Can you squash? 5 commits is a lot.

@LHolten
Copy link
Contributor Author
LHolten commented Jun 15, 2025

yes ofc, do you want one commit for everything?

@ChayimFriedman2
Copy link
Contributor

Yes.

@LHolten
Copy link
Contributor Author
LHolten commented Jun 15, 2025

done

@ChayimFriedman2
Copy link
Contributor

Thanks!

@ChayimFriedman2 ChayimFriedman2 added this pull request to the merge queue Jun 15, 2025
Merged via the queue into rust-lang:master with commit 4d30c53 Jun 15, 2025
15 checks passed
@joshka
Copy link
Contributor
joshka commented Jun 16, 2025

Post-review comment. This seems a bit under-documented and ambiguous as to what it does. I think it handles not selecting the "wrong" imports from re-exported values, but I have no idea what that criteria is (even having read the associated issue).

Consider a follow up PR that adds docs that describe the problem and what the setting does to address it, include examples etc. Consider adding some of this to the first PR comment here too. (I got here via the changelog).

@LHolten
Copy link
Contributor Author
LHolten commented Jun 20, 2025

Maybe a small table helps clarify

  • use foo::Foo is a regular import which will never show up in symbol search.
  • use foo::Foo as Bar is a renaming import which is shown in symbol search by default.
  • pub use foo::Foo is a public import (also known as re-export), this is also shown in symbol search by default.
    (pub(crate), pub(super) etc. are treated like pub).

Note that the above rules were not modified by this feature PR at all.

This PR adds a new config option rust-analyzer.workspace.symbol.search.excludeImports.
When this config option is enabled, it excludes ALL imports from workspace symbol search.
Since regular imports were already excluded, it just excludes the public ones and renaming ones too.

Personally I think the default behaviour might be a bit confusing, but the feature in this PR is really simple and doesn't need much documentation.

@joshka
Copy link
Contributor
joshka commented Jun 20, 2025

Got it, thanks for the details here. My confusion was over an assumption that only re-exports were to be excluded from the linked issue, and hence the naming of the option, and the description not mentioning re-exports seemed weird. That this PR excluded renames as well was the missing part of the puzzle that made it make sense.

I'd encourage adding that wording to the description, as it will make the option easier to search for in VSCode settings (e.g. someone searching for re-export, or rename), and clarifies the behavior better than the short description currently there. (note docs.rs uses the terminology "re-export", I'd assume that term is probably more familiar to many than "public import").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0