Skip to content

Goto definition speedup#509

Merged
jalvesaq merged 8 commits intomainfrom
goto-definition-speedup
Mar 1, 2026
Merged

Goto definition speedup#509
jalvesaq merged 8 commits intomainfrom
goto-definition-speedup

Conversation

@PMassicotte
Copy link
Collaborator

  • srcref_ cache (R side): Added nvim.build.srcref() in bol.R to pre-build a per-package cache (srcref_<pkg>_<version>) that maps functions to their source file, line, and column. Gets built at the same time as the objls_/args_ caches.

  • New definition.c module (C side): Goto-definition now lives in rnvimserver under a new 'G' command. It hits the in-memory srcref_ cache first, and only falls back to a live R call (nvimcom:::send_definition) if it doesn't find anything.

  • PkgData struct: Added a srcref field to load the srcref_ file into memory, same way objls_ and args_ are handled.

  • Lua cleanup: Dropped find_in_package() and handle_definition_response() from definition.lua. The handler just sends a 'G' to rnvimserver now, and it works as soon as R_Nvim_status >= 2 — no need to wait for R to be fully up (status 7) like before.

  • Formatting: Ran .clang-format on the C files. This changed code formatting in only 2-3 files.

…p functions (closes #508)

feat(bol.R): add source reference cache building for package functions

This change introduces a new function `nvim.build.srcref` to build
source reference caches for functions in R packages, enhancing the
goto definition feature.

fix(Makefile): include definition.c in source files for build

This ensures that the new definition handling logic is compiled
into the rnvimserver application.

fix(data_structures.c): add srcref file reading and validation

This change allows the application to read and validate source
reference files, supporting the new goto definition functionality.

fix(data_structures.h): add srcref field to PkgData struct

This addition supports storing source reference data for R packages.

feat(definition): add symbol definition resolution feature

Introduce a new feature to resolve symbol definitions using cached
data or fallback to R for lookup. This enhances the ability to locate
symbols efficiently within packages, improving the overall
functionality of the application.
Add .clang-format file to enforce code style based on LLVM style.
Adjust line breaks and indentation in C files for better readability.
I ran clang-format on all c files for consistency.

In my `conform.lua`, I have this now:

```lua
clang_format = {},
```
@PMassicotte PMassicotte force-pushed the goto-definition-speedup branch from f7b944d to 4b06caf Compare February 25, 2026 14:53
@PMassicotte PMassicotte marked this pull request as ready for review February 25, 2026 14:54
@jalvesaq
Copy link
Member

Is it ready for review?

@PMassicotte
Copy link
Collaborator Author

Yes, I think this is working pretty good. I do not feel any lag on my side.

@jalvesaq
Copy link
Member

jalvesaq commented Mar 1, 2026

Starting to review it now.

We have to increase the nvimcom version number to force its update and the first line of ~/.cache/R.nvim/README to force the update of cache files:

diff --git a/lua/r/config.lua b/lua/r/config.lua
index f65e674..7b04f9a 100644
--- a/lua/r/config.lua
+++ b/lua/r/config.lua
@@ -853,7 +853,7 @@ local check_readme = function()
     -- Create or update the README (objls_ files will be regenerated if older than
     -- the README).
     local need_readme = false
-    local first_line = "Last change in this file: 2026-01-17"
+    local first_line = "Last change in this file: 2026-03-01"
     if
         vim.fn.filereadable(config.compldir .. "/README") == 0
         or vim.fn.readfile(config.compldir .. "/README")[1] ~= first_line
diff --git a/nvimcom/DESCRIPTION b/nvimcom/DESCRIPTION
index d675fe3..29d40ec 100644
--- a/nvimcom/DESCRIPTION
+++ b/nvimcom/DESCRIPTION
@@ -1,6 +1,6 @@
 Package: nvimcom
-Version: 0.9.86
-Date: 2026-01-31
+Version: 0.9.87
+Date: 2026-03-01
 Title: Intermediate the Communication Between R and Neovim
 Author: Jakson Aquino
 Maintainer: Jakson Alves de Aquino <jalvesaq@gmail.com>

Also, I think it's time to add your name to the "Author" and "Maintainer" fields of nvimcom's DESCRIPTION.

@jalvesaq
Copy link
Member

jalvesaq commented Mar 1, 2026

In a quick test, the "go to definition" works instantly, although all srcref_ files in the cache directory are empty.

Perhaps, we don't need the version number in the srcref_ file names. Until recently, I included the version number in all files (objls_, args_, and alias_), but then I realized that it's enough and simpler to keep the version number in only one of them.

@jalvesaq
Copy link
Member

jalvesaq commented Mar 1, 2026

Result of running rnvimserver through Valgrind: no memory leak, no error either reading or writing buffers.

chore(nvimcom/DESCRIPTION): bump version to 0.9.87 and update date to 2026-03-01
@PMassicotte
Copy link
Collaborator Author

PMassicotte commented Mar 1, 2026

Perhaps, we don't need the version number in the srcref_ file names. Until recently, I included the version number in all files (objls_, args_, and alias_), but then I realized that it's enough and simpler to keep the version number in only one of them.

That makes sense! Should be addressed in bce6507.

@PMassicotte
Copy link
Collaborator Author

Also, I think it's time to add your name to the "Author" and "Maintainer" fields of nvimcom's DESCRIPTION.

Thank you very much for proposing it :) Should I do it in this PR?

