diff --git a/package-lock.json b/package-lock.json index 58c85bb5e..962133257 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1779,6 +1779,7 @@ "resolved": "https://registry.npmjs.org/@octokit/core/-/core-6.1.5.tgz", "integrity": "sha512-vvmsN0r7rguA+FySiCsbaTTobSftpIDIpPW81trAmsv9TGxg3YCujAxRYp/Uy8xmDgYCzzgulG62H7KYUFmeIg==", "license": "MIT", + "peer": true, "dependencies": { "@octokit/auth-token": "^5.0.0", "@octokit/graphql": "^8.2.2", @@ -2585,6 +2586,7 @@ "resolved": "https://registry.npmjs.org/@types/node/-/node-20.17.48.tgz", "integrity": "sha512-KpSfKOHPsiSC4IkZeu2LsusFwExAIVGkhG1KkbaBMLwau0uMhj0fCrvyg9ddM2sAvd+gtiBJLir4LAw1MNMIaw==", "license": "MIT", + "peer": true, "dependencies": { "undici-types": "~6.19.2" } @@ -2672,6 +2674,7 @@ "integrity": "sha512-Zhy8HCvBUEfBECzIl1PKqF4p11+d0aUJS1GeUiuqK9WmOug8YCmC4h4bjyBvMyAMI9sbRczmrYL5lKg/YMbrcQ==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@typescript-eslint/scope-manager": "8.38.0", "@typescript-eslint/types": "8.38.0", @@ -3021,6 +3024,7 @@ "integrity": "sha512-IipSzX+8DptUdXN/GWq3hq5z18MwnpphYdOMm0WndkRGYELzfq7NDP8dMpZT7JGW1uXFrIGxOW2D0Xi++ulByg==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@vitest/utils": "3.1.3", "fflate": "^0.8.2", @@ -3197,6 +3201,7 @@ "resolved": "https://registry.npmjs.org/@yeoman/types/-/types-1.6.0.tgz", "integrity": "sha512-7Deh9qpDCoOmQal7Sdicb1v/Qv0KFhLqDHnSKh9NBKvvU4EKAFXmd4MuXFvGTCLQfKhuZ6doiM9CMUd4jGp25w==", "license": "MIT", + "peer": true, "engines": { "node": "^16.13.0 || >=18.12.0" }, @@ -3245,6 +3250,7 @@ "integrity": "sha512-NZyJarBfL7nWwIq+FDL6Zp/yHEhePMNnnJ0y3qfieCrmNvYct8uvtiV41UvlSe6apAfk0fY1FbWx+NwfmpvtTg==", "dev": true, "license": "MIT", + "peer": true, "bin": { "acorn": "bin/acorn" }, @@ -3842,6 +3848,7 @@ "resolved": "https://registry.npmjs.org/chevrotain/-/chevrotain-11.0.3.tgz", "integrity": "sha512-ci2iJH6LeIkvP9eJW6gpueU8cnZhv85ELY8w8WiFtNjMHA5ad6pQLaJo9mEly/9qUyCpvqX8/POVUTf18/HFdw==", "license": "Apache-2.0", + "peer": true, "dependencies": { "@chevrotain/cst-dts-gen": "11.0.3", "@chevrotain/gast": "11.0.3", @@ -4739,6 +4746,7 @@ "integrity": "sha512-QldCVh/ztyKJJZLr4jXNUByx3gR+TDYZCRXEktiZoUR3PGy4qCmSbkxcIle8GEwGpb5JBZazlaJ/CxLidXdEbQ==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@eslint-community/eslint-utils": "^4.2.0", "@eslint-community/regexpp": "^4.12.1", @@ -7125,6 +7133,7 @@ "resolved": "https://registry.npmjs.org/mem-fs/-/mem-fs-4.1.2.tgz", "integrity": "sha512-CMwusHK+Kz0tu1qjgbd0rwcJxzgg76jlkPpqK+pDTv8Hth8JyM7JlgxNWaAFRKe969HATPTz/sp8T63QflyI+w==", "license": "MIT", + "peer": true, "dependencies": { "@types/node": ">=18", "@types/vinyl": "^2.0.8", @@ -10142,6 +10151,7 @@ "integrity": "sha512-p1diW6TqL9L07nNxvRMM7hMMw4c5XOo/1ibL4aAIGmSAt9slTE1Xgw5KWuof2uTOvCg9BY7ZRi+GaF+7sfgPeQ==", "dev": true, "license": "Apache-2.0", + "peer": true, "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" @@ -10329,6 +10339,7 @@ "integrity": "sha512-cZn6NDFE7wdTpINgs++ZJ4N49W2vRp8LCKrn3Ob1kYNtOo21vfDoaV5GzBfLU4MovSAB8uNRm4jgzVQZ+mBzPQ==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "esbuild": "^0.25.0", "fdir": "^6.4.4", @@ -10427,6 +10438,7 @@ "integrity": "sha512-188iM4hAHQ0km23TN/adso1q5hhwKqUpv+Sd6p5sOuh6FhQnRNW3IsiIpvxqahtBabsJ2SLZgmGSpcYK4wQYJw==", "dev": true, "license": "MIT", + "peer": true, "dependencies": { "@vitest/expect": "3.1.3", "@vitest/mocker": "3.1.3", diff --git a/packages/langium/src/grammar/lsp/grammar-code-actions.ts b/packages/langium/src/grammar/lsp/grammar-code-actions.ts index e07daa887..358ec3b3e 100644 --- a/packages/langium/src/grammar/lsp/grammar-code-actions.ts +++ b/packages/langium/src/grammar/lsp/grammar-code-actions.ts @@ -13,7 +13,7 @@ import type { CodeActionProvider } from '../../lsp/code-action.js'; import type { LangiumServices } from '../../lsp/lsp-services.js'; import type { AstReflection, Reference, ReferenceInfo } from '../../syntax-tree.js'; import { getContainerOfType } from '../../utils/ast-utils.js'; -import { findLeafNodeAtOffset } from '../../utils/cst-utils.js'; +import { findLeafNodeAtOffset, getNextNode } from '../../utils/cst-utils.js'; import type { MaybePromise } from '../../utils/promise-utils.js'; import { escapeRegExp } from '../../utils/regexp-utils.js'; import type { URI } from '../../utils/uri-utils.js'; @@ -81,6 +81,9 @@ export class LangiumGrammarCodeActionProvider implements CodeActionProvider { case IssueCodes.SuperfluousInfer: accept(this.fixSuperfluousInfer(diagnostic, document)); break; + case IssueCodes.ReplaceOperatorMultiAssignment: + accept(this.replaceOperator(diagnostic, document)); + break; case DocumentValidator.LinkingError: { const data = diagnostic.data as LinkingErrorData; if (data && data.containerType === 'RuleCall' && data.property === 'rule') { @@ -479,6 +482,34 @@ export class LangiumGrammarCodeActionProvider implements CodeActionProvider { return result; } + private replaceOperator(diagnostic: Diagnostic, document: LangiumDocument): CodeAction | undefined { + const rootCst = document.parseResult.value.$cstNode; + if (rootCst) { + const offset = document.textDocument.offsetAt(diagnostic.range.start); + const cstNodeFeature = findLeafNodeAtOffset(rootCst, offset); + const assignment = getContainerOfType(cstNodeFeature?.astNode, node => ast.isAssignment(node) || ast.isAction(node)); + // the validation check marks the 'feature' (for better visibility), but we need to replace the 'operator': + const cstNodeOperator = cstNodeFeature ? getNextNode(cstNodeFeature, false) : undefined; + if (cstNodeOperator && assignment?.$cstNode && assignment.feature && assignment.operator === '=') { // replacing '?=' by '+=' is usually not very helpful, e.g. in cases like "(marked?='marked')+" + return { + title: `Replace '${assignment.operator}' with '+='`, + kind: CodeActionKind.QuickFix, + diagnostics: [diagnostic], + isPreferred: true, + edit: { + changes: { + [document.textDocument.uri]: [{ + range: cstNodeOperator.range, + newText: '+=' + }] + } + } + }; + } + } + return undefined; + } + } function getRelativeImport(source: URI, target: URI): string { diff --git a/packages/langium/src/grammar/validation/validator.ts b/packages/langium/src/grammar/validation/validator.ts index a1a6d0f8f..b194dc446 100644 --- a/packages/langium/src/grammar/validation/validator.ts +++ b/packages/langium/src/grammar/validation/validator.ts @@ -10,15 +10,15 @@ import * as ast from '../../languages/generated/ast.js'; import type { NamedAstNode } from '../../references/name-provider.js'; import type { References } from '../../references/references.js'; import type { AstNode, Properties, Reference } from '../../syntax-tree.js'; -import { getContainerOfType, streamAllContents, streamAst } from '../../utils/ast-utils.js'; +import { getContainerOfType, getDocument, streamAllContents, streamAst } from '../../utils/ast-utils.js'; import { MultiMap } from '../../utils/collections.js'; import { toDocumentSegment } from '../../utils/cst-utils.js'; -import { assertUnreachable } from '../../utils/errors.js'; +import { assertCondition, assertUnreachable } from '../../utils/errors.js'; import { findNameAssignment, findNodeForKeyword, findNodeForProperty, getAllReachableRules, getAllRulesUsedForCrossReferences, isArrayCardinality, isDataTypeRule, isOptionalCardinality, terminalRegex } from '../../utils/grammar-utils.js'; import type { Stream } from '../../utils/stream.js'; import { stream } from '../../utils/stream.js'; import { UriUtils } from '../../utils/uri-utils.js'; -import type { DiagnosticData, ValidationAcceptor, ValidationChecks } from '../../validation/validation-registry.js'; +import type { DiagnosticData, DiagnosticInfo, ValidationAcceptor, ValidationChecks } from '../../validation/validation-registry.js'; import { diagnosticData } from '../../validation/validation-registry.js'; import type { AstNodeLocator } from '../../workspace/ast-node-locator.js'; import type { LangiumDocuments } from '../../workspace/documents.js'; @@ -44,6 +44,7 @@ export function registerValidationChecks(services: LangiumGrammarServices): void const checks: ValidationChecks = { Action: [ validator.checkAssignmentReservedName, + validator.checkActionAssignmentsToTheSameFeature, ], AbstractRule: validator.checkRuleName, Assignment: [ @@ -59,7 +60,7 @@ export function registerValidationChecks(services: LangiumGrammarServices): void validator.checkRuleParameters, validator.checkEmptyParserRule, validator.checkParserRuleReservedName, - validator.checkOperatorMultiplicitiesForMultiAssignments, + validator.checkRuleAssignmentsToTheSameFeature, ], InfixRule: [ validator.checkInfixRuleDataType, @@ -139,6 +140,8 @@ export namespace IssueCodes { export const SuperfluousInfer = 'superfluous-infer'; export const OptionalUnorderedGroup = 'optional-unordered-group'; export const ParsingRuleEmpty = 'parsing-rule-empty'; + export const ReplaceOperatorMultiAssignment = 'replace-operator-for-multi-assignments'; + export const MixedAssignmentOperators = 'mixed-use-of-assignment-operators'; } export class LangiumGrammarValidator { @@ -1029,48 +1032,118 @@ export class LangiumGrammarValidator { } } - /** This validation recursively looks at all assignments (and rewriting actions) with '=' as assignment operator and checks, - * whether the operator should be '+=' instead. */ - checkOperatorMultiplicitiesForMultiAssignments(rule: ast.ParserRule, accept: ValidationAcceptor): void { - // for usual parser rules AND for fragments, but not for data type rules! - if (!rule.dataType) { - this.checkOperatorMultiplicitiesForMultiAssignmentsIndependent([rule.definition], accept); + /** + * This validation recursively collects all assignments (and rewriting actions: see the check/entry point in the next function) to the same feature and validates them: + * Assignment operators '?=', '=' and '+=' should not be mixed. + * Assignments with '=' as assignment operator should be replaced be '+=', if assignments to the feature might occure more than once. + */ + checkRuleAssignmentsToTheSameFeature(rule: ast.ParserRule, accept: ValidationAcceptor): void { + // validate only usual parser rules, but no fragments (they are validated when validating their using parser rules) and no data type rules! + if (!rule.dataType && !rule.fragment) { + this.checkAssignmentsToTheSameFeature(rule, [rule.definition], new Map(), accept); } } - private checkOperatorMultiplicitiesForMultiAssignmentsIndependent(startNodes: AstNode[], accept: ValidationAcceptor, map: Map = new Map()): void { + checkActionAssignmentsToTheSameFeature(action: ast.Action, accept: ValidationAcceptor): void { + if (action.feature) { + // tree rewriting action + const mapForNewObject = new Map(); + storeAssignmentUse(mapForNewObject, action.feature, 1, action); // remember the special rewriting feature + // all following nodes are put into the new object => check their assignments independently + const sibblings = ast.isGroup(action.$container) || ast.isUnorderedGroup(action.$container) ? action.$container.elements : [action]; + this.checkAssignmentsToTheSameFeature(action, sibblings.slice(sibblings.indexOf(action) + 1), mapForNewObject, accept); + } else { + // this action does not create a new AstNode => no assignments to check + } + } + + private checkAssignmentsToTheSameFeature( + startRule: ast.ParserRule|ast.Action, startNodes: AstNode[], map: Map, accept: ValidationAcceptor + ): void { // check all starting nodes - this.checkOperatorMultiplicitiesForMultiAssignmentsNested(startNodes, 1, map, accept); + const circularFragments = new Set(); + this.checkAssignmentsToTheSameFeatureNested(startRule, startNodes, 1, map, [], circularFragments, accept); + + // handle properties of fragments which are called in circular way => count its properties multiple times + for (const values of map.values()) { + for (const assignment of values.assignments) { + // for each found assignment, check whether it is defined inside a fragment which is called in circular way + const fragmentContainer = getContainerOfType(assignment, ast.isParserRule); + if (fragmentContainer && circularFragments.has(fragmentContainer)) { + values.counter += 1; // This calculation is not accurate, it is only important to know, that this assignment is called multiple times (>= 2). + } + } + } // create the warnings for (const entry of map.values()) { - if (entry.counter >= 2) { + // check mixed use of ?=, = and += + const usedOperators = new Set(); + for (const assignment of entry.assignments) { + if (assignment.operator) { + usedOperators.add(assignment.operator); + } + } + if (usedOperators.size >= 2) { + const usedOperatorsPrinted = stream(usedOperators).map(op => `'${op}'`).join(', '); + for (const assignment of entry.assignments) { + const [info, docMsg] = createInfo(assignment, IssueCodes.MixedAssignmentOperators); + accept( + 'warning', + `Don't mix operators (${usedOperatorsPrinted}) when assigning values to the same feature '${assignment.feature}'${docMsg}.`, + info, // the issue code is not used for a code action, but is relevant for serializability + ); + } + } else if (entry.counter >= 2) { + // check for multiple assignments with '?=' or '=' instead of '+=' for (const assignment of entry.assignments) { if (assignment.operator !== '+=') { + const [info, docMsg] = createInfo(assignment, IssueCodes.ReplaceOperatorMultiAssignment); accept( 'warning', - `Found multiple assignments to '${assignment.feature}' with the '${assignment.operator}' assignment operator. Consider using '+=' instead to prevent data loss.`, - { node: assignment, property: 'feature' } // use 'feature' instead of 'operator', since it is pretty hard to see + `Found multiple assignments to '${assignment.feature}' with the '${assignment.operator}' assignment operator${docMsg}. Consider using '+=' instead to prevent data loss.`, + info, // the issue code is used for a code action, and it is relevant for serializability as well ); } } + } else { + // the assignments to this feature are fine + } + } + + // Utility to handle the case, that the critical assignment is located in another document (for which validation markers cannot be reported now). + function createInfo(assignment: ast.Assignment | ast.Action, code: string): [DiagnosticInfo, string] { + const assignmentDocument = getDocument(assignment); + if (assignmentDocument === getDocument(startRule)) { + // the assignment is located in the document of the start rule which is currently validated + return [>{ + node: assignment, + property: 'feature', // use 'feature' instead of 'operator', since it is pretty hard to see + data: diagnosticData(code), + }, '']; + } else { + // the assignment is located inside another document => annotate the issue at the start rule + return [>{ + node: startRule, + property: 'name', + data: diagnosticData(code), + }, ` by rule '${getContainerOfType(assignment, ast.isParserRule)?.name}' in the grammar '${UriUtils.basename(assignmentDocument.uri)}'`]; } } } - private checkOperatorMultiplicitiesForMultiAssignmentsNested(nodes: AstNode[], parentMultiplicity: number, map: Map, accept: ValidationAcceptor): boolean { + private checkAssignmentsToTheSameFeatureNested( + startRule: ast.ParserRule|ast.Action, nodes: AstNode[], parentMultiplicity: number, map: Map, + visiting: ast.ParserRule[], circularFragments: Set, accept: ValidationAcceptor + ): boolean { let resultCreatedNewObject = false; // check all given elements - for (let i = 0; i < nodes.length; i++) { - const currentNode = nodes[i]; + for (const currentNode of nodes) { // Tree-Rewrite-Actions are a special case: a new object is created => following assignments are put into the new object if (ast.isAction(currentNode) && currentNode.feature) { // (This does NOT count for unassigned actions, i.e. actions without feature name, since they change only the type of the current object.) - const mapForNewObject = new Map(); - storeAssignmentUse(mapForNewObject, currentNode.feature, 1, currentNode); // remember the special rewriting feature - // all following nodes are put into the new object => check their assignments independently - this.checkOperatorMultiplicitiesForMultiAssignmentsIndependent(nodes.slice(i + 1), accept, mapForNewObject); + // The assignments to AstNodes created by this action are validated by another validation check => nothing to do here resultCreatedNewObject = true; break; // breaks the current loop } @@ -1091,15 +1164,27 @@ export class LangiumGrammarValidator { // Search for assignments in used fragments as well, since their property values are stored in the current object. // But do not search in calls of regular parser rules, since parser rules create new objects. if (ast.isRuleCall(currentNode) && ast.isParserRule(currentNode.rule.ref) && currentNode.rule.ref.fragment) { - const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested([currentNode.rule.ref.definition], currentMultiplicity, map, accept); - resultCreatedNewObject = createdNewObject || resultCreatedNewObject; + const foundIndex = visiting.indexOf(currentNode.rule.ref); + if (foundIndex >= 0) { + // remember fragments which are called in circular way (their assignments will be counted more often later) + // all currently visited fragments are part of the circle/path + for (let i = foundIndex; i < visiting.length; i++) { + const circleParticipant = visiting[i]; + circularFragments.add(circleParticipant); + } + } else { + visiting.push(currentNode.rule.ref); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, [currentNode.rule.ref.definition], currentMultiplicity, map, visiting, circularFragments, accept); + resultCreatedNewObject = createdNewObject || resultCreatedNewObject; + assertCondition(visiting.pop() === currentNode.rule.ref); // prevent circles, but don't "cache" the results, since the fragment could be called multiple times in non-circular way + } } // look for assignments to the same feature nested within groups if (ast.isGroup(currentNode) || ast.isUnorderedGroup(currentNode)) { // all members of the group are relavant => collect them all const mapGroup: Map = new Map(); // store assignments for Alternatives separately - const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested(currentNode.elements, 1, mapGroup, accept); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, currentNode.elements, 1, mapGroup, visiting, circularFragments, accept); mergeAssignmentUse(mapGroup, map, createdNewObject ? (s, t) => (s + t) // if a new object is created in the group: ignore the current multiplicity, since a new object is created for each loop cycle! : (s, t) => (s * currentMultiplicity + t) // otherwise as usual: take the current multiplicity into account @@ -1111,9 +1196,9 @@ export class LangiumGrammarValidator { if (ast.isAlternatives(currentNode)) { const mapAllAlternatives: Map = new Map(); // store assignments for Alternatives separately let countCreatedObjects = 0; - for (const child of currentNode.elements) { + for (const alternative of currentNode.elements) { const mapCurrentAlternative: Map = new Map(); - const createdNewObject = this.checkOperatorMultiplicitiesForMultiAssignmentsNested([child], 1, mapCurrentAlternative, accept); + const createdNewObject = this.checkAssignmentsToTheSameFeatureNested(startRule, [alternative], 1, mapCurrentAlternative, visiting, circularFragments, accept); mergeAssignmentUse(mapCurrentAlternative, mapAllAlternatives, createdNewObject ? (s, t) => Math.max(s, t) // if a new object is created in an alternative: ignore the current multiplicity, since a new object is created for each loop cycle! : (s, t) => Math.max(s * currentMultiplicity, t) // otherwise as usual: take the current multiplicity into account diff --git a/packages/langium/test/grammar/grammar-validator.test.ts b/packages/langium/test/grammar/grammar-validator.test.ts index 715b3f632..550380a85 100644 --- a/packages/langium/test/grammar/grammar-validator.test.ts +++ b/packages/langium/test/grammar/grammar-validator.test.ts @@ -5,7 +5,7 @@ ******************************************************************************/ import type { AstNode, Grammar, LangiumDocument, Properties } from 'langium'; -import { AstUtils, EmptyFileSystem, GrammarAST, URI } from 'langium'; +import { AstUtils, EmptyFileSystem, GrammarAST, stream, URI } from 'langium'; import { expandToString } from 'langium/generate'; import { IssueCodes, createLangiumGrammarServices } from 'langium/grammar'; import type { ValidationResult } from 'langium/test'; @@ -1300,9 +1300,9 @@ describe('Property type is not a mix of cross-ref and non-cross-ref types.', () }); -describe('Assignments with = instead of +=', () => { - function getMessage(featureName: string): string { - return `Found multiple assignments to '${featureName}' with the '=' assignment operator. Consider using '+=' instead to prevent data loss.`; +describe('Assignments with = (or ?=) instead of +=', () => { + function getMessage(featureName: string, operator: ('=' | '?=') = '='): string { + return `Found multiple assignments to '${featureName}' with the '${operator}' assignment operator. Consider using '+=' instead to prevent data loss.`; } function getGrammar(content: string): string { return ` @@ -1475,6 +1475,30 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics[0].message).toBe(getMessage('persons')); }); + test('fragments called with different cardinalities', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign (';' Assign)?; + fragment Assign: + ',' persons=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + + test('fragments called with different cardinalities and looped', async () => { + const validation = await validate(getGrammar(` + entry Model: + (';' Assign?)*; + fragment Assign: + ',' persons=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + test('fragments in alternatives: once in 1st, twice in 2nd alternative', async () => { // This suggests the user of Langium to use a list in both cases, which might not be necessary for the 1st alternative. // But that is better than loosing a value in the 2nd alternative. @@ -1489,6 +1513,137 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics[0].message).toBe(getMessage('persons')); }); + test('fragment calls itself in circular way (assignment before the circular call)', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign; + fragment Assign: + ',' persons=Person ('and' Assign)?; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + + test('fragment calls itself in circular way (assignment after the circular call)', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign; + fragment Assign: + ',' ('and' Assign)? persons=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + + test('two fragments call each other in circular way', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign1; + fragment Assign1: + ',' ('and' Assign2)? persons1=Person; + fragment Assign2: + ',' ('and' Assign1)? persons2=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('persons2')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons1')); + }); + + test('two fragments call each other in circular way, another fragment not being part of the circle', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign1; + fragment Assign1: + ',' Assign2 ('and' Assign3)? persons1=Person; + fragment Assign2: + ',' persons2=Person; + fragment Assign3: + ',' ('and' Assign1)? persons3=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('persons3')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons1')); + }); + + test('circlic calls of three fragments (1 -> 2 -> 3 -> 1)', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign1; + fragment Assign1: + ',' ('and' Assign2)? persons1=Person; + fragment Assign2: + ',' ('and' Assign3)? persons2=Person; + fragment Assign3: + ',' ('and' Assign1)? persons3=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(3); + expect(validation.diagnostics[0].message).toBe(getMessage('persons3')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons2')); + expect(validation.diagnostics[2].message).toBe(getMessage('persons1')); + }); + + test('circlic calls of three fragments (1 -> 2 -> 1 -> 3 -> 1)', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign1; + fragment Assign1: + ',' ('and' Assign2)? persons1=Person ('and' Assign3)?; + fragment Assign2: + ',' ('and' Assign1)? persons2=Person; + fragment Assign3: + ',' ('and' Assign1)? persons3=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(3); + expect(validation.diagnostics[0].message).toBe(getMessage('persons2')); + expect(validation.diagnostics[1].message).toBe(getMessage('persons1')); + expect(validation.diagnostics[2].message).toBe(getMessage('persons3')); + }); + + test('circlic calls of fragments with rewrite action', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign1; + fragment Assign1: + ',' ('and' Assign2)? {infer Other.other=current} persons1=Person; + fragment Assign2: + ',' ('and' Assign1)? persons2=Person; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons2')); + }); + + test('dont validate fragments as starting point, since they are validated when validating their calling parser rules', async () => { + const validation = await validate(getGrammar(` + entry Model: + Assign; + fragment Assign: + 'persons' (persons=Person)*; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('persons')); + }); + + test('dont validate fragments as starting point, since they are validated when validating their calling parser rules: unused fragment', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person; + fragment Assign: // this fragment is not used + 'persons' (persons=Person)*; + Person: 'person' name=ID ; + `)); + expect(validation.diagnostics.length).toBe(1); + // We are informed about the unused fragment, but not about the assignment issue inside (this will happen, when using the fragment). + expect(validation.diagnostics[0].message).toBe('This rule is declared but never referenced.'); + }); + test('no problem: fragments in alternatives', async () => { const validation = await validate(getGrammar(` entry Model: @@ -1628,6 +1783,21 @@ describe('Assignments with = instead of +=', () => { expect(validation.diagnostics[0].message).toBe(getMessage('right')); }); + test('rewrite actions inside fragment, which is called by two different parser rules => validate action only once', async () => { + const validation = await validate(getGrammar(` + entry Model: + r1=R1 r2=R2; + + R1: F; + R2: F; + + fragment F: + a=ID {infer Equals.left=current} (b=ID)+; + `)); + expect(validation.diagnostics.length).toBe(1); // the fragment containing the rewrite action is called twice, but the rewrite action is validated only once + expect(validation.diagnostics[0].message).toBe(getMessage('b')); + }); + test('no problem with rewrite actions on top-level: unassigned action', async () => { const validation = await validate(getGrammar(` entry Model: @@ -1648,6 +1818,81 @@ describe('Assignments with = instead of +=', () => { expectNoIssues(validation); }); + // the test cases for multiple ?= assignments are not complete, since the logic to identify them is the same as for multiple = assignments + + test('Looped ?= assignment', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person; + Person: ('person' name+=ID active?='active')+; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('active', '?=')); + }); + + test('Looped ?= assignment via fragment', async () => { + const validation = await validate(getGrammar(` + entry Model: + persons=Person; + fragment Assign: + 'person' name+=ID active?='active'; + Person: Assign+; + `)); + expect(validation.diagnostics.length).toBe(1); + expect(validation.diagnostics[0].message).toBe(getMessage('active', '?=')); + }); + +}); + +describe('Mixed use of "?=", "=" and "+=" for the same feature', () => { + function getMessage(featureName: string, ...operators: Array<'?=' | '=' | '+='>): string { + return `Don't mix operators (${stream(operators).map(op => `'${op}'`).join(', ')}) when assigning values to the same feature '${featureName}'.`; + } + function getGrammar(content: string): string { + return ` + grammar HelloWorld + ${content} + hidden terminal WS: /\\s+/; + terminal ID: /[_a-zA-Z][\\w_]*/; + `; + } + + test('= and +=', async () => { + const validation = await validate(getGrammar(` + entry Person: 'person' name=ID name+=ID; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('name', '=', '+=')); + expect(validation.diagnostics[1].message).toBe(getMessage('name', '=', '+=')); + }); + + test('= and ?=', async () => { + const validation = await validate(getGrammar(` + entry Person: 'person' name=ID name?=ID; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('name', '=', '?=')); + expect(validation.diagnostics[1].message).toBe(getMessage('name', '=', '?=')); + }); + + test('?= and +=', async () => { + const validation = await validate(getGrammar(` + entry Person: 'person' name?=ID name+=ID; + `)); + expect(validation.diagnostics.length).toBe(2); + expect(validation.diagnostics[0].message).toBe(getMessage('name', '?=', '+=')); + expect(validation.diagnostics[1].message).toBe(getMessage('name', '?=', '+=')); + }); + + test('?=, = and +=', async () => { + const validation = await validate(getGrammar(` + entry Person: 'person' name?=ID name=ID name+=ID; + `)); + expect(validation.diagnostics.length).toBe(3); + expect(validation.diagnostics[0].message).toBe(getMessage('name', '?=', '=', '+=')); + expect(validation.diagnostics[1].message).toBe(getMessage('name', '?=', '=', '+=')); + expect(validation.diagnostics[2].message).toBe(getMessage('name', '?=', '=', '+=')); + }); }); describe('Missing required properties are not arrays or booleans', () => { diff --git a/packages/langium/test/grammar/lsp/grammar-code-actions.test.ts b/packages/langium/test/grammar/lsp/grammar-code-actions.test.ts index 4092001a6..d1112a4c0 100644 --- a/packages/langium/test/grammar/lsp/grammar-code-actions.test.ts +++ b/packages/langium/test/grammar/lsp/grammar-code-actions.test.ts @@ -95,3 +95,49 @@ describe('Langium grammar actions for validations: Parser rules used only as typ expect(action!.title).toBe('Replace with type declaration'); } }); + +describe('Replace = by += for multiple assignments to the same feature', () => { + function composeGrammatik(assignment: string): string { + return ` + grammar HelloWorld + entry Model: + ${assignment} + Person: 'person' name=ID ; + hidden terminal WS: /\\s+/; + terminal ID: /[_a-zA-Z][\\w_]*/; + `; + } + + test('no whitespace', async () => testReplaceAction( + composeGrammatik('persons=Person* ;'), + composeGrammatik('persons+=Person* ;') + )); + + test('some spaces', async () => testReplaceAction( + composeGrammatik('persons = Person* ;'), + composeGrammatik('persons += Person* ;') + )); + + test('some inline comments', async () => testReplaceAction( + composeGrammatik('persons/*before*/= /*after*/ Person* ;'), + composeGrammatik('persons/*before*/+= /*after*/ Person* ;') + )); + + test('some line comments', async () => testReplaceAction( + composeGrammatik(`persons// + = // more + + Person* ;`), + composeGrammatik(`persons// + += // more + + Person* ;`) + )); + + async function testReplaceAction(textBefore: string, textAfter: string) { + const result = await testCodeActions(textBefore, IssueCodes.ReplaceOperatorMultiAssignment, textAfter); + const action = result.action; + expect(action).toBeTruthy(); + expect(action!.title).toBe("Replace '=' with '+='"); + } +});