Skip to content

Commit 88f176c

Browse files
msujewmsujew
authored
Fix references in fragment rules (#1762)
1 parent 4e96814 commit 88f176c

File tree

9 files changed

+92
-25
lines changed

9 files changed

+92
-25
lines changed

‎examples/arithmetics/src/language-server/generated/grammar.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ export const ArithmeticsGrammar = (): Grammar => loadedArithmeticsGrammar ?? (lo
1414
"rules": [
1515
{
1616
"$type": "ParserRule",
17-
"name": "Module",
1817
"entry": true,
18+
"name": "Module",
1919
"definition": {
2020
"$type": "Group",
2121
"elements": [

‎examples/domainmodel/src/language-server/generated/grammar.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ export const DomainModelGrammar = (): Grammar => loadedDomainModelGrammar ?? (lo
1414
"rules": [
1515
{
1616
"$type": "ParserRule",
17-
"name": "Domainmodel",
1817
"entry": true,
18+
"name": "Domainmodel",
1919
"definition": {
2020
"$type": "Assignment",
2121
"feature": "elements",

‎examples/requirements/src/language-server/generated/grammar.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ export const RequirementsGrammar = (): Grammar => loadedRequirementsGrammar ?? (
1515
"rules": [
1616
{
1717
"$type": "ParserRule",
18-
"name": "RequirementModel",
1918
"entry": true,
19+
"name": "RequirementModel",
2020
"definition": {
2121
"$type": "Group",
2222
"elements": [
@@ -321,8 +321,8 @@ export const TestsGrammar = (): Grammar => loadedTestsGrammar ?? (loadedTestsGra
321321
"rules": [
322322
{
323323
"$type": "ParserRule",
324-
"name": "TestModel",
325324
"entry": true,
325+
"name": "TestModel",
326326
"definition": {
327327
"$type": "Group",
328328
"elements": [
@@ -519,8 +519,8 @@ export const TestsGrammar = (): Grammar => loadedTestsGrammar ?? (loadedTestsGra
519519
},
520520
{
521521
"$type": "ParserRule",
522-
"name": "RequirementModel",
523522
"entry": false,
523+
"name": "RequirementModel",
524524
"definition": {
525525
"$type": "Group",
526526
"elements": [

‎examples/statemachine/src/language-server/generated/grammar.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ export const StatemachineGrammar = (): Grammar => loadedStatemachineGrammar ?? (
1414
"rules": [
1515
{
1616
"$type": "ParserRule",
17-
"name": "Statemachine",
1817
"entry": true,
18+
"name": "Statemachine",
1919
"definition": {
2020
"$type": "Group",
2121
"elements": [

‎packages/langium/src/grammar/generated/grammar.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,8 @@ export const LangiumGrammarGrammar = (): Grammar => loadedLangiumGrammarGrammar
1414
"rules": [
1515
{
1616
"$type": "ParserRule",
17-
"name": "Grammar",
1817
"entry": true,
18+
"name": "Grammar",
1919
"definition": {
2020
"$type": "Group",
2121
"elements": [
@@ -1348,8 +1348,8 @@ export const LangiumGrammarGrammar = (): Grammar => loadedLangiumGrammarGrammar
13481348
},
13491349
{
13501350
"$type": "ParserRule",
1351-
"name": "RuleNameAndParams",
13521351
"fragment": true,
1352+
"name": "RuleNameAndParams",
13531353
"definition": {
13541354
"$type": "Group",
13551355
"elements": [

‎packages/langium/src/parser/cst-node-builder.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ export class CstNodeBuilder {
1818
private nodeStack: CompositeCstNodeImpl[] = [];
1919

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

2424
buildRootNode(input: string): RootCstNode {

‎packages/langium/src/parser/langium-parser.ts‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ export interface BaseParser {
9494
* Requires a unique index within the rule for a specific sub rule.
9595
* Arguments can be supplied to the rule invocation for semantic predicates
9696
*/
97-
subrule(idx: number, rule: RuleResult, feature: AbstractElement, args: Args): void;
97+
subrule(idx: number, rule: RuleResult, fragment: boolean, feature: AbstractElement, args: Args): void;
9898
/**
9999
* Executes a grammar action that modifies the currently active AST node
100100
*/
@@ -161,7 +161,7 @@ export abstract class AbstractLangiumParser implements BaseParser {
161161

162162
abstract rule(rule: ParserRule, impl: RuleImpl): RuleResult;
163163
abstract consume(idx: number, tokenType: TokenType, feature: AbstractElement): void;
164-
abstract subrule(idx: number, rule: RuleResult, feature: AbstractElement, args: Args): void;
164+
abstract subrule(idx: number, rule: RuleResult, fragment: boolean, feature: AbstractElement, args: Args): void;
165165
abstract action($type: string, action: Action): void;
166166
abstract construct(): unknown;
167167

@@ -251,7 +251,9 @@ export class LangiumParser extends AbstractLangiumParser {
251251

252252
private startImplementation($type: string | symbol | undefined, implementation: RuleImpl): RuleImpl {
253253
return (args) => {
254-
if (!this.isRecording()) {
254+
// Only create a new AST node in case the calling rule is not a fragment rule
255+
const createNode = !this.isRecording() && $type !== undefined;
256+
if (createNode) {
255257
const node: any = { $type };
256258
this.stack.push(node);
257259
if ($type === DatatypeSymbol) {
@@ -264,7 +266,7 @@ export class LangiumParser extends AbstractLangiumParser {
264266
} catch (err) {
265267
result = undefined;
266268
}
267-
if (!this.isRecording() && result === undefined) {
269+
if (result === undefined && createNode) {
268270
result = this.construct();
269271
}
270272
return result;
@@ -300,9 +302,13 @@ export class LangiumParser extends AbstractLangiumParser {
300302
return !token.isInsertedInRecovery && !isNaN(token.startOffset) && typeof token.endOffset === 'number' && !isNaN(token.endOffset);
301303
}
302304

303-
subrule(idx: number, rule: RuleResult, feature: AbstractElement, args: Args): void {
305+
subrule(idx: number, rule: RuleResult, fragment: boolean, feature: AbstractElement, args: Args): void {
304306
let cstNode: CompositeCstNode | undefined;
305-
if (!this.isRecording()) {
307+
if (!this.isRecording() && !fragment) {
308+
// We only want to create a new CST node if the subrule actually creates a new AST node.
309+
// In other cases like calls of fragment rules the current CST/AST is populated further.
310+
// Note that skipping this initialization and leaving cstNode unassigned also skips the subrule assignment later on.
311+
// This is intended, as fragment rules only enrich the current AST node
306312
cstNode = this.nodeBuilder.buildCompositeNode(feature);
307313
}
308314
const subruleResult = this.wrapper.wrapSubrule(idx, rule, args) as any;
@@ -325,11 +331,7 @@ export class LangiumParser extends AbstractLangiumParser {
325331
if (isDataTypeNode(current)) {
326332
current.value += result.toString();
327333
} else if (typeof result === 'object' && result) {
328-
const resultKind = result.$type;
329334
const object = this.assignWithoutOverride(result, current);
330-
if (resultKind) {
331-
object.$type = resultKind;
332-
}
333335
const newItem = object;
334336
this.stack.pop();
335337
this.stack.push(newItem);
@@ -592,7 +594,7 @@ export class LangiumCompletionParser extends AbstractLangiumParser {
592594
}
593595
}
594596

595-
subrule(idx: number, rule: RuleResult, feature: AbstractElement, args: Args): void {
597+
subrule(idx: number, rule: RuleResult, fragment: boolean, feature: AbstractElement, args: Args): void {
596598
this.before(feature);
597599
this.wrapper.wrapSubrule(idx, rule, args);
598600
this.after(feature);

‎packages/langium/src/parser/parser-builder-base.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,14 +99,15 @@ function buildRuleCall(ctx: RuleContext, ruleCall: RuleCall): Method {
9999
const rule = ruleCall.rule.ref;
100100
if (isParserRule(rule)) {
101101
const idx = ctx.subrule++;
102+
const fragment = rule.fragment;
102103
const predicate = ruleCall.arguments.length > 0 ? buildRuleCallPredicate(rule, ruleCall.arguments) : () => ({});
103-
return (args) => ctx.parser.subrule(idx, getRule(ctx, rule), ruleCall, predicate(args));
104+
return (args) => ctx.parser.subrule(idx, getRule(ctx, rule), fragment, ruleCall, predicate(args));
104105
} else if (isTerminalRule(rule)) {
105106
const idx = ctx.consume++;
106107
const method = getToken(ctx, rule.name);
107108
return () => ctx.parser.consume(idx, method, ruleCall);
108109
} else if (!rule) {
109-
throw new ErrorWithLocation(ruleCall.$cstNode, `Undefined rule type: ${ruleCall.$type}`);
110+
throw new ErrorWithLocation(ruleCall.$cstNode, `Undefined rule: ${ruleCall.rule.$refText}`);
110111
} else {
111112
assertUnreachable(rule);
112113
}
@@ -273,8 +274,10 @@ function buildCrossReference(ctx: RuleContext, crossRef: CrossReference, termina
273274
}
274275
return buildCrossReference(ctx, crossRef, assignTerminal);
275276
} else if (isRuleCall(terminal) && isParserRule(terminal.rule.ref)) {
277+
// The terminal is a data type rule here. Everything else will result in a validation error.
278+
const rule = terminal.rule.ref;
276279
const idx = ctx.subrule++;
277-
return (args) => ctx.parser.subrule(idx, getRule(ctx, terminal.rule.ref as ParserRule), crossRef, args);
280+
return (args) => ctx.parser.subrule(idx, getRule(ctx, rule), false, crossRef, args);
278281
} else if (isRuleCall(terminal) && isTerminalRule(terminal.rule.ref)) {
279282
const idx = ctx.consume++;
280283
const terminalRule = getToken(ctx, terminal.rule.ref.name);

‎packages/langium/test/parser/langium-parser-builder.test.ts‎

Lines changed: 64 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,11 @@
44
* terms of the MIT License, which is available in the project root.
55
******************************************************************************/
66

7+
/* eslint-disable @typescript-eslint/no-explicit-any */
8+
79
import type { TokenType, TokenVocabulary } from 'chevrotain';
8-
import type { AstNode, CstNode, GenericAstNode, Grammar, GrammarAST, LangiumParser, ParseResult, TokenBuilderOptions } from 'langium';
9-
import { EmptyFileSystem, DefaultTokenBuilder, GrammarUtils, CstUtils } from 'langium';
10+
import type { AstNode, CstNode, GenericAstNode, Grammar, GrammarAST, LangiumParser, ParseResult, ReferenceInfo, Scope, TokenBuilderOptions } from 'langium';
11+
import { EmptyFileSystem, DefaultTokenBuilder, GrammarUtils, CstUtils, DefaultScopeProvider, URI } from 'langium';
1012
import { describe, expect, test, onTestFailed, beforeAll } from 'vitest';
1113
import { createLangiumGrammarServices, createServicesForGrammar } from 'langium/grammar';
1214
import { expandToString } from 'langium/generate';
@@ -706,6 +708,66 @@ describe('Fragment rules', () => {
706708
expect(result.value).toHaveProperty('values', ['ab', 'cd', 'ef']);
707709
});
708710

711+
for (const [name, content] of [
712+
['Array in fragment', `
713+
Start:
714+
element=[Def]
715+
ArrayCallSignature?;
716+
fragment ArrayCallSignature:
717+
(isArray?='[' ']');`],
718+
['Reference in fragment', `
719+
Start:
720+
ElementRef
721+
(isArray?='[' ']')?;
722+
fragment ElementRef:
723+
element=[Def];`]
724+
]) {
725+
test(`Fragment rules don't create AST elements during parsing - ${name}`, async () => {
726+
// This test is mostly based on a bug report in a GitHub discussion:
727+
// https://github.com/eclipse-langium/langium/discussions/1638
728+
// In particular, the issue was that fragment rules used to create AST elements with type "undefined"
729+
// When passing those into references, the reference would fail to resolve because the created AST element was just a placeholder
730+
// Eventually, the parser would assign the properties of the fragment rule to the actual AST element
731+
// But the reference would still use the old reference
732+
// The fixed implementation no longer creates these fake AST elements, thereby skipping any potential issues completely.
733+
let resolved = false;
734+
const services = await createServicesForGrammar({
735+
grammar: `
736+
grammar FragmentRuleOverride
737+
entry Entry: items+=(MemberCall|Def)*;
738+
Def: 'def' name=ID;
739+
MemberCall:
740+
Start ({infer MemberCall.previous=current} "." element=[Def])*;
741+
742+
${content}
743+
744+
terminal ID: /[_a-zA-Z][\\w_]*/;
745+
hidden terminal WS: /\\s+/;
746+
`, module: {
747+
references: {
748+
ScopeProvider: (services: any) => new class extends DefaultScopeProvider {
749+
override getScope(context: ReferenceInfo): Scope {
750+
const item = context.container as any;
751+
const previous = item.previous;
752+
if (previous) {
753+
const previousElement = previous.element.ref;
754+
// Ensure that the reference can be resolved
755+
resolved = Boolean(previousElement);
756+
}
757+
return super.getScope(context);
758+
}
759+
}(services)
760+
}
761+
}
762+
});
763+
const document = services.shared.workspace.LangiumDocumentFactory.fromString('def a def b a[].b', URI.parse('file:///test'));
764+
await services.shared.workspace.DocumentBuilder.build([document]);
765+
expect(document.parseResult.lexerErrors).toHaveLength(0);
766+
expect(document.parseResult.parserErrors).toHaveLength(0);
767+
expect(resolved).toBe(true);
768+
});
769+
}
770+
709771
});
710772

711773
describe('Unicode terminal rules', () => {

0 commit comments

Comments
 (0)