didChange shouldn't recompile

We respond to textDocument/didChange with a recompile.

We should only compile on textDocument/didSave.

If textDocument/didChange doesn't recompile, how do you get compilation and syntax errors on unsaved changes?

Also, if you no longer want to compile these incremental changes, keeping track of the changes with SourceWriter becomes useless.

natacha!

compiling on every change introduces performance concerns

Andrew! 😄
Might be worth looking into compiling only after X amount of time has passed if you're sending a lot of textDocument/didChange. Otherwise I think you should consider removing keeping track of incremental changes for now since its not being used.

Aside from the option of doing that, you'd still want unsaved state for things like completions

feel we should just ask the frontend to send accumulated changes every 5 second, otherwise our language servers won't work with anything that doesn't have autosave

we aren't the first to do this: javac LS doesn't recompile on change, only on save

Agree that that's a concern, but right now our contract is pretty simple.

You save-you get diagnostics.

What would be the equivalent? Send changes every 5 seconds, BUT also if the user asks for completions, AND also if the user saves, AND {insert case I haven't thought of}

Completion uses the symbols in the CompilerWrapper but these are only updated when the code is compiled. This is because symbols are parsed from the successfully compiled AST (see:

language-servers/language-server-commons/src/main/java/com/palantir/ls/DefaultCompilerWrapper.java

Line 93 in c485a6b

parser.parseAllSymbols();

). If there are changes written to file by SourceWriter they will not be reflected in the symbols.
An alternative would be to compile when a completion request is received, but that might make it laggy and is IMO not the best approach.

updated symbols aside, it's still important to use didChange notifications to update the document because completion requests only provide text document positions

so although our symbols may be out of date, our document won't, & we'll be able to service completion requests using the set of symbols parsed on last save

That's a good point but for a different reason: the text document positions are not compared to the file that SourceWriter keeps track of, they are compared to the Symbols that are stored - which are (now) based on the old version of the file! So everything from completion to goto definition will not work correctly.

maybe we should change that, maybe text document positions should be compared to the SourceWriter?

How do you provide semantic meaning to a plain text file? Using a compiler. That's already what we're doing. There's really no way around this.