Generate an AST visitor in addition to the AST

[msujew]

Often times I want to just go through some objects of the AST (e.g. for validation) without streaming the whole AST tree. For this, we should think about generating an AST visitor which we can use to override certain visitor methods.

[dhuebner]

to just go through some objects

You mean object that e.g. have the same type?

[msujew]

Right, for example, if I want to process every Assignment within a rule.

[dhuebner]

I like the idea to iterate only once over the AST and all "registered" methods (or participants) for e.g. Assigment are than called. I would love to see a TS sketch for that

[spoenemann]

I don't quite get it. Can you give an example that you can't realize with streamAllContents?

[msujew]

Not really, since I thought to implement it via streamAllContents. The visitor would only be an easier way to access everything. But if your opinion is that streamAllContents is enough, then we can close this issue.

[spoenemann]

Ok of course we can implement additional utility on top of that.

[danieldietrich]

I would think twice before adding multiple concepts for the same thing.

This 'visitor' pattern can be described as .filter followed by a .map (resp. .forEach, which is just a map returning nothing/void). In Scala for example, switch / cases are partial functions (the opposite of total functions), they are defined for certain elements. This is why Scala introduced collect as shorthand for filter+ map.

But maybe we can do it even simpler. If map would return some type U | void, then we could consume elements like this:

stream.map(node => {
    if (node.isTypeA()) {
        consumeTypeA(node);
    } else if (node.isTypeB()) {
        consumeTypeB(node);
    }
})

OR just use forEach 😊.

Introducing a completely new concept is too heavy-weight IMO. Also the visitor pattern is very much OO and not so scalable. You need to provide visit methods for each kind of type, which is very verbose. We would not gain anything with a visitor, it would (internally) iterate the whole tree, though.

What do you think?

[msujew]

What I'm doing right know looks something like this:

streamAllContents(rule).forEach(e => {
    if (isRuleCall(e.node) && isParserRule(e.node.rule.ref)) {
        accept('error', 'Primitive rules can only call other primitive rules or terminal rules', { node: e.node });
    }
    if (isAction(e.node)) {
        accept('error', 'Primitive rules cannot contain actions.', { node: e.node });
    }
    if (isAssignment(e.node)) {
        accept('error', 'Primitive rules cannot contain assignments.', { node: e.node });
    }
});

But with a visitor I'd have something like this:

const visitor = new LangiumGrammarVisitor();
visitor.on('ParserRule', parserRule => ...);
visitor.on('Action', action => ...);
visitor.on('Assignment', assignment => ...);
visitor.visit(rule);

The second approach is way cleaner.

Introducing a completely new concept is too heavy-weight IMO. Also the visitor pattern is very much OO and not so scalable. You need to provide visit methods for each kind of type, which is very verbose.

Well, not really, see the example above. With TypeScript/JavaScript we can create much more concise visitors than in Java or other heavy OO languages. This design is inspired by how usual JavaScript listener/callback based frameworks operate instead of the usual OO stuff.

[dhuebner]

const visitor = new LangiumGrammarVisitor();
visitor.on('ParserRule', parserRule => ...);
visitor.on('Action', action => ...);
visitor.on('Assignment', assignment => ...);
visitor.visit(rule);

That is almost what I had in mind when reading this feature request. What I would expect to see from the generated implementation is that register functions (here on) are generated as onAction, onAssigment a.s.o. The idea behind it is that the existing code breaks as soon as you e.g. deletes Action or rename it to something else

[spoenemann]

@msujew I actually like the first variant more. It is more direct and does not require to understand the additional pattern.

[brucou]

@msujew I am chiming in. Apologies in advance if I misunderstood you but I more than often solved these issues with simple helpers.

So this:

streamAllContents(rule).forEach(e => {
    if (isRuleCall(e.node) && isParserRule(e.node.rule.ref)) {
        accept('error', 'Primitive rules can only call other primitive rules or terminal rules', { node: e.node });
    }
    if (isAction(e.node)) {
        accept('error', 'Primitive rules cannot contain actions.', { node: e.node });
    }
    if (isAssignment(e.node)) {
        accept('error', 'Primitive rules cannot contain assignments.', { node: e.node });
    }
});

can be written as:

streamAllContents(rule).forEach(patternMatch( [
    ['ParserRule', parserRule => ...],
    ['Action', action => ...],
    ...
]));

You have to write the patterMatch function of course. But you only have to write it once, it is not that hard to write, and you can use the functional library Ramda to do so if you are proficient in functional programming. I did that a number of times in less time that it takes to find a library that does it for me. The pattern matching pattern occurs fairly often.

Bottom-line is, as a fellow open-source maintainer, I generally support keeping the surface API of a library low, and pusing to userland what can be done there. So I would tend to agree with @danieldietrich here.

[msujew]

Thank you @brucou for your input. I guess you are right, using a pattern matching method in there makes more sense than going through the trouble of building a dedicated visitor. I'll close this issue.