Cyclic type definitions #33
Conversation
…#22, refactorings, improved identifiers (WIP)
…more little refactorings
…ic for types, simplified TypeReference, reworked listeners, improved test cases, more comments
There was a problem hiding this comment.
Wow, @JohannesMeierSE, this is really a heavy PR. Thanks for all the work you invested! I think it is pretty hard to grasp everything in just one pass. I left a number of detail remarks from my pass, though. I like the overall architecture, my only worry is about performance with the repeated checks of the waiting crowd, but as you say, the number of types is not related to the length of files. We should still keep an eye, I think.
| } | ||
| class B { } | ||
| `, [ // Typir works with this, but for LOX these validation errors are produced: | ||
| 'Declared classes need to be unique (A).', |
There was a problem hiding this comment.
I like that you thought of this case and the possibility of duplicates happening during the wait. I wonder about a validation like this and its relationship to the type initialiser that ensures a type is registered only once. Do we simply have both? In the sense of: Should a language normally have this validation, but whether it has or not, we make sure in the background that there is no double entry?
There was a problem hiding this comment.
Should a language normally have this validation, but whether it has or not, we make sure in the background that there is no double entry?
Yes, exactly!
Typir ensures, that types are existing only once in the type graph.
The (predefined) validations check, that there are not multiple AstNodes which declare the same Typir type.
| `, []); | ||
| expectTypirTypes(loxServices, isClassType, 'A', 'B'); | ||
| expectTypirTypes(loxServices, isFunctionType, 'myMethod', ...operatorNames); | ||
| }); |
There was a problem hiding this comment.
Somehow this (that being a member of a specific class does not affect the type of a method) feels weird to me though I cannot really explain it. I think it is correct, thinking of the type checking purposes, but the feeling comes from the different contexts (visibility of fields, different potential side effects). No issue though. Just a remark.
There was a problem hiding this comment.
You are right. I postponed the support for methods in classes for a while, since for me, the realization was not obvious as well.
I guess, we could realize methods also in a different way. The final reason for me to use Function types as well was to have a simple solution and to reuse all the implementation for functions.
In order to to make annotating further properties to methods easier (which are not supported by functions), I described methods of classes in the following way:
export interface MethodDetails {
type: TypeReference<FunctionType>;
// methods might have some more properties in the future
}
export class ClassType extends Type {
// ...
protected methods: MethodDetails[];
// ...
}Maybe, MethodDetails will get more properties in the future, or we change the whole approach to describe methods, I am open for both.
| Identifiable --> Invalid | ||
| ``` | ||
| */ | ||
| export type TypeInitializationState = 'Invalid' | 'Identifiable' | 'Completed'; |
There was a problem hiding this comment.
Same for me, why do we need Identifiable? For cycle resolution you only need waitingFor and resolved. Maybe it makes sense to extract this algorithm and solve in isolation.
|
I pushed the last fixes according to your review. The suggested additional test cases were very helpful to find some more bugs! Thank you very much for your reviews @Lotes and @insafuhrmann! |
| @@ -562,7 +571,7 @@ export class ClassTypeInitializer<T = unknown, T1 = unknown, T2 = unknown> exten | |||
| this.initialClassType = new ClassType(kind, typeDetails as CreateClassTypeDetails); | |||
| if (kind.options.typing === 'Structural') { | |||
| // register structural classes also by their names, since these names are usually used for reference in the DSL/AST! | |||
| this.services.graph.addNode(this.initialClassType, kind.getIdentifierPrefix() + typeDetails.className); | |||
| this.services.graph.addNode(this.initialClassType, kind.calculateNominalIdentifier(typeDetails)); | |||
There was a problem hiding this comment.
It is structural, but uses the nominal identifier?
There was a problem hiding this comment.
I renamed the function, since its name was misleading: When used in languages (e.g. TypeScript), structurally typed classes are used by other classes not by encoding their structure but by using their "names". Therefore "name-based identifiers" are relevant for structurally typed classes as well.
This PR fulfilles #22 ...
Hints for the review
Future work in separate PRs: