Merge pull request #270 from ruby-syntax-tree/visit-methods

BasicVisitor::visit_methods

Author: kddnewton

README.md

@@ -40,6 +40,7 @@ It is built with only standard library dependencies. It additionally ships with
   - [construct_keys](#construct_keys)
 - [Visitor](#visitor)
   - [visit_method](#visit_method)
+  - [visit_methods](#visit_methods)
   - [BasicVisitor](#basicvisitor)
   - [MutationVisitor](#mutationvisitor)
   - [WithEnvironment](#withenvironment)
@@ -517,6 +518,26 @@ Did you mean?  visit_binary
 	from bin/console:8:in `<main>'
 ```
 
+### visit_methods
+
+Similar to `visit_method`, `visit_methods` also checks that methods defined are valid visit methods. This variation however accepts a block and checks that all methods defined within that block are valid visit methods. It's meant to be used like:
+
+```ruby
+class ArithmeticVisitor < SyntaxTree::Visitor
+  visit_methods do
+    def visit_binary(node)
+      # ...
+    end
+
+    def visit_int(node)
+      # ...
+    end
+  end
+end
+```
+
+This is only checked when the methods are defined and does not impose any kind of runtime overhead after that. It is very useful for upgrading versions of Syntax Tree in case these methods names change.
+
 ### BasicVisitor
 
 When you're defining your own visitor, by default it will walk down the tree even if you don't define `visit_*` methods. This is to ensure you can define a subset of the necessary methods in order to only interact with the nodes you're interested in. If you'd like to change this default to instead raise an error if you visit a node you haven't explicitly handled, you can instead inherit from `BasicVisitor`.

lib/syntax_tree/basic_visitor.rb

@@ -29,7 +29,7 @@ def initialize(error)
       def corrections
         @corrections ||=
           DidYouMean::SpellChecker.new(
-            dictionary: Visitor.visit_methods
+            dictionary: BasicVisitor.valid_visit_methods
           ).correct(visit_method)
       end
 
@@ -40,7 +40,40 @@ def corrections
       end
     end
 
+    # This module is responsible for checking all of the methods defined within
+    # a given block to ensure that they are valid visit methods.
+    class VisitMethodsChecker < Module
+      Status = Struct.new(:checking)
+
+      # This is the status of the checker. It's used to determine whether or not
+      # we should be checking the methods that are defined. It is kept as an
+      # instance variable so that it can be disabled later.
+      attr_reader :status
+
+      def initialize
+        # We need the status to be an instance variable so that it can be
+        # accessed by the disable! method, but also a local variable so that it
+        # can be captured by the define_method block.
+        status = @status = Status.new(true)
+
+        define_method(:method_added) do |name|
+          BasicVisitor.visit_method(name) if status.checking
+          super(name)
+        end
+      end
+
+      def disable!
+        status.checking = false
+      end
+    end
+
     class << self
+      # This is the list of all of the valid visit methods.
+      def valid_visit_methods
+        @valid_visit_methods ||=
+          Visitor.instance_methods.grep(/^visit_(?!child_nodes)/)
+      end
+
       # This method is here to help folks write visitors.
       #
       # It's not always easy to ensure you're writing the correct method name in
@@ -51,15 +84,21 @@ class << self
       # name. It will raise an error if the visit method you're defining isn't
       # actually a method on the parent visitor.
       def visit_method(method_name)
-        return if visit_methods.include?(method_name)
+        return if valid_visit_methods.include?(method_name)
 
         raise VisitMethodError, method_name
       end
 
-      # This is the list of all of the valid visit methods.
+      # This method is here to help folks write visitors.
+      #
+      # Within the given block, every method that is defined will be checked to
+      # ensure it's a valid visit method using the BasicVisitor::visit_method
+      # method defined above.
       def visit_methods
-        @visit_methods ||=
-          Visitor.instance_methods.grep(/^visit_(?!child_nodes)/)
+        checker = VisitMethodsChecker.new
+        extend(checker)
+        yield
+        checker.disable!
       end
     end
 

test/visitor_test.rb

@@ -53,5 +53,19 @@ def test_visit_method_correction
         assert_match(/visit_binary/, message)
       end
     end
+
+    class VisitMethodsTestVisitor < BasicVisitor
+    end
+
+    def test_visit_methods
+      VisitMethodsTestVisitor.visit_methods do
+        assert_raises(BasicVisitor::VisitMethodError) do
+          # In reality, this would be a method defined using the def keyword,
+          # but we're using method_added here to trigger the checker so that we
+          # aren't defining methods dynamically in the test suite.
+          VisitMethodsTestVisitor.method_added(:visit_foo)
+        end
+      end
+    end
   end
 end