-
Notifications
You must be signed in to change notification settings - Fork 91
Support multi-target references #1509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
cc @cdietrich since you were interested in this feature. |
5912b62 to
942f802
Compare
Lotes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| if (description) { | ||
| descr.push(description); | ||
| streamReferences(astNode).forEach(refInfo => { | ||
| if (!refInfo.reference.error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this line ever be reached? What do you think about assertUnreachable?
|
|
||
| if (isReference(reference)) { | ||
| return reference.ref; | ||
| if (isReference(reference) || isMultiReference(reference)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or breaking the API by introducing isReference (for both; or isAnyReference), isSingleReference and isMultiReference
spoenemann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be possible to realize this in a way that most adopters of Langium won't notice a lot of changes. The actual core changes are not too large, and are mostly adding, but not changing API (we can try to add instead of change in a few more places). I'll have a more detailed look at this within the next two months.
844da20 to
489e234
Compare
spoenemann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a missing grammar validation: reference type (single or multi) matches declaration (or is consistent with other inferred occurrences).
489e234 to
bdcf9b2
Compare
spoenemann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This validation should still be added: #1509 (review)
spoenemann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now inconsistent usage of references are detected, but not the consistency of assignment vs. declaration:
interface Entity {
name: string
superType: @Entity
features: Feature[]
}
Entity returns Entity:
'entity' name=ID ('extends' superType=[+Entity:QualifiedName])? '{'
(features+=Feature)*
'}';
This should error because the declared property is a single-reference, but the assigned property is a multi-reference.
| _nodeDescription?: AstNodeDescription; | ||
| } | ||
|
|
||
| export interface DefaultMultiReference extends MultiReference { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msujew what was the reasoning behind exporting this one but not the one above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No particular reason, I thought it makes sense, and forgot that the other one isn't exported yet. Would you like to have both exported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we are looking into writing linking traps and currently (3.5) we need to c&p everything
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.