Ensure diagnostics are published after cancellation

[msujew]

Related to the discussion in https://github.com/eclipse-langium/langium/discussions/1562.

Fixes three core issues in the document build lifecycle:

1. Calls unlink on invalidateDocument. In case of change notifications on unchanged documents, this previously led to stale references in the document.
2. In case the build was cancelled during the linking phase, on-the-fly resolved references weren't added to the list of references on a document. These are now correctly added once they are resolved.
3. Adds a new onDocumentPhase callback listener for the DocumentBuilder. This is similar to onBuildPhase but triggers on every document.

[msujew]

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.

[cdietrich]

@msujew could you provide an alpha, this would make adoption the easiest

[msujew]

@cdietrich Published as 3.2.0-next.2191297

[cdietrich]

thx. am gonna do some test rounds

[cdietrich]

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

[cdietrich]

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

[cdietrich]

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

[cdietrich]

@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

[cdietrich]

[cdietrich]

another qustion: should changed files always be reparsed/unlinked?

[cdietrich]

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?

[cdietrich]

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()}`)
            }
          }
        })
      }
    })

[cdietrich]

created https://github.com/eclipse-langium/langium/issues/1567 for further discussion

[msujew]

@cdietrich Thanks, I was able to reproduce. Newest version available at 3.2.0-next.9134e27.

[cdietrich]

@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.

[msujew]

@cdietrich Yep, noticed that as well. Currently working on that :)

[cdietrich]

i have tried my unlink changes on top of your invalidateDocument change. but i still see broken links after parsing step. need to check

[cdietrich]

problem is the js you published. it has if (langiumDoc) { // const linker = this.serviceRegistry.getServices(uri).references.Linker; // linker.unlink(langiumDoc);

[msujew]

@cdietrich After uncommenting the change, everything works as expected? I probably forgot to recompile.

[cdietrich]

will check.

[cdietrich]

your invalidate with my unlink look promising.

[msujew]

@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.

[cdietrich]

tests look good. will continue the analyze the deleted case.

maybe the didChangeContent and the didChangeWatchedFiles reace with each other.

[cdietrich]

delete case seems to be a client side problem. so here we are looking good

[spoenemann]

Shall we add this to the 3.1.0 milestone and make another patch release?

[cdietrich]

a patch release would be welcome 🙏