Unified the call of resetToState in the default document builder
#2071
Conversation
sailingKieler
left a comment
sailingKieler
left a comment
There was a problem hiding this comment.
Hey @JohannesMeierSE! Thank you so much for taking care about this.
I have some remarks (e.g. regarding compatibility) and some improvement ideas, see below.
What do you think about them?
| langiumDoc.diagnostics = undefined; | ||
| } | ||
| return langiumDoc; | ||
| } |
There was a problem hiding this comment.
This is a breaking change that we cannot/should not merge into a minor release of Langium 4.
But we can delegate back to the document builder's resetToState() method if a document is present. ... and keep a ToDo of consolidating with the next major release — if that will ever emerge...
| } | ||
| } | ||
|
|
||
| resetToDeleted<T extends AstNode>(document: LangiumDocument<T>): void { |
There was a problem hiding this comment.
I see your attempt to stay in line with resetToState but it sounds weird to me. What about
| resetToDeleted<T extends AstNode>(document: LangiumDocument<T>): void { | |
| cleanUpDeleted<T extends AstNode>(document: LangiumDocument<T>): void { |
| expect(documentB.localSymbols).not.toBe(undefined); | ||
| expect(documentB.diagnostics).toBe(undefined); | ||
| }; | ||
| await builder.update([documentA.uri], []); |
There was a problem hiding this comment.
The builder should behave the same when triggered this way, shouldn't it?
| await builder.update([documentA.uri], []); | |
| await builder.update([], []); |
| this.langiumDocuments.addDocument(changedDocument); | ||
| } | ||
| this.resetToState(changedDocument, DocumentState.Changed); | ||
| this.buildState.delete(changedUri.toString()); |
There was a problem hiding this comment.
I suggest to move this line into resetToState (at the beginning).
Then the doc.state < DocumentState.Linked/doc.state < DocumentState.Validated part below can be dropped completely I think.
| for (const doc of deletedDocs) { | ||
| deletedUris.push(doc.uri); | ||
| this.resetToDeleted(doc); | ||
| this.buildState.delete(doc.uri.toString()); |
There was a problem hiding this comment.
Consequently, this line can go into the method called before.
| expect(builder.actuallyBuilt).toHaveLength(2); | ||
| expect(builder.actuallyBuilt[0]).toBe('/testA.txt'); | ||
| expect(builder.actuallyBuilt[1]).toBe('/testB.txt'); | ||
| }); |
There was a problem hiding this comment.
Very nice tests.
Could you add a sentence or two in comment on the practical use cases you had in mind for the latter two tests (if they exist)?
I would love to see one more test that cancels the build, e.g. after the scope computation phase or once one document has been linked, and resumes via another call of
await builder.update([], []);
Motivation for this PR: In a Langium project, we annotated the
LangiumDocumentwith additional properties to remember special situations in order to improve performance.resetToState()in theDefaultDocumentBuilder.resetToState()is not called for documents which are explicitly given as changed documents to the document builder.resetToStateis called only for documents which "should be relinked" due to the changed documents.Therefore this PR aims to unify the call of
resetToStatein the default document builder.Details and additional changes:
resetToStatefor all explicitly given changed documents.LangiumDocuments.invalidateDocumentbecame superflous, since all its statements are already part ofresetToState.this.indexManager.removeContent(document.uri);was missing. This didn't caused problems, since the content is replaced later withindexManager.updateContent(...). The same counts forindexManager.removeReferencesandindexManager.updateReferences.shouldRelink).LangiumDocumentsnow focuses on storing documents (add, remove, get documents), while the document builder focuses on processing and updating document-related data. Disadvantage: This is a breaking API change!resetToStateis used to mark documents to be updated withdocument.state >= DocumentState.Linkedandupdateis called later without explicitly providing the URI of this document, this document is not rebuild.resetToDeletedfor deleted documents, this refactoring ensures a similar code structure.getDocumentswas not exposed in theLangiumDocumentsservice.Draft, since this PR serves for discussions and follow-up investigations.