Add ArkDb, ArkFile, and tree_sitter() query for legacy handlers#1263
Conversation
| // Don't run oak (salsa) queries inside `f`. `f` executes on the R thread, and a | ||
| // `salsa::Cancelled` unwind there would cross R's C frames, which is UB. Pull | ||
| // whatever you need out of oak before the `r_task()`, on the calling thread. |
There was a problem hiding this comment.
😬, no way to guard against this i guess?
| pub(crate) trait FileExt { | ||
| fn tree_sitter(self, db: &dyn ArkDb) -> &tree_sitter::Tree; | ||
| } | ||
|
|
||
| impl FileExt for File { |
There was a problem hiding this comment.
| pub(crate) trait FileExt { | |
| fn tree_sitter(self, db: &dyn ArkDb) -> &tree_sitter::Tree; | |
| } | |
| impl FileExt for File { | |
| pub(crate) trait FileTreeSitterExt { | |
| fn tree_sitter(self, db: &dyn ArkDb) -> &tree_sitter::Tree; | |
| } | |
| impl FileTreeSitterExt for File { |
?
idk how i feel about having uninformative FileExt
There was a problem hiding this comment.
would also probably be fine with just exposing tree_sitter_query as tree_sitter directly and not having it as a method
There was a problem hiding this comment.
or more like FileArkExt? It's not necessarily about tree-sitter (although it now is)
| let mut actions = CodeActions::new(); | ||
|
|
||
| roxygen_documentation(&mut actions, uri, document, range, capabilities); | ||
| roxygen_documentation(db, file, &mut actions, range, capabilities); |
There was a problem hiding this comment.
I did like having &mut actions up front
There was a problem hiding this comment.
That makes sense too, but context params go first by convention.
| pub(crate) version: Option<i32>, | ||
| pub(crate) config: DocumentConfig, | ||
| pub(crate) url: Url, | ||
| pub(crate) encoding: PositionEncoding, |
There was a problem hiding this comment.
hoping this encoding field goes away entirely, see a review of a previous pr that i left
There was a problem hiding this comment.
ArkFile in general goes away entirely once we update Ark handlers to Oak layering
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { |
There was a problem hiding this comment.
do we really want tests in main_loop.rs?
| fn test_line_indent_oob() { | ||
| let doc = test_doc(""); | ||
| assert_match!(indent_edit(&doc, 1), Err(_)); | ||
| let (db, ark_file) = crate::lsp::ark_file::test_ark_file(""); |
There was a problem hiding this comment.
can you just use crate::lsp::ark_file to make this cleaner?
There was a problem hiding this comment.
(here and in all other files)
There was a problem hiding this comment.
If you ask me I don't think we should spend much time on test styling.
| pub(crate) db: &'a dyn ArkDb, | ||
| pub(crate) file: &'a ArkFile, | ||
|
|
||
| /// The symbols currently defined and available in the session. | ||
| pub session_symbols: HashSet<String>, |
There was a problem hiding this comment.
Use consistent pub and spacing and comments as in the rest of the struct?
| let url = url::Url::parse("file:///test.R").unwrap(); | ||
| let file = oak_db::File::new(&state.db, FilePath::from_url(&url), code.to_string(), None); | ||
| let ark_file = ArkFile { | ||
| file, | ||
| version: None, | ||
| config: DocumentConfig::default(), |
There was a problem hiding this comment.
is this not your test_ark_file() thing?
There was a problem hiding this comment.
hmm i guess not because WorldState is required
i wonder if you can get rid of current_state()? or at least this console_inputs() thing in favor of just empty vectors for console_scopes and installed_packages? not sure. if you could do that, you could probably remove the r_task() from all of these tests
There was a problem hiding this comment.
I'm not sure I want to spend too much time on this right now
lionel-
force-pushed
the
oak/salsa-22-testthat
branch
from
79df227 to
cac4c04
Compare
lionel-
force-pushed
the
oak/salsa-23-ark-file
branch
from
c3744b4 to
bac9a5f
Compare
lionel-
force-pushed
the
oak/salsa-22-testthat
branch
from
17a154b to
2ebe7c4
Compare
lionel-
force-pushed
the
oak/salsa-23-ark-file
branch
from
820ae96 to
0e8e3d5
Compare
2 participants
Branched from #1260
Progress towards #1212
This PR starts migrating legacy Ark handlers to the Oak database so that we get one source of truth and remove redundancy of representations (source, syntax trees) in the LSP to save on memory.
Introduces
ArkFilein the process. Note this is meant as a convenient temporary vessel until we fully migrate handlers to Oak.Add
ArkFilewhich bundles an OakFilewith some metadata used by Ark handlers (version, wireurl,encoding). This is meant to replaceDocumentin a subsequent PR. The LSP <> TS position converters are now methods onArkFile(previously onDocument).Add
ArkDbandFileExtas extension traits for Oak.The main addition is a tree-sitter query, capped at 128 trees (like the Rowan trees).
spawn_blocking()now catches Salsa cancellations when reads on backaground tasks (e.g. diagnostics) are cancelled by a concurrent write.Start migrating a bunch of handlers to that new Oak infra.