add implicit ID rule call for cross refs to getAllReachableRules #1152
Conversation
There was a problem hiding this comment.
I'm not sure this is correct. Langium never uses magic names for convention reasons (only exception is the name property). Instead Langium uses the terminal associated with the name property for cross references. See also this method.
cdietrich
force-pushed
the
cd_issue1151
branch
from
02b3bb9
to
8068189
Compare
|
|
||
| test('ID implicit called should be returned by getAllReachableRules', async () => { | ||
| // [A] is short for [A:ID] thus the ID rule is needed by the parser and getAllReachableRules should return ID | ||
| const grammar1 = await parse(` |
There was a problem hiding this comment.
is this proper setup to have 2 grammars reference each other?
cdietrich
force-pushed
the
cd_issue1151
branch
3 times, most recently
from
d837ea9
to
55f68cc
Compare
| for (const rule of grammar.rules) { | ||
| if (ruleNames.has(rule.name) || (ast.isTerminalRule(rule) && rule.hidden)) { | ||
| if ((ast.isTerminalRule(rule) && rule.hidden)) { |
There was a problem hiding this comment.
do i miss something now?
| rules.add(rule); | ||
| } | ||
| } | ||
| return rules; | ||
| } | ||
|
|
||
| function ruleDfs(rule: ast.AbstractRule, visitedSet: Set<string>, allTerminals: boolean): void { | ||
| visitedSet.add(rule.name); | ||
| function ruleDfs(rule: ast.AbstractRule, visitedRuleNames: Set<string>, visitedRules: Set<ast.AbstractRule> , allTerminals: boolean): void { |
There was a problem hiding this comment.
is there a more elegant way? do we need to make breadth first search / two loops
to catch overriden rules?
or just collect all rule names in a first run and the actual rules in a second?
There was a problem hiding this comment.
Langium prevents adopters from overriding imported rules.
| ruleDfs(refRule, visitedRuleNames, visitedRules, allTerminals); | ||
| } | ||
| } else if (ast.isCrossReference(node)) { | ||
| const term = getCrossReferenceTerminal(node) |
There was a problem hiding this comment.
what is the override semantic here: rule used at the name place is overriden/also defined in current grmmar
There was a problem hiding this comment.
Langium prevents adopters from overriding imported rules.
There was a problem hiding this comment.
ok, need to try that out
| function ruleDfs(rule: ast.AbstractRule, visitedSet: Set<string>, allTerminals: boolean): void { | ||
| visitedSet.add(rule.name); | ||
| function ruleDfs(rule: ast.AbstractRule, visitedRuleNames: Set<string>, visitedRules: Set<ast.AbstractRule> , allTerminals: boolean): void { | ||
| visitedRuleNames.add(rule.name); |
There was a problem hiding this comment.
Given that we don't allow rule overrides, I don't think we need this set?
cdietrich
force-pushed
the
cd_issue1151
branch
from
55f68cc
to
85a1851
Compare
|
is the build fail related to my changes? |
|
@cdietrich Yes, the errors are related to your changes. The order of terminals is very important for the Chevrotain lexer, which is why it needs to be preserved. |
|
what do i need to execute to replicate the github action build locally? |
|
You just need to run
Yes, local rules have priority over imported rules. They should be at the end. |
cdietrich
force-pushed
the
cd_issue1151
branch
2 times, most recently
from
bbd5d81
to
71ced3b
Compare
| // act | ||
| const reachableRules = [...getAllReachableRules(grammar2.parseResult.value, true)].map(r => r.name); | ||
| // assert | ||
| expect(reachableRules).toEqual(['B', 'STRING', 'ID', 'Called', 'INT' ]) |
There was a problem hiding this comment.
is this sufficient as testcase or do we need deeply nested calls too?
Fixes eclipse-langium#1151 Signed-off-by: Christian Dietrich <christian.dietrich.opensource@gmail.com>
cdietrich
force-pushed
the
cd_issue1151
branch
from
71ced3b
to
6f07706
Compare
add implicit ID rule call for cross refs to getAllReachableRules
Fixes #1151