Project

General

Profile

Actions

Bug #17007

closed

SystemStackError when using super inside Module included and lexically inside refinement

Added by Eregon (Benoit Daloze) over 4 years ago. Updated about 3 years ago.

Status:
Closed
Target version:
-
ruby -v:
ruby 2.7.1p83 (2020-03-31 revision a0c7c23c9c) [x86_64-linux]
[ruby-core:99044]

Description

class C
  def foo
    ["C"]
  end
end

refinement = Module.new do
  R = refine C do
    def foo
      ["R"] + super
    end

    include Module.new {
      def foo
        ["M"] + super
      end
    }
  end
end

using refinement
p C.new.foo

gives

$ ruby bug_refine_super.rb
Traceback (most recent call last):
	10920: from bug_refine_super.rb:22:in `<main>'
	10919: from bug_refine_super.rb:10:in `foo'
	10918: from bug_refine_super.rb:15:in `foo'
	10917: from bug_refine_super.rb:10:in `foo'
	10916: from bug_refine_super.rb:15:in `foo'
	10915: from bug_refine_super.rb:10:in `foo'
	10914: from bug_refine_super.rb:15:in `foo'
	10913: from bug_refine_super.rb:10:in `foo'
	 ... 10908 levels...
	    4: from bug_refine_super.rb:15:in `foo'
	    3: from bug_refine_super.rb:10:in `foo'
	    2: from bug_refine_super.rb:15:in `foo'
	    1: from bug_refine_super.rb:10:in `foo'
bug_refine_super.rb:15:in `foo': stack level too deep (SystemStackError)

OTOH defining the module lexically outside of refine works:

m = Module.new {
  def foo
    ["M"] + super
  end
}
refinement = Module.new do
  R = refine C do
    def foo
      ["R"] + super
    end

    include m
  end
end

# result: ["R", "M", "C"]

Related issues 2 (0 open2 closed)

Related to Ruby master - Bug #16977: Ambiguous lookup super for refinementsClosedActions
Related to Ruby master - Bug #17429: Prohibit include/prepend in refinement modulesClosedshugo (Shugo Maeda)Actions
Actions #1

Updated by Eregon (Benoit Daloze) over 4 years ago

  • Related to Bug #16977: Ambiguous lookup super for refinements added

Updated by jeremyevans0 (Jeremy Evans) over 4 years ago

After more analysis, I've determined that this issue is due to CREF handling in method lookup in search_refined_method. It fails in the lexical case because the module is affected by the refinement (refinement blocks have an implicit using of the refinement themselves, I guess).

You can reproduce the SystemStackError outside of the lexical case by having the refinement activated at the point of method definition:

class C
  def foo
    ["C"]
  end
end

module M; end

refinement = Module.new do
  R = refine C do
    def foo
      ["R"] + super
    end

    include M
  end
end

using refinement
M.define_method(:foo){["M"] + super()}
p C.new.foo

You can fix the issue by skipping any refined method during super lookup if the current method also uses the same refinement. I implemented this in a pull request: https://github.com/ruby/ruby/pull/3309.

Actions #3

Updated by jeremyevans (Jeremy Evans) over 4 years ago

  • Status changed from Open to Closed

Applied in changeset git|eeef16e190cdabc2ba474622720f8e3df7bac43b.


Prevent SystemStackError when calling super in module with activated refinement

Without this, if a refinement defines a method that calls super and
includes a module with a module that calls super and has a activated
refinement at the point super is called, the module method super call
will end up calling back into the refinement method, creating a loop.

Fixes [Bug #17007]

Updated by Eregon (Benoit Daloze) over 4 years ago

Thanks for the fix!
BTW this changes the behavior on a new spec, is that intended? (result is [:A, :C] instead of [:A, :LOCAL, :C] on < 2.8)
https://github.com/ruby/spec/commit/b0da11b52560860e844470d145acee0ff4d4acea?w=1

Updated by jeremyevans0 (Jeremy Evans) over 4 years ago

Eregon (Benoit Daloze) wrote in #note-4:

Thanks for the fix!
BTW this changes the behavior on a new spec, is that intended? (result is [:A, :C] instead of [:A, :LOCAL, :C] on < 2.8)
https://github.com/ruby/spec/commit/b0da11b52560860e844470d145acee0ff4d4acea?w=1

It's a consequence of the fix (skips the currently activated refinement during super), but I wouldn't say it is intended. Ideally, the fix would only affect looping cases. Unfortunately, I'm not sure if there is a better way to detect whether looping during super would occur. I'm also not sure whether the approach I committed can detect all cases of looping (there may be other ways to introduce looping during super).

Maybe the refinements spec needs to be changed if we want to forbid looping, spelling out how to handle super in modules included in refinements where the refinements are activated at the point of super call.

Updated by jeremyevans0 (Jeremy Evans) about 4 years ago

  • Status changed from Closed to Assigned

To restore compatibility with Ruby 2.7 and fix #17182, I've reverted eeef16e190cdabc2ba474622720f8e3df7bac43b at 179384a66862d5ef7413b6f4850b97d0becf4ec9.

@shugo (Shugo Maeda), I would appreciate your input here on how refinements should be handled in this case.

Updated by shugo (Shugo Maeda) almost 4 years ago

jeremyevans0 (Jeremy Evans) wrote in #note-6:

To restore compatibility with Ruby 2.7 and fix #17182, I've reverted eeef16e190cdabc2ba474622720f8e3df7bac43b at 179384a66862d5ef7413b6f4850b97d0becf4ec9.

@shugo (Shugo Maeda), I would appreciate your input here on how refinements should be handled in this case.

I think the change is acceptable, but it may be better to prohibit include/prepend in refinement modules in future versions to avoid such implementation difficulties.

Actions #8

Updated by jeremyevans0 (Jeremy Evans) almost 4 years ago

  • Related to Bug #17429: Prohibit include/prepend in refinement modules added

Updated by jeremyevans0 (Jeremy Evans) about 3 years ago

  • Status changed from Assigned to Closed

Refinement#include is now deprecated and will be removed in Ruby 3.2.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0