Bug #21193: Inherited callback returns `nil` for `Object.const_source_location`

• Status: Assigned
• Assignee: matz (Yukihiro Matsumoto)
• Target version: -
• Backport: 3.1: UNKNOWN, 3.2: UNKNOWN, 3.3: UNKNOWN, 3.4: UNKNOWN

[eileencodes (Eileen Uchitelle)]

Since https://github.com/ruby/ruby/pull/12927 the inherited callback no longer can access the Object.const_source_location. A simplified reproduction is the following:

class A
  def self.inherited(other)
    super
    p Object.const_source_location(other.name)
  end 
end

class B < A 
end

Prior to this change Object.const_source_location(other.name) would return ["test.rb", 8] and now returns nil.

Shopify's application uses the inherited callback to load configuration classes and since it's unable to find these classes they aren't loaded when they are called. It's not clear if this was intended or if there's a workaround. In chatting with folks at GitHub, they have a similar issue (although that case sounds less straightforward). A more accurate example of what we have (I did strip this down a bunch too):

def self.inherited(config)
  return super unless config.superclass == Config

  if (class_name = config.name) &&
    (config_location = Object.const_source_location(class_name)) &&
    (parent_dirname = File.dirname(config_location[0]))
    config_dirname = File.join(parent_dirname, "config")
      config.autoload(:Something, File.join(config_dirname, "something.rb"))
  end

  super
end

Original issue: https://bugs.ruby-lang.org/issues/21143

cc/ @byroot (Jean Boussier) @fxn

[byroot (Jean Boussier)]

• Assignee set to byroot (Jean Boussier)

[fxn (Xavier Noria)]

Eileen, I proposed to document the previous ordering (ruby#12759), but there was some speculative discussion in the associated ticket in Redmine (https://bugs.ruby-lang.org/issues/21143).

Could not provide a real use case for a dependency on the existing order, but I explained it could theoretically happen. Maybe this helps taking a final decision.

[Eregon (Benoit Daloze)]

• Related to Misc #21143: Speficy order of execution const_added vs inherited added

[byroot (Jean Boussier)]

I tried to manage both considerations in https://github.com/ruby/ruby/pull/12956.

[Eregon (Benoit Daloze)]

Probably worth trying:

1. set the superclass, without calling inherited
2. set the constant (which also names the module/class), without triggering const_added
3. inherited
4. const_added

If that's not compatible enough, then we'd have to go back to before #21143, which is the same except 3 and 4 are swapped.

[Eregon (Benoit Daloze)]

That's what your PR does, I reviewed it, LGTM.

[byroot (Jean Boussier)]

• Status changed from Open to Closed

Applied in changeset git|de097fbe5f3df105bd2a26e72db06b0f5139bc1a.

---

Trigger inherited and const_set callbacks after const has been defined

[Misc #21143]
[Bug #21193]

The previous change caused a backward compatibility issue with code
that called Object.const_source_location from the inherited callback.

To fix this, the order is now:

• Define the constant
• Invoke inherited
• Invoke const_set

[mame (Yusuke Endoh)]

• Status changed from Closed to Assigned
• Assignee changed from byroot (Jean Boussier) to matz (Yukihiro Matsumoto)

Hmmm, I think this handling is dirty. @matz (Yukihiro Matsumoto) said in #21143

But I don't want to break any existing code. I'd like to experiment inherited-then-const_added order works (or not).
If it works, I'd like to change the order. If it doesn't I'like to keep the current order (and document somewhere).

We actually found the real-world examples that are affected by the change. This means the change breaks existing code. I think we should revert the change and keep the previous order, instead of introducing dirty special handling.

At the least, we should ask matz to reconfirm this change before putting it in. @matz (Yukihiro Matsumoto) what do you think?

[Eregon (Benoit Daloze)]

mame (Yusuke Endoh) wrote in #note-8:

I think this handling is dirty

I think it's fine and not dirty, based on https://bugs.ruby-lang.org/issues/21193#note-5 it's clear that Ruby has always set the superclass separately from calling inherited.
So doing the same for setting the constant, i.e. setting it separately from calling const_added is not different.

I think the current state is the best: set the state internally (both superclass and constant) so when both hooks are called it's fully setup as expected and there is no "half-initialized state".

[fxn (Xavier Noria)]

Agree with @mame (Yusuke Endoh).

And, looking at that code from Shopify that showed up in nothing, I can imagine some other application loading with Zeitwerk and doing something like this

class AbstractConnection
  def self.inherited(base)
    base::Manager.configure # ConcreteConnection::Manager is autoloaded here.
  end
end

That works today, would not work with the new order.

[fxn (Xavier Noria)]

To clarify, when I said that I agree with @mame (Yusuke Endoh) I am personally not qualifying the current approach. Just saying the patches took a velocity and direction that does not seem to be aligned with what @matz (Yukihiro Matsumoto) said.

[fxn (Xavier Noria)]

I think the current state is the best: set the state internally (both superclass and constant) so when both hooks are called it's fully setup as expected and there is no "half-initialized state".

Let me stress this was already the case in the previous order, as I explained in the original ticket.

[mame (Yusuke Endoh)]

It was understandable that you wanted class C < D to behave the same as C = Class.new(D) for the sake of consistency.
However, the change proposed in this ticket intentionally differentiates these two behaviors, which seems to spoil the original motivation.
If we cannot make it consistent eventually, it is unclear to me why this order change was necessary.

If @matz (Yukihiro Matsumoto) prefers to change the order even in this halfway state, I don't mind, but we need to confirm.