Validation for multiple assignments takes rewrite actions into account now (#1766)

JohannesMeierSE · web-flow · commit e004619e1428 · 2024-12-11T16:08:40.000+01:00
* recursively take created objects in groups/alternatives into account, added some more specific test cases * simplified and reworked the validation to support actions as top-level elements in parser rules as well (was another bug) * derived test cases from the discussion in #1756 * refactoring: inlined method * improved error logging
diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts
@@ -877,18 +877,13 @@ export class LangiumGrammarValidator {
     checkOperatorMultiplicitiesForMultiAssignments(rule: ast.ParserRule, accept: ValidationAcceptor): void {
         // for usual parser rules AND for fragments, but not for data type rules!
         if (!rule.dataType) {
-            this.checkOperatorMultiplicitiesForMultiAssignmentsLogic([rule.definition], accept);
+            this.checkOperatorMultiplicitiesForMultiAssignmentsIndependent([rule.definition], accept);
         }
     }
 
-    private checkOperatorMultiplicitiesForMultiAssignmentsLogic(startNodes: AstNode[], accept: ValidationAcceptor): void {
-        // new map to store usage information of the assignments
-        const map: Map<string, AssignmentUse> = new Map();
-
-        // top-down traversal for all starting nodes
-        for (const node of startNodes) {
-            this.checkAssignmentNumbersForNode(node, 1, map, accept);
-        }
+    private checkOperatorMultiplicitiesForMultiAssignmentsIndependent(startNodes: AstNode[], accept: ValidationAcceptor, map: Map<string, AssignmentUse> = new Map()): void {
+        // check all starting nodes
+        this.checkOperatorMultiplicitiesForMultiAssignmentsNested(startNodes, 1, map, accept);
 
         // create the warnings
         for (const entry of map.values()) {
@@ -906,72 +901,79 @@ export class LangiumGrammarValidator {
         }
     }
 
-    private checkAssignmentNumbersForNode(currentNode: AstNode, parentMultiplicity: number, map: Map<string, AssignmentUse>, accept: ValidationAcceptor) {
-        // the current element can occur multiple times => its assignments can occur multiple times as well
-        let currentMultiplicity = parentMultiplicity;
-        if (ast.isAbstractElement(currentNode) && isArrayCardinality(currentNode.cardinality)) {
-            currentMultiplicity *= 2; // note, that the result is not exact (but it is sufficient for the current case)!
-        }
+    private checkOperatorMultiplicitiesForMultiAssignmentsNested(nodes: AstNode[], parentMultiplicity: number, map: Map<string, AssignmentUse>, accept: ValidationAcceptor): boolean {
+        let resultCreatedNewObject = false;
+        // check all given elements
+        for (let i = 0; i < nodes.length; i++) {
+            const currentNode = nodes[i];
+
+            // Tree-Rewrite-Actions are a special case: a new object is created => following assignments are put into the new object
+            if (ast.isAction(currentNode) && currentNode.feature) {
+                // (This does NOT count for unassigned actions, i.e. actions without feature name, since they change only the type of the current object.)
+                const mapForNewObject = new Map();
+                storeAssignmentUse(mapForNewObject, currentNode.feature, 1, currentNode); // remember the special rewriting feature
+                // all following nodes are put into the new object => check their assignments independently
+                this.checkOperatorMultiplicitiesForMultiAssignmentsIndependent(nodes.slice(i + 1), accept, mapForNewObject);
+                resultCreatedNewObject = true;
+                break; // breaks the current loop
+            }
 
-        // assignment
-        if (ast.isAssignment(currentNode)) {
-            storeAssignmentUse(map, currentNode.feature, 1 * currentMultiplicity, currentNode);
-        }
+            // all other elements don't create new objects themselves:
 
-        // Search for assignments in used fragments as well, since their property values are stored in the current object.
-        // But do not search in calls of regular parser rules, since parser rules create new objects.
-        if (ast.isRuleCall(currentNode) && ast.isParserRule(currentNode.rule.ref) && currentNode.rule.ref.fragment) {
-            this.checkAssignmentNumbersForNode(currentNode.rule.ref.definition, currentMultiplicity, map, accept);
-        }
+            // the current element can occur multiple times => its assignments can occur multiple times as well
+            let currentMultiplicity = parentMultiplicity;
+            if (ast.isAbstractElement(currentNode) && isArrayCardinality(currentNode.cardinality)) {
+                currentMultiplicity *= 2; // note that the result is not exact (but it is sufficient for the current use case)!
+            }
 
-        // rewriting actions are a special case for assignments
-        if (ast.isAction(currentNode) && currentNode.feature) {
-            storeAssignmentUse(map, currentNode.feature, 1 * currentMultiplicity, currentNode);
-        }
+            // assignment
+            if (ast.isAssignment(currentNode)) {
+                storeAssignmentUse(map, currentNode.feature, currentMultiplicity, currentNode);
+            }
 
-        // look for assignments to the same feature nested within groups
-        if (ast.isGroup(currentNode) || ast.isUnorderedGroup(currentNode) || ast.isAlternatives(currentNode)) {
-            const mapAllAlternatives: Map<string, AssignmentUse> = new Map(); // store assignments for Alternatives separately
-            let nodesForNewObject: AstNode[] = [];
-            // check all elements inside the current group
-            for (const child of currentNode.elements) {
-                if (ast.isAction(child)) {
-                    // Actions are a special case: a new object is created => following assignments are put into the new object
-                    // (This counts for rewriting actions as well as for unassigned actions, i.e. actions without feature name)
-                    if (nodesForNewObject.length > 0) {
-                        // all collected nodes are put into the new object => check their assignments independently
-                        this.checkOperatorMultiplicitiesForMultiAssignmentsLogic(nodesForNewObject, accept);
-                        // is it possible to have two or more Actions within the same parser rule? the grammar allows that ...
-                        nodesForNewObject = [];
-                    }
-                    // push the current node into a new object
-                    nodesForNewObject.push(child);
-                } else {
-                    // for non-Actions
-                    if (nodesForNewObject.length > 0) {
-                        // nodes go into a new object
-                        nodesForNewObject.push(child);
-                    } else {
-                        // count the relevant child assignments
-                        if (ast.isAlternatives(currentNode)) {
-                            // for alternatives, only a single alternative is used => assume the worst case and take the maximum number of assignments
-                            const mapCurrentAlternative: Map<string, AssignmentUse> = new Map();
-                            this.checkAssignmentNumbersForNode(child, currentMultiplicity, mapCurrentAlternative, accept);
-                            mergeAssignmentUse(mapCurrentAlternative, mapAllAlternatives, (s, t) => Math.max(s, t));
-                        } else {
-                            // all members of the group are relavant => collect them all
-                            this.checkAssignmentNumbersForNode(child, currentMultiplicity, map, accept);
-                        }
+            // Search for assignments in used fragments as well, since their property values are stored in the current object.
+            // But do not search in calls of regular parser rules, since parser rules create new objects.
+            if (ast.isRuleCall(currentNode) && ast.isParserRule(currentNode.rule.ref) && currentNode.rule.ref.fragment) {
+                const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested([currentNode.rule.ref.definition], currentMultiplicity, map, accept);
+                resultCreatedNewObject = createdNewObject || resultCreatedNewObject;
+            }
+
+            // look for assignments to the same feature nested within groups
+            if (ast.isGroup(currentNode) || ast.isUnorderedGroup(currentNode)) {
+                // all members of the group are relavant => collect them all
+                const mapGroup: Map<string, AssignmentUse> = new Map(); // store assignments for Alternatives separately
+                const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested(currentNode.elements, 1, mapGroup, accept);
+                mergeAssignmentUse(mapGroup, map, createdNewObject
+                    ? (s, t) => (s + t)                         // if a new object is created in the group: ignore the current multiplicity, since a new object is created for each loop cycle!
+                    : (s, t) => (s * currentMultiplicity + t)   // otherwise as usual: take the current multiplicity into account
+                );
+                resultCreatedNewObject = createdNewObject || resultCreatedNewObject;
+            }
+
+            // for alternatives, only a single alternative is used => assume the worst case and take the maximum number of assignments
+            if (ast.isAlternatives(currentNode)) {
+                const mapAllAlternatives: Map<string, AssignmentUse> = new Map(); // store assignments for Alternatives separately
+                let countCreatedObjects = 0;
+                for (const child of currentNode.elements) {
+                    const mapCurrentAlternative: Map<string, AssignmentUse> = new Map();
+                    const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested([child], 1, mapCurrentAlternative, accept);
+                    mergeAssignmentUse(mapCurrentAlternative, mapAllAlternatives, createdNewObject
+                        ? (s, t) => Math.max(s, t)                         // if a new object is created in an alternative: ignore the current multiplicity, since a new object is created for each loop cycle!
+                        : (s, t) => Math.max(s * currentMultiplicity, t)   // otherwise as usual: take the current multiplicity into account
+                    );
+                    if (createdNewObject) {
+                        countCreatedObjects++;
                     }
                 }
-            }
-            // merge alternatives
-            mergeAssignmentUse(mapAllAlternatives, map);
-            if (nodesForNewObject.length >= 1) {
-                // these nodes are put into a new object => check their assignments independently
-                this.checkOperatorMultiplicitiesForMultiAssignmentsLogic(nodesForNewObject, accept);
+                // merge alternatives
+                mergeAssignmentUse(mapAllAlternatives, map);
+                // since we assume the worst case, we define, that the entire Alternatives node created a new object, if ALL its alternatives created a new object
+                if (countCreatedObjects === currentNode.elements.length) {
+                    resultCreatedNewObject = true;
+                }
             }
         }
+        return resultCreatedNewObject; // indicates, whether a new object was created
     }
 
     checkInterfacePropertyTypes(interfaceDecl: ast.Interface, accept: ValidationAcceptor): void {
@@ -1243,6 +1245,7 @@ function mergeAssignmentUse(mapSoure: Map<string, AssignmentUse>, mapTarget: Map
             target.counter = counterOperation(source.counter, target.counter);
         } else {
             mapTarget.set(key, source);
+            source.counter = counterOperation(source.counter, 0);
         }
     }
     mapSoure.clear();
diff --git a/packages/langium/src/test/langium-test.ts b/packages/langium/src/test/langium-test.ts
@@ -4,24 +4,24 @@
  * terms of the MIT License, which is available in the project root.
  ******************************************************************************/
 
+import * as assert from 'node:assert';
 import type { CompletionItem, CompletionList, Diagnostic, DocumentSymbol, FoldingRange, FormattingOptions, Range, ReferenceParams, SemanticTokensParams, SemanticTokenTypes, TextDocumentIdentifier, TextDocumentPositionParams, WorkspaceSymbol } from 'vscode-languageserver-protocol';
+import { DiagnosticSeverity, MarkupContent } from 'vscode-languageserver-types';
+import { normalizeEOL } from '../generate/template-string.js';
+import type { LangiumServices, LangiumSharedLSPServices } from '../lsp/lsp-services.js';
+import { SemanticTokensDecoder } from '../lsp/semantic-token-provider.js';
+import type { ParserOptions } from '../parser/langium-parser.js';
 import type { LangiumCoreServices, LangiumSharedCoreServices } from '../services.js';
 import type { AstNode, CstNode, Properties } from '../syntax-tree.js';
-import { type LangiumDocument, TextDocument } from '../workspace/documents.js';
-import type { BuildOptions } from '../workspace/document-builder.js';
 import type { AsyncDisposable } from '../utils/disposable.js';
-import type { LangiumServices, LangiumSharedLSPServices } from '../lsp/lsp-services.js';
-import type { ParserOptions } from '../parser/langium-parser.js';
-import { DiagnosticSeverity, MarkupContent } from 'vscode-languageserver-types';
-import { escapeRegExp } from '../utils/regexp-utils.js';
-import { URI } from '../utils/uri-utils.js';
+import { Disposable } from '../utils/disposable.js';
 import { findNodeForProperty } from '../utils/grammar-utils.js';
-import { SemanticTokensDecoder } from '../lsp/semantic-token-provider.js';
-import * as assert from 'node:assert';
+import { escapeRegExp } from '../utils/regexp-utils.js';
 import { stream } from '../utils/stream.js';
-import { Disposable } from '../utils/disposable.js';
-import { normalizeEOL } from '../generate/template-string.js';
+import { URI } from '../utils/uri-utils.js';
 import { DocumentValidator } from '../validation/document-validator.js';
+import type { BuildOptions } from '../workspace/document-builder.js';
+import { TextDocument, type LangiumDocument } from '../workspace/documents.js';
 
 export interface ParseHelperOptions extends BuildOptions {
     /**
@@ -682,7 +682,7 @@ export function filterByOptions<T extends AstNode = AstNode, N extends AstNode =
 
 export function expectNoIssues<T extends AstNode = AstNode, N extends AstNode = AstNode>(validationResult: ValidationResult<T>, filterOptions?: ExpectDiagnosticOptions<N>): void {
     const filtered = filterOptions ? filterByOptions<T, N>(validationResult, filterOptions) : validationResult.diagnostics;
-    expectedFunction(filtered.length, 0, `Expected no issues, but found ${filtered.length}`);
+    expectedFunction(filtered.length, 0, `Expected no issues, but found ${filtered.length}:\n${printDiagnostics(filtered)}`);
 }
 
 export function expectIssue<T extends AstNode = AstNode, N extends AstNode = AstNode>(validationResult: ValidationResult<T>, filterOptions?: ExpectDiagnosticOptions<N>): void {
@@ -711,6 +711,10 @@ export function expectWarning<T extends AstNode = AstNode, N extends AstNode = A
     });
 }
 
+export function printDiagnostics(diagnostics: Diagnostic[] | undefined): string {
+    return diagnostics?.map(d => `line ${d.range.start.line}, column ${d.range.start.character}: ${d.message}`).join('\n') ?? '';
+}
+
 /**
  * Add the given document to the `TextDocuments` service, simulating it being opened in an editor.
  *
diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts
