Conversation
|
@cursor review |
|
bugbot run |
Sorry, should've moved this to draft. Already got some feedback through @emhgit, and will fix up this pr to make the code nicer soon. |
Divide into seperate files, move jdtls to standalone endpoint, add .project generation
add requirements.txt
|
@cursor review |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
adapters/jdtls.py
Outdated
| elif method == "textDocument/didChange": | ||
| text = obj["params"]["contentChanges"][-1]["text"] | ||
| with open(self._jdtls_main_path, "w", encoding="utf-8") as f: | ||
| f.write(text) |
There was a problem hiding this comment.
🔴 didChange handler writes incremental text delta as full file content
The didChange handler in JdtlsAdapter.ws_to_lsp writes contentChanges[-1]["text"] to the file as if it were the full document content. This is only correct when the language server uses TextDocumentSyncKind.Full. Eclipse JDT.LS defaults to TextDocumentSyncKind.Incremental (kind 2), meaning contentChanges entries contain incremental edits with range + text (where text is only the replacement for that range, not the full document).
Root Cause
When jdtls returns incremental sync in its initialize response, the client sends incremental didChange notifications. Each contentChanges entry looks like:
{"range": {"start": {"line": 0, "character": 5}, "end": {"line": 0, "character": 5}}, "text": "x"}At adapters/jdtls.py:60, the code does:
text = obj["params"]["contentChanges"][-1]["text"]
with open(self._jdtls_main_path, "w", encoding="utf-8") as f:
f.write(text)This writes only the delta (e.g., a single character "x") as the entire file content, overwriting the real document. This corrupts the on-disk file that jdtls reads, causing the language server to operate on incorrect source code.
Impact: Every keystroke after the first didOpen would corrupt the file on disk, making jdtls diagnostics, completions, and other features incorrect.
Prompt for agents
In adapters/jdtls.py, the didChange handler at lines 59-62 assumes contentChanges contains the full document text, but Eclipse JDT.LS uses incremental sync by default. To fix this, either:
1. Force full document sync by intercepting the initialize response in lsp_to_ws and overriding the textDocumentSync capability to Full (kind 1), so the client always sends full document content in didChange. This is the simplest fix.
2. Or, maintain the file content in memory and apply incremental changes (range-based text replacements) before writing the file. This is more complex but preserves incremental sync benefits.
Option 1 is recommended: in lsp_to_ws, parse the initialize response (look for id matching the initialize request), find result.capabilities.textDocumentSync and set it to 1 (Full) before forwarding to the client.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
seems true, however it works regardless? is this a necessary fix? would add a lot of complication
lsp_server.py
Outdated
| allow_concurrent_inputs=20, | ||
| timeout=60 * 60, | ||
| ) | ||
| @concurrent(max_inputs=20) |
There was a problem hiding this comment.
I think it's more conventional to do @modal.concurrent
lsp_server.py
Outdated
| memory=1024, | ||
| cpu=1, |
There was a problem hiding this comment.
don't think you need to specify this -- you should be able to just burst up to this amount
lsp_server.py
Outdated
| single_use_containers=True, | ||
| memory=1024, | ||
| cpu=1, | ||
| scaledown_window=60 * 60, |
There was a problem hiding this comment.
why is scaledown window so high? does this even do anything if single_use_containers is True? Also, why does it have to be a single use container?
There was a problem hiding this comment.
TL;DR: good questions, set @modal.concurrent to 3 and remove the single_use_containers and scaledown_window arguments.
from what i read about JDTLS, i assumed that using one container per user would prevent indexing errors; that's why i recommended sahil include the single_use_containers and scaledown_window arguments. however, i just did some testing, and it seems to work alright with multiple users in one container (which makes sense because each user gets a new process upon connection, and each process has an isolated workspace). so, i would set @modal.concurrent to 3 and remove the single_use_containers and scaledown_window arguments.
adapters/jdtls.py
Outdated
| if self._jdtls_main_path: | ||
| if method == "textDocument/didOpen": | ||
| text = obj["params"]["textDocument"]["text"] | ||
| with open(self._jdtls_main_path, "w", encoding="utf-8") as f: |
There was a problem hiding this comment.
is there any reason why we actually need to write out the files to disk? ChatGPT claims that you don't need to do this for simple cases: https://chatgpt.com/share/69a5524c-8958-8008-a164-b6c26805f462 of course ChatGPT could be wrong though. I did not investigate myself
There was a problem hiding this comment.
fwiw I did not need to do this for C++ language server which is why I would be surprised if we had to do it for Java. (But I haven't investigated myself)
There was a problem hiding this comment.
based on my own research of the docs and testing on my own, (see https://github.com/eclipse-jdtls/eclipse.jdt.ls/blob/main/README.md#running-from-the-command-line) it seems -data is required. if you know of a way to bypass it though, tell me and im happy to implement it.
There was a problem hiding this comment.
Even if -data is required is it necessary to actually write out every keystroke to disk? i.e. if you got rid of the file writing portion does this still work?
(Presumably you would need the data folder to actually exist & have project setup files though; the only thing you would get rid of is actually writing the files to disk)
There was a problem hiding this comment.
Wait great point, that would explain why things worked despite updating being basically broken. I can probably just create main. Java as an empty file or something, which means it could also lose its limit of 3 concurrency, simplifying the IDE code too since just one url. Will check if this is true tomorrow
There was a problem hiding this comment.
I'm not even sure if creating Main.java is strictly needed (but maybe it is!)
I guess one intuition for why writing to file on every keystroke should not be necessary is IDEs do not write to file on every keystroke, but their intellisense works just fine
|
Alright I made the basic changes in the main file, however it seems it is not possible to run jdtls without the files themselves to my knowledge, and the code detected by the bot although true, seems to work anyway and a solution would be unnecessary and very overcomplicated. |
|
You were completely correct @thecodingwizard , so I have removed a bunch of now unnecessary code. Also, since we no longer need to write code to the disk, I think we can also merge the 2 seperate modal servers into one. I really dont like how jdtls is alone just so it can have a different concurrency, and now that disk writing was made unnecessary, I don't see any other reason for seperate concurrency. |
|
Added one more commit that combines the project and data directory, because I realize there is no real reason for them to be two seperate paths |
3 participants
Note: This code isn't as clean as I hoped to make it, it simply has blocks of if checks inside the lsp server. If anyone has a cleaner way to implement it, send it my way and I will fix up the code. Also, in order to test this pr, you will also have to change a line in ide utils.ts, swapping
uri:
/workspace/main.${language},to be
uri: language === 'java' ? 'file:///workspace/main.java' :
/workspace/main.${language},and of course using a ternary operator like
language === 'cpp'
? 'clangd'
: language === 'java'
? 'jdtls'
: 'pyright'
}`;
so that jdtls also functions.
Additional Concern:
Not sure if this will work for many users, as it was hard to test for that locally. It does use temporary directories, pretty much identical to the cpp compiler options thing, so I think it would, but just thought I would mention that.