Skip to content

Refactored AST reflection - #1942

Merged
spoenemann merged 3 commits into
mainfrom
refactor-ast-reflection
Jun 13, 2025
Merged

spoenemann merged 3 commits into
mainfrom
refactor-ast-reflection

Conversation

@spoenemann

Copy link
Copy Markdown
Contributor

Closes #1184.

The new implementation of AstReflection no longer needs overriding any of the generic methods. All the necessary metadata is in the types field.

In addition, the new structure is useful for type-safe access to AST property names, e.g. reflection.types.MyType._myProperty. The underscore prefix is necessary to separate the property name constants from the metadata properties.

@spoenemann spoenemann added this to the v4.0.0 milestone Jun 6, 2025
@spoenemann

Copy link
Copy Markdown
Contributor Author

An alternative would be to move the property name constants to a separate structure so we can access them like reflection.props.MyType.myProperty. WDYT?

@ydaveluy

ydaveluy commented Jun 7, 2025

Copy link
Copy Markdown
Contributor

Hey @spoenemann,

And what about prefixing the metadata properties name, properties and superTypes with $ to avoid the underscore prefix for the property name constants ?

Something like:

 BinaryExpression: {
            $name: BinaryExpression,
            $properties: {
                left: {
                    name: 'left'
                },
                operator: {
                    name: 'operator'
                },
                right: {
                    name: 'right'
                }
            },
            $superTypes: ['Expression'],
            // Property name constants
            left: 'left',
            operator: 'operator',
            right: 'right'
        },

@spoenemann

Copy link
Copy Markdown
Contributor Author

Rethinking this, the property constants serve a very different purpose than the AstReflection service. The former are used in language-specific code to have stable access to AST types so you notice it with compiler errors when something has changed in the grammar spec. The latter is used for generic services that should work for all Langium languages.

I like @guiliannewiger's proposal in #1184 (comment) and updated this PR according to that.

I first thought it could be a problematic change to generate objects instead of string constants, but if you already have code like context.container.$type === Person, you'll get an error message:

This comparison appears to be unintentional because the types 'string' and '{ readonly $type: "Person" }' have no overlap.

The solution is to change the code to Person.$type. We should mark this very clearly in the CHANGELOG.

@spoenemann
spoenemann force-pushed the refactor-ast-reflection branch from 21e3dab to 6ead8a3 Compare June 12, 2025 12:35

@msujew msujew left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great, thank you. I'll mention this in detail when writing the changelog for version 4.0 of Langium.

Comment on lines +456 to +459
export const ${name} = {
$type: '${name}'${properties.length > 0 ? ',' : ''}
${joinToNode(properties.sort((a, b) => a.name.localeCompare(b.name)), prop => `${prop.name}: '${escapeQuotes(prop.name, "'")}'`, { separator: ',', appendNewLineIfNotEmpty: true })}
} as const;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggestion: Do we want to add a create function in here as a factory method? This would resolve #1755. However, I'm fine with adding this later in a separate PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't you just use normal object notation to create AST nodes programmatically?

@spoenemann
spoenemann merged commit 1a2984e into main Jun 13, 2025
@spoenemann
spoenemann deleted the refactor-ast-reflection branch June 13, 2025 21:55
@JohannesMeierSE JohannesMeierSE added ast AST structure related issue cli CLI related issue labels Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ast AST structure related issue cli CLI related issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Export property types for checking at compile time

4 participants