Skip to content

Conversation

msujew
Copy link
Member

@msujew msujew commented Sep 3, 2025

Follow up on #2011 which fixes an issue where the infix rules object was stored in the CST astNode item by accident.

@msujew msujew requested a review from sailingKieler September 3, 2025 12:20
Copy link
Contributor

@sailingKieler sailingKieler left a comment

Choose a reason for hiding this comment

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

See remark below.

// The specified item could be a datatype ($type is symbol), fragment ($type is undefined) or infix rule ($infix exists)
// Only if the $type is a string, we actually assign the element
if (typeof item.$type === 'string') {
if (typeof item.$type === 'string' && !('$infix' in item)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrt. performance: Would it be advantageous to add $infix?: boolean to the type def of item and just test !item.$infix?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it matters, but it makes it more readable 👍

@msujew msujew force-pushed the msujew/fix-infix-operator-cst branch from 9afd63a to b6855fc Compare September 4, 2025 09:44
Copy link
Contributor

@sailingKieler sailingKieler left a comment

Choose a reason for hiding this comment

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

Nice!

@msujew msujew merged commit 51f5d0a into main Sep 4, 2025
5 checks passed
@msujew msujew deleted the msujew/fix-infix-operator-cst branch September 4, 2025 10:49
msujew added a commit that referenced this pull request Sep 10, 2025
Fix infix operator CST issues
@cdietrich
Copy link
Contributor

@msujew should sth like this still happen with 4.0.1

grafik

@msujew
Copy link
Member Author

msujew commented Sep 17, 2025

@cdietrich Not really, but I'm not sure whether it's actually feasible to rewrite the whole CST during the binary operation AST rewrite. I'm quite unhappy with the feature myself (see also #1733) - it works better in another project where we embed the CST in the AST (non-langium project).

@msujew msujew added this to the v4.0.0 milestone Sep 17, 2025
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.

3 participants