Add code actions to replace parser rules with type declarations (#1391)

Author: JohannesMeierSE

packages/langium/src/grammar/lsp/grammar-code-actions.ts

@@ -5,25 +5,25 @@
  ******************************************************************************/
 
 import type { Diagnostic } from 'vscode-languageserver';
+import { CodeActionKind } from 'vscode-languageserver';
 import type { CodeActionParams } from 'vscode-languageserver-protocol';
 import type { CodeAction, Command, Position, TextEdit } from 'vscode-languageserver-types';
-import type { URI } from '../../utils/uri-utils.js';
+import * as ast from '../../languages/generated/ast.js';
 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 type { MaybePromise } from '../../utils/promise-utils.js';
-import type { LinkingErrorData } from '../../validation/document-validator.js';
-import type { DiagnosticData } from '../../validation/validation-registry.js';
-import type { LangiumDocument } from '../../workspace/documents.js';
-import type { IndexManager } from '../../workspace/index-manager.js';
-import { CodeActionKind } from 'vscode-languageserver';
 import { getContainerOfType } from '../../utils/ast-utils.js';
 import { findLeafNodeAtOffset } from '../../utils/cst-utils.js';
 import { findNodeForProperty } from '../../utils/grammar-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';
 import { UriUtils } from '../../utils/uri-utils.js';
+import type { LinkingErrorData } from '../../validation/document-validator.js';
 import { DocumentValidator } from '../../validation/document-validator.js';
-import * as ast from '../../languages/generated/ast.js';
+import type { DiagnosticData } from '../../validation/validation-registry.js';
+import type { LangiumDocument } from '../../workspace/documents.js';
+import type { IndexManager } from '../../workspace/index-manager.js';
 import { IssueCodes } from '../validation/validator.js';
 
 export class LangiumGrammarCodeActionProvider implements CodeActionProvider {
@@ -63,6 +63,9 @@ export class LangiumGrammarCodeActionProvider implements CodeActionProvider {
             case IssueCodes.CrossRefTokenSyntax:
                 accept(this.fixCrossRefSyntax(diagnostic, document));
                 break;
+            case IssueCodes.ParserRuleToTypeDecl:
+                accept(this.replaceParserRuleByTypeDeclaration(diagnostic, document));
+                break;
             case IssueCodes.UnnecessaryFileExtension:
                 accept(this.fixUnnecessaryFileExtension(diagnostic, document));
                 break;
@@ -180,6 +183,66 @@ export class LangiumGrammarCodeActionProvider implements CodeActionProvider {
         return undefined;
     }
 
+    private isRuleReplaceable(rule: ast.ParserRule): boolean {
+        /** at the moment, only "pure" parser rules are supported:
+         * - supported are only Alternatives (recursively) and "infers"
+         * - "returns" is not relevant, since cross-references would not refer to the parser rule, but to its "return type" instead
+         */
+        return !rule.fragment && !rule.entry && rule.parameters.length === 0 && !rule.definesHiddenTokens && !rule.wildcard && !rule.returnType && !rule.dataType;
+    }
+    private replaceRule(rule: ast.ParserRule): string {
+        const type = rule.inferredType ?? rule;
+        return type.name;
+    }
+    private isDefinitionReplaceable(node: ast.AbstractElement): boolean {
+        if (ast.isRuleCall(node)) {
+            return node.arguments.length === 0 && ast.isParserRule(node.rule.ref) && this.isRuleReplaceable(node.rule.ref);
+        }
+        if (ast.isAlternatives(node)) {
+            return node.elements.every(child => this.isDefinitionReplaceable(child));
+        }
+        return false;
+    }
+    private replaceDefinition(node: ast.AbstractElement): string {
+        if (ast.isRuleCall(node) && node.rule.ref) {
+            return node.rule.ref.name;
+        }
+        if (ast.isAlternatives(node)) {
+            return node.elements.map(child => this.replaceDefinition(child)).join(' | ');
+        }
+        throw new Error('missing code for ' + node);
+    }
+
+    private replaceParserRuleByTypeDeclaration(diagnostic: Diagnostic, document: LangiumDocument): CodeAction | undefined {
+        const rootCst = document.parseResult.value.$cstNode;
+        if (rootCst) {
+            const offset = document.textDocument.offsetAt(diagnostic.range.start);
+            const cstNode = findLeafNodeAtOffset(rootCst, offset);
+            const rule = getContainerOfType(cstNode?.astNode, ast.isParserRule);
+            if (rule && rule.$cstNode) {
+                const isReplaceable = this.isRuleReplaceable(rule) && this.isDefinitionReplaceable(rule.definition);
+                if (isReplaceable) {
+                    const newText = `type ${this.replaceRule(rule)} = ${this.replaceDefinition(rule.definition)};`;
+                    return {
+                        title: 'Replace with type declaration',
+                        kind: CodeActionKind.QuickFix,
+                        diagnostics: [diagnostic],
+                        isPreferred: true,
+                        edit: {
+                            changes: {
+                                [document.textDocument.uri]: [{
+                                    range: diagnostic.range,
+                                    newText
+                                }]
+                            }
+                        }
+                    };
+                }
+            }
+        }
+        return undefined;
+    }
+
     private fixUnnecessaryFileExtension(diagnostic: Diagnostic, document: LangiumDocument): CodeAction {
         const end = {...diagnostic.range.end};
         end.character -= 1;

packages/langium/src/grammar/validation/validator.ts

@@ -13,7 +13,7 @@ import type { AstNode, Properties, Reference } from '../../syntax-tree.js';
 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, isArrayCardinality, isDataTypeRule, isOptionalCardinality, terminalRegex } from '../../utils/grammar-utils.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 type { DiagnosticData, ValidationAcceptor, ValidationChecks } from '../../validation/validation-registry.js';
@@ -107,6 +107,7 @@ export namespace IssueCodes {
     export const UseRegexTokens = 'use-regex-tokens';
     export const EntryRuleTokenSyntax = 'entry-rule-token-syntax';
     export const CrossRefTokenSyntax = 'cross-ref-token-syntax';
+    export const ParserRuleToTypeDecl = 'parser-rule-to-type-decl';
     export const UnnecessaryFileExtension = 'unnecessary-file-extension';
     export const InvalidReturns = 'invalid-returns';
     export const InvalidInfers = 'invalid-infers';
@@ -605,17 +606,25 @@ export class LangiumGrammarValidator {
 
     checkGrammarForUnusedRules(grammar: ast.Grammar, accept: ValidationAcceptor): void {
         const reachableRules = getAllReachableRules(grammar, true);
+        const parserRulesUsedByCrossReferences = getAllRulesUsedForCrossReferences(grammar);
 
         for (const rule of grammar.rules) {
             if (ast.isTerminalRule(rule) && rule.hidden || isEmptyRule(rule)) {
                 continue;
             }
             if (!reachableRules.has(rule)) {
-                accept('hint', 'This rule is declared but never referenced.', {
-                    node: rule,
-                    property: 'name',
-                    tags: [DiagnosticTag.Unnecessary]
-                });
+                if (ast.isParserRule(rule) && parserRulesUsedByCrossReferences.has(rule)) {
+                    accept('hint', 'This parser rule is not used for parsing, but referenced by cross-references. Consider to replace this rule by a type declaration.', {
+                        node: rule,
+                        data: diagnosticData(IssueCodes.ParserRuleToTypeDecl)
+                    });
+                } else {
+                    accept('hint', 'This rule is declared but never referenced.', {
+                        node: rule,
+                        property: 'name',
+                        tags: [DiagnosticTag.Unnecessary]
+                    });
+                }
             }
         }
     }

packages/langium/src/test/langium-test.ts

@@ -6,7 +6,7 @@
 
 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 { CodeAction, 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';
@@ -22,7 +22,6 @@ 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 {
     /**
      * Specifies the URI of the generated document. Will use a counter variable if not specified.
@@ -770,3 +769,103 @@ export function expectSemanticToken(tokensWithRanges: DecodedSemanticTokensWithR
     });
     expectedFunction(result.length, 1, `Expected one token with the specified options but found ${result.length}`);
 }
+
+export interface CodeActionResult<T extends AstNode = AstNode> extends AsyncDisposable {
+    /** the document containing the AST */
+    document: LangiumDocument<T>;
+    /** all diagnostics of the validation */
+    diagnosticsAll: Diagnostic[];
+    /** the relevant Diagnostic with the given diagnosticCode, it is expected that the given input has exactly one such diagnostic */
+    diagnosticRelevant: Diagnostic;
+    /** the CodeAction to fix the found relevant problem, it is possible, that there is no such code action */
+    action?: CodeAction;
+}
+
+/**
+ * This is a helper function to easily test code actions (quick-fixes) for validation problems.
+ * @param services the Langium services for the language with code actions
+ * @returns A function to easily test a single code action on the given invalid 'input'.
+ * This function expects, that 'input' contains exactly one validation problem with the given 'diagnosticCode'.
+ * If 'outputAfterFix' is specified, this functions checks, that the diagnostic comes with a single code action for this validation problem.
+ * After applying this code action, 'input' is transformed to 'outputAfterFix'.
+ */
+export function testCodeAction<T extends AstNode = AstNode>(services: LangiumServices): (input: string, diagnosticCode: string, outputAfterFix: string | undefined, options?: ParseHelperOptions) => Promise<CodeActionResult<T>> {
+    const validateHelper = validationHelper<T>(services);
+    return async (input, diagnosticCode, outputAfterFix, options) => {
+        // parse + validate
+        const validationBefore = await validateHelper(input, options);
+        const document = validationBefore.document;
+        const diagnosticsAll = document.diagnostics ?? [];
+        // use only the diagnostics with the given validation code
+        const diagnosticsRelevant = diagnosticsAll.filter(d => d.data && 'code' in d.data && d.data.code === diagnosticCode);
+        // expect exactly one validation with the given code
+        expectedFunction(diagnosticsRelevant.length, 1);
+        const diagnosticRelevant = diagnosticsRelevant[0];
+
+        // check, that the code actions are generated for the selected validation:
+        // prepare the action provider
+        const actionProvider = expectTruthy(services.lsp.CodeActionProvider);
+        // request the actions for this diagnostic
+        const currentActions = await actionProvider!.getCodeActions(document, {
+            ...textDocumentParams(document),
+            range: diagnosticRelevant.range,
+            context: {
+                diagnostics: diagnosticsRelevant,
+                triggerKind: 1 // explicitly triggered by users (or extensions)
+            }
+        });
+
+        // evaluate the resulting actions
+        let action: CodeAction | undefined;
+        let validationAfter: ValidationResult | undefined;
+        if (outputAfterFix) {
+            // exactly one code action is expected
+            expectTruthy(currentActions);
+            expectTruthy(Array.isArray(currentActions));
+            expectedFunction(currentActions!.length, 1);
+            expectTruthy(CodeAction.is(currentActions![0]));
+            action = currentActions![0] as CodeAction;
+
+            // execute the found code action
+            const edits = expectTruthy(action.edit?.changes![document.textDocument.uri]);
+            const updatedText = TextDocument.applyEdits(document.textDocument, edits!);
+
+            // check the result after applying the code action:
+            // 1st text is updated as expected
+            expectedFunction(updatedText, outputAfterFix);
+            // 2nd the validation diagnostic is gone after applying the code action
+            validationAfter = await validateHelper(updatedText, options);
+            const diagnosticsUpdated = validationAfter.diagnostics.filter(d => d.data && 'code' in d.data && d.data.code === diagnosticCode);
+            expectedFunction(diagnosticsUpdated.length, 0);
+        } else {
+            // no code action is expected
+            expectFalsy(currentActions);
+        }
+
+        // collect the data to return
+        async function dispose(): Promise<void> {
+            validationBefore.dispose();
+            validationAfter?.dispose();
+        }
+        return {
+            document,
+            diagnosticsAll,
+            diagnosticRelevant: diagnosticRelevant,
+            action,
+            dispose: () => dispose()
+        };
+    };
+}
+
+function expectTruthy<T>(value: T): NonNullable<T> {
+    if (value) {
+        return value;
+    } else {
+        throw new Error();
+    }
+}
+function expectFalsy(value: unknown) {
+    if (value) {
+        throw new Error();
+    }
+}

packages/langium/src/utils/grammar-utils.ts

@@ -68,6 +68,28 @@ function ruleDfs(rule: ast.AbstractRule, visitedSet: Set<string>, allTerminals:
     });
 }
 
+/**
+ * Returns all parser rules which provide types which are used in the grammar as type in cross-references.
+ * @param grammar the grammar to investigate
+ * @returns the set of parser rules whose contributed types are used as type in cross-references
+ */
+export function getAllRulesUsedForCrossReferences(grammar: ast.Grammar): Set<ast.ParserRule> {
+    const result = new Set<ast.ParserRule>();
+    streamAllContents(grammar).forEach(node => {
+        if (ast.isCrossReference(node)) {
+            // the cross-reference refers directly to a parser rule (without "returns", without "infers")
+            if (ast.isParserRule(node.type.ref)) {
+                result.add(node.type.ref);
+            }
+            // the cross-reference refers to the explicitly inferred type of a parser rule
+            if (ast.isInferredType(node.type.ref) && ast.isParserRule(node.type.ref.$container)) {
+                result.add(node.type.ref.$container);
+            }
+        }
+    });
+    return result;
+}
+
 /**
  * Determines the grammar expression used to parse a cross-reference (usually a reference to a terminal rule).
  * A cross-reference can declare this expression explicitly in the form `[Type : Terminal]`, but if `Terminal`

packages/langium/test/grammar/grammar-validator.test.ts

@@ -4,14 +4,14 @@
  * terms of the MIT License, which is available in the project root.
  ******************************************************************************/
 
-import type { AstNode, Properties } from 'langium';
-import type { GrammarAST as GrammarTypes } from 'langium';
-import type { ValidationResult } from 'langium/test';
-import { afterEach, beforeAll, describe, expect, test } from 'vitest';
-import { DiagnosticSeverity } from 'vscode-languageserver';
+import type { AstNode, GrammarAST as GrammarTypes, Properties } from 'langium';
 import { AstUtils, EmptyFileSystem, GrammarAST } from 'langium';
 import { IssueCodes, createLangiumGrammarServices } from 'langium/grammar';
+import type { ValidationResult } from 'langium/test';
 import { clearDocuments, expectError, expectIssue, expectNoIssues, expectWarning, parseHelper, validationHelper } from 'langium/test';
+import { afterEach, beforeAll, describe, expect, test } from 'vitest';
+import { DiagnosticSeverity } from 'vscode-languageserver';
+import { beforeAnotherRule, beforeSinglelternative, beforeTwoAlternatives, beforeWithInfers } from './lsp/grammar-code-actions.test.js';
 
 const services = createLangiumGrammarServices(EmptyFileSystem);
 const parse = parseHelper(services.grammar);
@@ -514,6 +514,38 @@ describe('Unused rules validation', () => {
 
 });
 
+describe('Parser rules used only as type in cross-references are not marked as unused, but with a hint suggesting to use a type declaration instead', () => {
+    // The used test data are defined at the test cases for possible code actions for these validation problems.
+    // these test cases target https://github.com/eclipse-langium/langium/issues/1309
+
+    test('union of two types', async () => {
+        await validateRule(beforeTwoAlternatives);
+    });
+
+    test('only a single type', async () => {
+        await validateRule(beforeSinglelternative);
+    });
+
+    test('rule using a nested rule', async () => {
+        await validateRule(beforeAnotherRule, 2); // 2 hints, since there is another "unused" rule (which is out-of-scope here)
+    });
+
+    test('union of two types, with "infers" keyword', async () => {
+        await validateRule(beforeWithInfers);
+    });
+
+    async function validateRule(grammar: string, foundDiagnostics: number = 1) {
+        const validation = await validate(grammar);
+        expect(validation.diagnostics).toHaveLength(foundDiagnostics);
+        const ruleWithHint = validation.document.parseResult.value.rules.find(e => e.name === 'Person')!;
+        expectIssue(validation, {
+            node: ruleWithHint,
+            severity: DiagnosticSeverity.Hint
+        });
+        return ruleWithHint;
+    }
+});
+
 describe('Reserved names', () => {
 
     test('Reserved parser rule name', async () => {