eclipse-jdtls / eclipse.jdt.ls

Java language server
1.81k stars 403 forks source link

jdt ls should not assume that the file exists on the disk #1815

Open yyoncho opened 3 years ago

yyoncho commented 3 years ago

As per lsp spec the servers are supposed to use the text in didOpen notification and avoid reading the file content from the disk.

Recent versions of jdtls (not sure where the regression comes from) throw the following error:

[Error - 7:50:48 AM] Jul 15, 2021, 7:50:48 AM Problem with folding range for /test-project/src/main/java/temp/barrrrrrr.java
barrrrrrr.java [in temp [in src/main/java [in test-project]]] does not exist
Java Model Exception: Java Model Status [barrrrrrr.java [in temp [in src/main/java [in test-project]]] does not exist]
    at org.eclipse.jdt.internal.core.JavaElement.newJavaModelException(JavaElement.java:587)
    at org.eclipse.jdt.internal.core.Openable.generateInfos(Openable.java:254)
    at org.eclipse.jdt.internal.core.JavaElement.openWhenClosed(JavaElement.java:600)
    at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:330)
    at org.eclipse.jdt.internal.core.JavaElement.getElementInfo(JavaElement.java:316)
    at org.eclipse.jdt.internal.core.CompilationUnit.getSourceRange(CompilationUnit.java:955)
    at org.eclipse.jdt.ls.core.internal.handlers.FoldingRangeHandler.computeFoldingRanges(FoldingRangeHandler.java:79)
    at org.eclipse.jdt.ls.core.internal.handlers.FoldingRangeHandler.foldingRange(FoldingRangeHandler.java:70)
    at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$29(JDTLanguageServer.java:866)
    at org.eclipse.jdt.ls.core.internal.handlers.JDTLanguageServer.lambda$52(JDTLanguageServer.java:1008)
    at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:642)
    at java.base/java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:479)
    at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)
    at java.base/java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1020)
    at java.base/java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1656)
    at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1594)
    at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:183)

To reproduce in vscode:

  1. Open a new file Foo.java and make sure it has no content.
  2. Delete the file outside of vscode
  3. Shutdown vscode and then after you start vscode again try to do anything in Foo.java - it throws the error.
rgrunber commented 3 years ago

I'm not able to reproduce with the exact instructions above. The file disappears for me before I have a chance to type into it (whether I restart VS Code or just reload the windows). deleted_file_gone

I'm am able to reproduce this error if I simply type after the file has been marked as deleted by the UI. That's certainly a bug we should fix.

I don't think the issue is that the language server is holding a stale instance. I can see that the client sent a didChangeWatchedFiles() indicating that the file should be deleted, so it detects the external deletion. However, the client still sends the didChange() even after that.

yyoncho commented 3 years ago

Not sure if you are hitting that same issue. In emacs, when you create a new buffer(file) it doesn't actually exist on the disk. Then we send didOpen and jdtls goes crazy. In vscode, when you create a new file it is present on the disk. As per spec, the server should not assume that the file is being present on the disk but it should use the content sent by the client.

mfussenegger commented 3 years ago

I can reproduce the same problem also with the neovim lsp client. In nvim-jdtls I added a workaround to implicitly save the file before the lsp clients sents the didOpen. Would be great if this could be fixed.

rgrunber commented 3 years ago

Looks like both didChange and didOpen trigger foldingRange later on, so it could be that we're both seeing the same error, but maybe certain clients try to prevent a didOpen from happening on a deleted file, but not didChange.

The stack shows https://github.com/eclipse/eclipse.jdt.ls/blob/master/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/handlers/FoldingRangeHandler.java#L70 is called, which would indicate the the ITypeRoot (likely a CompilationUnit) is not null.

Going through the stacktrace around CompilationUnit.validateExistence(..) and Resource.isAccessible() , I found https://git.eclipse.org/c/platform/eclipse.platform.resources.git/tree/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java#n162 :

When external parties make changes to the files on disk, this representation becomes out of sync. A local refresh reconciles the state of the files on disk with this tree (@link {@link IResource#refreshLocal(int, IProgressMonitor)}).

rgrunber commented 3 years ago

Maybe something like :

diff --git a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java
index 4d691a46..ad47b113 100644
--- a/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java
+++ b/org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JDTUtils.java
@@ -189,7 +189,7 @@ public final class JDTUtils {
        }

        public static ICompilationUnit resolveCompilationUnit(IFile resource) {
-               if(resource != null){
+               if (resource != null && resource.exists()) {^M
                        if(!ProjectUtils.isJavaProject(resource.getProject())){
                                return null;
                        }

would guard against the NPEs . I still see some tests failing with this so would have to investigate.

bstaletic commented 3 years ago

As I mentioned in #1939, this regression happened with https://github.com/eclipse/eclipse.jdt.ls/pull/1445

That PR did introduce "if file doesn't exist, return". Hope this can be of some help.