Refactored document builder and affected analysis #1094

spoenemann · 2023-06-26T12:10:09Z

This PR fixes two issues that have not been noticed until recently:

When you open a DSL file, the whole workspace is validated even if the initial build skips validation. This is because DefaultDocumentBuilder.collectDocuments includes all documents for which state < DocumentState.Validated.
Even if you fix that, the whole workspace is validated in most cases:
- The language extension is often activated when a DSL file is opened for the first time.
- After starting and initializing the language server, the client immediately sends a didOpen notification, which triggers an update of the respective TextDocument (that's the standard behavior of TextDocuments provided by Microsoft).
- The update cancels the initial build if it's still running (if the workspace is sufficiently large, it's very likely that it's still running).
- The documents sent to the DocumentBuilder for the initial build are now continued with the new BuildOptions, which enable validation.

The issues are fixed with the following changes:

DocumentBuilder tracks the state of building (completed or not), including the last used build options.
This new information is now used to collect unfinished documents after a previous build was canceled.
Every document has its individual options during build; this is currently used to control validation.

Additional improvements:

API changes to simplify IndexManager and improve performance with large workspaces.
Added a public initialBuildOptions property to WorkspaceManager to control validation for the initial build.
Added a public updateBuildOptions property to DocumentBuilder to control validation for updates (document changes).
Added validation categories 'fast' (default) and 'slow'. You can set the category when you register a record of validation checks. The category can be selected in the build options. If no category is specified, all checks are executed.
Lexer, parser and linker errors are collected if the 'built-in' validation category is enabled.
Added more validation options to enable reporting only partial results, e.g. don't do custom validations if there are parsing errors.
Made document builder phases more consistent: the state of the document is always set by the document builder

msujew

This feels very heavy-handed and workaround-y. I don't think that the document should know about ephemeral build information (aside from its own state). I can also imagine that this property being set only once per completed build might lead to inconsistencies when multiple builds with different options are started. Maybe you want none to override all.

I would much rather like to see a change in BuildOptions.validationChecks to include a only-open value that only performs validation on files that are in the TextDocuments service. And use that as the default for the textDocument/open request we receive from the language client.

spoenemann · 2023-06-29T14:39:27Z

I moved the build state to DocumentBuilder and added validation options.

Edit: I improved the API again and am adding tests.

spoenemann · 2023-06-30T10:07:46Z

I used this opportunity to add tests for ValidationRegistry, DocumentValidator and DocumentBuilder (there weren't any).

Edit: I updated the PR description to capture the new approach and full set of changes.

sailingKieler

I have a few minor suggestions, no major objections

packages/langium/src/workspace/document-builder.ts

msujew

After rethinking my initial restraint to approve this, I'm happy with the new design. I'm sure there's still some space to iterate on the details, but this works well for now 👍

packages/langium/src/workspace/document-builder.ts

We had a longer (offline) discussion about this and decided that this needs another small refactoring

spoenemann · 2023-07-05T10:58:23Z

I changed where the state of a document is updated in the build process because that was quite inconsistent.

The new options merging logic in the build method of DocumentBuilder has become rather complex, @msujew @sailingKieler please have a look.

packages/langium/src/workspace/document-builder.ts

dhuebner · 2023-07-07T12:32:42Z

packages/langium/src/workspace/document-builder.ts

        // 4. Index references
        await this.runCancelable(documents, DocumentState.IndexedReferences, cancelToken, doc =>
            this.indexManager.updateReferences(doc, cancelToken)
        );
        // 5. Validation
-        const validateDocs = documents.filter(doc => this.shouldValidate(doc, options));
-        await this.runCancelable(validateDocs, DocumentState.Validated, cancelToken, doc =>
+        const toBeValidated = documents.filter(doc => this.shouldValidate(doc));


If shouldValidate returns false the document will never reach the Validated state. This state is checked in Line 113. Does that mean that the custom implementation inside the shouldValidate should take care about setting the build state? If so we should add this information to the documentation.

No, the document state is set in runCancelable (line 282). Custom implementations of shouldValidate don't need to check or modify that property.

toBeValidated will not contain the document for that shouldValidate returns false. For this document runCancelable will not be called. Or?

Right, so the state won't be set to Validated, but that's fine. The check you mentioned in line 113 is only for documents where validation is already partially done.

dhuebner · 2023-07-07T13:00:36Z

packages/langium/src/workspace/document-builder.ts

    protected async buildDocuments(documents: LangiumDocument[], options: BuildOptions, cancelToken: CancellationToken): Promise<void> {
+        this.prepareBuild(documents, options);
        // 0. Parse content


Could we numerate the phases in the comments using the Document State numbers?

Parse content => Parsed = 1
...

Validation => Validated = 6

I believe it will help to better understand what is going on when debugging here.

IMO the current numbering makes sense because it expresses that document of that state will be included in each phase. Example: documents with state Changed = 0 are included in the "Parse content" phase.

msujew

Looks good to me 👍

spoenemann added this to the v2.0.0 milestone Jun 26, 2023

spoenemann requested review from dhuebner and msujew June 26, 2023 12:10

spoenemann force-pushed the document-builder branch from 1f19308 to 6bfd0cb Compare June 26, 2023 12:14

msujew reviewed Jun 26, 2023

View reviewed changes

msujew added the documents Related to building documents and indexing label Jun 26, 2023

spoenemann requested a review from msujew June 29, 2023 14:38

spoenemann force-pushed the document-builder branch from d27f5bc to e261dec Compare July 3, 2023 06:30

sailingKieler approved these changes Jul 3, 2023

View reviewed changes

msujew previously approved these changes Jul 4, 2023

View reviewed changes

packages/langium/src/workspace/document-builder.ts Outdated Show resolved Hide resolved

spoenemann added 5 commits July 5, 2023 09:59

Refactored document builder and affected analysis

07d93a3

Further refactoring, added validation options

a112bab

Added tests

2a122a4

Further improvements

9ab6d1d

Added more comments

a22cd0c

spoenemann force-pushed the document-builder branch from a469e34 to f6a0667 Compare July 5, 2023 10:29

Added 'built-in' validation category, made build phases more consistent

71a89e4

spoenemann force-pushed the document-builder branch from f6a0667 to 71a89e4 Compare July 5, 2023 10:52

spoenemann requested a review from msujew July 5, 2023 10:57

msujew reviewed Jul 5, 2023

View reviewed changes

packages/langium/src/workspace/document-builder.ts Outdated Show resolved Hide resolved

Added validation result to build state

e3e932c

spoenemann requested a review from msujew July 6, 2023 06:50

dhuebner reviewed Jul 7, 2023

View reviewed changes

dhuebner approved these changes Jul 10, 2023

View reviewed changes

msujew approved these changes Jul 10, 2023

View reviewed changes

spoenemann merged commit 1b44292 into main Jul 10, 2023
3 checks passed

spoenemann deleted the document-builder branch July 10, 2023 13:47

spoenemann modified the milestones: v1.3.0, v2.0.0 Aug 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored document builder and affected analysis #1094

Refactored document builder and affected analysis #1094

spoenemann commented Jun 26, 2023 •

edited

Loading

msujew left a comment •

edited

Loading

spoenemann commented Jun 29, 2023 •

edited

Loading

spoenemann commented Jun 30, 2023 •

edited

Loading

sailingKieler left a comment

msujew left a comment

spoenemann commented Jul 5, 2023

dhuebner Jul 7, 2023

spoenemann Jul 10, 2023 •

edited

Loading

dhuebner Jul 10, 2023

spoenemann Jul 10, 2023

dhuebner Jul 7, 2023

spoenemann Jul 10, 2023

msujew left a comment

Refactored document builder and affected analysis #1094

Refactored document builder and affected analysis #1094

Conversation

spoenemann commented Jun 26, 2023 • edited Loading

msujew left a comment • edited Loading

Choose a reason for hiding this comment

spoenemann commented Jun 29, 2023 • edited Loading

spoenemann commented Jun 30, 2023 • edited Loading

sailingKieler left a comment

Choose a reason for hiding this comment

msujew left a comment

Choose a reason for hiding this comment

spoenemann commented Jul 5, 2023

dhuebner Jul 7, 2023

Choose a reason for hiding this comment

spoenemann Jul 10, 2023 • edited Loading

Choose a reason for hiding this comment

dhuebner Jul 10, 2023

Choose a reason for hiding this comment

spoenemann Jul 10, 2023

Choose a reason for hiding this comment

dhuebner Jul 7, 2023

Choose a reason for hiding this comment

spoenemann Jul 10, 2023

Choose a reason for hiding this comment

msujew left a comment

Choose a reason for hiding this comment

spoenemann commented Jun 26, 2023 •

edited

Loading

msujew left a comment •

edited

Loading

spoenemann commented Jun 29, 2023 •

edited

Loading

spoenemann commented Jun 30, 2023 •

edited

Loading

spoenemann Jul 10, 2023 •

edited

Loading