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

Improves Primitive Types #508

Merged
merged 6 commits into from
May 27, 2022
Merged

Improves Primitive Types #508

merged 6 commits into from
May 27, 2022

Conversation

montymxb
Copy link
Contributor

@montymxb montymxb commented May 19, 2022

Closes #482 and Closes #199, by improving the primitive types present in Langium. In particular, this PR adds the bigint built-in type, along with an associated default value converter.

In addition:

  • date was changed to Date to match the casing in typescript.
  • error messages for primitive types were updated to include bigint and Date
  • added default value converter for Date
  • modified existing tests to incorporate bigint and Date primitives
  • added new tests to enforce parsing/conversion behavior

There are some key points to note with this PR:

  • It is not recommended to parse date strings directly via the Date() constructor, but I have this in place for now as a proof of concept. I think it may be worth a little bit more work to recognize only ISO 8601 strings, or RFC 2822, a combination of the two, or some other approach that is safer. It could be as simple as deconstructing the date components and using another constructor on the Date object instead.

  • Booleans are still kind of odd, given that the values are either true for a successful parse or undefined, rather than true and false. In the token builder tests, I noticed a test that builds a grammar that tries to capture a false value incorrectly. Specifically,

    terminal BOOLEAN returns boolean: /true|false/;
    

    Which can parse both true and false to produce a value of true. If you try to remove false from the regex to only recognize true, you run into the awkward situation of being unable to distinguish between false and a failed parse.

    Ultimately, my thought is whether I should expand this PR to allow boolean assignments to have the expected true/false values, instead of true/undefined.

  • Recognizing bigints is a little tricky, and so I've attempted to provide good examples in the tests of how this can be done properly. Lookahead and terminal ordering are both important to disambiguate between ints and bigints. Beyond that, they actually work pretty well!

@msujew
Copy link
Member

msujew commented May 19, 2022

@montymxb Small remark: With the merge of #469, boolean AST properties are always initialized with false instead of undefined.

@montymxb montymxb force-pushed the montymxb/improve-primitive-types branch from 49c5599 to eb64e69 Compare May 19, 2022 15:01
@montymxb
Copy link
Contributor Author

Ah, thanks for pointing that out @msujew ! Rebased on current work to get those changes in.

The standard ?= case looks good. However, a prop of a mixed type that includes a bool in the union will still resolve to undefined instead of false.

Correct me if I'm wrong here, but based on what I've seen I think actively setting of the value to false when the ?= operator is used, and the following parse fails, could resolve this particular case?

@msujew
Copy link
Member

msujew commented May 20, 2022

@montymxb I guess the meta data generator for that is too strict. On the other hand, a union with a boolean property is quite weird from a parsing perspective IMO.

Correct me if I'm wrong here, but based on what I've seen I think actively setting of the value to false when the ?= operator is used, and the following parse fails, could resolve this particular case?

I will correct, since you are wrong ;-) The issue is that we don't know whether we will parse the ?= assignment, since it's usually optional. The AST builder inside of the parser has actually no real clue about the grammar at all, and doesn't know whether any ?= assignments belong to a certain AST node. That's why we ask the AstReflection later whether we've missed something during parsing:

https://github.com/langium/langium/blob/d5ab9378f5744abe5f7827f729e35e7ebd846e1b/packages/langium/src/parser/langium-parser.ts#L273-L283

@montymxb
Copy link
Contributor Author

Thanks for clarifying that point @msujew . With that in mind, I can focus on the Date aspect instead.

@montymxb
Copy link
Contributor Author

Thought about Date for awhile, and have decided to leave it as is. Trying to predict what the desired Date string format should be is unclear at best, as it may vary from case to case, and region to region. ISO 8601 still seems like a good choice, but I think it's best to leave targeted conversions up to the implementer, and go with the default for now.

@montymxb montymxb marked this pull request as ready for review May 20, 2022 15:06
@montymxb montymxb requested a review from spoenemann May 24, 2022 07:39
@@ -45,10 +45,14 @@ export class DefaultValueConverter implements ValueConverter {
case 'STRING': return convertString(input);
case 'ID': return convertID(input);
case 'REGEXLITERAL': return convertString(input);
case 'BIGINT': return convertBigint(input);
case 'DATE': return convertDate(input);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these two cases are not necessary because they are already handled by the return type below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah they're not critical, I can take them out. If they're desired for some reason, it would be easy to add them in later.

I figured they might be nice for shorter terminals, like terminal BIGINT: INT instead of writing out terminal BIGINT returns bigint: INT; based on what we have already for INT. Since the same can be expressed with the return type, they don't add any expressiveness, just a shorthand equivalent.

@spoenemann
Copy link
Contributor

This fixes #199 as well, right?

@montymxb
Copy link
Contributor Author

montymxb commented May 27, 2022

@spoenemann yep! This PR closes #199 as well 👍

@montymxb montymxb merged commit 74398ff into main May 27, 2022
@montymxb montymxb deleted the montymxb/improve-primitive-types branch May 27, 2022 08:06
@spoenemann spoenemann added this to the v0.4.0 milestone Jun 9, 2022
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.

Improve primitive types Add support for bigint and Date data types
3 participants