Unnecessary exception raised in AS::Dependencies.load_missing_constant

[Ryan Kinderman]

There seems to be a bug in Rails dependency auto-loading logic that is
detailed here:

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/2283

Andrew White created the issue in March of 2009, and it continues to
be an issue despite recent Rails 3 commits. A number of patches have
been submitted, and there's a healthy discussion that has been going
on for a while, but we're sort of at a dead-end.

We need input from someone that is involved in or deeply
knowledgeable of the current and/or planned future state of the
ActiveSupport dependency loading logic + ActiveRecord type name
resolution logic.

Please, if you are "in the know", we'd love for you to take a look at
what we've found so far, and suggest how we should move forward, or
provide us with a sense of where things are going with the code
involved in fixing the bug.

Thanks

Ryan Kinderman
http://kinderman.net

[Yehuda Katz]

What I'd like to understand is what the various cases are that could trigger this branch. In particular, I'm interested in understanding the cases without reference to ActiveRecord (which it looks like you and others have started to do).

It seems reasonable to remove the exception, but I'd like to see what the original case was that caused the exception to be added in the first place. Presumably, it was added in order to catch a common (but infuriating) case of some kind.

Any thoughts?

Yehuda Katz
Developer | Engine Yard
(ph) 718.877.1325

--
You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group.
To post to this group, send email to rubyonra...@googlegroups.com.
To unsubscribe from this group, send email to rubyonrails-co...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en.

[George]

Perhaps Nick Seckar remembers: http://dev.rubyonrails.org/changeset/4779
Corresponding git commit: 2b37d59976268013b7e518e5af244947f688d315

Something to do with anonymous modules, I guess.  The commit message doesn't explain why the raise is necessary, though--tests run fine if you revert the change to the double-load test case, and remove the raise.

- George.

[Andrew White]

On 15 Feb 2010, at 01:32, Yehuda Katz wrote:

> What I'd like to understand is what the various cases are that could trigger this branch. In particular, I'm interested in understanding the cases without reference to ActiveRecord (which it looks like you and others have started to do).

Well googling for "is not missing constant" turns up a few hits:
http://www.google.com/search?q=%22is+not+missing+constant%22

The problem about trying to separate the issue from ActiveRecord is that AR is the going to be the vast majority of cases, however here's another situation where it arises:

class Admin::User < ActiveRecord::Base
def self.authenticate
first
end
end

class Admin::UsersController < ApplicationController

before_filter :authenticate

def index
@users = User.all
end

private

def authenticate
@current_user = User.authenticate
end

end

Now this can be worked around by specifying Admin::User instead of User but the fact that the authenticate before_filter works and then it fails at the User.all call is extremely confusing to anyone who doesn't understand the error. Without the AS::Dependencies magic you'd get a NameError as the User constant doesn't exist in the Admin::UsersController namespace. Either we need to not search upwards (not sure what that'd break) or we need to search upwards in a consistent manner.

Looking at it a bit deeper it seems as though there's a bit of confusion about what's the right thing to do within AS::Dependencies as the check at:

http://github.com/rails/rails/blob/master/activesupport/lib/active_support/dependencies.rb#L453

would seem to be trying to prevent returning constants defined in parents but the const_missing method catches the NameError and tries to load the constant from the parent anyway and in doing so triggers the "is not missing constant" exception.

> It seems reasonable to remove the exception, but I'd like to see what the original case was that caused the exception to be added in the first place. Presumably, it was added in order to catch a common (but infuriating) case of some kind.

