diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index 3f62421de..b94697f72 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -877,18 +877,13 @@ export class LangiumGrammarValidator { checkOperatorMultiplicitiesForMultiAssignments(rule: ast.ParserRule, accept: ValidationAcceptor): void { // for usual parser rules AND for fragments, but not for data type rules! if (!rule.dataType) { - this.checkOperatorMultiplicitiesForMultiAssignmentsLogic([rule.definition], accept); + this.checkOperatorMultiplicitiesForMultiAssignmentsIndependent([rule.definition], accept); } } - private checkOperatorMultiplicitiesForMultiAssignmentsLogic(startNodes: AstNode[], accept: ValidationAcceptor): void { - // new map to store usage information of the assignments - const map: Map = new Map(); - - // top-down traversal for all starting nodes - for (const node of startNodes) { - this.checkAssignmentNumbersForNode(node, 1, map, accept); - } + private checkOperatorMultiplicitiesForMultiAssignmentsIndependent(startNodes: AstNode[], accept: ValidationAcceptor, map: Map = new Map()): void { + // check all starting nodes + this.checkOperatorMultiplicitiesForMultiAssignmentsNested(startNodes, 1, map, accept); // create the warnings for (const entry of map.values()) { @@ -906,72 +901,79 @@ export class LangiumGrammarValidator { } } - private checkAssignmentNumbersForNode(currentNode: AstNode, parentMultiplicity: number, map: Map, accept: ValidationAcceptor) { - // the current element can occur multiple times => its assignments can occur multiple times as well - let currentMultiplicity = parentMultiplicity; - if (ast.isAbstractElement(currentNode) && isArrayCardinality(currentNode.cardinality)) { - currentMultiplicity *= 2; // note, that the result is not exact (but it is sufficient for the current case)! - } + private checkOperatorMultiplicitiesForMultiAssignmentsNested(nodes: AstNode[], parentMultiplicity: number, map: Map, accept: ValidationAcceptor): boolean { + let resultCreatedNewObject = false; + // check all given elements + for (let i = 0; i < nodes.length; i++) { + const currentNode = nodes[i]; + + // Tree-Rewrite-Actions are a special case: a new object is created => following assignments are put into the new object + if (ast.isAction(currentNode) && currentNode.feature) { + // (This does NOT count for unassigned actions, i.e. actions without feature name, since they change only the type of the current object.) + const mapForNewObject = new Map(); + storeAssignmentUse(mapForNewObject, currentNode.feature, 1, currentNode); // remember the special rewriting feature + // all following nodes are put into the new object => check their assignments independently + this.checkOperatorMultiplicitiesForMultiAssignmentsIndependent(nodes.slice(i + 1), accept, mapForNewObject); + resultCreatedNewObject = true; + break; // breaks the current loop + } - // assignment - if (ast.isAssignment(currentNode)) { - storeAssignmentUse(map, currentNode.feature, 1 * currentMultiplicity, currentNode); - } + // all other elements don't create new objects themselves: - // Search for assignments in used fragments as well, since their property values are stored in the current object. - // But do not search in calls of regular parser rules, since parser rules create new objects. - if (ast.isRuleCall(currentNode) && ast.isParserRule(currentNode.rule.ref) && currentNode.rule.ref.fragment) { - this.checkAssignmentNumbersForNode(currentNode.rule.ref.definition, currentMultiplicity, map, accept); - } + // the current element can occur multiple times => its assignments can occur multiple times as well + let currentMultiplicity = parentMultiplicity; + if (ast.isAbstractElement(currentNode) && isArrayCardinality(currentNode.cardinality)) { + currentMultiplicity *= 2; // note that the result is not exact (but it is sufficient for the current use case)! + } - // rewriting actions are a special case for assignments - if (ast.isAction(currentNode) && currentNode.feature) { - storeAssignmentUse(map, currentNode.feature, 1 * currentMultiplicity, currentNode); - } + // assignment + if (ast.isAssignment(currentNode)) { + storeAssignmentUse(map, currentNode.feature, currentMultiplicity, currentNode); + } - // look for assignments to the same feature nested within groups - if (ast.isGroup(currentNode) || ast.isUnorderedGroup(currentNode) || ast.isAlternatives(currentNode)) { - const mapAllAlternatives: Map = new Map(); // store assignments for Alternatives separately - let nodesForNewObject: AstNode[] = []; - // check all elements inside the current group - for (const child of currentNode.elements) { - if (ast.isAction(child)) { - // Actions are a special case: a new object is created => following assignments are put into the new object - // (This counts for rewriting actions as well as for unassigned actions, i.e. actions without feature name) - if (nodesForNewObject.length > 0) { - // all collected nodes are put into the new object => check their assignments independently - this.checkOperatorMultiplicitiesForMultiAssignmentsLogic(nodesForNewObject, accept); - // is it possible to have two or more Actions within the same parser rule? the grammar allows that ... - nodesForNewObject = []; - } - // push the current node into a new object - nodesForNewObject.push(child); - } else { - // for non-Actions - if (nodesForNewObject.length > 0) { - // nodes go into a new object - nodesForNewObject.push(child); - } else { - // count the relevant child assignments - if (ast.isAlternatives(currentNode)) { - // for alternatives, only a single alternative is used => assume the worst case and take the maximum number of assignments - const mapCurrentAlternative: Map = new Map(); - this.checkAssignmentNumbersForNode(child, currentMultiplicity, mapCurrentAlternative, accept); - mergeAssignmentUse(mapCurrentAlternative, mapAllAlternatives, (s, t) => Math.max(s, t)); - } else { - // all members of the group are relavant => collect them all - this.checkAssignmentNumbersForNode(child, currentMultiplicity, map, accept); - } + // Search for assignments in used fragments as well, since their property values are stored in the current object. + // But do not search in calls of regular parser rules, since parser rules create new objects. + if (ast.isRuleCall(currentNode) && ast.isParserRule(currentNode.rule.ref) && currentNode.rule.ref.fragment) { + const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested([currentNode.rule.ref.definition], currentMultiplicity, map, accept); + resultCreatedNewObject = createdNewObject || resultCreatedNewObject; + } + + // look for assignments to the same feature nested within groups + if (ast.isGroup(currentNode) || ast.isUnorderedGroup(currentNode)) { + // all members of the group are relavant => collect them all + const mapGroup: Map = new Map(); // store assignments for Alternatives separately + const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested(currentNode.elements, 1, mapGroup, accept); + mergeAssignmentUse(mapGroup, map, createdNewObject + ? (s, t) => (s + t) // if a new object is created in the group: ignore the current multiplicity, since a new object is created for each loop cycle! + : (s, t) => (s * currentMultiplicity + t) // otherwise as usual: take the current multiplicity into account + ); + resultCreatedNewObject = createdNewObject || resultCreatedNewObject; + } + + // for alternatives, only a single alternative is used => assume the worst case and take the maximum number of assignments + if (ast.isAlternatives(currentNode)) { + const mapAllAlternatives: Map = new Map(); // store assignments for Alternatives separately + let countCreatedObjects = 0; + for (const child of currentNode.elements) { + const mapCurrentAlternative: Map = new Map(); + const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested([child], 1, mapCurrentAlternative, accept); + mergeAssignmentUse(mapCurrentAlternative, mapAllAlternatives, createdNewObject + ? (s, t) => Math.max(s, t) // if a new object is created in an alternative: ignore the current multiplicity, since a new object is created for each loop cycle! + : (s, t) => Math.max(s * currentMultiplicity, t) // otherwise as usual: take the current multiplicity into account + ); + if (createdNewObject) { + countCreatedObjects++; } } - } - // merge alternatives - mergeAssignmentUse(mapAllAlternatives, map); - if (nodesForNewObject.length >= 1) { - // these nodes are put into a new object => check their assignments independently - this.checkOperatorMultiplicitiesForMultiAssignmentsLogic(nodesForNewObject, accept); + // merge alternatives + mergeAssignmentUse(mapAllAlternatives, map); + // since we assume the worst case, we define, that the entire Alternatives node created a new object, if ALL its alternatives created a new object + if (countCreatedObjects === currentNode.elements.length) { + resultCreatedNewObject = true; + } } } + return resultCreatedNewObject; // indicates, whether a new object was created } checkInterfacePropertyTypes(interfaceDecl: ast.Interface, accept: ValidationAcceptor): void { @@ -1243,6 +1245,7 @@ function mergeAssignmentUse(mapSoure: Map, mapTarget: Map target.counter = counterOperation(source.counter, target.counter); } else { mapTarget.set(key, source); + source.counter = counterOperation(source.counter, 0); } } mapSoure.clear(); diff --git a/packages/langium/src/test/langium-test.ts b/packages/langium/src/test/langium-test.ts index af42694e3..ce142b081 100644 --- a/packages/langium/src/test/langium-test.ts +++ b/packages/langium/src/test/langium-test.ts @@ -4,24 +4,24 @@ * terms of the MIT License, which is available in the project root. ******************************************************************************/ +import * as assert from 'node:assert'; import type { CompletionItem, CompletionList, Diagnostic, DocumentSymbol, FoldingRange, FormattingOptions, Range, ReferenceParams, SemanticTokensParams, SemanticTokenTypes, TextDocumentIdentifier, TextDocumentPositionParams, WorkspaceSymbol } from 'vscode-languageserver-protocol'; +import { DiagnosticSeverity, MarkupContent } from 'vscode-languageserver-types'; +import { normalizeEOL } from '../generate/template-string.js'; +import type { LangiumServices, LangiumSharedLSPServices } from '../lsp/lsp-services.js'; +import { SemanticTokensDecoder } from '../lsp/semantic-token-provider.js'; +import type { ParserOptions } from '../parser/langium-parser.js'; import type { LangiumCoreServices, LangiumSharedCoreServices } from '../services.js'; import type { AstNode, CstNode, Properties } from '../syntax-tree.js'; -import { type LangiumDocument, TextDocument } from '../workspace/documents.js'; -import type { BuildOptions } from '../workspace/document-builder.js'; import type { AsyncDisposable } from '../utils/disposable.js'; -import type { LangiumServices, LangiumSharedLSPServices } from '../lsp/lsp-services.js'; -import type { ParserOptions } from '../parser/langium-parser.js'; -import { DiagnosticSeverity, MarkupContent } from 'vscode-languageserver-types'; -import { escapeRegExp } from '../utils/regexp-utils.js'; -import { URI } from '../utils/uri-utils.js'; +import { Disposable } from '../utils/disposable.js'; import { findNodeForProperty } from '../utils/grammar-utils.js'; -import { SemanticTokensDecoder } from '../lsp/semantic-token-provider.js'; -import * as assert from 'node:assert'; +import { escapeRegExp } from '../utils/regexp-utils.js'; import { stream } from '../utils/stream.js'; -import { Disposable } from '../utils/disposable.js'; -import { normalizeEOL } from '../generate/template-string.js'; +import { URI } from '../utils/uri-utils.js'; import { DocumentValidator } from '../validation/document-validator.js'; +import type { BuildOptions } from '../workspace/document-builder.js'; +import { TextDocument, type LangiumDocument } from '../workspace/documents.js'; export interface ParseHelperOptions extends BuildOptions { /** @@ -682,7 +682,7 @@ export function filterByOptions(validationResult: ValidationResult, filterOptions?: ExpectDiagnosticOptions): void { const filtered = filterOptions ? filterByOptions(validationResult, filterOptions) : validationResult.diagnostics; - expectedFunction(filtered.length, 0, `Expected no issues, but found ${filtered.length}`); + expectedFunction(filtered.length, 0, `Expected no issues, but found ${filtered.length}:\n${printDiagnostics(filtered)}`); } export function expectIssue(validationResult: ValidationResult, filterOptions?: ExpectDiagnosticOptions): void { @@ -711,6 +711,10 @@ export function expectWarning `line ${d.range.start.line}, column ${d.range.start.character}: ${d.message}`).join('\n') ?? ''; +} + /** * Add the given document to the `TextDocuments` service, simulating it being opened in an editor. * diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index 44b370e84..e5395cc4f 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -857,7 +857,7 @@ describe('Assignments with = instead of +=', () => { (persons=Person) | (persons=Person); Person: 'person' name=ID ; `)); - expect(validation.diagnostics.length).toBe(0); + expectNoIssues(validation); }); test('assignments in different alternatives, but looped', async () => { @@ -978,7 +978,7 @@ describe('Assignments with = instead of +=', () => { ',' persons=Person; Person: 'person' name=ID ; `)); - expect(validation.diagnostics.length).toBe(0); + expectNoIssues(validation); }); test('no problem: assignments in different parser rules', async () => { @@ -987,7 +987,7 @@ describe('Assignments with = instead of +=', () => { persons=Person; Person: 'person' name=ID persons=Person; `)); - expect(validation.diagnostics.length).toBe(0); + expectNoIssues(validation); }); test('no problem with actions: assignment is looped, but stored in a new object each time', async () => { @@ -997,7 +997,7 @@ describe('Assignments with = instead of +=', () => { Person infers Expression: {infer Person} 'person' name=ID ; `)); - expect(validation.diagnostics.length).toBe(0); + expectNoIssues(validation); }); test('no problem with actions: second assignment is stored in a new object', async () => { @@ -1007,7 +1007,7 @@ describe('Assignments with = instead of +=', () => { Person infers Expression: {infer Person} 'person' name=ID ; `)); - expect(validation.diagnostics.length).toBe(0); + expectNoIssues(validation); }); test('no problem with actions: three assignments into three different objects', async () => { @@ -1017,7 +1017,7 @@ describe('Assignments with = instead of +=', () => { Person infers Expression: {infer Person} 'person' name=ID ; `)); - expect(validation.diagnostics.length).toBe(0); + expectNoIssues(validation); }); test('actions: the rewrite part is a special assignment, which needs to be checked as well!', async () => { @@ -1031,6 +1031,104 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics[0].message).toBe(getMessage('left')); expect(validation.diagnostics[1].message).toBe(getMessage('left')); }); + + test('rewrite actions inside loop #1756 (complete examples, slightly adapted)', async () => { + const validation = await validate(getGrammar(` + entry Model: + expr=Expression; + + Expression: Equality; + + Equality infers Expression: + Literal ( ( {infer Equals.left=current} '==' | {infer NotEquals.left=current} '!=' ) right=Literal)*; + + Literal: value=ID; + `)); + expectNoIssues(validation); + }); + + test('rewrite actions inside loop #1756 (with a single alternative only)', async () => { + const validation = await validate(getGrammar(` + entry Model: + expr=Expression; + + Expression: Equality; + + Equality infers Expression: + Literal ( {infer Equals.left=current} '==' right=Literal)*; + + Literal: value=ID; + `)); + expectNoIssues(validation); + }); + + test('rewrite actions inside loop #1756 (single alternative in non-empty group)', async () => { + const validation = await validate(getGrammar(` + entry Model: + expr=Expression; + + Expression: Equality; + + Equality infers Expression: + Literal ( {infer Equals.left=current} 'nonemptygroup' '==' right=Literal)*; + + Literal: value=ID; + `)); + expectNoIssues(validation); + }); + + test('rewrite actions inside loop #1756 (single alternative in non-empty grouped group)', async () => { + const validation = await validate(getGrammar(` + entry Model: + expr=Expression; + + Expression: Equality; + + Equality infers Expression: + Literal ( ( {infer Equals.left=current} 'nonemptygroup' ) '==' right=Literal)*; + + Literal: value=ID; + `)); + expectNoIssues(validation); + }); + + test('rewrite actions inside loop #1756 (only a single alternative creates a new object)', async () => { + // Since we assume the worst case and since 'right' is assigned twice, if the alternative without the rewrite action is used, we expect a warning here as well! + const validation = await validate(getGrammar(` + entry Model: + expr=Expression; + + Expression: Equality; + + Equality infers Expression: + Literal ( ( {infer Equals.left=current} '==' | '!=' ) right=Literal)*; + + Literal: value=ID; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('right')); + }); + + test('no problem with rewrite actions on top-level: unassigned action', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person {infer Model} 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('no problem with rewrite actions on top-level: rewrite action', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person {infer Model.left=current} persons=Person; + Person: 'person' name=ID ; + `)); + expectNoIssues(validation); + }); + }); describe('Missing required properties are not arrays or booleans', () => {