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

Improve type-safety of the TokenBuilder's options #1586

Merged
merged 6 commits into from
Jul 27, 2024

Conversation

aabounegm
Copy link
Member

It now optionally accepts a generic parameter that is a type union of the names of the available terminals of the given language. It should generally be keyof typeof MyLanguageTerminals imported from './generated/ast.js'.

Usage:

import { MyLanguageTerminals } from './generated/ast.js';

type MyTerminals = keyof typeof MyLanguageTerminals;

export const MyLanguageModule: Module<MyLanguageServices, PartialLangiumServices & MyLanguageAddedServices> = {
    parser: {
        TokenBuilder: () => new IndentationAwareTokenBuilder<MyTerminals>({
            indentTokenName: '' // <-- Now this will require that the given string is actually the name of a token defined in the grammar
                                // and IntelliSense can suggest real token names
        }),
        Lexer: (services) => new IndentationAwareLexer(services),
    },
};

If the generic parameter is not passed, if falls back to the previous behaviour of simply using strings.

It now optionally accepts a generic parameter that is a type union of
the names of the available terminals of the given language.
It should generally be `keyof typeof MyLanguageTerminals`
imported from './generated/ast.js'
@aabounegm
Copy link
Member Author

aabounegm commented Jul 18, 2024

Hmm, I see that NoInfer was added in TS 5.4, but Langium currently uses 5.1.6. Is there a chance it will be updated soon or should I remove NoInfer for now?
Note that the IntelliSense suggestions can become misleading without it. For example:

new IndentationAwareTokenBuilder({
     indentTokenName: 'INDENT',
     dedentTokenName: '' // <-- Here it will suggest only "INDENT"
                         // (though it wouldn't mark other inputs as erroneous)
})

@msujew
Copy link
Member

msujew commented Jul 24, 2024

Feel free to update to TypeScript 5.4 as part of this PR. Aside from that the change looks great, thank you!

@aabounegm
Copy link
Member Author

Great, done. I'd like to note that I attempted updating to the latest version (5.5), but it gave the following errors (since it now does some RegExp validation), so I thought I'd use 5.4 instead for now and this issue can be addressed separately:

packages/langium/test/documentation/jsdoc.test.ts:173:78 - error TS1501: This regular expression flag is only available when targeting 'es2018' or later.

173                     const [paramName, description] = contentMd.split(/\s(.*)/s);
                                                                                 ~

packages/langium/test/parser/token-builder.test.ts:181:44 - error TS1501: This regular expression flag is only available when targeting 'es2018' or later.

181         expect(tokenA.PATTERN).toEqual(/A/is);
                                               ~

@aabounegm
Copy link
Member Author

I'd also like to add that it could be useful to expose the alias type MyTerminals = keyof typeof MyLanguageTerminals; directly from ./generated/ast.js. I didn't explore the generator source code, but from a quick look I assume it would just be a matter of adding:

export type ${config.projectName}TerminalNames = keyof typeof ${config.projectName}Terminals;

to packages/langium-cli/src/generator/ast-generator.ts:243, correct?
Is that something desirable?

@msujew
Copy link
Member

msujew commented Jul 24, 2024

I think that reasonable, yes. Since this is only a type definition, it doesn't even affect bundle size :)

@aabounegm
Copy link
Member Author

Hmm, I did not expect the side-effect this had on the tests. Any suggestions for how to best fix it?

@aabounegm
Copy link
Member Author

@msujew I'm not sure if you would count that as a fix or a hack, but I noticed that all terminal rules use the name "A" so I just added it to the prefix and it worked 😃

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.

I've updated the tests a bit. This now looks good to me, thank you!

@msujew msujew merged commit fad57e9 into eclipse-langium:main Jul 27, 2024
4 checks passed
@aabounegm aabounegm deleted the indentation-aware-type-safety branch July 27, 2024 17:04
@msujew msujew added this to the v3.2.0 milestone Sep 6, 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