Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend the API of the validation registry for more performant custom validations #1614

Merged

Conversation

JohannesMeierSE
Copy link
Contributor

When writing custom validations, often it is required to stream the (whole or parts of the) AST inside the registered checks, e.g. for checking unique names or for identifying circles/loops in relationships between the AST nodes (calls, inheritance). For huge ASTs, this approach has the drawback, that the AST is traversed multiple times, which decreases performance.

This PR proposes to register additional "checks" which are executed once before or after validating an AST/Langium document. The idea is to exploit the AST traversal already done by the DocumentBuilder and to remember only relevant information during AstNode-specific checks. The collected information is evaluated once after all checks are done.
The expected benefit are faster validations by less AST traversals inside custom checks for big ASTs.

In order to demonstrate the new API, I wrote simple test cases to check unique fully-qualified names and implemented the validation twice, once with the existing API (and with streamAllContents) and once with the new API (and without streamAllContents).

Aspects to consider during the review:

  • Did I found the best names for the new API?
  • Drawback is that the validations are not state-less anymore.
  • It is no breaking change, since the existing checks are unchanged and only some additional checks can be registered.

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, the changes look good to me! Just two remarks below.


getChecksAfter(): ValidationPreparation[] {
return this.entriesAfter;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd declare the properties as public readonly rather than using such getter methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with public properties is, that users could register checks directly in this property and without calling registerBeforeDocument(...), which bypasses our internal handling of exceptions.
Therefore I decided to keep the "simple getters".

Copy link
Contributor

@spoenemann spoenemann Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The getter does not change that: someone could still push elements to the returned array. If you really want to prevent that, you'd need to clone the array. But an easier and faster solution is just to document the property/method saying you shouldn't manipulate the array.

If you want to keep a getter, I suggest using a get property:

    get checksAfter(): ValidationPreparation[] {
        return this.entriesAfter;
    }

This is about code style: methods like getChecksAfter() are common in Java, while in JS/TS either value properties or get properties are used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

someone could still push elements to the returned array. If you really want to prevent that, you'd need to clone the array.

You are right.

I replaced getChecksBefore() by get checksBefore(), since it is important to have a unified code style.

Beyond that, I personally prefer to have the new getChecksBefore() and the already existing getChecks() in the same style, but I am fine with get ... as well.

@JohannesMeierSE
Copy link
Contributor Author

Thanks for the discussions in the Langium dev meeting and @spoenemann for your review! I just pushed the improvements.

Copy link
Contributor

@spoenemann spoenemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks. See comment thread above about code style of getter methods.

@JohannesMeierSE
Copy link
Contributor Author

@spoenemann I pushed the fix and rebased to solve the conflicts with main.

@JohannesMeierSE
Copy link
Contributor Author

FYI: We need this feature in Typir. Therefore we are looking forward to a Langium release with this feature!

@spoenemann spoenemann merged commit b2b1cbe into eclipse-langium:main Oct 25, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants