From 2b2bf37d3ee251e9c81afb76c1d375d3db4e0211 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Mon, 20 Oct 2025 10:56:30 +0200 Subject: [PATCH 1/4] code action to replace = by += for multiple assignments, more test cases --- .../src/grammar/lsp/grammar-code-actions.ts | 33 ++++++++++++- .../src/grammar/validation/validator.ts | 11 +++-- .../test/grammar/grammar-validator.test.ts | 30 ++++++++++-- .../grammar/lsp/grammar-code-actions.test.ts | 46 +++++++++++++++++++ 4 files changed, 113 insertions(+), 7 deletions(-) diff --git a/packages/langium/src/grammar/lsp/grammar-code-actions.ts b/packages/langium/src/grammar/lsp/grammar-code-actions.ts index e07daa887..358ec3b3e 100644 --- a/packages/langium/src/grammar/lsp/grammar-code-actions.ts +++ b/packages/langium/src/grammar/lsp/grammar-code-actions.ts @@ -13,7 +13,7 @@ import type { CodeActionProvider } from '../../lsp/code-action.js'; import type { LangiumServices } from '../../lsp/lsp-services.js'; import type { AstReflection, Reference, ReferenceInfo } from '../../syntax-tree.js'; import { getContainerOfType } from '../../utils/ast-utils.js'; -import { findLeafNodeAtOffset } from '../../utils/cst-utils.js'; +import { findLeafNodeAtOffset, getNextNode } from '../../utils/cst-utils.js'; import type { MaybePromise } from '../../utils/promise-utils.js'; import { escapeRegExp } from '../../utils/regexp-utils.js'; import type { URI } from '../../utils/uri-utils.js'; @@ -81,6 +81,9 @@ export class LangiumGrammarCodeActionProvider implements CodeActionProvider { case IssueCodes.SuperfluousInfer: accept(this.fixSuperfluousInfer(diagnostic, document)); break; + case IssueCodes.ReplaceOperatorMultiAssignment: + accept(this.replaceOperator(diagnostic, document)); + break; case DocumentValidator.LinkingError: { const data = diagnostic.data as LinkingErrorData; if (data && data.containerType === 'RuleCall' && data.property === 'rule') { @@ -479,6 +482,34 @@ export class LangiumGrammarCodeActionProvider implements CodeActionProvider { return result; } + private replaceOperator(diagnostic: Diagnostic, document: LangiumDocument): CodeAction | undefined { + const rootCst = document.parseResult.value.$cstNode; + if (rootCst) { + const offset = document.textDocument.offsetAt(diagnostic.range.start); + const cstNodeFeature = findLeafNodeAtOffset(rootCst, offset); + const assignment = getContainerOfType(cstNodeFeature?.astNode, node => ast.isAssignment(node) || ast.isAction(node)); + // the validation check marks the 'feature' (for better visibility), but we need to replace the 'operator': + const cstNodeOperator = cstNodeFeature ? getNextNode(cstNodeFeature, false) : undefined; + if (cstNodeOperator && assignment?.$cstNode && assignment.feature && assignment.operator === '=') { // replacing '?=' by '+=' is usually not very helpful, e.g. in cases like "(marked?='marked')+" + return { + title: `Replace '${assignment.operator}' with '+='`, + kind: CodeActionKind.QuickFix, + diagnostics: [diagnostic], + isPreferred: true, + edit: { + changes: { + [document.textDocument.uri]: [{ + range: cstNodeOperator.range, + newText: '+=' + }] + } + } + }; + } + } + return undefined; + } + } function getRelativeImport(source: URI, target: URI): string { diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index b89611d81..7147e45f4 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -138,6 +138,7 @@ export namespace IssueCodes { export const SuperfluousInfer = 'superfluous-infer'; export const OptionalUnorderedGroup = 'optional-unordered-group'; export const ParsingRuleEmpty = 'parsing-rule-empty'; + export const ReplaceOperatorMultiAssignment = 'replace-operator-for-multi-assignments'; } export class LangiumGrammarValidator { @@ -1038,7 +1039,11 @@ export class LangiumGrammarValidator { accept( 'warning', `Found multiple assignments to '${assignment.feature}' with the '${assignment.operator}' assignment operator. Consider using '+=' instead to prevent data loss.`, - { node: assignment, property: 'feature' } // use 'feature' instead of 'operator', since it is pretty hard to see + { + node: assignment, + property: 'feature', // use 'feature' instead of 'operator', since it is pretty hard to see + data: diagnosticData(IssueCodes.ReplaceOperatorMultiAssignment), + } ); } } @@ -1099,9 +1104,9 @@ export class LangiumGrammarValidator { if (ast.isAlternatives(currentNode)) { const mapAllAlternatives: Map = new Map(); // store assignments for Alternatives separately let countCreatedObjects = 0; - for (const child of currentNode.elements) { + for (const alternative of currentNode.elements) { const mapCurrentAlternative: Map = new Map(); - const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested([child], 1, mapCurrentAlternative, accept); + const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested([alternative], 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 diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index 5fedb7739..95ac31b2a 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -1269,9 +1269,9 @@ describe('Property type is not a mix of cross-ref and non-cross-ref types.', () }); -describe('Assignments with = instead of +=', () => { - function getMessage(featureName: string): string { - return `Found multiple assignments to '${featureName}' with the '=' assignment operator. Consider using '+=' instead to prevent data loss.`; +describe('Assignments with = (or ?=) instead of +=', () => { + function getMessage(featureName: string, operator: ('=' | '?=') = '='): string { + return `Found multiple assignments to '${featureName}' with the '${operator}' assignment operator. Consider using '+=' instead to prevent data loss.`; } function getGrammar(content: string): string { return ` @@ -1617,6 +1617,30 @@ describe('Assignments with = instead of +=', () => { expectNoIssues(validation); }); + // the test cases for multiple ?= assignments are not complete, since the logic to identify them is the same as for multiple = assignments + + test('Looped ?= assignment', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person; + Person: ('person' name+=ID active?='active')+; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('active', '?=')); + }); + + test('Looped ?= assignment via fragment', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person; + fragment Assign: + 'person' name+=ID active?='active'; + Person: Assign+; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('active', '?=')); + }); + }); describe('Missing required properties are not arrays or booleans', () => { diff --git a/packages/langium/test/grammar/lsp/grammar-code-actions.test.ts b/packages/langium/test/grammar/lsp/grammar-code-actions.test.ts index 4092001a6..d1112a4c0 100644 --- a/packages/langium/test/grammar/lsp/grammar-code-actions.test.ts +++ b/packages/langium/test/grammar/lsp/grammar-code-actions.test.ts @@ -95,3 +95,49 @@ describe('Langium grammar actions for validations: Parser rules used only as typ expect(action!.title).toBe('Replace with type declaration'); } }); + +describe('Replace = by += for multiple assignments to the same feature', () => { + function composeGrammatik(assignment: string): string { + return ` + grammar HelloWorld + entry Model: + ${assignment} + Person: 'person' name=ID ; + hidden terminal WS: /\\s+/; + terminal ID: /[_a-zA-Z][\\w_]*/; + `; + } + + test('no whitespace', async () => testReplaceAction( + composeGrammatik('persons=Person* ;'), + composeGrammatik('persons+=Person* ;') + )); + + test('some spaces', async () => testReplaceAction( + composeGrammatik('persons = Person* ;'), + composeGrammatik('persons += Person* ;') + )); + + test('some inline comments', async () => testReplaceAction( + composeGrammatik('persons/*before*/= /*after*/ Person* ;'), + composeGrammatik('persons/*before*/+= /*after*/ Person* ;') + )); + + test('some line comments', async () => testReplaceAction( + composeGrammatik(`persons// + = // more + + Person* ;`), + composeGrammatik(`persons// + += // more + + Person* ;`) + )); + + async function testReplaceAction(textBefore: string, textAfter: string) { + const result = await testCodeActions(textBefore, IssueCodes.ReplaceOperatorMultiAssignment, textAfter); + const action = result.action; + expect(action).toBeTruthy(); + expect(action!.title).toBe("Replace '=' with '+='"); + } +}); From 60f8b6e40311d71bbc734d805f8053380f487653 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Mon, 20 Oct 2025 11:56:22 +0200 Subject: [PATCH 2/4] Validate mixed use of ?=, = and += for the same feature --- .../src/grammar/validation/validator.ts | 54 ++++++++++++++----- .../test/grammar/grammar-validator.test.ts | 53 +++++++++++++++++- 2 files changed, 92 insertions(+), 15 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index 7147e45f4..801da6906 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -58,7 +58,7 @@ export function registerValidationChecks(services: LangiumGrammarServices): void validator.checkRuleParameters, validator.checkEmptyParserRule, validator.checkParserRuleReservedName, - validator.checkOperatorMultiplicitiesForMultiAssignments, + validator.checkAssignmentsToTheSameFeature, ], InfixRule: [ validator.checkInfixRuleDataType, @@ -139,6 +139,7 @@ export namespace IssueCodes { export const OptionalUnorderedGroup = 'optional-unordered-group'; export const ParsingRuleEmpty = 'parsing-rule-empty'; export const ReplaceOperatorMultiAssignment = 'replace-operator-for-multi-assignments'; + export const MixedAssignmentOperators = 'mixed-use-of-assignment-operators'; } export class LangiumGrammarValidator { @@ -1018,22 +1019,45 @@ export class LangiumGrammarValidator { } } - /** This validation recursively looks at all assignments (and rewriting actions) with '=' as assignment operator and checks, - * whether the operator should be '+=' instead. */ - checkOperatorMultiplicitiesForMultiAssignments(rule: ast.ParserRule, accept: ValidationAcceptor): void { + /** + * This validation recursively collects all assignments (and rewriting actions) to the same feature and validates them: + * Assignment operators '?=', '=' and '+=' should not be mixed. + * Assignments with '=' as assignment operator should be replaced be '+=', if assignments to the feature might occure more than once. + */ + checkAssignmentsToTheSameFeature(rule: ast.ParserRule, accept: ValidationAcceptor): void { // for usual parser rules AND for fragments, but not for data type rules! if (!rule.dataType) { - this.checkOperatorMultiplicitiesForMultiAssignmentsIndependent([rule.definition], accept); + this.checkAssignmentsToTheSameFeatureIndependent([rule.definition], accept); } } - private checkOperatorMultiplicitiesForMultiAssignmentsIndependent(startNodes: AstNode[], accept: ValidationAcceptor, map: Map = new Map()): void { + private checkAssignmentsToTheSameFeatureIndependent(startNodes: AstNode[], accept: ValidationAcceptor, map: Map = new Map()): void { // check all starting nodes - this.checkOperatorMultiplicitiesForMultiAssignmentsNested(startNodes, 1, map, accept); + this.checkAssignmentsToTheSameFeatureNested(startNodes, 1, map, accept); // create the warnings for (const entry of map.values()) { - if (entry.counter >= 2) { + // check mixed use of ?=, = and += + const usedOperators = new Set(); + for (const assignment of entry.assignments) { + if (assignment.operator) { + usedOperators.add(assignment.operator); + } + } + if (usedOperators.size >= 2) { + for (const assignment of entry.assignments) { + accept( + 'warning', + `Don't mix operators (${stream(usedOperators).map(op => `'${op}'`).join(', ')}) when assigning values to the same feature '${assignment.feature}'.`, + { + node: assignment, + property: 'feature', // use 'feature' instead of 'operator', since it is pretty hard to see + data: diagnosticData(IssueCodes.MixedAssignmentOperators), // no code action, but is relevant for serializability + } + ); + } + } else if (entry.counter >= 2) { + // check for multiple assignments with '?=' or '=' instead of '+=' for (const assignment of entry.assignments) { if (assignment.operator !== '+=') { accept( @@ -1042,16 +1066,18 @@ export class LangiumGrammarValidator { { node: assignment, property: 'feature', // use 'feature' instead of 'operator', since it is pretty hard to see - data: diagnosticData(IssueCodes.ReplaceOperatorMultiAssignment), + data: diagnosticData(IssueCodes.ReplaceOperatorMultiAssignment), // for code action, is relevant for serializability as well } ); } } + } else { + // the assignments to this feature are fine } } } - private checkOperatorMultiplicitiesForMultiAssignmentsNested(nodes: AstNode[], parentMultiplicity: number, map: Map, accept: ValidationAcceptor): boolean { + private checkAssignmentsToTheSameFeatureNested(nodes: AstNode[], parentMultiplicity: number, map: Map, accept: ValidationAcceptor): boolean { let resultCreatedNewObject = false; // check all given elements for (let i = 0; i < nodes.length; i++) { @@ -1063,7 +1089,7 @@ export class LangiumGrammarValidator { 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); + this.checkAssignmentsToTheSameFeatureIndependent(nodes.slice(i + 1), accept, mapForNewObject); resultCreatedNewObject = true; break; // breaks the current loop } @@ -1084,7 +1110,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) { - const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested([currentNode.rule.ref.definition], currentMultiplicity, map, accept); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested([currentNode.rule.ref.definition], currentMultiplicity, map, accept); resultCreatedNewObject = createdNewObject || resultCreatedNewObject; } @@ -1092,7 +1118,7 @@ export class LangiumGrammarValidator { if (ast.isGroup(currentNode) || ast.isUnorderedGroup(currentNode)) { // all members of the group are relavant => collect them all const mapGroup: Map = new Map(); // store assignments for Alternatives separately - const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested(currentNode.elements, 1, mapGroup, accept); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(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 @@ -1106,7 +1132,7 @@ export class LangiumGrammarValidator { let countCreatedObjects = 0; for (const alternative of currentNode.elements) { const mapCurrentAlternative: Map = new Map(); - const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested([alternative], 1, mapCurrentAlternative, accept); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested([alternative], 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 diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index 95ac31b2a..afaacfa39 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -5,7 +5,7 @@ ******************************************************************************/ import type { AstNode, Grammar, GrammarAST as GrammarTypes, LangiumDocument, Properties } from 'langium'; -import { AstUtils, EmptyFileSystem, GrammarAST, URI } from 'langium'; +import { AstUtils, EmptyFileSystem, GrammarAST, stream, URI } from 'langium'; import { IssueCodes, createLangiumGrammarServices } from 'langium/grammar'; import type { ValidationResult } from 'langium/test'; import { clearDocuments, expectError, expectIssue, expectNoIssues, expectWarning, parseHelper, validationHelper } from 'langium/test'; @@ -1643,6 +1643,57 @@ describe('Assignments with = (or ?=) instead of +=', () => { }); +describe('Mixed use of "?=", "=" and "+=" for the same feature', () => { + function getMessage(featureName: string, ...operators: Array<'?=' | '=' | '+='>): string { + return `Don't mix operators (${stream(operators).map(op => `'${op}'`).join(', ')}) when assigning values to the same feature '${featureName}'.`; + } + function getGrammar(content: string): string { + return ` + grammar HelloWorld + ${content} + hidden terminal WS: /\\s+/; + terminal ID: /[_a-zA-Z][\\w_]*/; + `; + } + + test('= and +=', async () => { + const validation = await validate(getGrammar(` + entry Person: 'person' name=ID name+=ID; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('name', '=', '+=')); + expect(validation.diagnostics[1].message).toBe(getMessage('name', '=', '+=')); + }); + + test('= and ?=', async () => { + const validation = await validate(getGrammar(` + entry Person: 'person' name=ID name?=ID; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('name', '=', '?=')); + expect(validation.diagnostics[1].message).toBe(getMessage('name', '=', '?=')); + }); + + test('?= and +=', async () => { + const validation = await validate(getGrammar(` + entry Person: 'person' name?=ID name+=ID; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('name', '?=', '+=')); + expect(validation.diagnostics[1].message).toBe(getMessage('name', '?=', '+=')); + }); + + test('?=, = and +=', async () => { + const validation = await validate(getGrammar(` + entry Person: 'person' name?=ID name=ID name+=ID; + `)); + expect(validation.diagnostics.length).toBe(3); + expect(validation.diagnostics[0].message).toBe(getMessage('name', '?=', '=', '+=')); + expect(validation.diagnostics[1].message).toBe(getMessage('name', '?=', '=', '+=')); + expect(validation.diagnostics[2].message).toBe(getMessage('name', '?=', '=', '+=')); + }); +}); + describe('Missing required properties are not arrays or booleans', () => { test('No missing properties', async () => { From 7d11c3e64ab7589c8303443cc4653682cff0fb58 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Mon, 20 Oct 2025 22:43:24 +0200 Subject: [PATCH 3/4] fixed bug: don't create validation markers for assignments in other documents --- .../src/grammar/validation/validator.ts | 64 ++++++++++++------- 1 file changed, 42 insertions(+), 22 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index 801da6906..5f626140a 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -10,7 +10,7 @@ import * as ast from '../../languages/generated/ast.js'; import type { NamedAstNode } from '../../references/name-provider.js'; import type { References } from '../../references/references.js'; import type { AstNode, Properties, Reference } from '../../syntax-tree.js'; -import { getContainerOfType, streamAllContents } from '../../utils/ast-utils.js'; +import { getContainerOfType, getDocument, streamAllContents } from '../../utils/ast-utils.js'; import { MultiMap } from '../../utils/collections.js'; import { toDocumentSegment } from '../../utils/cst-utils.js'; import { assertUnreachable } from '../../utils/errors.js'; @@ -18,7 +18,7 @@ import { findNameAssignment, findNodeForKeyword, findNodeForProperty, getAllReac import type { Stream } from '../../utils/stream.js'; import { stream } from '../../utils/stream.js'; import { UriUtils } from '../../utils/uri-utils.js'; -import type { DiagnosticData, ValidationAcceptor, ValidationChecks } from '../../validation/validation-registry.js'; +import type { DiagnosticData, DiagnosticInfo, ValidationAcceptor, ValidationChecks } from '../../validation/validation-registry.js'; import { diagnosticData } from '../../validation/validation-registry.js'; import type { AstNodeLocator } from '../../workspace/ast-node-locator.js'; import type { LangiumDocuments } from '../../workspace/documents.js'; @@ -1027,13 +1027,13 @@ export class LangiumGrammarValidator { checkAssignmentsToTheSameFeature(rule: ast.ParserRule, accept: ValidationAcceptor): void { // for usual parser rules AND for fragments, but not for data type rules! if (!rule.dataType) { - this.checkAssignmentsToTheSameFeatureIndependent([rule.definition], accept); + this.checkAssignmentsToTheSameFeatureIndependent(rule, [rule.definition], accept); } } - private checkAssignmentsToTheSameFeatureIndependent(startNodes: AstNode[], accept: ValidationAcceptor, map: Map = new Map()): void { + private checkAssignmentsToTheSameFeatureIndependent(startRule: ast.ParserRule|ast.Action, startNodes: AstNode[], accept: ValidationAcceptor, map: Map = new Map()): void { // check all starting nodes - this.checkAssignmentsToTheSameFeatureNested(startNodes, 1, map, accept); + this.checkAssignmentsToTheSameFeatureNested(startRule, startNodes, 1, map, accept); // create the warnings for (const entry of map.values()) { @@ -1045,29 +1045,24 @@ export class LangiumGrammarValidator { } } if (usedOperators.size >= 2) { + const usedOperatorsPrinted = stream(usedOperators).map(op => `'${op}'`).join(', '); for (const assignment of entry.assignments) { + const [info, docMsg] = createInfo(assignment, IssueCodes.MixedAssignmentOperators); accept( 'warning', - `Don't mix operators (${stream(usedOperators).map(op => `'${op}'`).join(', ')}) when assigning values to the same feature '${assignment.feature}'.`, - { - node: assignment, - property: 'feature', // use 'feature' instead of 'operator', since it is pretty hard to see - data: diagnosticData(IssueCodes.MixedAssignmentOperators), // no code action, but is relevant for serializability - } + `Don't mix operators (${usedOperatorsPrinted}) when assigning values to the same feature '${assignment.feature}'${docMsg}.`, + info, // the issue code is not used for a code action, but is relevant for serializability ); } } else if (entry.counter >= 2) { // check for multiple assignments with '?=' or '=' instead of '+=' for (const assignment of entry.assignments) { if (assignment.operator !== '+=') { + const [info, docMsg] = createInfo(assignment, IssueCodes.ReplaceOperatorMultiAssignment); accept( 'warning', - `Found multiple assignments to '${assignment.feature}' with the '${assignment.operator}' assignment operator. Consider using '+=' instead to prevent data loss.`, - { - node: assignment, - property: 'feature', // use 'feature' instead of 'operator', since it is pretty hard to see - data: diagnosticData(IssueCodes.ReplaceOperatorMultiAssignment), // for code action, is relevant for serializability as well - } + `Found multiple assignments to '${assignment.feature}' with the '${assignment.operator}' assignment operator${docMsg}. Consider using '+=' instead to prevent data loss.`, + info, // the issue code is used for a code action, and it is relevant for serializability as well ); } } @@ -1075,9 +1070,30 @@ export class LangiumGrammarValidator { // the assignments to this feature are fine } } + + const startDocument = getDocument(startRule); + // Utility to handle the case, that the critical assignment is located in another document (for which validation markers cannot be reported now). + function createInfo(assignment: ast.Assignment | ast.Action, code: string): [DiagnosticInfo, string] { + const assignmentDocument = getDocument(assignment); + if (assignmentDocument === startDocument) { + // the assignment is located in the document of the start rule which is currently validated + return [>{ + node: assignment, + property: 'feature', // use 'feature' instead of 'operator', since it is pretty hard to see + data: diagnosticData(code), + }, '']; + } else { + // the assignment is located inside another document => annotate the issue at the start rule + return [>{ + node: startRule, + property: 'name', + data: diagnosticData(code), + }, ` by rule '${getContainerOfType(assignment, ast.isParserRule)?.name}' in the grammar '${UriUtils.basename(assignmentDocument.uri)}'`]; + } + } } - private checkAssignmentsToTheSameFeatureNested(nodes: AstNode[], parentMultiplicity: number, map: Map, accept: ValidationAcceptor): boolean { + private checkAssignmentsToTheSameFeatureNested(startRule: ast.ParserRule|ast.Action, nodes: AstNode[], parentMultiplicity: number, map: Map, accept: ValidationAcceptor): boolean { let resultCreatedNewObject = false; // check all given elements for (let i = 0; i < nodes.length; i++) { @@ -1089,7 +1105,11 @@ export class LangiumGrammarValidator { 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.checkAssignmentsToTheSameFeatureIndependent(nodes.slice(i + 1), accept, mapForNewObject); + if (getDocument(currentNode) === getDocument(startRule)) { + this.checkAssignmentsToTheSameFeatureIndependent(currentNode, nodes.slice(i + 1), accept, mapForNewObject); + } else { + // if the rewrite action is inside another document, validate it, when its document is validated (otherwise, it is done twice and validation markers are added to the wrong document) + } resultCreatedNewObject = true; break; // breaks the current loop } @@ -1110,7 +1130,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) { - const createdNewObject = this.checkAssignmentsToTheSameFeatureNested([currentNode.rule.ref.definition], currentMultiplicity, map, accept); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, [currentNode.rule.ref.definition], currentMultiplicity, map, accept); resultCreatedNewObject = createdNewObject || resultCreatedNewObject; } @@ -1118,7 +1138,7 @@ export class LangiumGrammarValidator { if (ast.isGroup(currentNode) || ast.isUnorderedGroup(currentNode)) { // all members of the group are relavant => collect them all const mapGroup: Map = new Map(); // store assignments for Alternatives separately - const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(currentNode.elements, 1, mapGroup, accept); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, 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 @@ -1132,7 +1152,7 @@ export class LangiumGrammarValidator { let countCreatedObjects = 0; for (const alternative of currentNode.elements) { const mapCurrentAlternative: Map = new Map(); - const createdNewObject = this.checkAssignmentsToTheSameFeatureNested([alternative], 1, mapCurrentAlternative, accept); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, [alternative], 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 From 65dfd328598dc0d537314313a551ab7dfbc4487e Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Wed, 22 Oct 2025 22:58:03 +0200 Subject: [PATCH 4/4] fixed bugs: circular calls of fragments, validate rewrite actions as starting points, more test cases --- .../src/grammar/validation/validator.ts | 86 ++++++--- .../test/grammar/grammar-validator.test.ts | 170 ++++++++++++++++++ 2 files changed, 230 insertions(+), 26 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index 5f626140a..bd2411eb5 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -13,7 +13,7 @@ import type { AstNode, Properties, Reference } from '../../syntax-tree.js'; import { getContainerOfType, getDocument, streamAllContents } from '../../utils/ast-utils.js'; import { MultiMap } from '../../utils/collections.js'; import { toDocumentSegment } from '../../utils/cst-utils.js'; -import { assertUnreachable } from '../../utils/errors.js'; +import { assertCondition, assertUnreachable } from '../../utils/errors.js'; import { findNameAssignment, findNodeForKeyword, findNodeForProperty, getAllReachableRules, getAllRulesUsedForCrossReferences, isArrayCardinality, isDataTypeRule, isOptionalCardinality, terminalRegex } from '../../utils/grammar-utils.js'; import type { Stream } from '../../utils/stream.js'; import { stream } from '../../utils/stream.js'; @@ -44,6 +44,7 @@ export function registerValidationChecks(services: LangiumGrammarServices): void const checks: ValidationChecks = { Action: [ validator.checkAssignmentReservedName, + validator.checkActionAssignmentsToTheSameFeature, ], AbstractRule: validator.checkRuleName, Assignment: [ @@ -58,7 +59,7 @@ export function registerValidationChecks(services: LangiumGrammarServices): void validator.checkRuleParameters, validator.checkEmptyParserRule, validator.checkParserRuleReservedName, - validator.checkAssignmentsToTheSameFeature, + validator.checkRuleAssignmentsToTheSameFeature, ], InfixRule: [ validator.checkInfixRuleDataType, @@ -1020,20 +1021,47 @@ export class LangiumGrammarValidator { } /** - * This validation recursively collects all assignments (and rewriting actions) to the same feature and validates them: + * This validation recursively collects all assignments (and rewriting actions: see the check/entry point in the next function) to the same feature and validates them: * Assignment operators '?=', '=' and '+=' should not be mixed. * Assignments with '=' as assignment operator should be replaced be '+=', if assignments to the feature might occure more than once. */ - checkAssignmentsToTheSameFeature(rule: ast.ParserRule, accept: ValidationAcceptor): void { - // for usual parser rules AND for fragments, but not for data type rules! - if (!rule.dataType) { - this.checkAssignmentsToTheSameFeatureIndependent(rule, [rule.definition], accept); + checkRuleAssignmentsToTheSameFeature(rule: ast.ParserRule, accept: ValidationAcceptor): void { + // validate only usual parser rules, but no fragments (they are validated when validating their using parser rules) and no data type rules! + if (!rule.dataType && !rule.fragment) { + this.checkAssignmentsToTheSameFeature(rule, [rule.definition], new Map(), accept); } } - private checkAssignmentsToTheSameFeatureIndependent(startRule: ast.ParserRule|ast.Action, startNodes: AstNode[], accept: ValidationAcceptor, map: Map = new Map()): void { + checkActionAssignmentsToTheSameFeature(action: ast.Action, accept: ValidationAcceptor): void { + if (action.feature) { + // tree rewriting action + const mapForNewObject = new Map(); + storeAssignmentUse(mapForNewObject, action.feature, 1, action); // remember the special rewriting feature + // all following nodes are put into the new object => check their assignments independently + const sibblings = ast.isGroup(action.$container) || ast.isUnorderedGroup(action.$container) ? action.$container.elements : [action]; + this.checkAssignmentsToTheSameFeature(action, sibblings.slice(sibblings.indexOf(action) + 1), mapForNewObject, accept); + } else { + // this action does not create a new AstNode => no assignments to check + } + } + + private checkAssignmentsToTheSameFeature( + startRule: ast.ParserRule|ast.Action, startNodes: AstNode[], map: Map, accept: ValidationAcceptor + ): void { // check all starting nodes - this.checkAssignmentsToTheSameFeatureNested(startRule, startNodes, 1, map, accept); + const circularFragments = new Set(); + this.checkAssignmentsToTheSameFeatureNested(startRule, startNodes, 1, map, [], circularFragments, accept); + + // handle properties of fragments which are called in circular way => count its properties multiple times + for (const values of map.values()) { + for (const assignment of values.assignments) { + // for each found assignment, check whether it is defined inside a fragment which is called in circular way + const fragmentContainer = getContainerOfType(assignment, ast.isParserRule); + if (fragmentContainer && circularFragments.has(fragmentContainer)) { + values.counter += 1; // This calculation is not accurate, it is only important to know, that this assignment is called multiple times (>= 2). + } + } + } // create the warnings for (const entry of map.values()) { @@ -1071,11 +1099,10 @@ export class LangiumGrammarValidator { } } - const startDocument = getDocument(startRule); // Utility to handle the case, that the critical assignment is located in another document (for which validation markers cannot be reported now). function createInfo(assignment: ast.Assignment | ast.Action, code: string): [DiagnosticInfo, string] { const assignmentDocument = getDocument(assignment); - if (assignmentDocument === startDocument) { + if (assignmentDocument === getDocument(startRule)) { // the assignment is located in the document of the start rule which is currently validated return [>{ node: assignment, @@ -1093,23 +1120,18 @@ export class LangiumGrammarValidator { } } - private checkAssignmentsToTheSameFeatureNested(startRule: ast.ParserRule|ast.Action, nodes: AstNode[], parentMultiplicity: number, map: Map, accept: ValidationAcceptor): boolean { + private checkAssignmentsToTheSameFeatureNested( + startRule: ast.ParserRule|ast.Action, nodes: AstNode[], parentMultiplicity: number, map: Map, + visiting: ast.ParserRule[], circularFragments: Set, accept: ValidationAcceptor + ): boolean { let resultCreatedNewObject = false; // check all given elements - for (let i = 0; i < nodes.length; i++) { - const currentNode = nodes[i]; + for (const currentNode of nodes) { // 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 - if (getDocument(currentNode) === getDocument(startRule)) { - this.checkAssignmentsToTheSameFeatureIndependent(currentNode, nodes.slice(i + 1), accept, mapForNewObject); - } else { - // if the rewrite action is inside another document, validate it, when its document is validated (otherwise, it is done twice and validation markers are added to the wrong document) - } + // The assignments to AstNodes created by this action are validated by another validation check => nothing to do here resultCreatedNewObject = true; break; // breaks the current loop } @@ -1130,15 +1152,27 @@ 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) { - const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, [currentNode.rule.ref.definition], currentMultiplicity, map, accept); - resultCreatedNewObject = createdNewObject || resultCreatedNewObject; + const foundIndex = visiting.indexOf(currentNode.rule.ref); + if (foundIndex >= 0) { + // remember fragments which are called in circular way (their assignments will be counted more often later) + // all currently visited fragments are part of the circle/path + for (let i = foundIndex; i < visiting.length; i++) { + const circleParticipant = visiting[i]; + circularFragments.add(circleParticipant); + } + } else { + visiting.push(currentNode.rule.ref); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, [currentNode.rule.ref.definition], currentMultiplicity, map, visiting, circularFragments, accept); + resultCreatedNewObject = createdNewObject || resultCreatedNewObject; + assertCondition(visiting.pop() === currentNode.rule.ref); // prevent circles, but don't "cache" the results, since the fragment could be called multiple times in non-circular way + } } // 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 = new Map(); // store assignments for Alternatives separately - const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, currentNode.elements, 1, mapGroup, accept); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, currentNode.elements, 1, mapGroup, visiting, circularFragments, 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 @@ -1152,7 +1186,7 @@ export class LangiumGrammarValidator { let countCreatedObjects = 0; for (const alternative of currentNode.elements) { const mapCurrentAlternative: Map = new Map(); - const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, [alternative], 1, mapCurrentAlternative, accept); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, [alternative], 1, mapCurrentAlternative, visiting, circularFragments, 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 diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index afaacfa39..e1567afa3 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -1444,6 +1444,30 @@ describe('Assignments with = (or ?=) instead of +=', () => { expect(validation.diagnostics[0].message).toBe(getMessage('persons')); }); + test('fragments called with different cardinalities', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign (';' Assign)?; + fragment Assign: + ',' persons=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + + test('fragments called with different cardinalities and looped', async () => { + const validation = await validate(getGrammar(` + entry Model: + (';' Assign?)*; + fragment Assign: + ',' persons=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + test('fragments in alternatives: once in 1st, twice in 2nd alternative', async () => { // This suggests the user of Langium to use a list in both cases, which might not be necessary for the 1st alternative. // But that is better than loosing a value in the 2nd alternative. @@ -1458,6 +1482,137 @@ describe('Assignments with = (or ?=) instead of +=', () => { expect(validation.diagnostics[0].message).toBe(getMessage('persons')); }); + test('fragment calls itself in circular way (assignment before the circular call)', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign; + fragment Assign: + ',' persons=Person ('and' Assign)?; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + + test('fragment calls itself in circular way (assignment after the circular call)', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign; + fragment Assign: + ',' ('and' Assign)? persons=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + + test('two fragments call each other in circular way', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign1; + fragment Assign1: + ',' ('and' Assign2)? persons1=Person; + fragment Assign2: + ',' ('and' Assign1)? persons2=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('persons2')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons1')); + }); + + test('two fragments call each other in circular way, another fragment not being part of the circle', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign1; + fragment Assign1: + ',' Assign2 ('and' Assign3)? persons1=Person; + fragment Assign2: + ',' persons2=Person; + fragment Assign3: + ',' ('and' Assign1)? persons3=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('persons3')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons1')); + }); + + test('circlic calls of three fragments (1 -> 2 -> 3 -> 1)', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign1; + fragment Assign1: + ',' ('and' Assign2)? persons1=Person; + fragment Assign2: + ',' ('and' Assign3)? persons2=Person; + fragment Assign3: + ',' ('and' Assign1)? persons3=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(3); + expect(validation.diagnostics[0].message).toBe(getMessage('persons3')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons2')); + expect(validation.diagnostics[2].message).toBe(getMessage('persons1')); + }); + + test('circlic calls of three fragments (1 -> 2 -> 1 -> 3 -> 1)', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign1; + fragment Assign1: + ',' ('and' Assign2)? persons1=Person ('and' Assign3)?; + fragment Assign2: + ',' ('and' Assign1)? persons2=Person; + fragment Assign3: + ',' ('and' Assign1)? persons3=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(3); + expect(validation.diagnostics[0].message).toBe(getMessage('persons2')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons1')); + expect(validation.diagnostics[2].message).toBe(getMessage('persons3')); + }); + + test('circlic calls of fragments with rewrite action', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign1; + fragment Assign1: + ',' ('and' Assign2)? {infer Other.other=current} persons1=Person; + fragment Assign2: + ',' ('and' Assign1)? persons2=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons2')); + }); + + test('dont validate fragments as starting point, since they are validated when validating their calling parser rules', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign; + fragment Assign: + 'persons' (persons=Person)*; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + + test('dont validate fragments as starting point, since they are validated when validating their calling parser rules: unused fragment', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person; + fragment Assign: // this fragment is not used + 'persons' (persons=Person)*; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + // We are informed about the unused fragment, but not about the assignment issue inside (this will happen, when using the fragment). + expect(validation.diagnostics[0].message).toBe('This rule is declared but never referenced.'); + }); + test('no problem: fragments in alternatives', async () => { const validation = await validate(getGrammar(` entry Model: @@ -1597,6 +1752,21 @@ describe('Assignments with = (or ?=) instead of +=', () => { expect(validation.diagnostics[0].message).toBe(getMessage('right')); }); + test('rewrite actions inside fragment, which is called by two different parser rules => validate action only once', async () => { + const validation = await validate(getGrammar(` + entry Model: + r1=R1 r2=R2; + + R1: F; + R2: F; + + fragment F: + a=ID {infer Equals.left=current} (b=ID)+; + `)); + expect(validation.diagnostics.length).toBe(1); // the fragment containing the rewrite action is called twice, but the rewrite action is validated only once + expect(validation.diagnostics[0].message).toBe(getMessage('b')); + }); + test('no problem with rewrite actions on top-level: unassigned action', async () => { const validation = await validate(getGrammar(` entry Model: