Tighten the backtrace pollution from passing through callbacks

Callbacks are everywhere, so it's better if we can avoid making a mess
of the backtrace just because we've passed through a callback hook.

I'm making no effort to the before/after invocations: those only affect
backtraces while they're running. The calls that matter are the ones
that remain on the call stack after run_callbacks yields: around
callbacks, and internal book-keeping around the before/afters.

activesupport/lib/active_support/callbacks.rb

@@ -63,6 +63,8 @@ module Callbacks
 
     included do
       extend ActiveSupport::DescendantsTracker
+      class_attribute :__callbacks, instance_writer: false
+      self.__callbacks ||= {}
     end
 
     CALLBACK_FILTER_TYPES = [:before, :after, :around]
@@ -86,22 +88,58 @@ module Callbacks
     #   run_callbacks :save do
     #     save
     #   end
-    def run_callbacks(kind, &block)
-      send "_run_#{kind}_callbacks", &block
-    end
-
-    private
+    #
+    #--
+    #
+    # As this method is used in many places, and often wraps large portions of
+    # user code, it has an additional design goal of minimizing its impact on
+    # the visible call stack. An exception from inside a :before or :after
+    # callback can be as noisy as it likes -- but when control has passed
+    # smoothly through and into the supplied block, we want as little evidence
+    # as possible that we were here.
+    def run_callbacks(kind)
+      callbacks = __callbacks[kind.to_sym]
 
-      def __run_callbacks__(callbacks, &block)
       if callbacks.empty?
         yield if block_given?
       else
-          runner = callbacks.compile
-          e = Filters::Environment.new(self, false, nil, block)
-          runner.call(e).value
+        env = Filters::Environment.new(self, false, nil)
+        next_sequence = callbacks.compile
+
+        invoke_sequence = Proc.new do
+          skipped = nil
+          while true
+            current, next_sequence = next_sequence, next_sequence.nested
+            current.invoke_before(env)
+            if current.final?
+              env.value = !env.halted && (!block_given? || yield)
+            elsif current.skip?(env)
+              (skipped ||= []) << current
+              next
+            else
+              expanded = current.expand_call_template(env, invoke_sequence)
+              expanded.shift.send(*expanded, &expanded.shift)
+            end
+            current.invoke_after(env)
+            skipped.pop.invoke_after(env) while skipped && skipped.first
+            break env.value
           end
         end
 
+        # Common case: no 'around' callbacks defined
+        if next_sequence.final?
+          next_sequence.invoke_before(env)
+          env.value = !env.halted && (!block_given? || yield)
+          next_sequence.invoke_after(env)
+          env.value
+        else
+          invoke_sequence.call
+        end
+      end
+    end
+
+    private
+
       # A hook invoked every time a before callback is halted.
       # This can be overridden in ActiveSupport::Callbacks implementors in order
       # to provide better debugging/logging.
@@ -118,16 +156,7 @@ def call(target, value); @block.call(value); end
       end
 
       module Filters
-        Environment = Struct.new(:target, :halted, :value, :run_block)
-
-        class End
-          def call(env)
-            block = env.run_block
-            env.value = !env.halted && (!block || block.call)
-            env
-          end
-        end
-        ENDING = End.new
+        Environment = Struct.new(:target, :halted, :value)
 
         class Before
           def self.build(callback_sequence, user_callback, user_conditions, chain_config, filter)
@@ -246,51 +275,6 @@ def self.simple(callback_sequence, user_callback)
           end
           private_class_method :simple
         end
