Conversation
renkun-ken
left a comment
There was a problem hiding this comment.
The approach makes interactively attached packages available to the existing completion machinery, and the current CI checks pass. I found three synchronization/routing issues that should be addressed before merging; the inline comments include their triggers and suggested fixes.
Validation: reviewed head 4cc6b7b; TypeScript type checking, ESLint for all changed TypeScript files, and lintr for R/languageServer.R passed locally. Focused reproductions exercised the PR's attach handler and LanguageService startup with mocked VS Code/client dependencies, and its R synchronization handler with languageserver 0.3.20, including the actual workspace-folder notification path. These reproduced incorrect routing for a wrapped R process, a lost update during client startup, and missing completion in a workspace added after synchronization. Please add regression coverage for these cases.
| session.info = (params.info as SessionInfo | undefined) ?? { version: session.rVer, command: '', start_time: '' }; | ||
| session.sessionDir = String(params.tempdir); | ||
| session.workingDir = String(params.wd); | ||
| session.resource = terminal ? rTerminal.getTerminalResource(terminal) : undefined; |
There was a problem hiding this comment.
[P2] Resolve the workspace for sessions whose PID differs from the terminal PID
terminal is found only when Terminal.processId equals params.pid, but VS Code reports the shell/console process PID and sess reports Sys.getpid(). With a wrapped console or R started inside a shell, those can differ even when both the terminal cwd and params.wd are inside the workspace. This makes session.resource undefined, so multi-server mode sends that session's packages to unscoped instead of its workspace server; it can also replace package state for unrelated unscoped files. I reproduced this with terminal PID 41000, R PID 41001, and both paths in the same workspace. Please resolve the workspace independently of exact PID equality, using a reliable terminal/session association or the local session's reported working directory as a fallback, while preserving truly unscoped sessions.
| const sessionState = this.sessionStates.get(sessionScope)?.state; | ||
| if (sessionState) { | ||
| await this.applySessionState(client, sessionState); |
There was a problem hiding this comment.
[P2] Register the client before awaiting its initial state synchronization
The caller adds this client to clients/clientScopes only after createClient returns. If a package update or disconnect arrives while this initial sendRequest is pending, syncSessionState updates/deletes the cache but cannot deliver the change to this client. The client is then registered with the old server state, and identical later workspace refreshes are skipped by the state-key check. In a controlled reproduction, synchronizing [base], then [dplyr, base] before the first request completed left the cache at [dplyr, base] but sent only [base]. Please register the client and its scope before the asynchronous sync, or reconcile the latest state (including a cleared state) before making startup complete.
| for (workspace in self$workspaces$values()) { | ||
| workspace$startup_packages <- if (length(attached_packages)) { | ||
| # languageserver resolves package conflicts from the end of this list. | ||
| rev(attached_packages) |
There was a problem hiding this comment.
[P2] Apply the current session state to workspaces created after this request
This updates only the workspaces that already exist when r/syncSessionState runs. In single-server mode, adding another folder to an existing multi-root workspace creates a new Workspace through workspace/didChangeWorkspaceFolders, and that workspace starts with the default packages. Since the session/package state is unchanged, the TypeScript deduplication does not send another sync. I reproduced this by syncing dplyr, then invoking the real workspace-folder notification handler: the existing workspace offered mutate, while the newly added workspace did not. Please retain the active state on the server and apply it when adding a workspace, or explicitly replay it after workspace-folder additions so completion remains available across all documents handled by the single server.
There was a problem hiding this comment.
Rechecked at d53815d: this case is still unresolved. The R handler still updates only self$workspaces$values() at synchronization time, and no replay is triggered for added folders. Using this revision's handler with languageserver 0.3.20, I synced dplyr, invoked workspace_did_change_workspace_folders to add a folder, and requested workspace completion: mutate was present for the existing workspace and absent for the added workspace. Please retain/apply the active state when a workspace is created, or replay synchronization after folder additions, and add coverage for that path.
4cc6b7b to
0a5cf6f
Compare
renkun-ken
left a comment
There was a problem hiding this comment.
Reviewed d53815d. The local working-directory fallback and registering the client before its initial synchronization address the first two findings from my previous review. Focused reproductions now route a session without a terminal match to its workspace and deliver both updates and disconnects while the first sync is pending. The new bound-session separation also passed the four language-service regression tests in a mocked runtime.
The third finding remains: a workspace folder added after synchronization does not inherit the active session packages. I reran the actual languageserver workspace-folder notification path with this revision: the existing workspace offered mutate after syncing dplyr, while the newly added workspace did not. I have added an update to the original thread.
All three CI test jobs currently stop during TypeScript compilation because of the two new-test errors noted inline; the VS Code test suite never runs. Both errors also reproduce locally. Build and lint CI jobs pass, and local lintr for R/languageServer.R passes. The focused mocked/R checks above do not substitute for running the full test suite once these compile errors are fixed.
| service.syncSessionState({ | ||
| search: ['.GlobalEnv', 'package:base'], | ||
| loaded_namespaces: ['base'], | ||
| globalenv: {}, |
There was a problem hiding this comment.
[P1] Remove the extra property from the synchronization fixture
This direct object literal is passed to syncSessionState, whose SessionWorkspaceData parameter contains only search and loaded_namespaces. TypeScript rejects globalenv here with TS2353, so the new test stops the pretest compilation step on macOS, Linux, and Windows before any VS Code tests execute. The same error reproduces locally. Please omit globalenv from this literal or pass a separately typed WorkspaceData fixture, then rerun the test jobs.
| })}\n`); | ||
| const attached = await waitFor(() => session.activeSession?.sessionId === 'workspace-fallback' | ||
| ? session.activeSession : undefined); | ||
| assert.strictEqual(attached.resource?.toString(), workspaceUri.toString()); |
There was a problem hiding this comment.
[P1] Narrow the possibly undefined result before reading resource
waitFor returns Promise without narrowing T, and this callback returns Session | undefined, so attached remains possibly undefined here. With strict checking enabled, this line produces TS18048 in every CI test job and locally. The optional access on resource does not guard attached. Please add assert.ok(attached) before accessing attached.resource (as the test already does for socket), or give waitFor an appropriately narrowed return contract, so pretest can compile and the regression test can run.
Related to #1536, #1593
Summary
Use packages attached in the active R session for editor language completion, even when those packages are not referenced or through
source()in the active script.Currently, completion mainly relies on package information that
languageservercan discover from the script or its static source dependencies. This means a package attached interactively in the R session will not receive completion suggestions when it is not mentioned in the script.This change synchronises the active R session package state, provided via
sess, with the existing language server.Behaviour after this change
Single-server mode: packages attached in the active R session are available for completion across documents handled by the single language server, even when they are not referenced in the script.
Multi-server mode: packages attached in an R session are synchronised with the language server associated with that session's workspace. Package state remains isolated between different workspace language servers.
Unscoped files: scripts opened outside an existing workspace or no workspace folder is open, use the active unscoped R session package state for completion.
When the active R session changes, its attached package state replaces the previous session state. When the session disconnects, the session-derived package state is cleared.
Existing static package discovery, including resolvable
source()dependencies, remains unchanged.