Populate NAMESPACE and Depends imports in diagnostics globals for object_usage_linter() - #775
Conversation
9076b7b to
6758e07
Compare
renkun-ken
left a comment
There was a problem hiding this comment.
I reproduced three correctness issues that should be addressed before merging: changing NAMESPACE can stop the server, imported-function placeholders suppress valid argument warnings, and the package-root fallback drops source dependencies for scripts outside R/. Details and suggested fixes are in the inline comments.
Validation at 32514ec: the focused workspace, server, index, and lint tests passed, including integration tests. The existing coverage shard 3 failure in lintr config file works did not reproduce locally. These cases need additional regression coverage.
renkun-ken
left a comment
There was a problem hiding this comment.
The three findings from my previous review are addressed in a5c1608, and the added regression tests pass locally. The focused workspace, server, lint, and index tests also pass, including integration tests; all current CI checks are green.
I found one new correctness issue in the signature-preserving import resolution: it can select a signature from the wrong package when imports overlap. The inline comment includes a reproducer. I also checked renamed imports, primitives, re-exports, and non-function bindings without finding another issue in those paths.
renkun-ken
left a comment
There was a problem hiding this comment.
The previously reported import-precedence reproducer now passes: Depends bindings are populated before NAMESPACE directives, and later directives replace earlier imports. I found four follow-up issues in this rewrite, detailed inline.
Local validation: the workspace-core test reproduces the two failing mad assertions from CI. The focused language-server, lint (including integration), index, document, performance-index, and intelligence-performance tests pass. I also reproduced the skipped nested imports, stale imports after installing a previously missing dependency, and a namespace-loading error escaping process_events(). CI is currently failing on all three R-CMD-check platforms and coverage shards 3 and 4; shard 3 reports the previously seen lintr config file works failure, which did not reproduce locally.
renkun-ken
left a comment
There was a problem hiding this comment.
All findings from my previous reviews are addressed in 9353f07. I found no new actionable correctness issues in the latest changes.
The focused workspace, language-server, lint (including integration), index, and document tests pass locally. I also verified the fixes with a real dependency installation in an existing Workspace, both indexed and unindexed diagnostics, a real dependency .onLoad failure during NAMESPACE refresh, and imports inside braces and both branches of conditional directives.
All three R-CMD-check platforms and lint pass. Coverage shard 3 still fails in lintr works and lintr config file works with missing diagnostics/key-not-found errors. Those tests pass locally, and similar failures appeared on previous revisions; please rerun or investigate that coverage job before merging.
|
Thanks for the patient review! |
|
Thanks for the work! |
Closes #773; closes #652. Written by Gemini. It's a lot larger than I would prefer for a codebase I'm not too familiar with, so extensive feedback is welcome. LLM description follows:
Problem
Workspace$get_diagnostics_globals()previously populated"languageserver:globals"with top-level definitions fromR/*.Rfiles, but omitted bindings imported viaNAMESPACE(import(),import(..., except = ...),importFrom(),importMethodsFrom()) andDESCRIPTION(Depends). When linting an uninstalled package or newly addedNAMESPACEimports,lintr::object_usage_linter()reported false-positiveno visible global function definitionandno visible binding for global variablewarnings (e.g., for@import ggplot2or@importFrom dplyr .data).WorkspaceIndexwas enabled,Workspace$get_diagnostics_globals(uri)assignedNULLfor all indexed definitions (and skipped unindexed files still inself$index$pending). Becausecodetools::checkUsageEnterGlobal()checks function calls withexists(n, envir = w$globalenv, mode = "function"),NULLplaceholders caused false-positiveno visible global function definitionwarnings for cross-file package functions.Solution
NAMESPACEdirectives viabase::parseNamespaceFile()(extract_namespace_imports()) andDESCRIPTIONDepends(extract_package_imports()), sharing the parsed imports betweenWorkspace$import_from_namespace_file()andWorkspace$get_diagnostics_globals()."languageserver:globals"with exported symbols from imported packages (respectingimport(..., except = ...)), explicitly imported objects/methods, and function closures (functwith formals orany_args_function) for package definitions.WorkspaceIndexare indexed on demand inget_diagnostics_globals(uri)and invalidate diagnostic caches whenNAMESPACEis updated on disk.