Skip to content
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

Cyclic type definitions #33

Merged
merged 19 commits into from
Nov 27, 2024
Merged
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
a2bb2ee
first working version of type definitions in "wrong" order for classe…
JohannesMeierSE Nov 12, 2024
bf1eb78
listener for new inference rules, fixed several bugs
JohannesMeierSE Nov 12, 2024
36708f0
fixed important bug
JohannesMeierSE Nov 18, 2024
c1f02ec
refactorings, improved comments
JohannesMeierSE Nov 18, 2024
0c933fa
validation (instead of exception) for super-sub-class cycles
JohannesMeierSE Nov 19, 2024
ddda324
propagate cyclic types to the direct children
JohannesMeierSE Nov 19, 2024
6c29f57
propagate types to ignore recursively to indirect dependencies as well
JohannesMeierSE Nov 19, 2024
40bea25
added more expected error messages into existing test cases
JohannesMeierSE Nov 19, 2024
d4b1cde
simplified code, improved comments, more test cases, renamings, some …
JohannesMeierSE Nov 20, 2024
005c37f
fixed registration of inference rules of classes, new deconstruct log…
JohannesMeierSE Nov 20, 2024
072de08
more test cases for cyclic classes
JohannesMeierSE Nov 20, 2024
704d81c
small improvements for the lifecycle of types, renamings
JohannesMeierSE Nov 21, 2024
a07614b
realized cyclic types with Functions involved, slightly reworked the …
JohannesMeierSE Nov 21, 2024
14b21d5
fix for unique method validation, more test cases
JohannesMeierSE Nov 22, 2024
db66fba
more comments, renamings
JohannesMeierSE Nov 22, 2024
9f1b703
refactorings, more test cases and comments according to the review
JohannesMeierSE Nov 26, 2024
2a6c18f
added name as identifier for nominally typed classes earlier, refacto…
JohannesMeierSE Nov 27, 2024
3b471e2
improved existing test cases
JohannesMeierSE Nov 27, 2024
bc43521
renaming, refactoring, more comments
JohannesMeierSE Nov 27, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
added name as identifier for nominally typed classes earlier, refacto…
…ring
JohannesMeierSE committed Nov 27, 2024
commit 2a6c18fc3cfefbe6540982bdd820f0130ac5563e
8 changes: 4 additions & 4 deletions examples/lox/test/lox-type-checking.test.ts
Original file line number Diff line number Diff line change
@@ -576,7 +576,7 @@ describe('Cyclic type definitions where a Class is declared and already used', (
expectTypirTypes(loxServices, isFunctionType, 'myMethod', ...operatorNames);
});

test.todo('Mix of dependencies in classes: 1 method and 2 fields (order 2)', async () => {
test('Mix of dependencies in classes: 1 method and 2 fields (order 2)', async () => {
await validate(`
class A {
myMethod(input: number): B1 {}
@@ -592,7 +592,7 @@ describe('Cyclic type definitions where a Class is declared and already used', (
expectTypirTypes(loxServices, isFunctionType, 'myMethod', ...operatorNames);
});

test.todo('The same class is involved into two dependency cycles', async () => {
test('The same class is involved into two dependency cycles', async () => {
await validate(`
class A {
probA: C1
@@ -605,10 +605,10 @@ describe('Cyclic type definitions where a Class is declared and already used', (
propB1: A
}
class C1 {
methodC1(p: C2) {}
methodC1(p: C2): void {}
}
class C2 {
methodC2(p: A) {}
methodC2(p: A): void {}
}
`, []);
expectTypirTypes(loxServices, isClassType, 'A', 'B1', 'B2', 'C1', 'C2');
2 changes: 1 addition & 1 deletion packages/typir/src/graph/type-node.ts
Original file line number Diff line number Diff line change
@@ -61,7 +61,7 @@ export abstract class Type {
* Identifiers might have a naming schema for calculatable values.
*/
getIdentifier(): string {
this.assertStateOrLater('Identifiable');
// an Identifier must be available; note that the state might be 'Invalid' nevertheless, which is required to handle cyclic type definitions
assertTrue(this.identifier !== undefined);
return this.identifier;
}
15 changes: 11 additions & 4 deletions packages/typir/src/kinds/class-kind.ts
Original file line number Diff line number Diff line change
@@ -30,7 +30,9 @@ export class ClassType extends Type {
protected methods: MethodDetails[]; // unordered

constructor(kind: ClassKind, typeDetails: ClassTypeDetails) {
super(undefined);
super(kind.options.typing === 'Nominal'
? kind.calculateNominalIdentifier(typeDetails) // use the name of the class as identifier already now
: undefined); // the identifier for structurally typed classes will be set later after resolving all fields and methods
this.kind = kind;
this.className = typeDetails.className;

@@ -527,6 +529,10 @@ export class ClassKind implements Kind {
}
}

calculateNominalIdentifier<T>(typeDetails: ClassTypeDetails<T>): string {
return this.getIdentifierPrefix() + typeDetails.className;
}

getMethodKind(): FunctionKind {
// ensure, that Typir uses the predefined 'function' kind for methods
const kind = this.services.kinds.get(FunctionKindName);
@@ -565,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));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is structural, but uses the nominal identifier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.inferenceRules = createInferenceRules<T, T1, T2>(this.typeDetails, this.kind, this.initialClassType);
@@ -591,8 +597,9 @@ export class ClassTypeInitializer<T = unknown, T1 = unknown, T2 = unknown> exten

if (this.kind.options.typing === 'Structural') {
// replace the type in the type graph
this.services.graph.removeNode(classType, this.kind.getIdentifierPrefix() + this.typeDetails.className);
this.services.graph.addNode(readyClassType, this.kind.getIdentifierPrefix() + this.typeDetails.className);
const nominalIdentifier = this.kind.calculateNominalIdentifier(this.typeDetails);
this.services.graph.removeNode(classType, nominalIdentifier);
this.services.graph.addNode(readyClassType, nominalIdentifier);
}

// remove the inference rules for the invalid type