Provide package symbols before lint in diagnostics

[renkun-ken]

Closes REditorSupport/vscode-R#729.

Also addresses #567 (comment).

pkgload::load_all() is called before lintr::lint() if the workspace root contains a package.

[renkun-ken]

There are known limitations:

• If pkgload is not available, it will still lint the single file.
• If there are parse errors in any R file in R folder, or the R files cannot be evaluated properly, pkgload::load_all() will fail, and it will lint the single file.

[randy3k]

I am curious if it would be too slow for large package. (Though lintr is slow anyway).

[renkun-ken]

I tried system.time(pkgload(compile = FALSE, quiet = TRUE)) with lintr and the timing looks like

> system.time(pkgload::load_all(compile = FALSE, quiet = TRUE))                                                   
   user  system elapsed 
  0.772   0.060   0.893 

> system.time(pkgload::load_all(compile = FALSE, quiet = TRUE))                                                   
   user  system elapsed 
  0.358   0.016   0.375 

> system.time(pkgload::load_all(compile = FALSE, quiet = TRUE))                                                   
   user  system elapsed 
  0.258   0.015   0.272 

> system.time(pkgload::load_all(compile = FALSE, quiet = TRUE))                                                   
   user  system elapsed 
  0.254   0.015   0.270 

> system.time(pkgload::load_all(compile = FALSE, quiet = TRUE))                                                   
   user  system elapsed 
  0.262   0.015   0.277 

> system.time(pkgload::load_all(compile = FALSE, quiet = TRUE))                                                   
   user  system elapsed 
  0.233   0.017   0.250 

Since the diagnostics runs in reused sessions, the timing might look better after the first time.

I tried editing around in lintr package and I don't observe a notable difference in the delay of diagnostics than before.

[randy3k]

Since the diagnostics runs in reused sessions, the timing might look better after the first time.

Good point.

[dgkf]

I'm seeing this issue as well.

I am curious if it would be too slow for large package. (Though lintr is slow anyway).

I do have some packages where load_all() might be slow. I'm not sure how it would compare, but one alternative would be to filter these diagnostic messages from the lintr results when a definition is found within the package for the symbol.

---

Edit: also thank you for all the awesome work to provide an R LSP :)

[renkun-ken]

@dgkf

I do have some packages where load_all() might be slow.

Does it always take as much time as the first time for each load_all()? Since we reuse existing sessions to perform diagnostics, repeated load_all() should take much less time than the first time.

[dgkf]

@renkun-ken - it does get faster with each call, you're right.

I think either way you go with this would be fine, I only mentioned it because it was my first thought when thinking about how to approach this and wanted to make the suggestion in case it simplified things at all. If this feels like the right approach, I totally trust your judgement here.

[renkun-ken]

@randy3k Do you think we should opt-in or opt-out such behavior? AFAIK, only lintr::object_usage_linter needs such behavior. If user does not use this linter, there's no point to pay a cost for it.

[renkun-ken]

Another approach is that we somehow mimic the behavior of pkgload::load_all() but only does the minimal required work to make lintr::object_usage_linter, e.g. only populate a namespace where package objects are only named objects with no useful/actual values.

[randy3k]

There is really no much additional time added. I am happy either way.

[renkun-ken]

A notable impact of using pkgload::load_all() is that it seems to have high CPU and memory footprints as I type in the editor, an almost each key press triggers a load_all() call, which gradually becomes timely but looks quite expensive in terms of the usage of machine resources.

[randy3k]

That could be a potential issue of having high CPU and memory usage. Perhaps we should disable it in default and only allow interested users to opt in and test the feature.

[renkun-ken]

I just switch to a simpler approach by attaching a shim environment of globals defined in the package, i.e. functions and symbols, which has almost zero overhead. The overhead of load_all() looks quite significant, and much of it does not seem necessary.

@dgkf Would you like to try it out and see if it works for you?

[dgkf]

I just switch to a simpler approach by attaching a shim environment of globals defined in the package, i.e. functions and symbols, which has almost zero overhead. The overhead of load_all() looks quite significant, and much of it does not seem necessary.

@dgkf Would you like to try it out and see if it works for you?

Sure - I'll give the brancha go tonight.

[dgkf]

I tried out the branch and performance seemed pretty good. Nice work!

I did notice that it will still report the "no visible binding" warnings until the file is saved for the first time, even if the names are found in the local source code.

Otherwise it looks great, and having to do a quick save once they pop up is a small inconvenience given how much of an improvement this already is.

[renkun-ken]

If the document is open before the package R files are parsed, its diagnostics will report no visible binding. If the package is finished parsing, then the warning will disappear on any edit of the file.

In fact, if you edit other files and don't save them, it should still work because language server workspaces work without assuming all files are saved. By constrast, load_all() will only load the saved version of the files, instead of modified by unsaved ones.

Maybe we could delay the publish of diagnostics until package R files are all parsed on startup.

[renkun-ken]

@randy3k I think this PR is ready to merge. Let me know if you have any other suggestions.

[randy3k]

This is a smart solution. Thank you for working on it. Just a small nitpick.

[renkun-ken]

parse_data now preserves the functions defined in the documents with only formals, which could make it easier to work with. If a future version of lintr checks if function arguments supplied match the function formals, it should be still working as the formals are preserved.

Also, it make it easier to works with match.call(fun, expr) in the future.