-
-        class Around
-          def self.build(callback_sequence, user_callback, user_conditions, chain_config)
-            if user_conditions.any?
-              halting_and_conditional(callback_sequence, user_callback, user_conditions)
-            else
-              halting(callback_sequence, user_callback)
-            end
-          end
-
-          def self.halting_and_conditional(callback_sequence, user_callback, user_conditions)
-            callback_sequence.around do |env, &run|
-              target = env.target
-              value  = env.value
-              halted = env.halted
-
-              if !halted && user_conditions.all? { |c| c.call(target, value) }
-                user_callback.call(target, value) {
-                  run.call.value
-                }
-                env
-              else
-                run.call
-              end
-            end
-          end
-          private_class_method :halting_and_conditional
-
-          def self.halting(callback_sequence, user_callback)
-            callback_sequence.around do |env, &run|
-              target = env.target
-              value  = env.value
-
-              if env.halted
-                run.call
-              else
-                user_callback.call(target, value) {
-                  run.call.value
-                }
-                env
-              end
-            end
-          end
-          private_class_method :halting
-        end
       end
 
       class Callback #:nodoc:#
@@ -349,22 +333,95 @@ def duplicates?(other)
         # Wraps code with filter
         def apply(callback_sequence)
           user_conditions = conditions_lambdas
-          user_callback = make_lambda @filter
+          user_callback = CallTemplate.build(@filter, self)
 
           case kind
           when :before
-            Filters::Before.build(callback_sequence, user_callback, user_conditions, chain_config, @filter)
+            Filters::Before.build(callback_sequence, user_callback.make_lambda, user_conditions, chain_config, @filter)
           when :after
-            Filters::After.build(callback_sequence, user_callback, user_conditions, chain_config)
+            Filters::After.build(callback_sequence, user_callback.make_lambda, user_conditions, chain_config)
           when :around
-            Filters::Around.build(callback_sequence, user_callback, user_conditions, chain_config)
+            callback_sequence.around(user_callback, user_conditions)
           end
         end
 
+        def current_scopes
+          Array(chain_config[:scope]).map { |s| public_send(s) }
+        end
+
         private
+          def compute_identifier(filter)
+            case filter
+            when String, ::Proc
+              filter.object_id
+            else
+              filter
+            end
+          end
 
-          def invert_lambda(l)
-            lambda { |*args, &blk| !l.call(*args, &blk) }
+          def conditions_lambdas
+            @if.map { |c| CallTemplate.build(c, self).make_lambda } +
+              @unless.map { |c| CallTemplate.build(c, self).inverted_lambda }
+          end
+      end
+
+      # A future invocation of user-supplied code (either as a callback,
+      # or a condition filter).
+      class CallTemplate # :nodoc:
+        def initialize(target, method, arguments, block)
+          @override_target = target
+          @method_name = method
+          @arguments = arguments
+          @override_block = block
+        end
+
+        # Return the parts needed to make this call, with the given
+        # input values.
+        #
+        # Returns an array of the form:
+        #
+        #   [target, block, method, *arguments]
+        #
+        # This array can be used as such:
+        #
+        #   target.send(method, *arguments, &block)
+        #
+        # The actual invocation is left up to the caller to minimize
+        # call stack pollution.
+        def expand(target, value, block)
+          result = @arguments.map { |arg|
+            case arg
+            when :value; value
+            when :target; target
+            when :block; block || raise(ArgumentError)
+            end
+          }
+
+          result.unshift @method_name
+          result.unshift @override_block || block
+          result.unshift @override_target || target
+
+          # target, block, method, *arguments = result
+          # target.send(method, *arguments, &block)
+          result
+        end
+
+        # Return a lambda that will make this call when given the input
+        # values.
+        def make_lambda
+          lambda do |target, value, &block|
+            c = expand(target, value, block)
+            c.shift.send(*c, &c.shift)
+          end
+        end
+
+        # Return a lambda that will make this call when given the input
+        # values, but then return the boolean inverse of that result.
+        def inverted_lambda
+          lambda do |target, value, &block|
+            c = expand(target, value, block)
+            ! c.shift.send(*c, &c.shift)
+          end
         end
 
         # Filters support:
@@ -374,60 +431,45 @@ def invert_lambda(l)
         #   Procs::   A proc to call with the object.
         #   Objects:: An object with a <tt>before_foo</tt> method on it to call.
         #
-          # All of these objects are converted into a lambda and handled
+        # All of these objects are converted into a CallTemplate and handled
         # the same after this point.
-          def make_lambda(filter)
+        def self.build(filter, callback)
           case filter
           when Symbol
