zed-r icon indicating copy to clipboard operation
zed-r copied to clipboard

lintr is running on rust code

Open rgbkrk opened this issue 1 year ago • 13 comments

When this extension is enabled and the user is working on Rust code, the R language server gets used.

image

rgbkrk avatar Jul 28 '24 01:07 rgbkrk

Super odd. On my system it's running lintr not just on R & Rust but also any other file that is opened (.scm, .toml, etc.), which is not great. I'll try to look into why later today when I have some time! Thanks for bringing to my attention!

ocsmit avatar Jul 29 '24 13:07 ocsmit

Ah, good to know. I thought maybe it was a prefix thing when I first saw it. Thank you!

rgbkrk avatar Jul 29 '24 14:07 rgbkrk

Any development on this front? I spent some time today attempting to figure out what's going on. The closest thing I could get was using a .lintr file to specify directories to exclude, but it would be nice if the lintr was only run on R files.

jtlandis avatar Nov 26 '24 19:11 jtlandis

~After updating to yesterday's 0.167.2 Zed release I think this may be fixed in Zed itself.~ apologies I was wrong

remlapmot avatar Jan 07 '25 07:01 remlapmot

Regarding this issue, #12, & https://github.com/REditorSupport/languageserver/issues/681 I believe I have tracked down the problem. The R languageserver is linting non R files when they are being saved, and not when they are being opened.

These events are handled by the methods text_document_did_open and text_document_did_save. The payload received on open has languageId information while the payload on save only contains a uri.

Just to make sure, I tested this out on my local machine by updating the languageserver source code (handlers-textsync.R#L85) to only lint if the uri has the extension "R". E.g.

    path <- path_from_uri(uri)
    fp_ext <- tools::file_ext(path)
    if (fp_ext == "R") {
        self$text_sync(uri, document = doc, run_lintr = TRUE, parse = TRUE)
    }

This then will only return diagnostics for R files and no others.

I am still trying to track down why exactly the payload received by the language server on save does not have languageId information, and figure out an elegant solution to this problem. I believe the solution will require me to patch the languageserver code to handle this behavior and try to get it merged upstream, but I would be happy to be wrong.

ocsmit avatar Jan 08 '25 20:01 ocsmit

I believe the cause is from handlers-textsync.R#L81. My understanding is that when an R file is opened its uri is registered with the language server, and when the language server receives a uri that has been previously registered it will track its version id and handle updates.

However, when a didSave event is handled for a file that is /not/ registered with the language server, the current behavior is to register the file as a new TextDocument. This then treats every file that is saved as an R document regardless of if it had previously been registered with the language server through an didOpen event. I think the behavior should be to skip any unregistered uri to prevent the current behavior.

Something like

text_document_did_save <- function(self, params) {
# ... 
    if (self$workspace$documents$has(uri)) {
        doc <- self$workspace$documents$get(uri)
        doc$set_content(doc$version, content)
    } else {
-        doc <- Document$new(uri, language = NULL, version = NULL, content = content)
-        self$workspace$documents$set(uri, doc)
+        logger$error("uri not valid", uri)
+        return(NULL)
    }
    doc$did_open()
    self$text_sync(uri, document = doc, run_lintr = TRUE, parse = TRUE)
}

or like

text_document_did_save <- function(self, params) {
# ... 
-    if (self$workspace$documents$has(uri)) {
+    if (!self$workspace$documents$has(uri)) {
+        logger$error("uri not valid", uri)
+        return(NULL)
+     }
     doc <- self$workspace$documents$get(uri)
     doc$set_content(doc$version, content)
-    } else {
-        doc <- Document$new(uri, language = NULL, version = NULL, content = content)
-        self$workspace$documents$set(uri, doc)
-    }
-    doc$did_open()
     self$text_sync(uri, document = doc, run_lintr = TRUE, parse = TRUE)
}

I'll plan to open an issue in the r languageserver repo and figure out a way forward.

ocsmit avatar Jan 08 '25 22:01 ocsmit

@ocsmit Wow, thank you so much for your solution! After editing the source code and reinstalling the languageserver package, the issue is all fixed for me!

lict99 avatar Jan 09 '25 11:01 lict99

Lovely! Appreciate the confirmation! Will try to open an issue upstream tonight and figure out a fix that makes sense for everyone.

I still have some gaps in my understanding of how zed chooses to dispatch requests to individual language servers, so I want to do my due diligence to verify wether or not this should be resolved in the zed codebase or in the languageserver codebase.

ocsmit avatar Jan 09 '25 13:01 ocsmit

Great work!

It's interesting that the VSCode R extension https://github.com/REditorSupport/vscode-R which uses languageserver doesn't suffer from this problem.

remlapmot avatar Jan 09 '25 14:01 remlapmot

It's interesting that the VSCode R extension https://github.com/REditorSupport/vscode-R which uses languageserver doesn't suffer from this problem.

100% agree, which is why I am not fully convinced its fully a languageserver issue. I also haven't had the issue occur when using the lsp with Emacs, neovim, or sublime text.

ocsmit avatar Jan 09 '25 14:01 ocsmit

After coding in R for a few hours, I wanted to report a bug related to these changes. In VSCode, I get a memory limit error when plotting with the httpgd plot engine. However, using the default plot engine works fine. Here's what I observed:

{"r.plot.useHttpgd": true} // raises a memory limit error when plotting in VSCode
{"r.plot.useHttpgd": false} // works fine

After reinstalling the languageserver package from CRAN, the bug seems to have disappeared. It also seems like this bug doesn’t affect Zed.

lict99 avatar Jan 09 '25 14:01 lict99

After coding in R for a few hours, I wanted to report a bug related to these changes. In VSCode, I get a memory limit error when plotting with the httpgd plot engine. However, using the default plot engine works fine. Here's what I observed:

{"r.plot.useHttpgd": true} // raises a memory limit error when plotting in VSCode
{"r.plot.useHttpgd": false} // works fine

After reinstalling the languageserver package from CRAN, the bug seems to have disappeared. It also seems like this bug doesn’t affect Zed.

Sorry, it seems like this bug isn’t easily reproducible. After quitting both Zed and VSCode and reopening them, the bug disappeared. However, after using both apps for a while, the issue popped up again. I’m not sure how to consistently trigger the bug.

Since it seems like a memory limit issue, I’m guessing it might be triggered by Zed's REPL kernel. I tried using the Ark kernel, and I noticed that Ark stays active even after quitting Zed. (Just a thought)

lict99 avatar Jan 09 '25 16:01 lict99

I was just filing a bug report in Zed about this before finding this issue here. This sounds a bit like a Zed proper error to be honest.

https://github.com/zed-industries/zed/issues/28316

JonGretar avatar Apr 08 '25 10:04 JonGretar

Came here to file this report. The R langauge server shouldn't be trying to lint on .sh or .rs files it would be great if this extension worked on only R files. I dont encounter this with other extension lsps in zed

JosiahParry avatar Nov 01 '25 15:11 JosiahParry

Came here to file this report. The R langauge server shouldn't be trying to lint on .sh or .rs files it would be great if this extension worked on only R files. I dont encounter this with other extension lsps in zed

Apologies I have not kept up with this. I do not use zed, and have not for about a year now. I will double check to see if what I outlined here is still applicable

ocsmit avatar Nov 03 '25 23:11 ocsmit