@jalvesaq
Copy link
Member

jalvesaq commented Mar 1, 2026

Thank you very much for proposing it :) Should I do it in this PR?

Yes, please.

@jalvesaq
Copy link
Member

jalvesaq commented Mar 1, 2026

Last observation, from R CMD check nvimcom_0.9.87.tar.gz:

* checking R code for possible problems ... NOTE
nvim.build.srcref: no visible global function definition for
  ‘getSrcref’
nvim.build.srcref: no visible global function definition for
  ‘getSrcFilename’
nvim.interlace.rmd: no visible binding for global variable ‘params’
send_definition : get_def_info: no visible global function definition
  for ‘getSrcref’
send_definition : get_def_info: no visible global function definition
  for ‘getSrcFilename’
Undefined global functions or variables:
  getSrcFilename getSrcref params
Consider adding
  importFrom("utils", "getSrcFilename", "getSrcref")

It suggests adding importFrom("utils", "getSrcFilename", "getSrcref") to nvimcom/NAMESPACE.

(Obs: The Note on params can be ignored because we check if params exists before trying to use it.)

@PMassicotte
Copy link
Collaborator Author

PMassicotte commented Mar 1, 2026

diff --git a/nvimcom/R/interlace.R b/nvimcom/R/interlace.R
index 1d0aa2c..6029afd 100644
--- a/nvimcom/R/interlace.R
+++ b/nvimcom/R/interlace.R
@@ -428,7 +428,7 @@ nvim.interlace.rmd <- function(Rmdfile, outform = NULL, rmddir, ...) {
     } else {
         if (exists("params", envir = .GlobalEnv)) {
             old_params <- get("params", envir = .GlobalEnv)
-            rm(params, envir = .GlobalEnv)
+            rm("params", envir = .GlobalEnv)
         }
         res <- rmarkdown::render(Rmdfile, outform, ...)
         if (exists("old_params", inherits = FALSE)) {

I think that using an unquoted name causes R CMD check to flag it as a global variable binding. Should it be changed?

@jalvesaq
Copy link
Member

jalvesaq commented Mar 1, 2026

I think that using an unquoted name causes R CMD check to flag it as a global variable binding. Should it be changed?

Yes, please!

Use a string in the rm function to avoid potential issues with variable
name resolution and improve code clarity.
@jalvesaq
Copy link
Member

jalvesaq commented Mar 1, 2026

Thank you for the great work! I'll squash and merge the pull request.

@jalvesaq jalvesaq merged commit 1e93501 into main Mar 1, 2026
9 checks passed
@jalvesaq jalvesaq deleted the goto-definition-speedup branch March 1, 2026 17:17
@PMassicotte
Copy link
Collaborator Author

Thank you very much! Most of it was just using your r_ls framework ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants