Support multi-target references #1509
Conversation
|
cc @cdietrich since you were interested in this feature. |
msujew
force-pushed
the
msujew/multi-reference
branch
from
5912b62
to
942f802
Compare
There was a problem hiding this comment.
Thanks, looks good, I like especially that one can now ask for an array and do not need to check for undefined when getting references :D.
All in all I added some code improvement suggestions.
I would suggest to wait for more feedback, since I am not so deep in this area of the code, yet.
| const document = await parse(text); | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const model = document.parseResult.value as any; | ||
| const alice2 = model.persons[2]; |
There was a problem hiding this comment.
For future work it would be nice to have a type instead of any.
| includeDeclaration: true | ||
| }).toArray().sort((a, b) => a.segment.offset - b.segment.offset); | ||
| expect(references).toHaveLength(3); | ||
| for (let i = 0; i < references.length; i++) { |
There was a problem hiding this comment.
Minor thing, decide yourself, if you want to take it: For-loops in tests, similar to branching, make it harder to read the code. Because you have to evaluate values in your mind. But I also see that this test is quite simple. I see two options:
- with a
references.forEach((reference, index) => ...)you remove the need to think aboutreferences[index]. And the second parameter is theindexwhich you can reuse for the line computation. - Roll out the for loop to only have expect().toBe.... lines.
| expect(model.persons).toHaveLength(3); | ||
| expect(model.greetings).toHaveLength(1); | ||
| const greeting = model.greetings[0]; | ||
| const ref = greeting.person as MultiReference; |
There was a problem hiding this comment.
Minor: Unfortunately we cannot do a expect(isMultiReference(greeting.person)).toBeTruthy()... T_T
You can add an assertion. It is not so important to me, I thought it would be great to have an error in the opposite case.
| grammar test | ||
| entry Model: (persons+=Person | greetings+=Greeting)*; | ||
| Person: 'person' name=ID; | ||
| Greeting: 'hello' person=[*Person:ID]; |
There was a problem hiding this comment.
Thinking loud: * stands normally for "zero or more". Is it possible here to have zero references? Maybe + will lead to a better association... on the other hand it is just syntax. Maybe I get some ideas later in this review...
| if (description) { | ||
| descr.push(description); | ||
| streamReferences(astNode).forEach(refInfo => { | ||
| if (!refInfo.reference.error) { |
There was a problem hiding this comment.
From the diff: I would prefer a !isLinkingError(refInfo). Looks more official.
| } else if (isMultiReference(reference)) { | ||
| return reference.items.map(item => item.ref); | ||
| } | ||
| return []; |
There was a problem hiding this comment.
Can this line ever be reached? What do you think about assertUnreachable?
| if (sourceCstNode) { | ||
| const assignment = findAssignment(sourceCstNode); | ||
| const nodeElem = sourceCstNode.astNode; | ||
| if (assignment && nodeElem) { | ||
| const reference = (nodeElem as GenericAstNode)[assignment.feature]; | ||
|
|
||
| if (isReference(reference)) { | ||
| return reference.ref; | ||
| if (isReference(reference) || isMultiReference(reference)) { |
There was a problem hiding this comment.
Minor: I saw this double call several times now. Would it make sense to introduce something that addresses both?
There was a problem hiding this comment.
Or breaking the API by introducing isReference (for both; or isAnyReference), isSingleReference and isMultiReference
Adds support for multi-target references. These are references that have the possibility to target multiple elements at once. See following illustration for an example:
Note that by default, only elements that are defined on the same scope can be targeted. I.e. a multi reference as in the following will only target one element:
This behavior can be freely overwritten by adopters of Langium. However, due to the inherent complexity of aligning the
Referencesservice implementation and the scoping wrt their behavior, this feature is intended for proficient Langium developers.The intended use cases for this feature are stuff like declaration merging in TypeScript or partial classes in C#.
Contains a bunch of breaking changes:
Referencesinterface now returns arrays instead ofT | undefined.Referencein a generic manner (linker mostly) now also works withMultiReference.LinkingErrorno longer inheritsReferenceInfo(which didn't make a lot of sense in the first place), but contains ainfoproperty.