-              lambda { |target, _, &blk| target.send filter, &blk }
+            new(nil, filter, [], nil)
           when String
-              l = eval "lambda { |value| #{filter} }"
-              lambda { |target, value| target.instance_exec(value, &l) }
-            when Conditionals::Value then filter
+            new(nil, :instance_exec, [:value], compile_lambda(filter))
+          when Conditionals::Value
+            new(filter, :call, [:target, :value], nil)
           when ::Proc
             if filter.arity > 1
-                return lambda { |target, _, &block|
-                  raise ArgumentError unless block
-                  target.instance_exec(target, block, &filter)
-                }
-              end
-
-              if filter.arity <= 0
-                lambda { |target, _| target.instance_exec(&filter) }
+              new(nil, :instance_exec, [:target, :block], filter)
+            elsif filter.arity > 0
+              new(nil, :instance_exec, [:target], filter)
             else
-                lambda { |target, _| target.instance_exec(target, &filter) }
+              new(nil, :instance_exec, [], filter)
             end
           else
-              scopes = Array(chain_config[:scope])
-              method_to_call = scopes.map { |s| public_send(s) }.join("_")
+            method_to_call = callback.current_scopes.join("_")
 
-              lambda { |target, _, &blk|
-                filter.public_send method_to_call, target, &blk
-              }
+            new(filter, method_to_call, [:target], nil)
           end
         end
 
-          def compute_identifier(filter)
-            case filter
-            when String, ::Proc
-              filter.object_id
-            else
-              filter
-            end
-          end
-
-          def conditions_lambdas
-            @if.map { |c| make_lambda c } +
-              @unless.map { |c| invert_lambda make_lambda c }
+        def self.compile_lambda(filter)
+          eval("lambda { |value| #{filter} }")
         end
       end
 
       # Execute before and after filters in a sequence instead of
       # chaining them with nested lambda calls, see:
       # https://github.com/rails/rails/issues/18011
-      class CallbackSequence
-        def initialize(&call)
-          @call = call
+      class CallbackSequence # :nodoc:
+        def initialize(nested = nil, call_template = nil, user_conditions = nil)
+          @nested = nested
+          @call_template = call_template
+          @user_conditions = user_conditions
+
           @before = []
           @after = []
         end
@@ -442,19 +484,32 @@ def after(&after)
           self
         end
 
-        def around(&around)
-          CallbackSequence.new do |arg|
-            around.call(arg) {
-              call(arg)
-            }
+        def around(call_template, user_conditions)
+          CallbackSequence.new(self, call_template, user_conditions)
         end
+
+        def skip?(arg)
+          arg.halted || !@user_conditions.all? { |c| c.call(arg.target, arg.value) }
         end
 
-        def call(arg)
+        def nested
+          @nested
+        end
+
+        def final?
+          !@call_template
+        end
+
+        def expand_call_template(arg, block)
+          @call_template.expand(arg.target, arg.value, block)
+        end
+
+        def invoke_before(arg)
           @before.each { |b| b.call(arg) }
-          value = @call.call(arg)
+        end
+
+        def invoke_after(arg)
           @after.each { |a| a.call(arg) }
-          value
         end
       end
 
@@ -503,7 +558,7 @@ def initialize_copy(other)
 
         def compile
           @callbacks || @mutex.synchronize do
-            final_sequence = CallbackSequence.new { |env| Filters::ENDING.call(env) }
+            final_sequence = CallbackSequence.new
             @callbacks ||= @chain.reverse.inject(final_sequence) do |callback_sequence, callback|
               callback.apply callback_sequence
             end
@@ -747,12 +802,25 @@ def define_callbacks(*names)
           options = names.extract_options!
 
           names.each do |name|
-            class_attribute "_#{name}_callbacks", instance_writer: false
+            name = name.to_sym
+
             set_callbacks name, CallbackChain.new(name, options)
 
             module_eval <<-RUBY, __FILE__, __LINE__ + 1
               def _run_#{name}_callbacks(&block)
