Skip to content

Commit

Permalink
Fix references in fragment rules
Browse files Browse the repository at this point in the history
  • Loading branch information
msujew committed Dec 2, 2024
1 parent 3170af9 commit 18662c3
Show file tree
Hide file tree
Showing 9 changed files with 78 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ export const ArithmeticsGrammar = (): Grammar => loadedArithmeticsGrammar ?? (lo
"rules": [
{
"$type": "ParserRule",
"name": "Module",
"entry": true,
"name": "Module",
"definition": {
"$type": "Group",
"elements": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ export const DomainModelGrammar = (): Grammar => loadedDomainModelGrammar ?? (lo
"rules": [
{
"$type": "ParserRule",
"name": "Domainmodel",
"entry": true,
"name": "Domainmodel",
"definition": {
"$type": "Assignment",
"feature": "elements",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ export const RequirementsGrammar = (): Grammar => loadedRequirementsGrammar ?? (
"rules": [
{
"$type": "ParserRule",
"name": "RequirementModel",
"entry": true,
"name": "RequirementModel",
"definition": {
"$type": "Group",
"elements": [
Expand Down Expand Up @@ -321,8 +321,8 @@ export const TestsGrammar = (): Grammar => loadedTestsGrammar ?? (loadedTestsGra
"rules": [
{
"$type": "ParserRule",
"name": "TestModel",
"entry": true,
"name": "TestModel",
"definition": {
"$type": "Group",
"elements": [
Expand Down Expand Up @@ -519,8 +519,8 @@ export const TestsGrammar = (): Grammar => loadedTestsGrammar ?? (loadedTestsGra
},
{
"$type": "ParserRule",
"name": "RequirementModel",
"entry": false,
"name": "RequirementModel",
"definition": {
"$type": "Group",
"elements": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ export const StatemachineGrammar = (): Grammar => loadedStatemachineGrammar ?? (
"rules": [
{
"$type": "ParserRule",
"name": "Statemachine",
"entry": true,
"name": "Statemachine",
"definition": {
"$type": "Group",
"elements": [
Expand Down
4 changes: 2 additions & 2 deletions packages/langium/src/grammar/generated/grammar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ export const LangiumGrammarGrammar = (): Grammar => loadedLangiumGrammarGrammar
"rules": [
{
"$type": "ParserRule",
"name": "Grammar",
"entry": true,
"name": "Grammar",
"definition": {
"$type": "Group",
"elements": [
Expand Down Expand Up @@ -1348,8 +1348,8 @@ export const LangiumGrammarGrammar = (): Grammar => loadedLangiumGrammarGrammar
},
{
"$type": "ParserRule",
"name": "RuleNameAndParams",
"fragment": true,
"name": "RuleNameAndParams",
"definition": {
"$type": "Group",
"elements": [
Expand Down
2 changes: 1 addition & 1 deletion packages/langium/src/parser/cst-node-builder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export class CstNodeBuilder {
private nodeStack: CompositeCstNodeImpl[] = [];

private get current(): CompositeCstNodeImpl {
return this.nodeStack[this.nodeStack.length - 1];
return this.nodeStack[this.nodeStack.length - 1] ?? this.rootNode;
}

buildRootNode(input: string): RootCstNode {
Expand Down
22 changes: 11 additions & 11 deletions packages/langium/src/parser/langium-parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ export interface BaseParser {
* Requires a unique index within the rule for a specific sub rule.
* Arguments can be supplied to the rule invocation for semantic predicates
*/
subrule(idx: number, rule: RuleResult, feature: AbstractElement, args: Args): void;
subrule(idx: number, rule: RuleResult, fragment: boolean, feature: AbstractElement, args: Args): void;
/**
* Executes a grammar action that modifies the currently active AST node
*/
Expand Down Expand Up @@ -161,7 +161,7 @@ export abstract class AbstractLangiumParser implements BaseParser {

abstract rule(rule: ParserRule, impl: RuleImpl): RuleResult;
abstract consume(idx: number, tokenType: TokenType, feature: AbstractElement): void;
abstract subrule(idx: number, rule: RuleResult, feature: AbstractElement, args: Args): void;
abstract subrule(idx: number, rule: RuleResult, fragment: boolean, feature: AbstractElement, args: Args): void;
abstract action($type: string, action: Action): void;
abstract construct(): unknown;

Expand Down Expand Up @@ -251,7 +251,9 @@ export class LangiumParser extends AbstractLangiumParser {

private startImplementation($type: string | symbol | undefined, implementation: RuleImpl): RuleImpl {
return (args) => {
if (!this.isRecording()) {
// Only create a new AST node in case the calling rule is not a fragment rule
const createNode = $type !== undefined;
if (!this.isRecording() && createNode) {
const node: any = { $type };
this.stack.push(node);
if ($type === DatatypeSymbol) {
Expand All @@ -264,7 +266,7 @@ export class LangiumParser extends AbstractLangiumParser {
} catch (err) {
result = undefined;
}
if (!this.isRecording() && result === undefined) {
if (!this.isRecording() && result === undefined && createNode) {
result = this.construct();
}
return result;
Expand Down Expand Up @@ -300,9 +302,11 @@ export class LangiumParser extends AbstractLangiumParser {
return !token.isInsertedInRecovery && !isNaN(token.startOffset) && typeof token.endOffset === 'number' && !isNaN(token.endOffset);
}

subrule(idx: number, rule: RuleResult, feature: AbstractElement, args: Args): void {
subrule(idx: number, rule: RuleResult, fragment: boolean, feature: AbstractElement, args: Args): void {
let cstNode: CompositeCstNode | undefined;
if (!this.isRecording()) {
if (!this.isRecording() && !fragment) {
// If the called rule is a fragment rule, we just add all parsed CST nodes to the already existing CST node
// Note that this also skips the subrule assignment later on. This is intended, as fragment rules only enrich the current AST node
cstNode = this.nodeBuilder.buildCompositeNode(feature);
}
const subruleResult = this.wrapper.wrapSubrule(idx, rule, args) as any;
Expand All @@ -325,11 +329,7 @@ export class LangiumParser extends AbstractLangiumParser {
if (isDataTypeNode(current)) {
current.value += result.toString();
} else if (typeof result === 'object' && result) {
const resultKind = result.$type;
const object = this.assignWithoutOverride(result, current);
if (resultKind) {
object.$type = resultKind;
}
const newItem = object;
this.stack.pop();
this.stack.push(newItem);
Expand Down Expand Up @@ -592,7 +592,7 @@ export class LangiumCompletionParser extends AbstractLangiumParser {
}
}

subrule(idx: number, rule: RuleResult, feature: AbstractElement, args: Args): void {
subrule(idx: number, rule: RuleResult, fragment: boolean, feature: AbstractElement, args: Args): void {
this.before(feature);
this.wrapper.wrapSubrule(idx, rule, args);
this.after(feature);
Expand Down
6 changes: 4 additions & 2 deletions packages/langium/src/parser/parser-builder-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,9 @@ function buildRuleCall(ctx: RuleContext, ruleCall: RuleCall): Method {
const rule = ruleCall.rule.ref;
if (isParserRule(rule)) {
const idx = ctx.subrule++;
const fragment = rule.fragment;
const predicate = ruleCall.arguments.length > 0 ? buildRuleCallPredicate(rule, ruleCall.arguments) : () => ({});
return (args) => ctx.parser.subrule(idx, getRule(ctx, rule), ruleCall, predicate(args));
return (args) => ctx.parser.subrule(idx, getRule(ctx, rule), fragment, ruleCall, predicate(args));
} else if (isTerminalRule(rule)) {
const idx = ctx.consume++;
const method = getToken(ctx, rule.name);
Expand Down Expand Up @@ -273,8 +274,9 @@ function buildCrossReference(ctx: RuleContext, crossRef: CrossReference, termina
}
return buildCrossReference(ctx, crossRef, assignTerminal);
} else if (isRuleCall(terminal) && isParserRule(terminal.rule.ref)) {
const rule = terminal.rule.ref;
const idx = ctx.subrule++;
return (args) => ctx.parser.subrule(idx, getRule(ctx, terminal.rule.ref as ParserRule), crossRef, args);
return (args) => ctx.parser.subrule(idx, getRule(ctx, rule), false, crossRef, args);
} else if (isRuleCall(terminal) && isTerminalRule(terminal.rule.ref)) {
const idx = ctx.consume++;
const terminalRule = getToken(ctx, terminal.rule.ref.name);
Expand Down
56 changes: 54 additions & 2 deletions packages/langium/test/parser/langium-parser-builder.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@
* terms of the MIT License, which is available in the project root.
******************************************************************************/

/* eslint-disable @typescript-eslint/no-explicit-any */

import type { TokenType, TokenVocabulary } from 'chevrotain';
import type { AstNode, CstNode, GenericAstNode, Grammar, GrammarAST, LangiumParser, ParseResult, TokenBuilderOptions } from 'langium';
import { EmptyFileSystem, DefaultTokenBuilder, GrammarUtils, CstUtils } from 'langium';
import type { AstNode, CstNode, GenericAstNode, Grammar, GrammarAST, LangiumParser, ParseResult, ReferenceInfo, Scope, TokenBuilderOptions } from 'langium';
import { EmptyFileSystem, DefaultTokenBuilder, GrammarUtils, CstUtils, DefaultScopeProvider, URI } from 'langium';
import { describe, expect, test, onTestFailed, beforeAll } from 'vitest';
import { createLangiumGrammarServices, createServicesForGrammar } from 'langium/grammar';
import { expandToString } from 'langium/generate';
Expand Down Expand Up @@ -706,6 +708,56 @@ describe('Fragment rules', () => {
expect(result.value).toHaveProperty('values', ['ab', 'cd', 'ef']);
});

test("Fragment rules don't create AST elements during parsing", async () => {
// This test is mostly based on a bug report in a GitHub discussion:
// https://github.com/eclipse-langium/langium/discussions/1638
// In particular, the issue was that fragment rules used to create AST elements with type "undefined"
// When passing those into references, the reference would fail to resolve because the created AST element was just a placeholder
// Eventually, the parser would assign the properties of the fragment rule to the actual AST element
// But the reference would still use the old reference
// The fixed implementation no longer creates these fake AST elements, thereby skipping any potential issues completely.
let resolved = false;
const services = await createServicesForGrammar({
grammar: `
grammar FragmentRuleOverride
entry Entry: items+=(MemberCall|Def)*;
Def: 'def' name=ID;
MemberCall:
Start ({infer MemberCall.previous=current} "." element=[Def]
ArrayCallSignature? )*;
Start:
element=[Def]
ArrayCallSignature?;
fragment ArrayCallSignature:
(isArray?='[' ']');
terminal ID: /[_a-zA-Z][\\w_]*/;
hidden terminal WS: /\\s+/;
`, module: {
references: {
ScopeProvider: (services: any) => new class extends DefaultScopeProvider {
override getScope(context: ReferenceInfo): Scope {
const item = context.container as any;
const previous = item.previous;
if (previous) {
const previousElement = previous.element.ref;
// Ensure that the reference can be resolved
resolved = Boolean(previousElement);
}
return super.getScope(context);
}
}(services)
}
}
});
const document = services.shared.workspace.LangiumDocumentFactory.fromString('def a def b a[].b', URI.parse('file:///test'));
await services.shared.workspace.DocumentBuilder.build([document]);
expect(document.parseResult.lexerErrors).toHaveLength(0);
expect(document.parseResult.parserErrors).toHaveLength(0);
expect(resolved).toBe(true);
});
});

describe('Unicode terminal rules', () => {
Expand Down

0 comments on commit 18662c3

Please sign in to comment.