Skip to content
This repository was archived by the owner on Mar 1, 2019. It is now read-only.

Conversation

@matklad
Copy link
Contributor

@matklad matklad commented Jun 26, 2018

WIP to get the feedback on the general idea.

This switches from prefix_trie to fst by @BurntSushi, which allows for much more powerful API:

  • search symbols either by prefix or by subsequence. This should fix the problem that ctrl+T > vfs in VS Code does not find range_from_vfs_file.

  • limit the number of results returned. This should fix the problem of VS Code freezing after pressing ctrl+T and not typing a query string (which currently retrieves all of the symbols)

  • support for streaming API. Now it is possible to get a huge list of symbols in small chunks, so that symbol search in the editor can give results instantly, and then load more results as user scrolls the list. Note that LSP/VS Code API in this area is suboptimal: symbol search returns a Future<Vec<Symbol>> and not a Stream<Symbol>. Hopefully that will be fixed one day.

matklad added 7 commits June 26, 2018 21:21
* support for subsequence match (useful for fuzzy-search in IDEs)

* support for pagination: you can limit the number of results and
  resume searching from the last result
@matklad
Copy link
Contributor Author

matklad commented Jun 26, 2018

BTW, VS Code docs explicitly advice against using prefix-match :)

A good rule of thumb is to match case-insensitive and to simply check that the characters of query appear in their order in a candidate symbol. Don't use prefix, substring, or similar strict matching.

https://code.visualstudio.com/docs/extensionAPI/vscode-api#WorkspaceSymbolProvider

Copy link
Contributor

@nrc nrc left a comment

Choose a reason for hiding this comment

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

Thanks! This looks like a good move.

src/analysis.rs Outdated
pub children: HashMap<Id, HashSet<Id>>,
pub def_names: HashMap<String, Vec<Id>>,
pub def_trie: Trie<String, Vec<Id>>,
// pub def_trie: Trie<String, Vec<Id>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove this rather than comment it out please?

src/fstq.rs Outdated
@@ -0,0 +1,82 @@
use fst;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add some comments explaining what this module is for?

src/analysis.rs Outdated
}

pub enum MatchingDefsQueryKind {
Preifx(String),
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: Prefix

src/analysis.rs Outdated
})
}

pub fn matching_defs_q(&self, query: MatchingDefsQuery) -> Vec<Def> {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/_q/_query please

Copy link
Contributor

Choose a reason for hiding this comment

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

or _with_query

@matklad
Copy link
Contributor Author

matklad commented Jun 27, 2018

Cleaned-up the implementation, should be ready for the review now

@matklad matklad force-pushed the burntsushis-work-is-awesome branch from 9c81276 to b4d15bb Compare June 27, 2018 12:03
@matklad
Copy link
Contributor Author

matklad commented Jun 27, 2018

Might be a good idea to cut a release after this PR.

Given that we are allocating in the `.to_lowercase` anyway, there's no
need to accept an `Into`.
@nrc nrc merged commit 701f4a6 into rust-dev-tools:master Jun 27, 2018
@nrc
Copy link
Contributor

nrc commented Jun 27, 2018

Thanks!

@nrc
Copy link
Contributor

nrc commented Jun 27, 2018

Publishd 0.14.0

@matklad matklad deleted the burntsushis-work-is-awesome branch June 28, 2018 20:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants