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

Reworked the sub-type relationship infrastructure #58

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

JohannesMeierSE
Copy link
Collaborator

@JohannesMeierSE JohannesMeierSE commented Jan 20, 2025

This PR contributes:

  • Sub-type relationships are now explicitly stored in the type graph
    • They are not calculated by the asymmetric analyzeIsSubTypeOf and analyzeIsSuperTypeOf anymore
    • Types store their sub-type-reationships themselves in the type graph
    • no caching of (transitive) sub-type relationships anymore
    • new feature: now sub-type relationships can be explicitly defined by the user of Typir
  • This is the precondition to improve assignability checks:
    • arbitrary paths of implicit conversion and sub-type relationships can be exploited now for advanced assignability checks, by path search in the type graph
    • implementation for finding and controlling best matches of overloaded functions/operators
  • moved the existing graph algorithms into its own dedicated service in order to reuse and to customize them (+ created variants of the existing algorithms)
  • I started to tackle the terminology issue with "problems" as result of e.g. sub-type checking by introducing SubTypeProblem and SubTypeSuccess interfaces, combined as type SubTypeResult = SubTypeSuccess | SubTypeProblem
  • provided some predefined language elements for reuse in test cases inside typir in packages/typir/src/test/predefined-language-nodes.ts (they are strongly inspired from our joint work @insafuhrmann)
  • more test utilities
  • more test cases

Aspects for the review:

  • How to produce meaningful subProblems for failed checks of sub-type and assignability? As an example, a user is interested, why two structually typed classes are not sub-types.
  • How to make the signatures in GraphAlgorithms in graph-algorithms.ts more type-safe?
  • @insafuhrmann conversion.test.ts contains a test case to check for cycles in conversion rules: Could you check, whether is reflects the kind of test cases you looked for you?

@JohannesMeierSE JohannesMeierSE marked this pull request as draft January 21, 2025 12:15
@JohannesMeierSE JohannesMeierSE marked this pull request as ready for review January 21, 2025 19:59
Copy link
Collaborator

@Lotes Lotes left a comment

Choose a reason for hiding this comment

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

All in all just a bit coding style and a bit knowledge transfer -Typescript-wise. I still do not get the purpose of the 300+-changes file which I commented as latest. I hope you will find these comments helpful :). Thanks for your work.

packages/typir/src/graph/graph-algorithms.ts Show resolved Hide resolved
packages/typir/src/graph/type-node.ts Outdated Show resolved Hide resolved
packages/typir/src/kinds/bottom/bottom-type.ts Outdated Show resolved Hide resolved
packages/typir/src/kinds/class/top-class-type.ts Outdated Show resolved Hide resolved
packages/typir/src/kinds/function/function-initializer.ts Outdated Show resolved Hide resolved
packages/typir/src/services/assignability.ts Show resolved Hide resolved
packages/typir/src/services/subtype.ts Outdated Show resolved Hide resolved
Copy link
Member

@insafuhrmann insafuhrmann left a comment

Choose a reason for hiding this comment

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

Thanks for this work @JohannesMeierSE. I am pretty happy to get rid of analyzeIsSubTypeOf and analyzeIsSuperTypeOf. The rest looks fine to me too, where I had something to add, I placed a comment. Let's discuss the first two "aspects for review" in the meeting. I do not have a ready made solution at hand.

README.md Outdated Show resolved Hide resolved
packages/typir/src/initialization/type-selector.ts Outdated Show resolved Hide resolved
packages/typir/src/kinds/class/class-validation.ts Outdated Show resolved Hide resolved
packages/typir/src/kinds/function/function-initializer.ts Outdated Show resolved Hide resolved
packages/typir/src/kinds/function/function-initializer.ts Outdated Show resolved Hide resolved
packages/typir/src/services/assignability.ts Show resolved Hide resolved
@JohannesMeierSE
Copy link
Collaborator Author

Thanks @insafuhrmann and @Lotes for your helpful reviews!

I just pushed the corresponding improvements (and resolved the corresponding discussions here), but some discussions are still open. I suggest to continue them in our next joint meeting.

@JohannesMeierSE
Copy link
Collaborator Author

@Lotes @insafuhrmann I just pushed the last improvements according to our discussions today.

Copy link
Collaborator

@Lotes Lotes left a comment

Choose a reason for hiding this comment

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

I have found a small thing about values vs. types.

Otherwise, LGTM :-).



describe('Test multiple matches for overloaded operators and ensures that the best match is chosen', () => {
test('2 + 3 => both are integers', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overloading can also be on type level. Like the assignments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if it looks nice, the values play no role here.

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 agree, that the actual values are not relevant (we could test "123 + 456" instead without any differences).

But here I want to test the type inference, in particular, whether the "best overload" is found during the type inference.
(Note that FunctionType is nearly unchanged, while there are lots of changes in the function-*-inference.ts files)

Testing type inference is easier with actual values, therefore, I prefer to keep these "easy-to-read" test cases. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can live with it :-) As I said, it looks nice, nevertheless.

Copy link
Member

Choose a reason for hiding this comment

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

Fine for me as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure New Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants