Repository navigation
Conversation
renkun-ken
left a comment
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
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.
Preserve session package synchronization with the updated language client and terminal registry. Retain workspace fallback coverage alongside the relocated terminal lifecycle tests.
- Track successful delivery per client and serialise updates to preserve the latest state. Retry failures on refresh or completion, retain disconnect clears, and guard against replaced clients. - Add regression tests and real R server coverage for workspace replay and disconnect cleanup.
|
@renkun-ken Could you take a look at this, and determine whether this is still needed? Autocompletion from 
|
|
The static analysis for project-wide indexing (including |
Do you think this PR is still necessary then? |
|
I think this PR still has value, but languageserver#751 narrows its purpose to completion from runtime session state. The current languageserver already discovers packages referenced in the script or in closed files reached through statically resolvable I checked this with real LSP requests against languageserver 0.3.20 and this PR's launcher: completion through a closed sourced setup file works already; a script without a static package reference gains I found two remaining routing issues in head
There is also a completion UX concern: languageserver's static analysis and vscode-R's live session provider can suggest the same symbol, with different provenance, documentation, or insertion behavior. For example, a function defined in the script and then evaluated into For maintainability, I would also prefer a supported session-state handler in languageserver over replacing My recommendation is to keep the feature, describe it specifically as session-only package completion, and address the routing issues and static/live completion UX before merging. |
Update owned workspace language servers even when their session is not globally selected, and clear recorded scopes on disconnect after folders are removed. Add regression tests for session handoff, competing updates and folder removal. Apply oxfmt to the PR's changed TypeScript sources.
renkun-ken
left a comment
renkun-ken
left a comment
There was a problem hiding this comment.
The feature still has value with the current languageserver: static discovery can follow resolvable source() dependencies, but it cannot infer a package attached only in the running session. I found remaining synchronization and compatibility problems that should be addressed before merging; details and reproductions are inline.
The earlier client-startup and single-server workspace replay fixes are present, and all six CI checks passed on this head. Locally, TypeScript type checking passed. I exercised the current languageserver at REditorSupport/languageserver@0add7cc with focused provider and session-state reproductions, tested the 0.3.18 handler, and used mocked clients/source-level probes for routing and Interactive lifecycle cases. Those probes are not full VS Code UI tests.
| } else { | ||
| languageserver:::workspace_startup_packages() | ||
| } | ||
| workspace$update_loaded_packages() |
There was a problem hiding this comment.
[P2] Synchronize the current server's package metadata and resolution context
Updating startup_packages and loaded_packages alone does not make a session-only package available to the latest server's member and S3 providers. Against languageserver 0add7cc, after syncing an attached polars, pl$all()$su produced no member suggestions until its member metadata was prepared. Similarly, after syncing dplyr, arrange(data.frame(x=1), ) returned the generic signature without .locale, whereas a static library(dplyr) produced the data-frame method signature. Preparing the S3 catalog alone did not fix the latter because its resolver also needs the effective session package context. Please integrate session packages with the server's metadata preparation and provider context, preferably through a supported server-side interface, and cover attach/detach for these providers.
| attachedPackages: data.search | ||
| .filter((value) => value.startsWith('package:')) | ||
| .map((value) => value.substring(8)), | ||
| loadedNamespaces: [...data.loaded_namespaces], |
There was a problem hiding this comment.
[P2] Carry the attached packages' library locations to the server
This snapshot sends only names, while ordinary workspace/global servers use their own configured R library paths. If the session attaches a package from a session-specific .libPaths() or library(..., lib.loc=...), workspace$get_namespace() cannot find it and the handler silently reports success, so the advertised completion never appears. I reproduced this with a package installed only in a temporary session library: it was in loaded_packages, but the current languageserver returned NULL for its namespace. Please synchronize enough library/package-location information to resolve the actual attached packages, or explicitly constrain and surface this limitation.
| # languageserver resolves package conflicts from the end of this list. | ||
| rev(attached_packages) | ||
| } else { | ||
| languageserver:::workspace_startup_packages() |
There was a problem hiding this comment.
[P2] Handle installed languageserver versions without this internal function
The extension does not enforce a minimum languageserver version, and released 0.3.18 has workspaces but no workspace_startup_packages(). With that version I synced splines successfully, then sent the empty state: the request failed with object 'workspace_startup_packages' not found and bs remained in completion after disconnect. Version 0.3.17 also lacks self$workspaces. Please preserve a compatible reset path or gate this integration with an explicit minimum-version check and graceful fallback, and test the supported versions.
| session.sessionDir = params.tempdir; | ||
| session.workingDir = params.wd; | ||
| session.resource = | ||
| (native ? rTerminal.getTerminalResource(native.terminal) : undefined) ?? |
There was a problem hiding this comment.
[P2] Do not use the configuration fallback to route an external session
getTerminalResource() calls getCurrentWorkspaceFolder(), which falls back to the active or first workspace when the terminal cwd is outside all folders or missing. Consequently, with multi-server enabled, an associated terminal and R process both in /outside can be assigned to the unrelated /project workspace; its packages go to the project server instead of the unscoped server. I reproduced that exact routing with the PR's functions. Please use an actual terminal/session association or the local session's reported working directory without the configuration fallback when deciding the session scope.
| : (previous ?? new Session(id, host, '', '', new net.Socket())); | ||
| target.requester = requester; | ||
| target.workingDir = directory; | ||
| target.resource = vscode.workspace.getWorkspaceFolder(Uri.file(directory))?.uri; |
There was a problem hiding this comment.
[P2] Reclassify sessions when their directory is added to the workspace
This stores undefined if an Interactive session starts outside the folders currently open, losing its directory as a routing resource. With folder A already open, I started a session in B, added B to the workspace, and opened B's R file: the new workspace client was created, but the session remained in unscoped and no package state reached B even on refresh. The same loss occurs in the unassociated local attach fallback. Preserve the local directory and re-evaluate/replay affected sessions when folders are added.
| } else if (ownedScope && this.sessionStates.get(ownedScope)?.sessionId === sessionId) { | ||
| // An unselected session may refresh its own workspace, but never | ||
| // replace the state now owned by another session. | ||
| this.syncSessionScope(ownedScope, data, sessionId); |
There was a problem hiding this comment.
[P2] Refresh the owned background session before syncing its workspace
This branch handles an inactive session only once updateSessionWorkspace() has received fresh data. For native terminal sessions, however, workspace_updated calls refresh.invalidate() without a delay when the session is inactive, so the existing scheduler does not fetch that data until a later explicit demand or selection. An ordinary file in the owned workspace does not demand that background session. I reproduced a B workspace retaining stats after B was invalidated with splines; only an explicit ensure() delivered the new state. Please arrange a coalesced refresh on invalidation or on completion demand for the recorded owner; the new direct-sync test bypasses this path.
|
|
||
| export function updateSessionWorkspace(target: Session, data: WorkspaceData): void { | ||
| target.workspaceData = data; | ||
| rLanguageService?.syncSessionState(data, target.resource, target.sessionId, false); |
There was a problem hiding this comment.
[P2] Clear package state when an Interactive R process stops
Stopping an Interactive session retains its view and transcript. InteractiveManager.updateStatus() sets workspaceUnavailable on exit and calls updateSessionWorkspace() with the old cached data, so this line re-sends the stopped process's packages rather than clearing them; unregisterSessionTransport() is only called when the view is disposed. I reproduced that data flow with a bound client. Please clear the session-derived state on a definitive stop/exit and avoid replaying target.workspaceData when a client is later created for that stopped target, while handling transient reconnects deliberately.
|
|
||
| /** Move document routing to a new process; existing data viewers keep their old owner. */ | ||
| export function replaceSessionTransport(previous: Session, next: Session): void { | ||
| rLanguageService?.syncSessionState(undefined, previous.resource, previous.sessionId); |
There was a problem hiding this comment.
[P2] Rebind language clients when an Interactive session is restarted
replaceSessionTransport() clears the old session scope, then changes documentSessions to the new generation without firing onDidBindSessionDocument. The retained input client therefore keeps session:<old id> as its scope and never receives the new process's packages. Calling bindSessionDocument() afterward also emits no event because the owner was already replaced. A mocked run of the actual listeners retained one client on the old scope after restart. Please notify/restart or migrate affected clients during replacement, and test the retained Interactive input across a restart.
…e lifecycles Use actual R session paths when routing workspaces, reconcile ownership as folders change, and refresh background owners. Clear exited and replaced process state, rebind retained Interactive clients, and propagate process-specific package library paths. Prepare member and S3 metadata in languageserver workers and fall back cleanly when its integration internals are unavailable. Cover scope changes, stop/restart/reconnect, custom libraries, background refresh, and provider resolution.
Persist the R language server's session-owned package baseline across synchronization requests so detaching a private-library package restores the actual workspace imports. Apply repository oxfmt output, narrow test callback types for oxlint, and compare Windows resource URIs without drive-letter casing assumptions.
…adata Restore configured language-server library paths on detach and evict session-only namespace caches. Settle canceled metadata RPCs and prevent stale worker results from restoring old packages. Retain failed synchronization for retry and test server rejection. Resolve R lint indentation findings. TypeScript sources and regression test were checked using oxfmt, typecheck and oxlint on the validation candidate.
Track asynchronous sync reply completion in an explicit environment instead of superassignment. In the managed jgd preparation test, retry transient startup.status read failures while the child process writes the file, using the existing bounded startup deadline. Keep production jgd preparation behavior unchanged.
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.