-                __run_callbacks__(_#{name}_callbacks, &block)
+                run_callbacks #{name.inspect}, &block
+              end
+
+              def self._#{name}_callbacks
+                get_callbacks(#{name.inspect})
+              end
+
+              def self._#{name}_callbacks=(value)
+                set_callbacks(#{name.inspect}, value)
+              end
+
+              def _#{name}_callbacks
+                __callbacks[#{name.inspect}]
               end
             RUBY
           end
@@ -761,11 +829,11 @@ def _run_#{name}_callbacks(&block)
         protected
 
           def get_callbacks(name) # :nodoc:
-            send "_#{name}_callbacks"
+            __callbacks[name.to_sym]
           end
 
           def set_callbacks(name, callbacks) # :nodoc:
-            send "_#{name}_callbacks=", callbacks
+            self.__callbacks = __callbacks.merge(name.to_sym => callbacks)
           end
 
           def deprecated_false_terminator # :nodoc:

activesupport/test/callbacks_test.rb

@@ -56,6 +56,8 @@ def self.after(model)
   end
 
   class Person < Record
+    attr_accessor :save_fails
+
     [:before_save, :after_save].each do |callback_method|
       callback_method_sym = callback_method.to_sym
       send(callback_method, callback_symbol(callback_method_sym))
@@ -67,7 +69,9 @@ class Person < Record
     end
 
     def save
-      run_callbacks :save
+      run_callbacks :save do
+        raise "inside save" if save_fails
+      end
     end
   end
 
@@ -222,6 +226,7 @@ class MySuper
 
   class AroundPerson < MySuper
     attr_reader :history
+    attr_accessor :save_fails
 
     set_callback :save, :before, :nope,           if: :no
     set_callback :save, :before, :nope,           unless: :yes
@@ -285,6 +290,7 @@ def initialize
 
     def save
       run_callbacks :save do
+        raise "inside save" if save_fails
         @history << "running"
       end
     end
@@ -402,6 +408,71 @@ def test_save_around
     end
   end
 
+  class CallStackTest < ActiveSupport::TestCase
+    def test_tidy_call_stack
+      around = AroundPerson.new
+      around.save_fails = true
+
+      exception = (around.save rescue $!)
+
+      # Make sure we have the exception we're expecting
+      assert_equal "inside save", exception.message
+
+      call_stack = exception.backtrace_locations
+      call_stack.pop caller_locations(0).size
+
+      # Yes, this looks like an implementation test, but it's the least
+      # obtuse way of asserting that there aren't a load of entries in
+      # the call stack for each callback.
+      #
+      # If you've renamed a method, or squeezed more lines out, go ahead
+      # and update this assertion. But if you're here because a
+      # refactoring added new lines, please reconsider.
+
+      # As shown here, our current budget is one line for run_callbacks
+      # itself, plus N+1 lines where N is the number of :around
+      # callbacks that have been invoked, if there are any (plus
+      # whatever the callbacks do themselves, of course).
+
+      assert_equal [
+        "block in save",
+        "block in run_callbacks",
+        "tweedle_deedle",
+        "block in run_callbacks",
+        "w0tyes",
+        "block in run_callbacks",
+        "tweedle_dum",
+        "block in run_callbacks",
+        ("call" if RUBY_VERSION < "2.3"),
+        "run_callbacks",
+        "save"
+      ].compact, call_stack.map(&:label)
+    end
+
+    def test_short_call_stack
+      person = Person.new
+      person.save_fails = true
+
+      exception = (person.save rescue $!)
+
+      # Make sure we have the exception we're expecting
+      assert_equal "inside save", exception.message
+
+      call_stack = exception.backtrace_locations
+      call_stack.pop caller_locations(0).size
+
+      # This budget much simpler: with no :around callbacks invoked,
+      # there should be just one line. run_callbacks yields directly
+      # back to its caller.
+
+      assert_equal [
+        "block in save",
+        "run_callbacks",
+        "save"
+      ], call_stack.map(&:label)
+    end
+  end
+
   class AroundCallbackResultTest < ActiveSupport::TestCase
     def test_save_around
       around = AroundPersonResult.new