Conversation
Lotes
left a comment
Lotes
left a comment
There was a problem hiding this comment.
I like the type specific bundle. It gives the opportunity to carry along language-specific types, similar to the dependency injection, which bundles values of services.
I also like those lookup functions, so that you do not have to break apart the specifics on your own.
Most stuff is a replacement for the old types, but I found one little thing, where I wonder if we can make this better.
insafuhrmann
left a comment
insafuhrmann
left a comment
There was a problem hiding this comment.
Thanks @JohannesMeierSE, I have only some detail remarks as you can see in the comments. Please feel free to ping me for an approving re-review when conflicts are resolved and the PR is updated.
# Conflicts: # packages/typir/src/utils/rule-registration.ts
…ctions and operators
…ting definitions even more
…anguageNode`, wrote documentation about validation
…loped together with Markus and Insa)
JohannesMeierSE
force-pushed
the
jm/more-type-safety
branch
from
de601c7 to
a5e7ea1
Compare
|
@insafuhrmann @Lotes Thanks for your reviews! I just pushed updates, added the jointly discussed calculation of language key unions and rebased the branch on |
|
What do you think about supporting |
insafuhrmann
left a comment
insafuhrmann
left a comment
There was a problem hiding this comment.
Thanks for the updates @JohannesMeierSE! I think they are very nice and I also tried to answer your question regarding the possible extension in a comment in the code.
Regarding supporting the features addValidationRulesForAstNodes and addInferenceRulesForAstNodes also in core: I am in favour of this. Let's do it.
…zable (result of a long coding session with Insa)
There was a problem hiding this comment.
Thanks for the additions @JohannesMeierSE. Let's discuss the intersection problem
| * Contains properties of language nodes, which shall be omitted for validation issues, | ||
| * i.e. these properties are not possible to attach validation markers to. | ||
| * | ||
| * The types given here are usable as (object) keys in general and therefore enable concrete, inheriting `TypirSpecifics` to specify more concrete keys. |
There was a problem hiding this comment.
concrete ...? Is there a word missing?
concrete specifications?
| [K in LanguageKey<Specifics>]?: Specifics['LanguageKeys'][K] extends Specifics['LanguageType'] ? ValidationRule<Specifics, Specifics['LanguageKeys'][K]> | Array<ValidationRule<Specifics, Specifics['LanguageKeys'][K]>> : never | ||
| } & { | ||
| export type LangiumValidationRules<Specifics extends TypirLangiumSpecifics> = ValidationRulesForLanguageKeys<Specifics> & { | ||
| // TODO nodes inside ValidationRules are typed by the TypeScript compiler as `any` not as `AstNode` |
There was a problem hiding this comment.
Should we maybe put this on the agenda for the upcoming meeting to discuss possible solutions?
3 participants
This PR increases the TypeScript type-safety and can be seen as a follow-up of #90
TypirSpecificsnow contain the propertyLanguageKeys: Record<string, unknown>;to make language keys explicit. This is the precondition for the following improvements:Specifics['LanguageKeys']to simplify inference rules for functions and operators: Now it is possible to skip the TypeScript type when using onlylanguageKeyand nofilter. @Lotes You sketched this idea some time ago and now we have everything to make your idea happen 🙂Specifics['LanguageKeys']now even in Typir (core), we support the featuresaddValidationRulesForAstNodesandaddInferenceRulesForAstNodes(which are Typir-Langium only so far) also in Typir (core), but slightly renamed ("Language" instead of "Ast").Contributions of this PR which are not related to language keys:
languagePropertysupports only valid property names of the givenlanguageNodeinside validations. These properties could be reduced by the newTypirSpecifics['OmittedLanguageNodeProperties'].watchscriptOpen issue:
AstNodein Typir-Langium as key for registering validation/inference rules, the TypeScript compiler infersanyinstead ofAstNodefor the current language node.I recommend to review this PR commit by commit.