From a6ee465968e043ba4ac35858959c2316b1596772 Mon Sep 17 00:00:00 2001
From: Johannes Meier <johannes.meier@typefox.io>
Date: Mon, 15 Apr 2024 10:28:59 +0200
Subject: [PATCH] improvements after the second review

---
 .../src/grammar/validation/validator.ts        | 18 ++++++++++++------
 .../test/grammar/grammar-validator.test.ts     | 10 ++++++++++
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts
index ecf3f9ff2..a650ece26 100644
--- a/packages/langium/src/grammar/validation/validator.ts
+++ b/packages/langium/src/grammar/validation/validator.ts
@@ -802,9 +802,8 @@ export class LangiumGrammarValidator {
         // new map to store usage information of the assignments
         const map: Map<string, AssignmentUse> = new Map();
 
-        // top-down traversal
+        // top-down traversal for all starting nodes
         for (const node of startNodes) {
-            // TODO dürfen mehr als 1 Action auftauchen? ansonsten funktioniert das hier nicht so ganz wie gedacht!
             this.checkNodeRegardingAssignmentNumbers(node, 1, map, accept);
         }
 
@@ -839,7 +838,7 @@ export class LangiumGrammarValidator {
         // 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.checkNodeRegardingAssignmentNumbers(currentNode.rule.ref.definition, currentMultiplicity, map, accept); // TODO fragment rules are evaluated multiple times!
+            this.checkNodeRegardingAssignmentNumbers(currentNode.rule.ref.definition, currentMultiplicity, map, accept);
         }
 
         // rewriting actions are a special case for assignments
@@ -851,6 +850,7 @@ export class LangiumGrammarValidator {
         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
@@ -1120,7 +1120,15 @@ function findLookAheadGroup(rule: AstNode | undefined): ast.TerminalGroup | unde
     }
 }
 
+/*
+ * Internal helper stuff for collecting information about assignments to features and their cardinalities
+ */
+
 interface AssignmentUse {
+    /**
+     * Collects assignments for the same feature, while an Action represents a "special assignment", when it is a rewrite action.
+     * The Set is used in order not to store the same assignment multiple times.
+     */
     assignments: Set<ast.Assignment | ast.Action>;
     /**
      * Note, that this number is not exact and "estimates the potential number",
@@ -1145,9 +1153,7 @@ function storeAssignmentUse(map: Map<string, AssignmentUse>, feature: string, in
 }
 
 function mergeAssignmentUse(mapSoure: Map<string, AssignmentUse>, mapTarget: Map<string, AssignmentUse>, counterOperation: (s: number, t: number) => number = (s, t) => s + t): void {
-    for (const sourceEntry of mapSoure.entries()) {
-        const key = sourceEntry[0];
-        const source = sourceEntry[1];
+    for (const [key, source] of mapSoure.entries()) {
         const target = mapTarget.get(key);
         if (target) {
             source.assignments.forEach(a => target.assignments.add(a));
diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts
index 48f20b3b1..8a0108ca0 100644
--- a/packages/langium/test/grammar/grammar-validator.test.ts
+++ b/packages/langium/test/grammar/grammar-validator.test.ts
@@ -927,6 +927,16 @@ describe('Assignments with = instead of +=', () => {
         expect(validation.diagnostics.length).toBe(0);
     });
 
+    test('no problem with actions: three assignments into three different objects', async () => {
+        const validation = await validate(getGrammar(`
+            entry Model infers Expression:
+                Person (operator=('+' | '-') right=Person {infer Model.left=current} right=Person {infer Model.left=current} right=Person)?;
+            Person infers Expression:
+                {infer Person} 'person' name=ID ;
+        `));
+        expect(validation.diagnostics.length).toBe(0);
+    });
+
     test('actions: the rewrite part is a special assignment, which needs to be checked as well!', async () => {
         const validation = await validate(getGrammar(`
             entry Model infers Expression: