Adapt document to IFile when unable to retrieve file from buffer manager#1501
Adapt document to IFile when unable to retrieve file from buffer manager#1501raghucssit wants to merge 1 commit intoeclipse-lsp4e:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for Eclipse's adapter framework to LSPEclipseUtils.getFile() and LSPEclipseUtils.toUri(IDocument) methods to handle documents from frameworks like Xtext that don't connect their documents to Eclipse's buffer manager. The change enables GitHub Copilot and other LSP-based tools to work with custom document implementations.
Changes:
- Modified
toUri(IDocument)to attempt adaptation to URI when buffer manager resolution fails - Modified
getFile(IDocument)to attempt adaptation to IFile when buffer manager resolution fails
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Can you explain a bit more about the problem? I think that if you are using Xtext with LSP4E, Xtext should only be part of the Language Server and it should not be present on the UI plugins. |
This is an General Purpose Utility feature we need. We should be able to adapt Why we need this ? Here I already have PR to support adaptable feature in Xtext. Real world use case: Because copilot cannot derive URI of the file. It does not support any auto complete for So we can support this feature and help any clients who has an instance of Once |
|
FYI @iloveeclipse |
|
@rubenporras : this PR helps Copilot plugin in Eclipse to "see" connection between current document in the opened Xtext based editor and the underlined IFile or Uri. Copilot uses LSP4E. Without that connection between document and file/uri, nothing works in Copilot with Xtext based text editors. "Chat" can't link the file in question to the context, "code predictions" and all other "AI" functionality simply doesn't work. |
|
Note: currently, even if Xtext patch is not yet merged (see eclipse-xtext/xtext#3615), Eclipse based products can still work around that by contributing an adapter factory, but it would only work if LSP4E would support it via current PR. |
|
Hi @iloveeclipse and @raghucssit, while I understand that technically adding an LSP4E has been designed to work only with the generic editor (or more concrete with editors that use the a ITextFileBufferManager) and the takes the assumption at many places that this is the case, which the Xtext editor is not doing. With that in mind, I do not think it is correct that Copilot uses this code outside of this context. They could as well wrap the |
The problem is, that Copilot doesn't have any dependency to Xtext (it doesn't need it at all) so it can't just take "XtextDocument" and do something specific with that. They have lot of very generic code that works on top of IDokcument API & LSP4E and that works quite well.
Could you point to possible problems? I believe Raghu didn't saw any after patching LSP4E locally. |
|
I do not have time to look for possible problems. My main point is that LSP4E code does not need this code to work, it is rather CoPilot that needs to do the mapping, so why not add a similar code to the copilot codebase? |
@raghucssit : could you please evaluate whether it will be possible? |
I moved document adaption logic to copilot. But this did not work as expected because document connection with lspe4e fails.
LSP4E don't support custom documents they don't register with Eclipse Buffer Manager.. @rubenporras Do we need to support this ? I have explianed one major usecase that is xtext. |
|
Github copilot for eclipse tries to connect document and IFile using LanguageServerWrapper#connect(@Nullable IDocument document, IFile file). This API internally tries to instantiate DocumentContentSynchronizer. DocumentContentSynchronizer constructor throws NPE because it tries to derive URI using |
|
Hi @raghucssit , thanks for looking it up. If they actually call connect and not I would expect that merging this PR will cause follow-up problems. From LSP4E point of view, the ideal solution would be that the Xtext Editor uses a file buffer manager, but I do not know how involved such change would be. |
We evaluated that, it is unfortunately not possible. |
|
As a note, the LSFileBufferListener is not the only place. There are other places which rely on a file buffer mannager. |
Let assume What other functionality in LSP4E would need to be touched to add proper support for Xtext editors in LSP4E? |
3d5290e to
66de3c5
Compare
Some framework like Xtext does not connect their document to buffer manager. So LSPEclipseUtils returns null for URI and IFile in such cases. So we can try to adapt the IDocument to URI and IFile. see eclipse-lsp4e#1500
|
I have added Fallback resource listener which listens to IFile resource changes.. Here the challenge is any file managed by buffer manager also triggers resource change notifications and both the listeners gets triggered. To avoid that I have used time stamp logic. Buffer listener is used in places in LSP4E
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
org.eclipse.lsp4e/src/org/eclipse/lsp4e/LSPEclipseUtils.java:1371
getFile(IDocument)now falls back toAdapters.adapt(document, IFile.class, true), but this new code path isn’t covered by tests. Please add a unit test similar to thetoUri(IDocument)case to verify that when the buffer manager can’t resolve a path, the adapter-providedIFileis returned (and that existing behavior still returns the workspace file when a buffer-backed path exists).
public static @Nullable IFile getFile(@Nullable IDocument document) {
IPath path = toPath(document);
IFile file = getFile(path);
//if we cannot determine file via buffer manager then try to adapt from document.
if (file == null) {
file = Adapters.adapt(document, IFile.class, true);
}
org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java:334
PRE_DELETE/PRE_CLOSEevents are not save operations, but this callsDocumentContentSynchronizer.documentAboutToBeSaved(), which can trigger willSaveWaitUntil/format-on-save edits. This can unexpectedly modify the document during a delete/close and send incorrect LSP lifecycle signals. Consider removing this call here, or only invoking it for actual save events (e.g., when you can reliably detect a save/CONTENT change).
DocumentContentSynchronizer dcs = connectedDocuments.get(uri);
if (dcs != null) {
// Mirror buffer.stateChanging -> documentAboutToBeSaved
try {
dcs.documentAboutToBeSaved();
} catch (Exception e) {
LanguageServerPlugin.logError(e);
}
}
org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java:376
- The workspace fallback path sends
textDocument/didSaveunconditionally on anyIResourceDelta.CONTENTchange.DocumentContentSynchronizer.documentSaved(...)explicitly checks the server'stextDocumentSync.savecapability and suppresses didSave whensave == null; this code bypasses that and may spam servers that don't support save notifications. Please mirror the same capability check here (and ideally reuse the existingdocumentSavedlogic where possible).
// Content change (save/replace) -> attempt to notify documentSaved
if (kind == IResourceDelta.CHANGED && (flags & IResourceDelta.CONTENT) != 0) {
URI uri = LSPEclipseUtils.toUri(file);
if (uri != null) {
// If buffer listener recently handled this URI, skip to avoid duplicate handling
Long last = bufferEventTimestamps.get(uri);
if (last != null && System.currentTimeMillis() - last < BUFFER_EVENT_DEBOUNCE_MS) {
return false; // skip this file
}
DocumentContentSynchronizer dcs = connectedDocuments.get(uri);
if (dcs != null) {
try {
// Mirror buffer.dirtyStateChanged(..., false) -> documentSaved
final var identifier = LSPEclipseUtils.toTextDocumentIdentifier(uri);
final var params = new DidSaveTextDocumentParams(identifier, dcs.getDocument().get());
// send didSave notification via wrapper to keep ordering
LanguageServerWrapper.this.sendNotification(ls -> ls.getTextDocumentService().didSave(params));
} catch (Exception e) {
org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java:339
- In the PRE_DELETE/PRE_CLOSE branch,
disconnectTextFileBuffer(uri)is called and thendisconnect(uri)is called, butdisconnect(uri)already callsdisconnectTextFileBuffer(uri)when a document listener is present. This is redundant and makes the control flow harder to follow; consider lettingdisconnect(uri)handle the buffer cleanup and remove the extra call here.
if (isConnectedTo(uri)) {
LanguageServerPlugin.logInfo("Workspace PRE_DELETE/PRE_CLOSE disconnect for: " + uri); //$NON-NLS-1$
disconnectTextFileBuffer(uri);
disconnect(uri);
}
org.eclipse.lsp4e/src/org/eclipse/lsp4e/LanguageServerWrapper.java:309
- The PR description focuses on adapting
IDocumenttoURI/IFile, but this file also introduces a new workspace-levelIResourceChangeListenerwith debouncing and custom didSave/disconnect behavior. If this additional listener is required for the adaptation feature to work (e.g., to get save/delete events without file buffers), it should be called out explicitly in the PR description (and ideally justified with the specific failure mode it addresses), since it changes lifecycle behavior at workspace scope.
// Fallback workspace listener to catch resource-level events for files that are not backed by file buffers
private final IResourceChangeListener resourceFallbackListener = new ResourceFallbackListener();
private final class ResourceFallbackListener implements IResourceChangeListener {
org.eclipse.lsp4e/src/org/eclipse/lsp4e/LSPEclipseUtils.java:418
toUri(IDocument)now falls back toAdapters.adapt(document, URI.class, true), but there is no corresponding test coverage to ensure adapter-based URI resolution works (and doesn’t regress the existing buffer-based behavior). Since this is a behavior change used by downstream integrations (e.g., Xtext/Copilot), please add a unit test that registers anIAdapterFactoryfor a customIDocumentand assertsLSPEclipseUtils.toUri(document)returns the adapted URI when no file buffer is present.
public static @Nullable URI toUri(@Nullable IDocument document) {
if(document == null) {
return null;
}
ITextFileBuffer buffer = toBuffer(document);
IPath path = toPath(buffer);
IFile file = getFile(path);
if (file != null) {
return toUri(file);
} else if(path != null) {
return toUri(path.toFile());
} else if (buffer != null && buffer.getFileStore() != null) {
return buffer.getFileStore().toURI();
}
return Adapters.adapt(document, URI.class, true);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| DocumentContentSynchronizer dcs = connectedDocuments.get(uri); | ||
| if (dcs != null) { | ||
| // Mirror buffer.stateChanging -> documentAboutToBeSaved | ||
| try { | ||
| dcs.documentAboutToBeSaved(); | ||
| } catch (Exception e) { | ||
| LanguageServerPlugin.logError(e); | ||
| } | ||
| } | ||
| if (isConnectedTo(uri)) { | ||
| LanguageServerPlugin.logInfo("Workspace PRE_DELETE/PRE_CLOSE disconnect for: " + uri); //$NON-NLS-1$ | ||
| disconnectTextFileBuffer(uri); | ||
| disconnect(uri); | ||
| } |
There was a problem hiding this comment.
ResourceFallbackListener runs on the workspace resource notification thread, but it directly reads connectedDocuments/calls isConnectedTo(...). connectedDocuments is a plain HashMap and elsewhere writes are guarded with synchronized (connectedDocuments), so this introduces racy access and potential map corruption/CMEs. Please make access to connectedDocuments consistently thread-safe (e.g., guard reads/writes with the same lock, or switch to a concurrent map and update disconnect(...) accordingly, or marshal resource events onto a single thread before touching the map).
| DocumentContentSynchronizer dcs = connectedDocuments.get(uri); | |
| if (dcs != null) { | |
| // Mirror buffer.stateChanging -> documentAboutToBeSaved | |
| try { | |
| dcs.documentAboutToBeSaved(); | |
| } catch (Exception e) { | |
| LanguageServerPlugin.logError(e); | |
| } | |
| } | |
| if (isConnectedTo(uri)) { | |
| LanguageServerPlugin.logInfo("Workspace PRE_DELETE/PRE_CLOSE disconnect for: " + uri); //$NON-NLS-1$ | |
| disconnectTextFileBuffer(uri); | |
| disconnect(uri); | |
| } | |
| synchronized (connectedDocuments) { | |
| DocumentContentSynchronizer dcs = connectedDocuments.get(uri); | |
| if (dcs != null) { | |
| // Mirror buffer.stateChanging -> documentAboutToBeSaved | |
| try { | |
| dcs.documentAboutToBeSaved(); | |
| } catch (Exception e) { | |
| LanguageServerPlugin.logError(e); | |
| } | |
| } | |
| if (isConnectedTo(uri)) { | |
| LanguageServerPlugin.logInfo("Workspace PRE_DELETE/PRE_CLOSE disconnect for: " + uri); //$NON-NLS-1$ | |
| disconnectTextFileBuffer(uri); | |
| disconnect(uri); | |
| } | |
| } |
Some framework like Xtext does not connect their document to buffer manager. So LSPEclipseUtils returns null for URI and IFile in such cases.
So we can try to adapt the IDocument to URI and IFile.
see #1500