Skip to content

Consider ZSUPER methods as separate methods for Method/UnboundMethod - #5802

Closed
jeremyevans wants to merge 1 commit into
ruby:masterfrom
jeremyevans:zsuper-owner-super-method-18729
Closed

Consider ZSUPER methods as separate methods for Method/UnboundMethod#5802
jeremyevans wants to merge 1 commit into
ruby:masterfrom
jeremyevans:zsuper-owner-super-method-18729

Conversation

@jeremyevans

Copy link
Copy Markdown
Contributor

Previously, Method/UnboundMethod basically skipped ZSUPER methods,
using the super method instead. The most visible indication of
this is that owner would return method referenced by the ZSUPER
method, instead of the class containing the ZSUPER method. It
also had the result that calling super_method on the method
resulted in the super method of the method referenced by ZSUPER.

Fix this by tracking the ZSUPER class, and using it as the owner.
Additionally, if super_method is called on a ZSUPER method, use
the existing method entry (which already points to ZSUPER's super
method), instead of looking for the super method of that method
entry.

This has the effect of making a ZSUPER method treated more similarly
to a regularly defined method that calls super.

Fixes [Bug #18729]

Previously, Method/UnboundMethod basically skipped ZSUPER methods,
using the super method instead.  The most visible indication of
this is that owner would return method referenced by the ZSUPER
method, instead of the class containing the ZSUPER method.  It
also had the result that calling super_method on the method
resulted in the super method of the method referenced by ZSUPER.

Fix this by tracking the ZSUPER class, and using it as the owner.
Additionally, if super_method is called on a ZSUPER method, use
the existing method entry (which already points to ZSUPER's super
method), instead of looking for the super method of that method
entry.

This has the effect of making a ZSUPER method treated more similarly
to a regularly defined method that calls super.

Fixes [Bug #18729]
Comment thread test/ruby/test_method.rb
mod = EnvUtil.labeled_module("Mod") {private def foo; :ok end}
mods = [mod]
obj = Object.new.extend(mod)
mods = [obj.singleton_class, mod]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting, so now the expected result is [sclass, Mod, Mod0, Mod1], which is what would be the chain if super was caller in each of these methods, so that makes a lot of sense.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's actually [Mod0, Mod1, sclass, Mod], since Mod0 and Mod1 are prepended to the singleton class.

@eregon eregon Apr 16, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, on a similar simplified example, calling these methods means only the public foo is called, and the private one is skipped:

class C
  def foo
    puts caller(0), nil
    ['C']
  end
end

module Mod
  private def foo
    ['.'] + super
  end
end

obj = C.new
obj.extend(Mod)

class << obj
  public :foo
end

p obj.foo

gives (on 3.0.3)

public.rb:3:in `foo'
public.rb:10:in `foo'
public.rb:21:in `<main>'

[".", "C"]

That's I think because super doesn't consider the owner module but instead the "declaring module" (the one that actually def it, I think that's defined_class in CRuby terminology), which for both sclass#foo and Mod#foo is Mod.
And there is a similar cases with aliases IIRC.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So I think ideally super_method wouldn't change here, and the result of this test would be [Mod0, Mod1, sclass].
That's what actually gets called if super is used, and it's also the result of this test on TruffleRuby.

eregon
eregon previously approved these changes Apr 15, 2022

@eregon eregon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great to me.
I'm not familiar with the module chain details on CRuby though, so if you'd like a review of that it seems best to ask someone else.

@jeremyevans
jeremyevans requested a review from nobu April 22, 2022 16:10
@eregon
eregon dismissed their stale review April 23, 2022 11:34

See comment on ticket

@jeremyevans jeremyevans closed this Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants