From 674866e7c0b5fc11e3c9827167f02b7e6cb4ebcd Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Fri, 23 Feb 2024 18:34:06 +0100 Subject: [PATCH 01/13] first easy case for validating a multi-value assignment with = as assignment operator --- .../src/grammar/validation/validator.ts | 15 +++++ .../test/grammar/grammar-validator.test.ts | 65 +++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index b6c319c3b..a1af5ab6d 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -36,6 +36,7 @@ export function registerValidationChecks(services: LangiumGrammarServices): void validator.checkAssignmentWithFeatureName, validator.checkAssignmentToFragmentRule, validator.checkAssignmentTypes, + validator.checkAssignmentOperator, validator.checkAssignmentReservedName ], ParserRule: [ @@ -809,6 +810,20 @@ export class LangiumGrammarValidator { } } + checkAssignmentOperator(assignment: ast.Assignment, accept: ValidationAcceptor): void { + if (assignment.operator === '=') { + // the assignment has a multi-value cardinality + if (assignment.cardinality === '*' || assignment.cardinality === '+') { + accept( + 'warning', + `It seems, that you assign multiple values to the feature '${assignment.feature}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`, + { node: assignment, property: 'operator' } + ); + } + // TODO more cases + } + } + checkInterfacePropertyTypes(interfaceDecl: ast.Interface, accept: ValidationAcceptor): void { for (const attribute of interfaceDecl.attributes) { if (attribute.type) { diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index adc13d6a7..a88996aa3 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -728,6 +728,71 @@ 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 `It seems, that you assign multiple values to the feature '${featureName}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`; + } + function getGrammar(content: string): string { + return ` + grammar HelloWorld + ${content} + hidden terminal WS: /\\s+/; + terminal ID: /[_a-zA-Z][\\w_]*/; + `; + } + + test('assignment with * cardinality', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons = Person* ; + Person: + 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + test('assignment with + cardinality', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons = Person+ ; + Person: + 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + + test.only('two assignments with single cardinality', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons = Person ',' persons = Person; + Person: + 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + + test('Simple case', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons += Person* + greetings = Greeting*; + + Person: + 'person' name=ID; + + Greeting: + 'Hello' person=[Person:ID] '!'; + `)); + + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('greetings')); + // expectError(validation, /Mixing a cross-reference with other types is not supported. Consider splitting property /); + }); + +}); + describe('Missing required properties are not arrays or booleans', () => { test('No missing properties', async () => { From 86dad6184b02700334ae9e3e09bbd1606a35a821 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Fri, 23 Feb 2024 19:01:22 +0100 Subject: [PATCH 02/13] check the container group of the assignment --- .../src/grammar/validation/validator.ts | 40 ++++++++++++++----- .../test/grammar/grammar-validator.test.ts | 21 ++++++---- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index a1af5ab6d..4f0b58597 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -811,17 +811,39 @@ export class LangiumGrammarValidator { } checkAssignmentOperator(assignment: ast.Assignment, accept: ValidationAcceptor): void { - if (assignment.operator === '=') { - // the assignment has a multi-value cardinality - if (assignment.cardinality === '*' || assignment.cardinality === '+') { - accept( - 'warning', - `It seems, that you assign multiple values to the feature '${assignment.feature}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`, - { node: assignment, property: 'operator' } - ); + // this validation is specific for assignments with '=' as assignment operator + if (assignment.operator !== '=') { + return; + } + // the assignment has a multi-value cardinality itself + if (this.isMany(assignment)) { + this.markAssignment(assignment, accept); + return; + } + // check the container group of the assignment + if (ast.isGroup(assignment.$container) || ast.isUnorderedGroup(assignment.$container)) { + // the group can occur multiple times + if (this.isMany(assignment.$container)) { + this.markAssignment(assignment, accept); + return; + } + // look for at least a second assignments to the same feature within the container group, the cardinality does not matter + if (assignment.$container.elements.filter(child => ast.isAssignment(child) && child.feature === assignment.feature).length >= 2) { + this.markAssignment(assignment, accept); + return; } - // TODO more cases } + // TODO more cases + } + private isMany(node: AstNode): boolean { + return ast.isAbstractElement(node) && (node.cardinality === '*' || node.cardinality === '+'); + } + private markAssignment(assignment: ast.Assignment, accept: ValidationAcceptor): void { + accept( + 'warning', + `It seems, that you assign multiple values to the feature '${assignment.feature}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`, + { node: assignment, property: 'operator' } + ); } checkInterfacePropertyTypes(interfaceDecl: ast.Interface, accept: ValidationAcceptor): void { diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index a88996aa3..ece3b4961 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -745,8 +745,7 @@ describe('Assignments with = instead of +=', () => { const validation = await validate(getGrammar(` entry Model: persons = Person* ; - Person: - 'person' name=ID ; + Person: 'person' name=ID ; `)); expect(validation.diagnostics.length).toBe(1); expect(validation.diagnostics[0].message).toBe(getMessage('persons')); @@ -755,24 +754,32 @@ describe('Assignments with = instead of +=', () => { const validation = await validate(getGrammar(` entry Model: persons = Person+ ; - Person: - 'person' name=ID ; + Person: 'person' name=ID ; `)); expect(validation.diagnostics.length).toBe(1); expect(validation.diagnostics[0].message).toBe(getMessage('persons')); }); - test.only('two assignments with single cardinality', async () => { + test('two assignments with single cardinality', async () => { const validation = await validate(getGrammar(` entry Model: persons = Person ',' persons = Person; - Person: - 'person' name=ID ; + Person: 'person' name=ID ; `)); expect(validation.diagnostics.length).toBe(2); expect(validation.diagnostics[0].message).toBe(getMessage('persons')); }); + test('single assignment with outer * cardinality', async () => { + const validation = await validate(getGrammar(` + entry Model: + (',' persons = Person)* ; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + test('Simple case', async () => { const validation = await validate(getGrammar(` entry Model: From fe51cd99cd2db24ae86129a85905adeae8930dc4 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Mon, 11 Mar 2024 10:23:42 +0100 Subject: [PATCH 03/13] reworked the validation to check neighbored and nested assignment as well, added more test cases --- .../src/grammar/validation/validator.ts | 89 ++++++++------ .../test/grammar/grammar-validator.test.ts | 110 +++++++++++++++++- 2 files changed, 158 insertions(+), 41 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index 4f0b58597..d9a95230d 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -4,23 +4,23 @@ * terms of the MIT License, which is available in the project root. ******************************************************************************/ +import type { Range } from 'vscode-languageserver-types'; +import { DiagnosticTag } from 'vscode-languageserver-types'; +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 type { Stream } from '../../utils/stream.js'; -import type { DiagnosticData, ValidationAcceptor, ValidationChecks } from '../../validation/validation-registry.js'; -import type { LangiumDocuments } from '../../workspace/documents.js'; -import type { LangiumGrammarServices } from '../langium-grammar-module.js'; -import type { Range } from 'vscode-languageserver-types'; -import { DiagnosticTag } from 'vscode-languageserver-types'; import { getContainerOfType, streamAllContents } from '../../utils/ast-utils.js'; import { MultiMap } from '../../utils/collections.js'; import { toDocumentSegment } from '../../utils/cst-utils.js'; -import { findNameAssignment, findNodeForKeyword, findNodeForProperty, getAllReachableRules, isDataTypeRule, isOptionalCardinality, terminalRegex } from '../../utils/grammar-utils.js'; +import { findNameAssignment, findNodeForKeyword, findNodeForProperty, getAllReachableRules, isArrayCardinality, isDataTypeRule, isOptionalCardinality, terminalRegex } from '../../utils/grammar-utils.js'; +import type { Stream } from '../../utils/stream.js'; import { stream } from '../../utils/stream.js'; +import type { DiagnosticData, ValidationAcceptor, ValidationChecks } from '../../validation/validation-registry.js'; import { diagnosticData } from '../../validation/validation-registry.js'; -import * as ast from '../../languages/generated/ast.js'; +import type { LangiumDocuments } from '../../workspace/documents.js'; import { getTypeNameWithoutError, hasDataTypeReturn, isPrimitiveGrammarType, isStringGrammarType, resolveImport, resolveTransitiveImports } from '../internal-grammar-util.js'; +import type { LangiumGrammarServices } from '../langium-grammar-module.js'; import { typeDefinitionToPropertyType } from '../type-system/type-collector/declared-types.js'; import { flattenPlainType, isPlainReferenceType } from '../type-system/type-collector/plain-types.js'; @@ -812,38 +812,55 @@ export class LangiumGrammarValidator { checkAssignmentOperator(assignment: ast.Assignment, accept: ValidationAcceptor): void { // this validation is specific for assignments with '=' as assignment operator - if (assignment.operator !== '=') { - return; - } - // the assignment has a multi-value cardinality itself - if (this.isMany(assignment)) { - this.markAssignment(assignment, accept); - return; + if (assignment.operator === '=') { + // check the initial assignment and all of its containers + let currentElement: AstNode | undefined = assignment; + while (currentElement) { + const countAssignments = this.searchAssignmentsRecursively(currentElement, assignment.feature); + if (countAssignments >= 2) { + accept( + 'warning', + `It seems, that you assign multiple values to the feature '${assignment.feature}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`, + { node: assignment, property: 'operator' } + ); + return; + } + // check the next container + currentElement = currentElement.$container; + } } - // check the container group of the assignment - if (ast.isGroup(assignment.$container) || ast.isUnorderedGroup(assignment.$container)) { - // the group can occur multiple times - if (this.isMany(assignment.$container)) { - this.markAssignment(assignment, accept); - return; + } + + private searchAssignmentsRecursively(node: AstNode, featureName: string): number { + let countResult = 0; + // assignment + if (ast.isAssignment(node) && node.feature === featureName) { + countResult += 1; + } + // search for assignments in used fragments as well, since their property values are stored in the current object, + // but not in calls of regular parser rules, since they create new objects + if (ast.isRuleCall(node) && ast.isParserRule(node.rule.ref) && node.rule.ref.fragment) { + countResult += this.searchAssignmentsRecursively(node.rule.ref.definition, featureName); + } + // look for assignments to the same feature within groups + if (ast.isGroup(node) || ast.isUnorderedGroup(node)) { + for (const child of node.elements) { + countResult += this.searchAssignmentsRecursively(child, featureName); } - // look for at least a second assignments to the same feature within the container group, the cardinality does not matter - if (assignment.$container.elements.filter(child => ast.isAssignment(child) && child.feature === assignment.feature).length >= 2) { - this.markAssignment(assignment, accept); - return; + } + // look for assignments to the same feature within alternatives + if (ast.isAlternatives(node)) { + let alternativeCount = 0; + for (const child of node.elements) { + alternativeCount = Math.max(alternativeCount, this.searchAssignmentsRecursively(child, featureName)); } + countResult += alternativeCount; } - // TODO more cases - } - private isMany(node: AstNode): boolean { - return ast.isAbstractElement(node) && (node.cardinality === '*' || node.cardinality === '+'); - } - private markAssignment(assignment: ast.Assignment, accept: ValidationAcceptor): void { - accept( - 'warning', - `It seems, that you assign multiple values to the feature '${assignment.feature}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`, - { node: assignment, property: 'operator' } - ); + // the current element can occur multiple times => its assignments occur multiple times as well + if (ast.isAbstractElement(node) && isArrayCardinality(node.cardinality)) { + countResult *= 2; // note, that the result is not exact (but it is sufficient for the current case)! + } + return countResult; } checkInterfacePropertyTypes(interfaceDecl: ast.Interface, accept: ValidationAcceptor): void { diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index ece3b4961..c96eb0469 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -744,7 +744,7 @@ describe('Assignments with = instead of +=', () => { test('assignment with * cardinality', async () => { const validation = await validate(getGrammar(` entry Model: - persons = Person* ; + persons=Person* ; Person: 'person' name=ID ; `)); expect(validation.diagnostics.length).toBe(1); @@ -753,7 +753,7 @@ describe('Assignments with = instead of +=', () => { test('assignment with + cardinality', async () => { const validation = await validate(getGrammar(` entry Model: - persons = Person+ ; + persons=Person+ ; Person: 'person' name=ID ; `)); expect(validation.diagnostics.length).toBe(1); @@ -763,7 +763,7 @@ describe('Assignments with = instead of +=', () => { test('two assignments with single cardinality', async () => { const validation = await validate(getGrammar(` entry Model: - persons = Person ',' persons = Person; + persons=Person ',' persons=Person; Person: 'person' name=ID ; `)); expect(validation.diagnostics.length).toBe(2); @@ -773,7 +773,7 @@ describe('Assignments with = instead of +=', () => { test('single assignment with outer * cardinality', async () => { const validation = await validate(getGrammar(` entry Model: - (',' persons = Person)* ; + (',' persons=Person)* ; Person: 'person' name=ID ; `)); expect(validation.diagnostics.length).toBe(1); @@ -795,9 +795,109 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics.length).toBe(1); expect(validation.diagnostics[0].message).toBe(getMessage('greetings')); - // expectError(validation, /Mixing a cross-reference with other types is not supported. Consider splitting property /); }); + test('no problem: assignments in different alternatives', async () => { + const validation = await validate(getGrammar(` + entry Model: + (persons=Person) | (persons=Person); + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(0); + // TODO more "no problem" cases! + }); + + test('assignments in different alternatives, but looped', async () => { + const validation = await validate(getGrammar(` + entry Model: + ((persons=Person) | (persons=Person))*; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons')); + }); + + test('assignments in different alternatives, written twice', async () => { + const validation = await validate(getGrammar(` + entry Model: + ((persons=Person) | (persons=Person)) ',' ((persons=Person) | (persons=Person)); + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(4); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons')); + expect(validation.diagnostics[2].message).toBe(getMessage('persons')); + expect(validation.diagnostics[3].message).toBe(getMessage('persons')); + }); + + test('multiple optional assignments', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person (',' persons=Person (',' persons=Person )?)?; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(3); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons')); + expect(validation.diagnostics[2].message).toBe(getMessage('persons')); + }); + + test('multiple assignments on different nesting levels', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person (',' persons=Person (',' persons=Person )); + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(3); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons')); + expect(validation.diagnostics[2].message).toBe(getMessage('persons')); + }); + + test('fragments: 2nd assignment is in fragment', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person ';' 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: assignments only in fragment', 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: alternatives with no problems', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign | (';' Assign); + fragment Assign: + ',' persons=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(0); + }); + + test('no problem: assignments in different parser rules', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person; + Person: 'person' name=ID persons=Person; + `)); + expect(validation.diagnostics.length).toBe(0); + }); }); describe('Missing required properties are not arrays or booleans', () => { From 05951b1c561b368dc33b3d48a8b9005aaa3e3ba9 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Mon, 11 Mar 2024 17:11:00 +0100 Subject: [PATCH 04/13] check assignments in fragments as well --- .../src/grammar/validation/validator.ts | 60 ++++++++++++++----- .../test/grammar/grammar-validator.test.ts | 41 ++++++++++--- 2 files changed, 76 insertions(+), 25 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index d9a95230d..481024775 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -23,6 +23,7 @@ import { getTypeNameWithoutError, hasDataTypeReturn, isPrimitiveGrammarType, isS import type { LangiumGrammarServices } from '../langium-grammar-module.js'; import { typeDefinitionToPropertyType } from '../type-system/type-collector/declared-types.js'; import { flattenPlainType, isPlainReferenceType } from '../type-system/type-collector/plain-types.js'; +import type { AstNodeLocator } from '../../workspace/ast-node-locator.js'; export function registerValidationChecks(services: LangiumGrammarServices): void { const registry = services.validation.ValidationRegistry; @@ -117,10 +118,12 @@ export namespace IssueCodes { export class LangiumGrammarValidator { protected readonly references: References; + protected readonly nodeLocator: AstNodeLocator; protected readonly documents: LangiumDocuments; constructor(services: LangiumGrammarServices) { this.references = services.references.References; + this.nodeLocator = services.workspace.AstNodeLocator; this.documents = services.shared.workspace.LangiumDocuments; } @@ -814,24 +817,49 @@ export class LangiumGrammarValidator { // this validation is specific for assignments with '=' as assignment operator if (assignment.operator === '=') { // check the initial assignment and all of its containers - let currentElement: AstNode | undefined = assignment; - while (currentElement) { - const countAssignments = this.searchAssignmentsRecursively(currentElement, assignment.feature); - if (countAssignments >= 2) { - accept( - 'warning', - `It seems, that you assign multiple values to the feature '${assignment.feature}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`, - { node: assignment, property: 'operator' } - ); - return; + const reportedProblem = this.searchAssignmentsRecursivelyUp(assignment, assignment, accept); + + // check a special case: the current assignment is located within a fragment => check, whether the fragment is called multiple times + if (!reportedProblem) { + const containerFragment = getContainerOfType(assignment, ast.isParserRule); + if (containerFragment && containerFragment.fragment) { + // for all calls of the fragment ... + for (const ref of this.references.findReferences(containerFragment, {})) { + const doc = this.documents.getDocument(ref.sourceUri); + const call = doc ? this.nodeLocator.getAstNode(doc.parseResult.value, ref.sourcePath) : undefined; + if (call) { + // ... check whether there are multiple assignments to the same feature + if (this.searchAssignmentsRecursivelyUp(call, assignment, accept)) { + return; + } + } + } } - // check the next container - currentElement = currentElement.$container; } } } - private searchAssignmentsRecursively(node: AstNode, featureName: string): number { + private searchAssignmentsRecursivelyUp(node: AstNode, assignment: ast.Assignment, accept: ValidationAcceptor): boolean { + let currentElement: AstNode | undefined = node; + while (currentElement) { + // check neighbored and nested assignments + const countAssignments = this.searchAssignmentsRecursivelyDown(currentElement, assignment.feature); + if (countAssignments >= 2) { + accept( + 'warning', + `It seems, that you are assigning multiple values to the feature '${assignment.feature}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`, + { node: assignment, property: 'operator' } + ); + return true; + } + + // check the next container + currentElement = currentElement.$container; + } + return false; + } + + private searchAssignmentsRecursivelyDown(node: AstNode, featureName: string): number { let countResult = 0; // assignment if (ast.isAssignment(node) && node.feature === featureName) { @@ -840,19 +868,19 @@ export class LangiumGrammarValidator { // search for assignments in used fragments as well, since their property values are stored in the current object, // but not in calls of regular parser rules, since they create new objects if (ast.isRuleCall(node) && ast.isParserRule(node.rule.ref) && node.rule.ref.fragment) { - countResult += this.searchAssignmentsRecursively(node.rule.ref.definition, featureName); + countResult += this.searchAssignmentsRecursivelyDown(node.rule.ref.definition, featureName); } // look for assignments to the same feature within groups if (ast.isGroup(node) || ast.isUnorderedGroup(node)) { for (const child of node.elements) { - countResult += this.searchAssignmentsRecursively(child, featureName); + countResult += this.searchAssignmentsRecursivelyDown(child, featureName); } } // look for assignments to the same feature within alternatives if (ast.isAlternatives(node)) { let alternativeCount = 0; for (const child of node.elements) { - alternativeCount = Math.max(alternativeCount, this.searchAssignmentsRecursively(child, featureName)); + alternativeCount = Math.max(alternativeCount, this.searchAssignmentsRecursivelyDown(child, featureName)); } countResult += alternativeCount; } diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index c96eb0469..3b8d6d5fe 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -730,7 +730,7 @@ 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 `It seems, that you assign multiple values to the feature '${featureName}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`; + return `It seems, that you are assigning multiple values to the feature '${featureName}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`; } function getGrammar(content: string): string { return ` @@ -780,7 +780,7 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics[0].message).toBe(getMessage('persons')); }); - test('Simple case', async () => { + test('correct and wrong assignments next to each other', async () => { const validation = await validate(getGrammar(` entry Model: persons += Person* @@ -804,7 +804,6 @@ describe('Assignments with = instead of +=', () => { Person: 'person' name=ID ; `)); expect(validation.diagnostics.length).toBe(0); - // TODO more "no problem" cases! }); test('assignments in different alternatives, but looped', async () => { @@ -831,7 +830,18 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics[3].message).toBe(getMessage('persons')); }); - test('multiple optional assignments', async () => { + test('assignments only in some alternatives, assume the worst case', async () => { + const validation = await validate(getGrammar(` + entry Model: + ((persons=Person) | (';')) ',' ((persons=Person) | (';')); + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons')); + }); + + test('multiple, nested optional assignments', async () => { const validation = await validate(getGrammar(` entry Model: persons=Person (',' persons=Person (',' persons=Person )?)?; @@ -843,7 +853,7 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics[2].message).toBe(getMessage('persons')); }); - test('multiple assignments on different nesting levels', async () => { + test('multiple, nested mandatory assignments', async () => { const validation = await validate(getGrammar(` entry Model: persons=Person (',' persons=Person (',' persons=Person )); @@ -855,7 +865,7 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics[2].message).toBe(getMessage('persons')); }); - test('fragments: 2nd assignment is in fragment', async () => { + test('fragments: 2nd critical assignment is located in a fragment', async () => { const validation = await validate(getGrammar(` entry Model: persons=Person ';' Assign; @@ -863,11 +873,12 @@ describe('Assignments with = instead of +=', () => { ',' persons=Person; Person: 'person' name=ID ; `)); - expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics.length).toBe(2); expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons')); }); - test('fragments: assignments only in fragment', async () => { + test('fragments: all assignments are located in a fragment', async () => { const validation = await validate(getGrammar(` entry Model: Assign ';' Assign; @@ -879,7 +890,19 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics[0].message).toBe(getMessage('persons')); }); - test('fragments: alternatives with no problems', async () => { + test('fragments: assignment in looped fragment', 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('no problem: fragments in alternatives', async () => { const validation = await validate(getGrammar(` entry Model: Assign | (';' Assign); From 96202fd7b047fa881b995443132ed70d0019b473 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Tue, 12 Mar 2024 10:39:11 +0100 Subject: [PATCH 05/13] started to support actions --- .../src/grammar/validation/validator.ts | 85 +++++++++++++++---- .../test/grammar/grammar-validator.test.ts | 22 +++++ 2 files changed, 90 insertions(+), 17 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index 481024775..bbdf1f862 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -840,10 +840,11 @@ export class LangiumGrammarValidator { } private searchAssignmentsRecursivelyUp(node: AstNode, assignment: ast.Assignment, accept: ValidationAcceptor): boolean { - let currentElement: AstNode | undefined = node; - while (currentElement) { + let currentContainer: AstNode | undefined = node; + let previousChild: AstNode | undefined = undefined; + while (currentContainer) { // check neighbored and nested assignments - const countAssignments = this.searchAssignmentsRecursivelyDown(currentElement, assignment.feature); + const countAssignments = this.searchAssignmentsRecursivelyDown(currentContainer, previousChild, assignment.feature); if (countAssignments >= 2) { accept( 'warning', @@ -854,38 +855,88 @@ export class LangiumGrammarValidator { } // check the next container - currentElement = currentElement.$container; + previousChild = currentContainer; + currentContainer = currentContainer.$container; } return false; } - private searchAssignmentsRecursivelyDown(node: AstNode, featureName: string): number { + private searchAssignmentsRecursivelyDown(currentContainer: AstNode, relevantChild: AstNode | undefined, featureName: string): number { let countResult = 0; + let containerMultiplicityMatters = true; + // assignment - if (ast.isAssignment(node) && node.feature === featureName) { + if (ast.isAssignment(currentContainer) && currentContainer.feature === featureName) { countResult += 1; } // search for assignments in used fragments as well, since their property values are stored in the current object, // but not in calls of regular parser rules, since they create new objects - if (ast.isRuleCall(node) && ast.isParserRule(node.rule.ref) && node.rule.ref.fragment) { - countResult += this.searchAssignmentsRecursivelyDown(node.rule.ref.definition, featureName); + if (ast.isRuleCall(currentContainer) && ast.isParserRule(currentContainer.rule.ref) && currentContainer.rule.ref.fragment) { + countResult += this.searchAssignmentsRecursivelyDown(currentContainer.rule.ref.definition, undefined, featureName); + } + // TODO test this special assignment! + if (ast.isAction(currentContainer) && currentContainer.feature === featureName) { + countResult++; } + // look for assignments to the same feature within groups - if (ast.isGroup(node) || ast.isUnorderedGroup(node)) { - for (const child of node.elements) { - countResult += this.searchAssignmentsRecursivelyDown(child, featureName); + if (ast.isGroup(currentContainer) || ast.isUnorderedGroup(currentContainer)) { + let countGroup = 0; + let foundRelevantChild = false; + for (const child of currentContainer.elements) { + if (child === relevantChild) { + foundRelevantChild = true; + } + if (ast.isAction(child)) { + // a new object is created => following assignments are put into the new object + if (relevantChild) { + if (foundRelevantChild) { + // the previous assignments are put into the same object as the given relevant child => ignore the following assignments to the new object + break; + } else { + // + containerMultiplicityMatters = false; // since an additional object is created for each time, */+ around the current group don't matter! + countGroup = 0; + } + } else { + break; + } + } + countGroup += this.searchAssignmentsRecursivelyDown(child, undefined, featureName); } + countResult += countGroup; } // look for assignments to the same feature within alternatives - if (ast.isAlternatives(node)) { - let alternativeCount = 0; - for (const child of node.elements) { - alternativeCount = Math.max(alternativeCount, this.searchAssignmentsRecursivelyDown(child, featureName)); + if (ast.isAlternatives(currentContainer)) { + // TODO 2x if vereinheitlichen! + let countAlternatives = 0; + let foundRelevantChild = false; + for (const child of currentContainer.elements) { + if (child === relevantChild) { + foundRelevantChild = true; + } + if (ast.isAction(child)) { + // a new object is created => following assignments are put into the new object + if (relevantChild) { + if (foundRelevantChild) { + // the assignment to check + break; // => break the for-loop here, since all following assignments are put into the new object + } else { + containerMultiplicityMatters = false; + countAlternatives = 0; + } + } else { + // + break; + } + } + countAlternatives = Math.max(countAlternatives, this.searchAssignmentsRecursivelyDown(child, undefined, featureName)); } - countResult += alternativeCount; + countResult += countAlternatives; } + // the current element can occur multiple times => its assignments occur multiple times as well - if (ast.isAbstractElement(node) && isArrayCardinality(node.cardinality)) { + if (containerMultiplicityMatters && ast.isAbstractElement(currentContainer) && isArrayCardinality(currentContainer.cardinality)) { countResult *= 2; // note, that the result is not exact (but it is sufficient for the current case)! } return countResult; diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index 3b8d6d5fe..22f7c0d59 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -921,6 +921,28 @@ describe('Assignments with = instead of +=', () => { `)); expect(validation.diagnostics.length).toBe(0); }); + + test('no problem with action: assignment is looped, but stored in a new object each time', async () => { + const validation = await validate(getGrammar(` + entry Model infers Expression: + Person ({infer Model.left=current} operator=('+' | '-') 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: + Person ({infer Model.left=current} operator=('+' | '-') right=Person left=Model)*; + Person infers Expression: + {infer Person} 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('left')); + expect(validation.diagnostics[1].message).toBe(getMessage('left')); + }); }); describe('Missing required properties are not arrays or booleans', () => { From 79e4659af4e069995ee0bb4b02c7a24b3967732a Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Tue, 12 Mar 2024 10:45:46 +0100 Subject: [PATCH 06/13] refactoring --- .../src/grammar/validation/validator.ts | 42 ++++++------------- 1 file changed, 12 insertions(+), 30 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index bbdf1f862..49089e24c 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -880,10 +880,11 @@ export class LangiumGrammarValidator { } // look for assignments to the same feature within groups - if (ast.isGroup(currentContainer) || ast.isUnorderedGroup(currentContainer)) { + if (ast.isGroup(currentContainer) || ast.isUnorderedGroup(currentContainer) || ast.isAlternatives(currentContainer)) { let countGroup = 0; let foundRelevantChild = false; for (const child of currentContainer.elements) { + // special case: Actions if (child === relevantChild) { foundRelevantChild = true; } @@ -902,37 +903,18 @@ export class LangiumGrammarValidator { break; } } - countGroup += this.searchAssignmentsRecursivelyDown(child, undefined, featureName); - } - countResult += countGroup; - } - // look for assignments to the same feature within alternatives - if (ast.isAlternatives(currentContainer)) { - // TODO 2x if vereinheitlichen! - let countAlternatives = 0; - let foundRelevantChild = false; - for (const child of currentContainer.elements) { - if (child === relevantChild) { - foundRelevantChild = true; - } - if (ast.isAction(child)) { - // a new object is created => following assignments are put into the new object - if (relevantChild) { - if (foundRelevantChild) { - // the assignment to check - break; // => break the for-loop here, since all following assignments are put into the new object - } else { - containerMultiplicityMatters = false; - countAlternatives = 0; - } - } else { - // - break; - } + + // count the relevant child assignments + const countCurrent = this.searchAssignmentsRecursivelyDown(child, undefined, featureName); + if (ast.isAlternatives(currentContainer)) { + // for alternatives, only a single alternative is used => assume the worst case and take the maximum number of assignments + countGroup = Math.max(countGroup, countCurrent); + } else { + // all members of the group are relavant => count them all + countGroup += countCurrent; } - countAlternatives = Math.max(countAlternatives, this.searchAssignmentsRecursivelyDown(child, undefined, featureName)); } - countResult += countAlternatives; + countResult += countGroup; } // the current element can occur multiple times => its assignments occur multiple times as well From b97eac49468a40621d44a53756e9ac13d7964148 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Tue, 12 Mar 2024 14:07:01 +0100 Subject: [PATCH 07/13] check assignments of rewrite actions, added more comments, renamings --- .../src/grammar/validation/validator.ts | 132 ++++++++++++------ .../test/grammar/grammar-validator.test.ts | 12 +- 2 files changed, 99 insertions(+), 45 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index 49089e24c..342b182cc 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -31,6 +31,7 @@ export function registerValidationChecks(services: LangiumGrammarServices): void const checks: ValidationChecks = { Action: [ validator.checkAssignmentReservedName, + validator.checkActionOperator, ], AbstractRule: validator.checkRuleName, Assignment: [ @@ -813,25 +814,39 @@ export class LangiumGrammarValidator { } } + /** This validation is specific for assignments with '=' as assignment operator and checks, + * whether the operator should be '+=' instead. */ checkAssignmentOperator(assignment: ast.Assignment, accept: ValidationAcceptor): void { - // this validation is specific for assignments with '=' as assignment operator if (assignment.operator === '=') { - // check the initial assignment and all of its containers - const reportedProblem = this.searchAssignmentsRecursivelyUp(assignment, assignment, accept); - - // check a special case: the current assignment is located within a fragment => check, whether the fragment is called multiple times - if (!reportedProblem) { - const containerFragment = getContainerOfType(assignment, ast.isParserRule); - if (containerFragment && containerFragment.fragment) { - // for all calls of the fragment ... - for (const ref of this.references.findReferences(containerFragment, {})) { - const doc = this.documents.getDocument(ref.sourceUri); - const call = doc ? this.nodeLocator.getAstNode(doc.parseResult.value, ref.sourcePath) : undefined; - if (call) { - // ... check whether there are multiple assignments to the same feature - if (this.searchAssignmentsRecursivelyUp(call, assignment, accept)) { - return; - } + this.checkAssignable(assignment, accept); + } + } + + /** This validation is specific for rewriting actions with '=' as assignment operator and checks, + * whether the operator of the (rewriting) assignment should be '+=' instead. */ + checkActionOperator(action: ast.Action, accept: ValidationAcceptor): void { + if (action.operator === '=' && action.feature) { + this.checkAssignable(action, accept); + } + } + + private checkAssignable(assignment: ast.Assignment | ast.Action, accept: ValidationAcceptor): void { + // check the initial assignment and all of its containers + const reportedProblem = this.searchRecursivelyUpForAssignments(assignment, assignment, accept); + + // check a special case: the current assignment is located within a fragment + // => check, whether the fragment is called multiple times by parser rules + if (!reportedProblem) { + const containerFragment = getContainerOfType(assignment, ast.isParserRule); + if (containerFragment && containerFragment.fragment) { + // for all callers of the fragment ... + for (const callerReference of this.references.findReferences(containerFragment, {})) { + const document = this.documents.getDocument(callerReference.sourceUri); + const callingNode = document ? this.nodeLocator.getAstNode(document.parseResult.value, callerReference.sourcePath) : undefined; + if (callingNode) { + // ... check whether there are multiple assignments to the same feature + if (this.searchRecursivelyUpForAssignments(callingNode, assignment, accept)) { + return; } } } @@ -839,12 +854,19 @@ export class LangiumGrammarValidator { } } - private searchAssignmentsRecursivelyUp(node: AstNode, assignment: ast.Assignment, accept: ValidationAcceptor): boolean { - let currentContainer: AstNode | undefined = node; - let previousChild: AstNode | undefined = undefined; + /** + * Searches in the given start node and its containers for assignments to the same feature as the given assignment xor action. + * @param startNode the node to start the search + * @param assignment the assignment for which "conflicting" assigments shall be searched + * @param accept acceptor for warnings + * @returns true, if the given assignment got a warning, false otherwise + */ + private searchRecursivelyUpForAssignments(startNode: AstNode, assignment: ast.Assignment | ast.Action, accept: ValidationAcceptor): boolean { + let currentContainer: AstNode | undefined = startNode; // the current node to search in + let previousChild: AstNode | undefined = undefined; // remember the previous node, which is now a direct child of the current container while (currentContainer) { // check neighbored and nested assignments - const countAssignments = this.searchAssignmentsRecursivelyDown(currentContainer, previousChild, assignment.feature); + const countAssignments = this.searchRecursivelyDownForAssignments(currentContainer, previousChild, assignment.feature!); if (countAssignments >= 2) { accept( 'warning', @@ -861,52 +883,73 @@ export class LangiumGrammarValidator { return false; } - private searchAssignmentsRecursivelyDown(currentContainer: AstNode, relevantChild: AstNode | undefined, featureName: string): number { + /** + * Searches in the given current node and its contained nodes for assignments with the given feature name. + * @param currentNode the element whose assignments should be (recursively) counted + * @param relevantChild if given, this node is a direct child of the given 'currentNode' + * and is required in case of Actions contained in the 'currentNode' to identify which assignments are relevant and which not, + * depending on the positions of the Action and the 'relevantChild', + * i.e. only assignments to the same object which contains the given 'relevantChild' matter. + * @param featureName the feature name of assignments to search for + * @returns the number of found assignments with the given name, + * note, that the returned number is not exact and "estimates the potential number", + * i.e. multiplicities like + and * are counted as 2x/twice, + * and for alternatives, the worst case is assumed. + * In other words, here it is enough to know, whether there are two or more assignments possible to the same feature. + */ + private searchRecursivelyDownForAssignments(currentNode: AstNode, relevantChild: AstNode | undefined, featureName: string): number { let countResult = 0; let containerMultiplicityMatters = true; // assignment - if (ast.isAssignment(currentContainer) && currentContainer.feature === featureName) { + if (ast.isAssignment(currentNode) && currentNode.feature === featureName) { countResult += 1; } - // search for assignments in used fragments as well, since their property values are stored in the current object, - // but not in calls of regular parser rules, since they create new objects - if (ast.isRuleCall(currentContainer) && ast.isParserRule(currentContainer.rule.ref) && currentContainer.rule.ref.fragment) { - countResult += this.searchAssignmentsRecursivelyDown(currentContainer.rule.ref.definition, undefined, featureName); + + // 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) { + countResult += this.searchRecursivelyDownForAssignments(currentNode.rule.ref.definition, undefined, featureName); } - // TODO test this special assignment! - if (ast.isAction(currentContainer) && currentContainer.feature === featureName) { - countResult++; + + // rewriting actions are a special case for assignments + if (ast.isAction(currentNode) && currentNode.feature === featureName) { + countResult += 1; } - // look for assignments to the same feature within groups - if (ast.isGroup(currentContainer) || ast.isUnorderedGroup(currentContainer) || ast.isAlternatives(currentContainer)) { + // look for assignments to the same feature nested within groups + if (ast.isGroup(currentNode) || ast.isUnorderedGroup(currentNode) || ast.isAlternatives(currentNode)) { let countGroup = 0; let foundRelevantChild = false; - for (const child of currentContainer.elements) { - // special case: Actions - if (child === relevantChild) { - foundRelevantChild = true; - } + for (const child of currentNode.elements) { + // 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 (ast.isAction(child)) { - // a new object is created => following assignments are put into the new object if (relevantChild) { + // there is a child given => ensure, that only assignments to the same object which contains this child are counted if (foundRelevantChild) { // the previous assignments are put into the same object as the given relevant child => ignore the following assignments to the new object break; } else { - // - containerMultiplicityMatters = false; // since an additional object is created for each time, */+ around the current group don't matter! + // the previous assignments are stored in a different object than the given relevant child => ignore those assignments countGroup = 0; + // since an additional object is created for each time, */+ around the current group don't matter! + containerMultiplicityMatters = false; } } else { + // all following assignments are put into the new object => ignore following assignments, but count previous assignments break; } } + // remember, whether the given child is already found in the current group + if (child === relevantChild) { + foundRelevantChild = true; + } + // count the relevant child assignments - const countCurrent = this.searchAssignmentsRecursivelyDown(child, undefined, featureName); - if (ast.isAlternatives(currentContainer)) { + const countCurrent = this.searchRecursivelyDownForAssignments(child, undefined, featureName); + if (ast.isAlternatives(currentNode)) { // for alternatives, only a single alternative is used => assume the worst case and take the maximum number of assignments countGroup = Math.max(countGroup, countCurrent); } else { @@ -917,10 +960,11 @@ export class LangiumGrammarValidator { countResult += countGroup; } - // the current element can occur multiple times => its assignments occur multiple times as well - if (containerMultiplicityMatters && ast.isAbstractElement(currentContainer) && isArrayCardinality(currentContainer.cardinality)) { + // the current element can occur multiple times => its assignments can occur multiple times as well + if (containerMultiplicityMatters && ast.isAbstractElement(currentNode) && isArrayCardinality(currentNode.cardinality)) { countResult *= 2; // note, that the result is not exact (but it is sufficient for the current case)! } + return countResult; } diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index 22f7c0d59..d2eac6206 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -922,7 +922,7 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics.length).toBe(0); }); - test('no problem with action: assignment is looped, but stored in a new object each time', async () => { + test('no problem with actions: assignment is looped, but stored in a new object each time', async () => { const validation = await validate(getGrammar(` entry Model infers Expression: Person ({infer Model.left=current} operator=('+' | '-') right=Person)*; @@ -932,6 +932,16 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics.length).toBe(0); }); + test('no problem with actions: second assignment is stored in a new object', async () => { + const validation = await validate(getGrammar(` + entry Model infers Expression: + Person (operator=('+' | '-') 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: From e7f54473c698ec96f6709747085913ae0204c198 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Mon, 8 Apr 2024 13:17:03 +0200 Subject: [PATCH 08/13] fixes for the review --- .../test/grammar/grammar-validator.test.ts | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index d2eac6206..1510d6b20 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -768,6 +768,7 @@ describe('Assignments with = instead of +=', () => { `)); expect(validation.diagnostics.length).toBe(2); expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons')); }); test('single assignment with outer * cardinality', async () => { @@ -902,6 +903,20 @@ describe('Assignments with = instead of +=', () => { 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. + const validation = await validate(getGrammar(` + entry Model: + Assign | (';' 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('no problem: fragments in alternatives', async () => { const validation = await validate(getGrammar(` entry Model: From d7ecfb3f6c254bd25400c9464ab0644f0f4a54c6 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Wed, 10 Apr 2024 17:24:17 +0200 Subject: [PATCH 09/13] big refactoring: do not mix upwards and downwards, go top-down only --- .../src/grammar/validation/validator.ts | 230 ++++++++---------- .../test/grammar/grammar-validator.test.ts | 2 +- 2 files changed, 109 insertions(+), 123 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index 342b182cc..7cefaa209 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -18,12 +18,12 @@ import type { Stream } from '../../utils/stream.js'; import { stream } from '../../utils/stream.js'; import type { DiagnosticData, 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'; import { getTypeNameWithoutError, hasDataTypeReturn, isPrimitiveGrammarType, isStringGrammarType, resolveImport, resolveTransitiveImports } from '../internal-grammar-util.js'; import type { LangiumGrammarServices } from '../langium-grammar-module.js'; import { typeDefinitionToPropertyType } from '../type-system/type-collector/declared-types.js'; import { flattenPlainType, isPlainReferenceType } from '../type-system/type-collector/plain-types.js'; -import type { AstNodeLocator } from '../../workspace/ast-node-locator.js'; export function registerValidationChecks(services: LangiumGrammarServices): void { const registry = services.validation.ValidationRegistry; @@ -31,20 +31,19 @@ export function registerValidationChecks(services: LangiumGrammarServices): void const checks: ValidationChecks = { Action: [ validator.checkAssignmentReservedName, - validator.checkActionOperator, ], AbstractRule: validator.checkRuleName, Assignment: [ validator.checkAssignmentWithFeatureName, validator.checkAssignmentToFragmentRule, validator.checkAssignmentTypes, - validator.checkAssignmentOperator, validator.checkAssignmentReservedName ], ParserRule: [ validator.checkParserRuleDataType, validator.checkRuleParametersUsed, validator.checkParserRuleReservedName, + validator.checkAssignmentOperatorMultiplicities, ], TerminalRule: [ validator.checkTerminalRuleReturnType, @@ -814,158 +813,106 @@ export class LangiumGrammarValidator { } } - /** This validation is specific for assignments with '=' as assignment operator and checks, + /** This validation recursively looks at all assignments (and rewriting actions) with '=' as assignment operator and checks, * whether the operator should be '+=' instead. */ - checkAssignmentOperator(assignment: ast.Assignment, accept: ValidationAcceptor): void { - if (assignment.operator === '=') { - this.checkAssignable(assignment, accept); - } - } - - /** This validation is specific for rewriting actions with '=' as assignment operator and checks, - * whether the operator of the (rewriting) assignment should be '+=' instead. */ - checkActionOperator(action: ast.Action, accept: ValidationAcceptor): void { - if (action.operator === '=' && action.feature) { - this.checkAssignable(action, accept); + checkAssignmentOperatorMultiplicities(rule: ast.ParserRule, accept: ValidationAcceptor): void { + // for usual parser rules AND for fragments, but not for data type rules! + if (!rule.dataType) { + this.checkAssignmentOperatorMultiplicitiesLogic([rule.definition], accept); } } - private checkAssignable(assignment: ast.Assignment | ast.Action, accept: ValidationAcceptor): void { - // check the initial assignment and all of its containers - const reportedProblem = this.searchRecursivelyUpForAssignments(assignment, assignment, accept); - - // check a special case: the current assignment is located within a fragment - // => check, whether the fragment is called multiple times by parser rules - if (!reportedProblem) { - const containerFragment = getContainerOfType(assignment, ast.isParserRule); - if (containerFragment && containerFragment.fragment) { - // for all callers of the fragment ... - for (const callerReference of this.references.findReferences(containerFragment, {})) { - const document = this.documents.getDocument(callerReference.sourceUri); - const callingNode = document ? this.nodeLocator.getAstNode(document.parseResult.value, callerReference.sourcePath) : undefined; - if (callingNode) { - // ... check whether there are multiple assignments to the same feature - if (this.searchRecursivelyUpForAssignments(callingNode, assignment, accept)) { - return; - } + private checkAssignmentOperatorMultiplicitiesLogic(startNodes: AstNode[], accept: ValidationAcceptor): void { + // new map to store usage information of the assignments + const map: Map = new Map(); + + // top-down traversal + 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); + } + + // create the warnings + for (const entry of map.entries()) { + if (entry[1].counter >= 2) { + for (const assignment of entry[1].assignments) { + if (assignment.operator !== '+=') { + accept( + 'warning', + `It seems, that you are assigning multiple values to the feature '${assignment.feature}', while you are using '${assignment.operator}' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned values.`, + { node: assignment, property: 'operator' } + ); } } } } } - /** - * Searches in the given start node and its containers for assignments to the same feature as the given assignment xor action. - * @param startNode the node to start the search - * @param assignment the assignment for which "conflicting" assigments shall be searched - * @param accept acceptor for warnings - * @returns true, if the given assignment got a warning, false otherwise - */ - private searchRecursivelyUpForAssignments(startNode: AstNode, assignment: ast.Assignment | ast.Action, accept: ValidationAcceptor): boolean { - let currentContainer: AstNode | undefined = startNode; // the current node to search in - let previousChild: AstNode | undefined = undefined; // remember the previous node, which is now a direct child of the current container - while (currentContainer) { - // check neighbored and nested assignments - const countAssignments = this.searchRecursivelyDownForAssignments(currentContainer, previousChild, assignment.feature!); - if (countAssignments >= 2) { - accept( - 'warning', - `It seems, that you are assigning multiple values to the feature '${assignment.feature}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`, - { node: assignment, property: 'operator' } - ); - return true; - } - - // check the next container - previousChild = currentContainer; - currentContainer = currentContainer.$container; + private checkNodeRegardingAssignmentNumbers(currentNode: AstNode, parentMultiplicity: number, map: Map, 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)! } - return false; - } - - /** - * Searches in the given current node and its contained nodes for assignments with the given feature name. - * @param currentNode the element whose assignments should be (recursively) counted - * @param relevantChild if given, this node is a direct child of the given 'currentNode' - * and is required in case of Actions contained in the 'currentNode' to identify which assignments are relevant and which not, - * depending on the positions of the Action and the 'relevantChild', - * i.e. only assignments to the same object which contains the given 'relevantChild' matter. - * @param featureName the feature name of assignments to search for - * @returns the number of found assignments with the given name, - * note, that the returned number is not exact and "estimates the potential number", - * i.e. multiplicities like + and * are counted as 2x/twice, - * and for alternatives, the worst case is assumed. - * In other words, here it is enough to know, whether there are two or more assignments possible to the same feature. - */ - private searchRecursivelyDownForAssignments(currentNode: AstNode, relevantChild: AstNode | undefined, featureName: string): number { - let countResult = 0; - let containerMultiplicityMatters = true; // assignment - if (ast.isAssignment(currentNode) && currentNode.feature === featureName) { - countResult += 1; + if (ast.isAssignment(currentNode)) { + storeAssignmentUse(map, currentNode.feature, 1 * currentMultiplicity, currentNode); } // 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) { - countResult += this.searchRecursivelyDownForAssignments(currentNode.rule.ref.definition, undefined, featureName); + this.checkNodeRegardingAssignmentNumbers(currentNode.rule.ref.definition, currentMultiplicity, map, accept); // TODO fragment rules are evaluated multiple times! } // rewriting actions are a special case for assignments - if (ast.isAction(currentNode) && currentNode.feature === featureName) { - countResult += 1; + if (ast.isAction(currentNode) && currentNode.feature) { + storeAssignmentUse(map, currentNode.feature, 1 * currentMultiplicity, currentNode); } // look for assignments to the same feature nested within groups if (ast.isGroup(currentNode) || ast.isUnorderedGroup(currentNode) || ast.isAlternatives(currentNode)) { - let countGroup = 0; - let foundRelevantChild = false; + const mapAllAlternatives: Map = new Map(); // store assignments for Alternatives separately + let nodesForNewObject: AstNode[] = []; for (const child of currentNode.elements) { - // 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 (ast.isAction(child)) { - if (relevantChild) { - // there is a child given => ensure, that only assignments to the same object which contains this child are counted - if (foundRelevantChild) { - // the previous assignments are put into the same object as the given relevant child => ignore the following assignments to the new object - break; + // 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 >= 1) { + // all collected nodes are put into the new object => check their assignments independently + this.checkAssignmentOperatorMultiplicitiesLogic(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 >= 1) { + // 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 = new Map(); + this.checkNodeRegardingAssignmentNumbers(child, currentMultiplicity, mapCurrentAlternative, accept); + mergeAssignmentUse(mapCurrentAlternative, mapAllAlternatives, (s, t) => Math.max(s, t)); } else { - // the previous assignments are stored in a different object than the given relevant child => ignore those assignments - countGroup = 0; - // since an additional object is created for each time, */+ around the current group don't matter! - containerMultiplicityMatters = false; + // all members of the group are relavant => collect them all + this.checkNodeRegardingAssignmentNumbers(child, currentMultiplicity, map, accept); } - } else { - // all following assignments are put into the new object => ignore following assignments, but count previous assignments - break; } } - - // remember, whether the given child is already found in the current group - if (child === relevantChild) { - foundRelevantChild = true; - } - - // count the relevant child assignments - const countCurrent = this.searchRecursivelyDownForAssignments(child, undefined, featureName); - if (ast.isAlternatives(currentNode)) { - // for alternatives, only a single alternative is used => assume the worst case and take the maximum number of assignments - countGroup = Math.max(countGroup, countCurrent); - } else { - // all members of the group are relavant => count them all - countGroup += countCurrent; - } } - countResult += countGroup; - } - - // the current element can occur multiple times => its assignments can occur multiple times as well - if (containerMultiplicityMatters && ast.isAbstractElement(currentNode) && isArrayCardinality(currentNode.cardinality)) { - countResult *= 2; // note, that the result is not exact (but it is sufficient for the current case)! + // merge alternatives + mergeAssignmentUse(mapAllAlternatives, map); + if (nodesForNewObject.length >= 1) { + // these nodes are put into a new object => check their assignments independently + this.checkAssignmentOperatorMultiplicitiesLogic(nodesForNewObject, accept); + } } - - return countResult; } checkInterfacePropertyTypes(interfaceDecl: ast.Interface, accept: ValidationAcceptor): void { @@ -1196,3 +1143,42 @@ function findLookAheadGroup(rule: AstNode | undefined): ast.TerminalGroup | unde return findLookAheadGroup(terminalGroup.$container); } } + +interface AssignmentUse { + assignments: Set; + /** + * Note, that this number is not exact and "estimates the potential number", + * i.e. multiplicities like + and * are counted as 2x/twice, + * and for alternatives, the worst case is assumed. + * In other words, here it is enough to know, whether there are two or more assignments possible to the same feature. + */ + counter: number; +} + +function storeAssignmentUse(map: Map, feature: string, increment: number, ...assignments: Array) { + let entry = map.get(feature); + if (!entry) { + entry = { + assignments: new Set(), + counter: 0, + }; + map.set(feature, entry); + } + assignments.forEach(a => entry!.assignments.add(a)); // a Set is necessary, since assignments in Fragements might be used multiple times by different parser rules, but they should be marked only once! + entry.counter += increment; +} + +function mergeAssignmentUse(mapSoure: Map, mapTarget: Map, 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]; + const target = mapTarget.get(key); + if (target) { + source.assignments.forEach(a => target.assignments.add(a)); + target.counter = counterOperation(source.counter, target.counter); + } else { + mapTarget.set(key, source); + } + } + mapSoure.clear(); +} diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index 1510d6b20..a0988baab 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -730,7 +730,7 @@ 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 `It seems, that you are assigning multiple values to the feature '${featureName}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned value.`; + return `It seems, that you are assigning multiple values to the feature '${featureName}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned values.`; } function getGrammar(content: string): string { return ` From 0acd1f75d2aa48d2c30aeaf80930cecd58cd2fa0 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Mon, 15 Apr 2024 10:28:59 +0200 Subject: [PATCH 10/13] 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 7cefaa209..e56dbe4b6 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -826,9 +826,8 @@ export class LangiumGrammarValidator { // new map to store usage information of the assignments const map: Map = 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); } @@ -863,7 +862,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 @@ -875,6 +874,7 @@ export class LangiumGrammarValidator { if (ast.isGroup(currentNode) || ast.isUnorderedGroup(currentNode) || ast.isAlternatives(currentNode)) { const mapAllAlternatives: Map = 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 @@ -1144,7 +1144,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; /** * Note, that this number is not exact and "estimates the potential number", @@ -1169,9 +1177,7 @@ function storeAssignmentUse(map: Map, feature: string, in } function mergeAssignmentUse(mapSoure: Map, mapTarget: Map, 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 a0988baab..d200f8470 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -957,6 +957,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: From 5dc9426a25bc7ae356ed475bb6d2771383810377 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Mon, 15 Apr 2024 10:34:51 +0200 Subject: [PATCH 11/13] fixed the issue in an already existing test case --- packages/langium/test/grammar/grammar-validator.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index d200f8470..95aaf6397 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -343,7 +343,7 @@ describe('Check grammar with primitives', () => { const grammar = ` grammar PrimGrammar entry Expr: - primitives=Primitive*; + primitives+=Primitive*; Primitive: (Word | Bool | Num | LargeInt | DateObj); Word: From da4ce63208a227952ac20d2aa9b74c75da2eae39 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Wed, 17 Apr 2024 15:21:42 +0200 Subject: [PATCH 12/13] renamed functions and added comments in order to distinguish two similar validations --- .../src/grammar/validation/validator.ts | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index e56dbe4b6..979d1c914 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -43,7 +43,7 @@ export function registerValidationChecks(services: LangiumGrammarServices): void validator.checkParserRuleDataType, validator.checkRuleParametersUsed, validator.checkParserRuleReservedName, - validator.checkAssignmentOperatorMultiplicities, + validator.checkOperatorMultiplicitiesForMultiAssignments, ], TerminalRule: [ validator.checkTerminalRuleReturnType, @@ -79,7 +79,7 @@ export function registerValidationChecks(services: LangiumGrammarServices): void validator.checkUsedHiddenTerminalRule, validator.checkUsedFragmentTerminalRule, validator.checkRuleCallParameters, - validator.checkRuleCallMultiplicity + validator.checkMultiRuleCallsAreAssigned ], TerminalRuleCall: validator.checkUsedHiddenTerminalRule, CrossReference: [ @@ -680,7 +680,8 @@ export class LangiumGrammarValidator { } } - checkRuleCallMultiplicity(call: ast.RuleCall, accept: ValidationAcceptor): void { + /** This validation checks, that parser rules which are called multiple times are assigned (except for fragments). */ + checkMultiRuleCallsAreAssigned(call: ast.RuleCall, accept: ValidationAcceptor): void { const findContainerWithCardinality = (node: AstNode) => { let result: AstNode | undefined = node; while (result !== undefined) { @@ -815,20 +816,20 @@ 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. */ - checkAssignmentOperatorMultiplicities(rule: ast.ParserRule, accept: ValidationAcceptor): void { + checkOperatorMultiplicitiesForMultiAssignments(rule: ast.ParserRule, accept: ValidationAcceptor): void { // for usual parser rules AND for fragments, but not for data type rules! if (!rule.dataType) { - this.checkAssignmentOperatorMultiplicitiesLogic([rule.definition], accept); + this.checkOperatorMultiplicitiesForMultiAssignmentsLogic([rule.definition], accept); } } - private checkAssignmentOperatorMultiplicitiesLogic(startNodes: AstNode[], accept: ValidationAcceptor): void { + private checkOperatorMultiplicitiesForMultiAssignmentsLogic(startNodes: AstNode[], accept: ValidationAcceptor): void { // new map to store usage information of the assignments const map: Map = new Map(); // top-down traversal for all starting nodes for (const node of startNodes) { - this.checkNodeRegardingAssignmentNumbers(node, 1, map, accept); + this.checkAssignmentNumbersForNode(node, 1, map, accept); } // create the warnings @@ -847,7 +848,7 @@ export class LangiumGrammarValidator { } } - private checkNodeRegardingAssignmentNumbers(currentNode: AstNode, parentMultiplicity: number, map: Map, accept: ValidationAcceptor) { + private checkAssignmentNumbersForNode(currentNode: AstNode, parentMultiplicity: number, map: Map, 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)) { @@ -862,7 +863,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); + this.checkAssignmentNumbersForNode(currentNode.rule.ref.definition, currentMultiplicity, map, accept); } // rewriting actions are a special case for assignments @@ -881,7 +882,7 @@ export class LangiumGrammarValidator { // (This counts for rewriting actions as well as for unassigned actions, i.e. actions without feature name) if (nodesForNewObject.length >= 1) { // all collected nodes are put into the new object => check their assignments independently - this.checkAssignmentOperatorMultiplicitiesLogic(nodesForNewObject, accept); + this.checkOperatorMultiplicitiesForMultiAssignmentsLogic(nodesForNewObject, accept); // is it possible to have two or more Actions within the same parser rule? the grammar allows that ... nodesForNewObject = []; } @@ -897,11 +898,11 @@ export class LangiumGrammarValidator { 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 = new Map(); - this.checkNodeRegardingAssignmentNumbers(child, currentMultiplicity, mapCurrentAlternative, accept); + 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.checkNodeRegardingAssignmentNumbers(child, currentMultiplicity, map, accept); + this.checkAssignmentNumbersForNode(child, currentMultiplicity, map, accept); } } } @@ -910,7 +911,7 @@ export class LangiumGrammarValidator { mergeAssignmentUse(mapAllAlternatives, map); if (nodesForNewObject.length >= 1) { // these nodes are put into a new object => check their assignments independently - this.checkAssignmentOperatorMultiplicitiesLogic(nodesForNewObject, accept); + this.checkOperatorMultiplicitiesForMultiAssignmentsLogic(nodesForNewObject, accept); } } } From 4821a995930b5054131b2757f26c737b7769d4a7 Mon Sep 17 00:00:00 2001 From: Johannes Meier Date: Wed, 4 Sep 2024 17:49:08 +0200 Subject: [PATCH 13/13] changes according to the review --- .../langium/src/grammar/validation/validator.ts | 14 +++++++------- .../langium/test/grammar/grammar-validator.test.ts | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index 979d1c914..b443cc81d 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -833,14 +833,14 @@ export class LangiumGrammarValidator { } // create the warnings - for (const entry of map.entries()) { - if (entry[1].counter >= 2) { - for (const assignment of entry[1].assignments) { + for (const entry of map.values()) { + if (entry.counter >= 2) { + for (const assignment of entry.assignments) { if (assignment.operator !== '+=') { accept( 'warning', - `It seems, that you are assigning multiple values to the feature '${assignment.feature}', while you are using '${assignment.operator}' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned values.`, - { node: assignment, property: 'operator' } + `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 ); } } @@ -880,7 +880,7 @@ export class LangiumGrammarValidator { 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 >= 1) { + 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 ... @@ -890,7 +890,7 @@ export class LangiumGrammarValidator { nodesForNewObject.push(child); } else { // for non-Actions - if (nodesForNewObject.length >= 1) { + if (nodesForNewObject.length > 0) { // nodes go into a new object nodesForNewObject.push(child); } else { diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index 95aaf6397..2c31bd7ad 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -730,7 +730,7 @@ 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 `It seems, that you are assigning multiple values to the feature '${featureName}', while you are using '=' as assignment operator. Consider to use '+=' instead in order not to loose some of the assigned values.`; + return `Found multiple assignments to '${featureName}' with the '=' assignment operator. Consider using '+=' instead to prevent data loss.`; } function getGrammar(content: string): string { return `