Spent an hour digging through dev.rubyonrails.org to try and find a ticket that references changeset 4779 but came up blank, so we'll only get to hear the reason from Nick Seckar if he's still about. I did find ticket 6272 (http://dev.rubyonrails.org/ticket/6272) which has some more discussion about the issues. The ticket is marked as wontfix due to the fact that the nesting of Foo::Bar isn't always ["Foo::Bar", "Foo"], but your recent changes make that assumption so I don't think the resolution is valid. Also the fact that the raise can be triggered by errors elsewhere leading to hours of fruitless debugging needs to be addressed in some way I feel.

Andrew White

[Yehuda Katz]

Yehuda Katz
Developer | Engine Yard
(ph) 718.877.1325

I'll review this in the morning but I just want to point out that my changing only make the nesting EXPLICIT in the code. Rails has no way to determine the true nesting (I have an open ticket with ruby-core to address this issue in 1.9). My changes normalize the following two cases:

class Foo

  class Bar

    Baz # previously looked in "foo/bar/baz.rb" and "foo/baz.rb"

  end

end

module Foo

  module Bar

    Baz # previously looked in "foo/bar/baz.rb"

  end

end

I couldn't think of or find any reason for these cases to be different. My guess is that the behavior was added to classes at some point (due to STI needs) but never added to modules (since it never came up). In any event, there's no way to deal with:

module Foo

  module Bar

    Baz # should look in "foo/bar/baz.rb" and "foo/baz.rb"

  end

end

module Foo::Bar

  Baz # should look in "foo/bar/baz.rb"

end

unless the fix I proposed to ruby-core is accepted.

 

Andrew White

[Andrew White]

On 15 Feb 2010, at 01:32, Yehuda Katz wrote:

> What I'd like to understand is what the various cases are that could trigger this branch. In particular, I'm interested in understanding the cases without reference to ActiveRecord (which it looks like you and others have started to do).

Actually looking at it again this morning it's class_eval and the difference between class Foo::Bar; end and class Foo; class Bar; end; end. that seems to be the trigger, e.g:

>> module Foo; class Baz; end; end
=> nil
>> module Foo; class Bar; def self.baz; Baz; end; end; end
=> nil
>> Foo::Bar.baz
=> Foo::Baz
>> Foo::Bar.class_eval { Baz }
NameError: uninitialized constant Foo::Bar::Baz
from (irb):6:in `block in irb_binding'
from (irb):6:in `class_eval'
from (irb):6
from /Users/andyw/.rvm/rubies/ruby-1.9.1-p378/bin/irb:17:in `<main>'
>> module Foo; Bar.class_eval { Baz }; end
=> Foo::Baz

This is why it's mainly seen when dealing with ActiveRecord - the compute_type method in AR::Base seems to be the only place where this pattern is used. Basically, there's a mismatch between load_missing_constant and Ruby's constant resolution. Personally I'd consider this a bug in Ruby - I don't see any valid reason for the two different scopes.

Andrew White

[Andrew White]

On 15 Feb 2010, at 12:11, Yehuda Katz wrote:

> I couldn't think of or find any reason for these cases to be different. My guess is that the behavior was added to classes at some point (due to STI needs) but never added to modules (since it never came up). In any event, there's no way to deal with:

I agree - there's no valid reason that the lookup shouldn't always go foo/bar/baz.rb, foo/baz.rb & baz.rb - missing out the middle one seems illogical. I'd keep your normalisation changes even if your patch for Ruby was accepted. In fact your normalisation changes will make it more likely that the "is not missing constant" error is raised as we're checking for more places where it might be found, e.g:

Taking your Foo::Bar::Baz example:

>> "Foo::Bar::Baz".constantize
ArgumentError: Foo is not missing constant Baz!

>> "Foo::Bar".const_get("Baz")
ArgumentError: Foo is not missing constant Baz!

If we check for the presence of the constant before calling load_missing_constant then we prevent these errors. Okay it may be not the cleanest or the correct solution but the alternatives aren't great.

Andrew White

[Yehuda Katz]

Yehuda Katz
Developer | Engine Yard
(ph) 718.877.1325

On Mon, Feb 15, 2010 at 5:19 AM, Andrew White <an...@pixeltrix.co.uk> wrote:

On 15 Feb 2010, at 12:11, Yehuda Katz wrote:

> I couldn't think of or find any reason for these cases to be different. My guess is that the behavior was added to classes at some point (due to STI needs) but never added to modules (since it never came up). In any event, there's no way to deal with:

I agree - there's no valid reason that the lookup shouldn't always go foo/bar/baz.rb, foo/baz.rb & baz.rb - missing out the middle one seems illogical. I'd keep your normalisation changes even if your patch for Ruby was accepted.

I wouldn't. I consider Rails' lookup from disk to be an alternate form of constant lookup, and I'd want it to use semantics that are identical to Ruby's semantics:

class Foo::Bar

  Baz

  # normal Ruby: will search Foo::Bar::Baz and Object::Bar

  # rails: will search Foo::Bar::Baz, Foo::Baz, and Object::Baz

end

This discrepancy means that the behavior could be different depending on whether autoload is in effect (for instance, if all constants are preloaded, and autoload is turned off, the behavior would be different than when it was on). I'm not happy about that, and would remove the discrepancy if possible.

 

In fact your normalisation changes will make it more likely that the "is not missing constant" error is raised as we're checking for more places where it might be found, e.g:

Taking your Foo::Bar::Baz example:

>> "Foo::Bar::Baz".constantize
ArgumentError: Foo is not missing constant Baz!

>> "Foo::Bar".const_get("Baz")
ArgumentError: Foo is not missing constant Baz!

If we check for the presence of the constant before calling load_missing_constant then we prevent these errors. Okay it may be not the cleanest or the correct solution but the alternatives aren't great.

Andrew White

[Andrew White]

On 15 Feb 2010, at 13:31, Yehuda Katz wrote:

> I wouldn't. I consider Rails' lookup from disk to be an alternate form of constant lookup, and I'd want it to use semantics that are identical to Ruby's semantics:
>
> class Foo::Bar
> Baz
> # normal Ruby: will search Foo::Bar::Baz and Object::Bar
> # rails: will search Foo::Bar::Baz, Foo::Baz, and Object::Baz
> end
>
> This discrepancy means that the behavior could be different depending on whether autoload is in effect (for instance, if all constants are preloaded, and autoload is turned off, the behavior would be different than when it was on). I'm not happy about that, and would remove the discrepancy if possible.

Following the Ruby semantics will mean that this will never work:

class Admin::Account < ActiveRecord::Base
has_many :users
end

You'll always have to specify the class on the association:

class Admin::Account < ActiveRecord::Base
has_many :users, :class_name => "Admin::User"
end

My aesthetic sense rejects this as a solution :-)

The alternative is to implement a custom constant lookup system within ActiveRecord which we probably don't want to do.

Andrew White

[Yehuda Katz]

Yehuda Katz
Developer | Engine Yard
(ph) 718.877.1325

Actually that's precisely where it belongs. ActiveSupport::Dependencies should mirror the Ruby logic exactly (in other words, it should be possible to require all the files manually and have everything work). If Admin::Account has_many :users needs to be able to pull Admin::User, ActiveRecord should know how to do that.

[Andrew White]

On 15 Feb 2010, at 14:26, Yehuda Katz wrote:

> Actually that's precisely where it belongs. ActiveSupport::Dependencies should mirror the Ruby logic exactly (in other words, it should be possible to require all the files manually and have everything work). If Admin::Account has_many :users needs to be able to pull Admin::User, ActiveRecord should know how to do that.

Okay, I'm having a stab at rewriting compute_type but I've still got a few failing tests.
BTW, who thought it'd be a good idea to support relative nested constants for associations? e.g:

module MyApplication
module Billing
module Nested
class Firm < ActiveRecord::Base
end
end

class Account < ActiveRecord::Base
belongs_to :nested_unqualified_billing_firm,
:foreign_key => :firm_id,
:class_name => 'Nested::Firm'
end
end
end

Makes life interesting!

Andrew

[Andrew White]

On 15 Feb 2010, at 14:26, Yehuda Katz wrote:

> Actually that's precisely where it belongs. ActiveSupport::Dependencies should mirror the Ruby logic exactly (in other words, it should be possible to require all the files manually and have everything work). If Admin::Account has_many :users needs to be able to pull Admin::User, ActiveRecord should know how to do that.

Okay, I've patched compute_type so that the error doesn't get triggered:

https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/2283

Andrew White

[Ryan Kinderman]

Do you have a link to that proposed fix? I'd like to take a look at
the thread.

Thanks

Ryan Kinderman

> >http://github.com/rails/rails/blob/master/activesupport/lib/active_su...

> > rubyonrails-co...@googlegroups.com<rubyonrails-core%2Bunsubscrib e...@googlegroups.com>

[Ryan Kinderman]

On Feb 15, 6:11 am, Yehuda Katz <wyc...@gmail.com> wrote:

> I'll review this in the morning but I just want to point out that my
> changing only make the nesting EXPLICIT in the code. Rails has no way to
> determine the true nesting (I have an open ticket with ruby-core to address
> this issue in 1.9). My changes normalize the following two cases:

Sorry, my last reply didn't quote as I expected. Hopefully this is
clearer.

Yahuda, could you provide a link to the ruby-core ticket you
mentioned?

Thanks,

Ryan Kinderman