-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: enable excluding refs search results in test #16441
Conversation
I look a bit more about the latency in rust-analyzer server LSP log trace, compared the
|
@bors r+ |
☀️ Test successful - checks-actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see this! Supporting this feature is exactly why the search API was designed as it was, with reference categories and such!
@@ -1055,6 +1055,7 @@ pub(crate) fn handle_references( | |||
let position = from_proto::file_position(&snap, params.text_document_position)?; | |||
|
|||
let exclude_imports = snap.config.find_all_refs_exclude_imports(); | |||
let exclude_tests = snap.config.find_all_refs_exclude_tests(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exclude_tests should be pushed down as a parameter to find_all_refs.
that way, a potentially more optimal implementation would be able to avoid even analyzing the test, which would be faster than filtering out results after the fact.
I think the second parameter we are passing here, which is currently None, is something like a SearchScope. It exists exactly to narrow down search results, and I think could be extended to exclude tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, I had create an issue for this
(sorry for the slow response, kind of busy recently)
@@ -134,6 +134,7 @@ pub enum ReferenceCategory { | |||
// FIXME: Some day should be able to search in doc comments. Would probably | |||
// need to switch from enum to bitflags then? | |||
// DocComment | |||
Test, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something can be a test and a read at the same time. So, the time has come to use a bitset here! This is precisely the situation the old comment just above this line talks about
Change
Here I introduce a new
ReferenceCategory::Test
type to indicate whether the function where this reference is located is marked as#[test]
, and expose an config item (rust-analyzer.references.excludeTests
) to client.I also changed the signature of
ReferenceCategory::new
, adding asema: &Semantics<'_, RootDatabase>
param to do some hir analysis. Hope the current implementation is good to go.Demo
"rust-analyzer.references.excludeTests": false
"rust-analyzer.references.excludeTests": true
Performance
I look a bit more about the latency in rust-analyzer server LSP log trace, compared the
Find All References
between v0.4.1825 and this PR oversyntax::ast::AstNode::syntax()
andide_assists::tests::check_assist()
, taking an average of 5 operations response time (every operation is perform after restart rust-analyzer server so it's not the cache reslut). I found this PR does't bring noticable regression.close #14530