Skip to content

Commit

Permalink
improvements according to the review: comments regarding stateful val…
Browse files Browse the repository at this point in the history 
…idation, typed and simplified test cases
  • Loading branch information
@JohannesMeierSE
JohannesMeierSE committed Aug 8, 2024
1 parent 507d4a8 commit 4b5d5e9
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 48 deletions.
13 changes: 13 additions & 0 deletions packages/langium/src/validation/validation-registry.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,12 @@ export class ValidationRegistry {
* As an example, for validating unique fully-qualified names of nodes in the AST,
* here the map for mapping names to nodes could be established.
* During the usual checks on the nodes, they are put into this map with their name.
*
* Note that this approach makes validations stateful, which is relevant e.g. when cancelling the validation.
* Therefore it is recommended to clear stored information
* _before_ validating an AST to validate each AST unaffected from other ASTs
* AND _after_ validating the AST to free memory by information which are no longer used.
*
* @param checkBefore a set-up function which will be called once before actually validating an AST
* @param thisObj Optional object to be used as `this` when calling the validation check functions.
*/
Expand All @@ -226,6 +232,12 @@ export class ValidationRegistry {
* As an example, for validating unique fully-qualified names of nodes in the AST,
* here the map with all the collected nodes and their names is checked
* and validation hints are created for all nodes with the same name.
*
* Note that this approach makes validations stateful, which is relevant e.g. when cancelling the validation.
* Therefore it is recommended to clear stored information
* _before_ validating an AST to validate each AST unaffected from other ASTs
* AND _after_ validating the AST to free memory by information which are no longer used.
*
* @param checkBefore a set-up function which will be called once before actually validating an AST
* @param thisObj Optional object to be used as `this` when calling the validation check functions.
*/
Expand All @@ -246,4 +258,5 @@ export class ValidationRegistry {
getChecksAfter(): ValidationPreparation[] {
return this.entriesAfter;
}

}
106 changes: 58 additions & 48 deletions packages/langium/test/validation/document-validator.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@
* terms of the MIT License, which is available in the project root.
******************************************************************************/

import { AstUtils, type AstNode, type ValidationChecks } from 'langium';
import { AstUtils } from 'langium';
import type { AstNode, ValidationAcceptor, ValidationChecks } from 'langium';
import { createServicesForGrammar } from 'langium/grammar';
import type { LangiumServices } from 'langium/lsp';
import type { ValidationResult } from 'langium/test';
import { validationHelper } from 'langium/test';
import { beforeAll, describe, expect, test } from 'vitest';
import { Position, Range } from 'vscode-languageserver';
import { createServicesForGrammar } from 'langium/grammar';
import { validationHelper } from 'langium/test';
import type { LangiumServices } from 'langium/lsp';

// Related to https://github.com/eclipse-langium/langium/issues/571
describe('Parser error is thrown on resynced token with NaN position', () => {
Expand Down Expand Up @@ -114,8 +115,8 @@ describe('Generic `AstNode` validation applies to all nodes', () => {

describe('Register Before/AfterDocument logic for validations with state', () => {

// the grammar
const grammar = `grammar NestedNamedNodes
entry Model:
(elements+=NamedNode)*;
Expand All @@ -126,6 +127,23 @@ describe('Register Before/AfterDocument logic for validations with state', () =>
terminal ID: /[_a-zA-Z][\\w_]*/;
`;

// AST types derived from the grammar
type NestedNamedNodesAstTypes = {
Model: Model;
NamedNode: NamedNode;
}
interface Model extends AstNode {
$type: 'Model';
elements: NamedNode[];
}
interface NamedNode extends AstNode {
$type: 'NamedNode';
$container: Model | NamedNode;
name: string;
children: NamedNode[];
}

// utilities
let validate: (input: string) => Promise<ValidationResult<AstNode>>;
let services: LangiumServices;

Expand All @@ -140,7 +158,7 @@ describe('Register Before/AfterDocument logic for validations with state', () =>
return '';
}

function rememberNamedNode(child: AstNode, fqnMap: Map<string, AstNode[]>): void {
function rememberNamedNode(child: NamedNode, fqnMap: Map<string, NamedNode[]>): void {
const fqn = getFQN(child);
let list = fqnMap.get(fqn);
if (!list) {
Expand All @@ -150,61 +168,53 @@ describe('Register Before/AfterDocument logic for validations with state', () =>
list.push(child);
}

function checkForDuplicates(fqnMap: Map<string, NamedNode[]>, accept: ValidationAcceptor): void {
for (const [key, value] of fqnMap.entries()) {
if (value.length >= 2) {
value.forEach(child => accept('error', `The FQN '${key}' is not unique.`, { node: child }));
}
}
}

// 1. realize validation in state-less way
function registerValidationForRootElement() {
const validationChecks: ValidationChecks<object> = {
// Note that there are no generated types like Model and NamedElement here
AstNode: (node, accept) => {
if ('name' in node) {
// ignore nodes with names here
} else {
// do the expensive validation once on the root node only (usually, this is registered like "Model: ..."!)
const fqnMap = new Map<string, AstNode[]>();
// collect the FQN of all nodes
// !! This 'streamAllContents' is saved in the alternative version of the validation !!
AstUtils.streamAllContents(node).forEach(child => {
rememberNamedNode(child, fqnMap);
});
// check for duplicates
for (const [key, value] of fqnMap.entries()) {
if (value.length >= 2) {
value.forEach(child => accept('error', `The FQN '${key}' is not unique.`, { node: child }));
}
}
}
const validationChecks: ValidationChecks<NestedNamedNodesAstTypes> = {
// do the expensive validation once on the root Model only
Model: (node, accept) => {
const fqnMap = new Map<string, NamedNode[]>();
// collect the FQN of all nodes
// !! This 'streamAllContents' is saved in the alternative version of the validation !!
AstUtils.streamAllContents(node).forEach(child => {
rememberNamedNode(child as NamedNode, fqnMap);
});
// check for duplicates
checkForDuplicates(fqnMap, accept);
},
};
services.validation.ValidationRegistry.register(validationChecks);
}

// 2. realize validation without an additional AST traversal by exploiting the stateful validation approach
function registerValidationBeforeAfter() {
const fqnMap = new Map<string, AstNode[]>();
const fqnMap = new Map<string, NamedNode[]>(); // this is the new state: remember all NamedNode nodes, classified by their name
services.validation.ValidationRegistry.registerBeforeDocument((_rootNode, _accept, _categories) => {
// clear everything to be sure
fqnMap.clear();
fqnMap.clear(); // clear everything to be sure when starting to validate an AST
});
const validationChecks: ValidationChecks<object> = {
AstNode: (node, _accept) => {
if ('name' in node) {
// only register the named nodes in the map
// Streaming the whole AST is not required with this approach, since the streaming is already done by the DocumentValidator!
rememberNamedNode(node, fqnMap);
} else {
// do nothing for the root node
}
services.validation.ValidationRegistry.register(<ValidationChecks<NestedNamedNodesAstTypes>>{
// register the named nodes in the map (but don't validate/check them now)
NamedNode: (node, _accept) => {
// Streaming the whole AST is not required with this approach, since the streaming is already done by the DocumentValidator!
rememberNamedNode(node, fqnMap);
},
};
services.validation.ValidationRegistry.register(validationChecks);
});
services.validation.ValidationRegistry.registerAfterDocument((_rootNode, accept, _categories) => {
// check for duplicates
for (const [key, value] of fqnMap.entries()) {
if (value.length >= 2) {
value.forEach(child => accept('error', `The FQN '${key}' is not unique.`, { node: child }));
}
}
fqnMap.clear();
// check for duplicates after all checks for the single nodes of the AST are done
checkForDuplicates(fqnMap, accept);
fqnMap.clear(); // free memory afterwards
});
}

// test cases to ensure, that both approaches produce the same validation hints
describe('Using the stateless validation', () => {
beforeAll(async () => {
services = await createServicesForGrammar({
Expand All @@ -227,7 +237,7 @@ describe('Register Before/AfterDocument logic for validations with state', () =>
});
});

describe('Using the validation with state', () => {
describe('Using the stateful validation', () => {
beforeAll(async () => {
services = await createServicesForGrammar({
grammar
Expand Down

0 comments on commit 4b5d5e9

Please sign in to comment.