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

Add AST Visitor #1975

Merged
merged 1 commit into from
Aug 28, 2024
Merged

Add AST Visitor #1975

merged 1 commit into from
Aug 28, 2024

Conversation

Morriar
Copy link
Contributor

@Morriar Morriar commented Aug 22, 2024

The Visitor class implements the Visitor pattern for traversing the RBS Abstract Syntax Tree (AST).

It provides methods to visit each type of node in the AST, allowing for custom processing of each node type.

This class is designed to be subclassed, with specific visit methods overridden to implement custom behavior for different node types.

Example usage:

class MyVisitor < RBS::AST::Visitor
 def visit_declaration_class(node)
   puts "Visiting class: #{node.name}"

   super # call `super` to run the default visiting behavior
 end
end

visitor = MyVisitor.new
visitor.visit(ast_node)

I needed something like this and couldn't find anything in the repo other than the EnvironmentWalker that is way to specialized for my purpose.

@ParadoxV5
Copy link
Contributor

-1. This visitor is a bloat to RBS’s infrastructure.

❌ You need to update all visitors each time a class gets added to or removed from the element hierarchy.
https://refactoring.guru/design-patterns/visitor

An (not necessarily the) idiomatic design for Ruby is the simple enumerator.

# https://github.com/ruby/rbs/blob/v3.5.2/sig/parser.rbs#L67-L69
_buffer, _directives, declarations = RBS::Parser.parse_signature RBS_STRING
declarations.each do|node|
  case node
  when RBS::AST::Declarations::Class
    puts "Visiting class: #{node.name}"
    # node.each_member / each_decl …
  # …
  end
end

Not to mention that Ruby doesn’t have abstract classes.

Sometimes patterns are also used because of specific inflexibilities of e.g. Java ;-)
⸺ Lapizistik, https://ptb.discord.com/channels/518658712081268738/766466740069859349/1268121025195933801

@soutaro
Copy link
Member

soutaro commented Aug 23, 2024

I don't think providing a visitor is a bad idea, while I understand technically tree walking can be easily implemented with the pattern matching and recursive calls. I know there are some features that can be easily implemented on the top of visitor, without boring pattern matching enumeration.

One concern I have is requiring #accept methods in all of the AST classes. How about implementing the down-cast logics in the visitor class? I know it's different from the traditional visitor pattern, but it's possible and I'm curious if having the logic in one place makes more sense.

@soutaro soutaro added this to the RBS 3.6 milestone Aug 23, 2024
@ParadoxV5
Copy link
Contributor

How about implementing the down-cast logics in the visitor class?

This means the visitor class itself will be the boring pattern matching enumeration.

Co-authored-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Co-authored-by: Ufuk Kayserilioglu <ufuk.kayserilioglu@shopify.com>
@Morriar
Copy link
Contributor Author

Morriar commented Aug 27, 2024

@ParadoxV5

An (not necessarily the) idiomatic design for Ruby is the simple enumerator

As shown by your own example, the simple enumerator doesn't absolve you to write the burdensome visit logic hidden in your # node.each_member / each_decl … comment.

The goal of the visitor is exactly to not have you write this logic yourself and focus on what you want to do with the nodes.

Not to mention that Ruby doesn’t have abstract classes

Thank you for being so pedantic. By abstract I meant the base visitor doesn't provide anything more than the visit logic, it's up to the subclass to do something with each node.

This pattern tremendously simplifies the implementation of any tree walker and keeps things nicely organized in classes and methods rather than a long case..when statement. It's just a better practice.

@soutaro

How about implementing the down-cast logics in the visitor class?

Good idea, I moved the logic in Visitor#visit to avoid the need for an accept method in each node class.

@ParadoxV5
Copy link
Contributor

This pattern tremendously simplifies the implementation of any tree walker and keeps things nicely organized in classes and methods rather than a long case..when statement.

Fair. Not all tree walkers care about every class and method, but most probably do.

Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

Thanks! 🎉

@soutaro soutaro added this pull request to the merge queue Aug 28, 2024
Merged via the queue into ruby:master with commit 6e82f34 Aug 28, 2024
19 checks passed
@soutaro soutaro added the Released PRs already included in the released version label Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Released PRs already included in the released version
Development

Successfully merging this pull request may close these issues.

3 participants