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

Respect the declared/returned type of data type rules, which is relevant for the default value converter #1500

Conversation

JohannesMeierSE
Copy link
Contributor

@JohannesMeierSE JohannesMeierSE commented May 16, 2024

This PR contributes:

  • fix to respect the declared/returned type of data type rules, when (among others) checked by the default value converter
  • test cases for the default value converter, when using data type rules with declared return type which use terminal rules

This issue was found in a real-world application, where a data type rule (with returns number) which referred to a terminal rule (with returns number) produced a string value at runtime in the AST (while the generated property in the TypeScript interface had the correct type number). This conforms to the "number case" in the new test case. The cases for string and boolean are added only to be more complete.

This issue is not related to #1478.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looking at the lint CI, it seems like the change leads to a regression. The generated names actually change from the data type name to their declared types. I'm suprised we don't have actual tests for that. Do you mind adding some?

As for the change, I would recommend something else:

  1. Create a new getRuleTypeName function that reuses the old behavior of the getRuleType function. It should be used everywhere except for the ValueConverter.
  2. Add appropriate comments to explain the difference between those two functions. getRuleType should be used during the runtime, while getRuleTypeName is useful for code generation and the internal type system.

@JohannesMeierSE JohannesMeierSE force-pushed the fix-value-conversion-for-data-type-rules branch from 37c4924 to cf535e2 Compare June 14, 2024 15:22
@JohannesMeierSE
Copy link
Contributor Author

Thanks @msujew for your review! I created two functions, one to be used at runtime and the other one at development time.

However, there are already test cases which check the generated code for data type rules with return type:
https://github.com/eclipse-langium/langium/blob/main/packages/langium-cli/test/generator/ast-generator.test.ts#L170
But I don't understand, why they didn't show the regression.

Any ideas?

@msujew
Copy link
Member

msujew commented Jun 14, 2024

@JohannesMeierSE These tests don't test the generated property types. For example:

Value: id=ID;
ID returns string: ...;

is supposed to generate:

export interface Value extends AstNode {
  id: ID // <-- this is the part that broke
}
export type ID = string;

@JohannesMeierSE
Copy link
Contributor Author

Thanks for the suggestion @msujew! I added two test cases which test properties with data types which have an explicit return type.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@JohannesMeierSE JohannesMeierSE force-pushed the fix-value-conversion-for-data-type-rules branch from 8864a25 to eb60e34 Compare June 18, 2024 12:58
@msujew msujew merged commit 4bbc180 into eclipse-langium:main Jun 18, 2024
4 checks passed
@msujew msujew added this to the v3.1.0 milestone Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants