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
Show file tree
Hide file tree
Changes from 3 commits
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
345 changes: 234 additions & 111 deletions examples/lox/test/lox-type-checking.test.ts

Large diffs are not rendered by default.

8 changes: 5 additions & 3 deletions packages/typir/src/graph/type-graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
******************************************************************************/

import { EdgeCachingInformation } from '../features/caching.js';
import { assertTrue } from '../utils/utils.js';
import { TypeEdge } from './type-edge.js';
import { Type } from './type-node.js';

Expand All @@ -28,10 +29,11 @@ export class TypeGraph {
* Therefore it is usually not needed to call this method in an other context.
* @param type the new type
* @param key an optional key to register the type, since it is allowed to register the same type with different keys in the graph
* TODO oder stattdessen einen ProxyType verwenden? wie funktioniert das mit isClassType und isSubType? wie funktioniert removeType?
*/
addNode(type: Type, key?: string): void {
// TODO überprüfen, dass Identifiable-State erreicht ist??
if (!key) {
assertTrue(type.isInStateOrLater('Identifiable')); // the key of the type must be available!
}
const mapKey = key ?? type.getIdentifier();
if (this.nodes.has(mapKey)) {
if (this.nodes.get(mapKey) === type) {
Expand Down Expand Up @@ -62,7 +64,7 @@ export class TypeGraph {
const contained = this.nodes.delete(mapKey);
if (contained) {
this.listeners.slice().forEach(listener => listener.removedType(typeToRemove, mapKey));
typeToRemove.deconstruct();
typeToRemove.dispose();
} else {
throw new Error(`Type does not exist: ${mapKey}`);
}
Expand Down
25 changes: 13 additions & 12 deletions packages/typir/src/graph/type-node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ stateDiagram-v2
Completed --> Invalid
Identifiable --> Invalid
```
* A state is 'Completed', when all its dependencies are available, i.e. the types of all its properties are available.
* A state is 'Identifiable', when all those dependencies are available which are required to calculate the identifier of the type.
* A state is 'Invalid' otherwise.
* 'Invalid' is made explicit, since it might require less dependencies than 'Completed' and therefore speed-ups the resolution of dependencies.
*/
export type TypeInitializationState = 'Invalid' | 'Identifiable' | 'Completed';
Copy link
Member

Choose a reason for hiding this comment

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

For me 'Identifiable' and 'Completed' are quite clear, but what exactly 'Invalid' means is harder to grasp, especially a going back to 'Invalid'. And then: Is the 'Invalid' after going back final (or can we get on again to Identifiable or Completed after being set back once)? Are there examples/test cases for this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why do we need Identifiable?

'Invalid' is made explicit, since it might require less dependencies than 'Completed' and therefore speed-ups the resolution of dependencies.

I added more comments.

Is the 'Invalid' after going back final (or can we get on again to Identifiable or Completed after being set back once)?

I guess, we can switch to Identifiable/Completed again after going to 'Invalid', but I am not yet sure.

Are there examples/test cases for this?

Not yet, since they depend on the previous decision.


Expand Down Expand Up @@ -57,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;
}
Expand Down Expand Up @@ -130,20 +134,20 @@ export abstract class Type {

protected stateListeners: TypeStateListener[] = [];

addListener(listener: TypeStateListener, informIfNotInvalidAnymore: boolean): void {
this.stateListeners.push(listener);
addListener(newListeners: TypeStateListener, informIfNotInvalidAnymore: boolean): void {
this.stateListeners.push(newListeners);
if (informIfNotInvalidAnymore) {
JohannesMeierSE marked this conversation as resolved.
Show resolved Hide resolved
const currentState = this.getInitializationState();
switch (currentState) {
case 'Invalid':
// don't inform about the Invalid state!
break;
case 'Identifiable':
listener.switchedToIdentifiable(this);
newListeners.switchedToIdentifiable(this);
break;
case 'Completed':
listener.switchedToIdentifiable(this); // inform about both Identifiable and Completed!
listener.switchedToCompleted(this);
newListeners.switchedToIdentifiable(this); // inform about both Identifiable and Completed!
newListeners.switchedToCompleted(this);
break;
default:
assertUnreachable(currentState);
Expand Down Expand Up @@ -197,21 +201,18 @@ export abstract class Type {
this.onCompletion = preconditions.onCompleted ?? (() => {});
this.onInvalidation = preconditions.onInvalidated ?? (() => {});

if (this.kind.$name === 'ClassKind') {
console.log('');
}
// preconditions for Identifiable
this.waitForIdentifiable = new WaitingForIdentifiableAndCompletedTypeReferences(
preconditions.preconditionsForIdentifiable?.referencesToBeIdentifiable,
preconditions.preconditionsForIdentifiable?.referencesToBeCompleted,
);
this.waitForIdentifiable.addTypesToIgnoreForCycles(new Set([this]));
this.waitForIdentifiable.addTypesToIgnoreForCycles(new Set([this])); // start of the principle: children don't need to wait for their parents
// preconditions for Completed
this.waitForCompleted = new WaitingForIdentifiableAndCompletedTypeReferences(
preconditions.preconditionsForCompleted?.referencesToBeIdentifiable,
preconditions.preconditionsForCompleted?.referencesToBeCompleted,
);
this.waitForCompleted.addTypesToIgnoreForCycles(new Set([this]));
this.waitForCompleted.addTypesToIgnoreForCycles(new Set([this])); // start of the principle: children don't need to wait for their parents
// preconditions for Invalid
this.waitForInvalid = new WaitingForInvalidTypeReferences(
preconditions.referencesRelevantForInvalidation ?? [],
Expand Down Expand Up @@ -262,7 +263,7 @@ export abstract class Type {
this.waitForCompleted.addTypesToIgnoreForCycles(additionalTypesToIgnore);
}

deconstruct(): void {
dispose(): void {
// clear everything
this.stateListeners.splice(0, this.stateListeners.length);
this.waitForInvalid.getWaitForRefsInvalid().forEach(ref => ref.deconstruct());
Expand Down
30 changes: 20 additions & 10 deletions packages/typir/src/kinds/class-kind.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -388,6 +390,12 @@ export interface CreateFieldDetails {
type: TypeSelector;
}

/**
* Describes all properties of Methods of a Class.
* The final reason to describe methods with Function types was to have a simple solution and to reuse all the implementations for functions,
* since methods and functions are the same from a typing perspective.
* This interfaces makes annotating further properties to methods easier (which are not supported by functions).
*/
export interface MethodDetails {
type: TypeReference<FunctionType>;
// methods might have some more properties in the future
Expand Down Expand Up @@ -444,8 +452,6 @@ export class ClassKind implements Kind {
assertTrue(this.options.maximumNumberOfSuperClasses >= 0); // no negative values
}

// zwei verschiedene Use cases für Calls: Reference/use (e.g. Var-Type) VS Creation (e.g. Class-Declaration)

/**
* For the use case, that a type is used/referenced, e.g. to specify the type of a variable declaration.
* @param typeDetails all information needed to identify the class
Expand Down Expand Up @@ -523,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);
Expand All @@ -546,7 +556,6 @@ export function isClassKind(kind: unknown): kind is ClassKind {
}


// TODO Review: Is it better to not have "extends TypeInitializer" and to merge TypeInitializer+ClassTypeInitializer into one class?
export class ClassTypeInitializer<T = unknown, T1 = unknown, T2 = unknown> extends TypeInitializer<ClassType> implements TypeStateListener {
protected readonly typeDetails: CreateClassTypeDetails<T, T1, T2>;
protected readonly kind: ClassKind;
Expand All @@ -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));
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);
Expand All @@ -588,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
Expand All @@ -613,7 +623,7 @@ export class ClassTypeInitializer<T = unknown, T1 = unknown, T2 = unknown> exten
if (this.typeDetails.inferenceRuleForDeclaration === null) {
// check for cycles in sub-type-relationships of classes
if ((classType as ClassType).hasSubSuperClassCycles()) {
throw new Error(`Circles in super-sub-class-relationships are not allowed: ${classType.getName()}`);
throw new Error(`Cycles in super-sub-class-relationships are not allowed: ${classType.getName()}`);
}
}

Expand All @@ -622,7 +632,7 @@ export class ClassTypeInitializer<T = unknown, T1 = unknown, T2 = unknown> exten
}

switchedToInvalid(_previousClassType: Type): void {
// do nothing
// nothing specific needs to be done for Classes here, since the base implementation takes already care about all relevant stuff
}

override getTypeInitial(): ClassType {
Expand Down Expand Up @@ -919,7 +929,7 @@ export function createNoSuperClassCyclesValidation(isRelevant: (domainElement: u
$problem: ValidationProblem,
domainElement,
severity: 'error',
message: `Circles in super-sub-class-relationships are not allowed: ${classType.getName()}`,
message: `Cycles in super-sub-class-relationships are not allowed: ${classType.getName()}`,
});
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/typir/src/kinds/function-kind.ts
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ export class FunctionTypeInitializer<T> extends TypeInitializer<FunctionType> im
}

switchedToInvalid(_functionType: Type): void {
// empty
// nothing specific needs to be done for Functions here, since the base implementation takes already care about all relevant stuff
}
}

Expand Down
11 changes: 10 additions & 1 deletion packages/typir/src/utils/test-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ import { expect } from 'vitest';
import { Type } from '../graph/type-node.js';
import { TypirServices } from '../typir.js';

/**
* Testing utility to check, that exactly the expected types are in the type system.
* @param services the Typir services
* @param filterTypes used to identify the types of interest
* @param namesOfExpectedTypes the names (not the identifiers!) of the expected types;
* ensures that there are no more types;
* it is possible to specify names multiple times, if there are multiple types with the same name (e.g. for overloaded functions)
* @returns all the found types
*/
export function expectTypirTypes(services: TypirServices, filterTypes: (type: Type) => boolean, ...namesOfExpectedTypes: string[]): Type[] {
JohannesMeierSE marked this conversation as resolved.
Show resolved Hide resolved
const types = services.graph.getAllRegisteredTypes().filter(filterTypes);
types.forEach(type => expect(type.getInitializationState()).toBe('Completed')); // check that all types are 'Completed'
Expand All @@ -18,6 +27,6 @@ export function expectTypirTypes(services: TypirServices, filterTypes: (type: Ty
expect(index >= 0).toBeTruthy();
typeNames.splice(index, 1); // removing elements is needed to work correctly with duplicated entries
}
expect(typeNames).toHaveLength(0);
expect(typeNames, `There are more types than expected: ${typeNames.join(', ')}`).toHaveLength(0);
return types;
}
8 changes: 6 additions & 2 deletions packages/typir/src/utils/type-initialization.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ export type TypeInitializerListener<T extends Type = Type> = (type: T) => void;
* Since the first Typir type is not yet in the type systems (since it still waits for B) and therefore remains unknown,
* it will be tried to create A a second time, again delayed, since B is still not yet available.
* When B is created, A is waiting twice and might be created twice, if no TypeInitializer is used.
*
* Design decision: While this class does not provide some many default implementations,
* a common super class (or interface) of all type initializers is useful,
* since they all can be used as TypeSelector in an easy way.
*/
export abstract class TypeInitializer<T extends Type = Type> {
protected readonly services: TypirServices;
Expand All @@ -40,7 +44,7 @@ export abstract class TypeInitializer<T extends Type = Type> {
if (existingType) {
// ensure, that the same type is not duplicated!
this.typeToReturn = existingType as T;
newType.deconstruct();
newType.dispose();
} else {
this.typeToReturn = newType;
this.services.graph.addNode(this.typeToReturn);
Expand All @@ -54,7 +58,7 @@ export abstract class TypeInitializer<T extends Type = Type> {
return this.typeToReturn;
}

// TODO using this type feels wrong, but otherwise, it seems not to work ...
// TODO using this type feels wrong, but without this approach, it seems not to work ...
abstract getTypeInitial(): T

getTypeFinal(): T | undefined {
Expand Down
7 changes: 6 additions & 1 deletion packages/typir/src/utils/utils-definitions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,11 @@ export class WaitingForIdentifiableAndCompletedTypeReferences<T extends Type = T
}
}

/**
* This method is called to inform about additional types which can be ignored during the waiting/resolving process.
* This helps to deal with cycles in type dependencies.
* @param moreTypesToIgnore might contain duplicates, which are filtered internally
*/
addTypesToIgnoreForCycles(moreTypesToIgnore: Set<Type>): void {
JohannesMeierSE marked this conversation as resolved.
Show resolved Hide resolved
// identify the actual new types to ignore (filtering out the types which are already ignored)
const newTypesToIgnore: Set<Type> = new Set();
Expand Down Expand Up @@ -164,7 +169,7 @@ export class WaitingForIdentifiableAndCompletedTypeReferences<T extends Type = T
}

onTypeReferenceResolved(_reference: TypeReference<Type>, resolvedType: Type): void {
// inform the referenced type about the types to ignore for completion
// inform the referenced type about the types to ignore for completion, so that the type could switch to its next phase (if needed)
resolvedType.ignoreDependingTypesDuringInitialization(this.typesToIgnoreForCycles);
resolvedType.addListener(this, false);
// check, whether all TypeReferences are resolved and the resolved types are in the expected state
Expand Down