-
Notifications
You must be signed in to change notification settings - Fork 2
Crate-level API changes for tree-sitter-lint #63
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
base: run-context
Are you sure you want to change the base?
Changes from 11 commits
b2c06c0
551e376
3d4682c
422058d
2118d3c
54b39ed
0602f3c
b05884b
d341747
1930ccf
3c2a567
5fe38b3
5a3ede1
0ad0481
c0ae26e
c73c3da
d8f21b0
3a039da
debbbc0
30a1c71
c0bf4ca
2eba2c5
e471b0d
672ac41
c5f3fa2
c371ae9
53e844f
57495f8
31959d1
2061904
6daf28a
6bb81e1
325d3e0
355ea2b
2e842eb
3b1230c
c5a8c85
d250c7c
c5b2e88
7626af8
526d110
d93d175
db220fe
7543d9d
045b98d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ use crate::{ | |
|
|
||
| const ALL_NODES_QUERY: &str = "(_) @node"; | ||
|
|
||
| #[derive(Parser)] | ||
| #[derive(Clone, Parser)] | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if the need to do |
||
| #[clap(group( | ||
| ArgGroup::new("query_or_filter") | ||
| .multiple(true) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,7 +32,8 @@ mod use_printer; | |
| mod use_searcher; | ||
|
|
||
| pub use args::Args; | ||
| use language::{BySupportedLanguage, SupportedLanguage}; | ||
| use language::BySupportedLanguage; | ||
| pub use language::SupportedLanguage; | ||
| pub use plugin::PluginInitializeReturn; | ||
| use query_context::QueryContext; | ||
| use treesitter::maybe_get_query; | ||
|
|
@@ -74,6 +75,8 @@ pub enum Error { | |
| FilterPluginExpectedArgument, | ||
| #[error("plugin couldn't parse argument {filter_arg:?}")] | ||
| FilterPluginCouldntParseArgument { filter_arg: String }, | ||
| #[error("language is required when passing a slice")] | ||
| LanguageMissingForSlice, | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Error)] | ||
|
|
@@ -290,9 +293,14 @@ pub fn run_print(args: Args) -> Result<RunStatus, Error> { | |
| ) | ||
| } | ||
|
|
||
| pub struct CaptureInfo<'node> { | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming? |
||
| pub node: Node<'node>, | ||
| pub pattern_index: usize, | ||
| } | ||
|
|
||
| pub fn run_with_callback( | ||
| args: Args, | ||
| callback: impl Fn(Node, &[u8], &Path) + Sync, | ||
| callback: impl Fn(CaptureInfo, &[u8], &Path) + Sync, | ||
| ) -> Result<RunStatus, Error> { | ||
| run_for_context( | ||
| args, | ||
|
|
@@ -307,8 +315,8 @@ pub fn run_with_callback( | |
| .search_path_callback::<_, io::Error>( | ||
| query_context, | ||
| path, | ||
| |node: Node, file_contents: &[u8], path: &Path| { | ||
| callback(node, file_contents, path); | ||
| |capture_info: CaptureInfo, file_contents: &[u8], path: &Path| { | ||
| callback(capture_info, file_contents, path); | ||
| matched.store(true, Ordering::SeqCst); | ||
| }, | ||
| ) | ||
|
|
@@ -412,6 +420,50 @@ fn run_for_context<TContext: Sync>( | |
| }) | ||
| } | ||
|
|
||
| pub fn run_for_slice_with_callback( | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This entry point is needed for the use case where |
||
| slice: &[u8], | ||
| args: Args, | ||
| mut callback: impl FnMut(CaptureInfo) + Sync, | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Vs the things that get passed to the callback in the |
||
| ) -> Result<RunStatus, Error> { | ||
| let language = args.language.ok_or(Error::LanguageMissingForSlice)?; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically I started thinking that maybe the slice itself could/should be a field on But that might be weird because then you'd have to sort of assert that "for this entry point we expect |
||
| let query_text = args.get_loaded_query_text()?; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't know if there are great ways to DRY this up more wrt other entry points, didn't worry about that too much at the moment |
||
| let filter = args.get_loaded_filter()?; | ||
| let cached_queries: CachedQueries = Default::default(); | ||
| let capture_index = CaptureIndex::default(); | ||
| let matched = AtomicBool::new(false); | ||
| let non_fatal_errors: Arc<Mutex<Vec<NonFatalError>>> = Default::default(); | ||
|
|
||
| let query = match cached_queries.get_and_cache_query_for_language(&query_text, language) { | ||
| Some(query) => query, | ||
| None => { | ||
| return Err(cached_queries | ||
| .error_if_no_successful_query_parsing() | ||
| .unwrap_err()) | ||
| } | ||
| }; | ||
| let capture_index = capture_index.get_or_init(&query, args.capture_name.as_deref())?; | ||
|
|
||
| let query_context = QueryContext::new(query, capture_index, language.language(), filter); | ||
|
|
||
| get_searcher(&args) | ||
| .borrow_mut() | ||
| .search_slice_callback_no_path(query_context, slice, |capture_info: CaptureInfo| { | ||
| callback(capture_info); | ||
| matched.store(true, Ordering::SeqCst); | ||
| }) | ||
| .unwrap(); | ||
|
|
||
| let non_fatal_errors = non_fatal_errors.lock().unwrap().clone(); | ||
| if non_fatal_errors.is_empty() { | ||
| cached_queries.error_if_no_successful_query_parsing()?; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may not make any sense/be redundant since above we already checked that "the query for the definitely-specified language is parseable" |
||
| } | ||
|
|
||
| Ok(RunStatus { | ||
| matched: matched.load(Ordering::SeqCst), | ||
| non_fatal_errors, | ||
| }) | ||
| } | ||
|
|
||
| fn for_each_project_file( | ||
| args: &Args, | ||
| non_fatal_errors: Arc<Mutex<Vec<NonFatalError>>>, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ use std::{ | |
| }; | ||
|
|
||
| use encoding_rs_io::DecodeReaderBytesBuilder; | ||
| use tree_sitter::{Node, QueryCursor}; | ||
| use tree_sitter::QueryCursor; | ||
|
|
||
| pub use self::mmap::MmapChoice; | ||
| use crate::{ | ||
|
|
@@ -19,6 +19,7 @@ use crate::{ | |
| searcher::glue::MultiLine, | ||
| sink::{Sink, SinkError}, | ||
| treesitter::get_parser, | ||
| CaptureInfo, | ||
| }; | ||
|
|
||
| mod core; | ||
|
|
@@ -218,7 +219,7 @@ impl Searcher { | |
| &mut self, | ||
| query_context: QueryContext, | ||
| path: P, | ||
| callback: impl Fn(Node, &[u8], &Path), | ||
| callback: impl Fn(CaptureInfo, &[u8], &Path), | ||
| ) -> Result<(), TError> | ||
| where | ||
| P: AsRef<Path>, | ||
|
|
@@ -338,7 +339,7 @@ impl Searcher { | |
| &mut self, | ||
| query_context: QueryContext, | ||
| slice: &[u8], | ||
| callback: impl Fn(Node, &[u8], &Path), | ||
| callback: impl Fn(CaptureInfo, &[u8], &Path), | ||
| path: &Path, | ||
| ) -> Result<(), ConfigError> { | ||
| self.check_config()?; | ||
|
|
@@ -349,11 +350,53 @@ impl Searcher { | |
| Ok(()) | ||
| } | ||
|
|
||
| pub fn search_slice_callback_no_path( | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "organization" of these searcher methods is probably weird but not like I 100% understand them in the first place Here there was already a So just made a separate |
||
| &mut self, | ||
| query_context: QueryContext, | ||
| slice: &[u8], | ||
| mut callback: impl FnMut(CaptureInfo), | ||
| ) -> Result<(), ConfigError> { | ||
| self.check_config()?; | ||
|
|
||
| log::trace!("slice reader: searching via multiline strategy"); | ||
| let mut query_cursor = QueryCursor::new(); | ||
| let tree = get_parser(query_context.language) | ||
| .parse(slice, None) | ||
| .unwrap(); | ||
| let query = &query_context.query; | ||
| let capture_index = query_context.capture_index; | ||
| let filter = &query_context.filter; | ||
| query_cursor | ||
| .captures(query, tree.root_node(), slice) | ||
| .filter_map(|(match_, index_into_query_match_captures)| { | ||
| let this_capture = &match_.captures[index_into_query_match_captures]; | ||
| if this_capture.index != capture_index { | ||
| return None; | ||
| } | ||
| let single_captured_node = this_capture.node; | ||
| match filter.as_ref() { | ||
| None => Some(CaptureInfo { | ||
| node: single_captured_node, | ||
| pattern_index: match_.pattern_index, | ||
| }), | ||
| Some(filter) => filter.call(&single_captured_node).then_some(CaptureInfo { | ||
| node: single_captured_node, | ||
| pattern_index: match_.pattern_index, | ||
| }), | ||
| } | ||
| }) | ||
| .for_each(|capture_info| { | ||
| callback(capture_info); | ||
| }); | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| fn run_with_callback( | ||
| &self, | ||
| query_context: QueryContext, | ||
| slice: &[u8], | ||
| callback: impl Fn(Node, &[u8], &Path), | ||
| callback: impl Fn(CaptureInfo, &[u8], &Path), | ||
| path: &Path, | ||
| ) { | ||
| let mut query_cursor = QueryCursor::new(); | ||
|
|
@@ -365,26 +408,25 @@ impl Searcher { | |
| let filter = &query_context.filter; | ||
| query_cursor | ||
| .captures(query, tree.root_node(), slice) | ||
| .filter_map(|(match_, found_capture_index)| { | ||
| let found_capture_index = found_capture_index as u32; | ||
| if found_capture_index != capture_index { | ||
| .filter_map(|(match_, index_into_query_match_captures)| { | ||
| let this_capture = &match_.captures[index_into_query_match_captures]; | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These changes (up through What I ran into was that my understanding of this (what was being called) Based on a little sanity-checking, it appears to be an index into the (not a "capture index" for the query as a whole) (maybe the But so this should get merged into |
||
| if this_capture.index != capture_index { | ||
| return None; | ||
| } | ||
| let mut nodes_for_this_capture = match_.nodes_for_capture_index(capture_index); | ||
| let single_captured_node = nodes_for_this_capture.next().unwrap(); | ||
| assert!( | ||
| nodes_for_this_capture.next().is_none(), | ||
| "I guess .captures() always wraps up the single capture like this?" | ||
| ); | ||
| let single_captured_node = this_capture.node; | ||
| match filter.as_ref() { | ||
| None => Some(single_captured_node), | ||
| Some(filter) => filter | ||
| .call(&single_captured_node) | ||
| .then_some(single_captured_node), | ||
| None => Some(CaptureInfo { | ||
| node: single_captured_node, | ||
| pattern_index: match_.pattern_index, | ||
| }), | ||
| Some(filter) => filter.call(&single_captured_node).then_some(CaptureInfo { | ||
| node: single_captured_node, | ||
| pattern_index: match_.pattern_index, | ||
| }), | ||
| } | ||
| }) | ||
| .for_each(|node| { | ||
| callback(node, slice, path); | ||
| .for_each(|capture_info| { | ||
| callback(capture_info, slice, path); | ||
| }); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,16 @@ | ||
| use std::{ | ||
| cell::{OnceCell, RefCell}, | ||
| ptr, | ||
| rc::Rc, | ||
| }; | ||
| use std::{cell::RefCell, collections::HashMap, rc::Rc}; | ||
|
|
||
| use crate::{searcher::Searcher, Args}; | ||
|
|
||
| thread_local! { | ||
| static SEARCHER: OnceCell<(Rc<RefCell<Searcher>>, *const Args)> = Default::default(); | ||
| static SEARCHER_PER_ARGS_INSTANCE: RefCell<HashMap<*const Args, Rc<RefCell<Searcher>>>> = Default::default(); | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd forgotten that we have now invalidated this invariant (but glad I was asserting it!) (that there's only ever once instance of So just made this a per- This same change should be made to the printer which is using the same pattern |
||
| } | ||
| pub(crate) fn get_searcher(args: &Args) -> Rc<RefCell<Searcher>> { | ||
| SEARCHER.with(|searcher| { | ||
| let (searcher, args_when_initialized) = | ||
| searcher.get_or_init(|| (Rc::new(RefCell::new(args.get_searcher())), args)); | ||
| assert!( | ||
| ptr::eq(*args_when_initialized, args), | ||
| "Using multiple instances of args not supported" | ||
| ); | ||
| searcher.clone() | ||
| SEARCHER_PER_ARGS_INSTANCE.with(|searcher_per_args_instance| { | ||
| searcher_per_args_instance | ||
| .borrow_mut() | ||
| .entry(args) | ||
| .or_insert_with(|| Rc::new(RefCell::new(args.get_searcher()))) | ||
| .clone() | ||
| }) | ||
| } | ||
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.
Per helixbass/tree-sitter-lint#4, updating this to allow publishing that crate while this hasn't "landed"