eclipse-langium
Ensure diagnostics are published after cancellation
Fixes an issue discovered in https://github.com/eclipse-langium/langium/discussions/1562.
This is currently a draft, because I want to add some tests to confirm the behavior.
cc @cdietrich Can you try including this change into your project? It should now ensure that diagnostics are published as soon as any document reaches the Validated state.
it looks much better now. but i very seldomly still see issues. am not sure if other phases might have similar side effects need also to figure out if this is a server side or client side problem
i indeed can see
- all documents are in state 6 (validated)
- there are references that cannot be resolved.
it happens very very rarely. i dont have deeper insights yet. i guess i need to reestablish more extensive logging ontop of your changes again
the two effects i see.
- doLink skips some references cause they are in state linking error
- sometimes elements are not in the index at all.
=> there seem to be more problems, but as indicated due to bad reproducibility terribly hard to analyze
@msujew
get ref() { this one may turn references into errors, but wont push them to document.references. (also for foreign documents)
thus i wonder if then, after a cancellation.
the unlink on the forein document, that was not linked yet,
will really unlink it. if i check the code, it wont
or we even wont attempt to unlink, as shouldRelink also doesnt check that
hmmmmmmm
have found this one:
// Some of these documents can be pretty large, so parsing them again can be quite expensive.
// Therefore, we only parse if the text has actually changed.
if (oldText !== text) {
```
=> the current way of invalidateDocument might not be sufficicent?
patched the code and the trap hits
// 0. Parse content
await this.runCancelable(documents, DocumentState.Parsed, cancelToken, async doc => {
await this.langiumDocumentFactory.update(doc, cancelToken)
for (const node of AstUtils.streamAst(doc.parseResult.value)) {
AstUtils.streamReferences(node).forEach(refInfo => {
const ref = refInfo.reference as DefaultReference
if (ref._ref !== undefined) {
if (isLinkingError(ref._ref)) {
console.log(`mimimi langiumDocumentFactory.update failure ${ref._ref.message} ${doc.uri.toString()}`)
}
}
})
}
})
created https://github.com/eclipse-langium/langium/issues/1567 for further discussion
@cdietrich Thanks, I was able to reproduce. Newest version available at 3.2.0-next.9134e27.
@msujew i still think the problem is also that unlink/shouldRelink only evaluates document.references, but not the references that were attempted to link with the get ref call before, which does not maintain document.referencs.
i have tried my unlink changes on top of your invalidateDocument change. but i still see broken links after parsing step. need to check
problem is the js you published. it has if (langiumDoc) { // const linker = this.serviceRegistry.getServices(uri).references.Linker; // linker.unlink(langiumDoc);
@cdietrich After uncommenting the change, everything works as expected? I probably forgot to recompile.
@cdietrich Can you try 3.2.0-next.bb9f2d3 without any special modifications? I.e. no special unlink. The issue was mainly that we were missing this code:
https://github.com/eclipse-langium/langium/blob/bb9f2d38a927b382870a309c3f18e46099be4e75/packages/langium/src/references/linker.ts#L165-L166
when resolving references on-the-fly.
tests look good. will continue the analyze the deleted case.
maybe the didChangeContent and the didChangeWatchedFiles reace with each other.
delete case seems to be a client side problem. so here we are looking good
Shall we add this to the 3.1.0 milestone and make another patch release?
