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

Internal type split #845

Merged
merged 2 commits into from
Feb 10, 2023
Merged

Internal type split #845

merged 2 commits into from
Feb 10, 2023

Conversation

msujew
Copy link
Member

@msujew msujew commented Dec 19, 2022

This change splits the type system into a "plain" (string-based) type system and a concrete (reference-based) type system. Splitting the types allows for easier handling of purely generated information (i.e. container types, union/interface hierarchies and type names). It also has an impact on how we deal with property types and union types in general, which are now way more flexible.

Aims to resolve multiple issues:

@msujew msujew added validation Validation related issue types Types related issue labels Dec 19, 2022
@msujew msujew force-pushed the msujew/type-refactoring2 branch 2 times, most recently from 8849b7b to 2e97d06 Compare January 30, 2023 18:00
@msujew msujew marked this pull request as ready for review January 30, 2023 18:00
@spoenemann
Copy link
Contributor

The result looks mostly really good! But it doesn't properly solve #534: with the rule

entry X: id=ID ({infer Y} 'a' | {infer Z} 'b');

I get the types

export type X = Y | Z;

export interface Y extends AstNode {
    readonly $type: 'Y';
}

export interface Z extends AstNode {
    readonly $type: 'Z';
}

so the id property is forgotten. Could you add a test for this?

@spoenemann spoenemann requested a review from pluralia January 31, 2023 07:18
@msujew
Copy link
Member Author

msujew commented Jan 31, 2023

@spoenemann Thanks for the hint 👍 It should as expected now.

@spoenemann
Copy link
Contributor

How does this relate to #554?

@pluralia
Copy link
Contributor

pluralia commented Feb 1, 2023

@msujew
Copy link
Member Author

msujew commented Feb 1, 2023

How does this relate to #554?

It's closely related, but not 100% there yet. My code can be used as a basis to build the graph mentioned in #554.

@msujew
Copy link
Member Author

msujew commented Feb 1, 2023

After generating the code, the empty line at the end of these files is deleted

Unrelated to the changes in my PR. It might be due to some recent changes in the generator API.

@pluralia
Copy link
Contributor

pluralia commented Feb 1, 2023

Unrelated to the changes in my PR. It might be due to some recent changes in the generator API.

True. Could you fix it in this PR?

@msujew
Copy link
Member Author

msujew commented Feb 1, 2023

True. Could you fix it in this PR?

I would rather see this fixed in a separate PR.

@spoenemann
Copy link
Contributor

After generating the code, the empty line at the end of these files is deleted

Unrelated to the changes in my PR. It might be due to some recent changes in the generator API.

It's my fault: I had to manually fix the generated files (#888 (comment)), then @msujew fixed the generator, but we didn't regenerate the files. The file ending is different because the text editor automatically changes it on save.

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

General suggestions & comments. In particular I'm curious if we can still retain some (or all) of the previous inferred type tests structure. and then there's the point about the subtle change from undefined to false for values when parsing fails.

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

Tests look good after a double check. There was one that was slightly changed (inferred types using chained actions), where G, H, and I are changed around. But this looks okay based on testing locally. Also checked this against some other Langium projects as well, and nothing appears out of the ordinary from what I can tell.

@msujew
Copy link
Member Author

msujew commented Feb 6, 2023

@montymxb Thanks for the thorough review!

@pluralia Do you have time this week to look into this? pinging @sailingKieler as well. Otherwise I would merge this soonish.

@msujew msujew added this to the v1.1.0 milestone Feb 6, 2023
@msujew msujew force-pushed the msujew/type-refactoring2 branch from 977ff9a to 22aef30 Compare February 8, 2023 23:48
@msujew msujew force-pushed the msujew/type-refactoring2 branch from 22aef30 to 49973f6 Compare February 10, 2023 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types Types related issue validation Validation related issue
Projects
None yet
4 participants