Unnecessary exception raised in AS::Dependencies.load_missing_constant

rubyonrails-core
1

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

2

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

3

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.
4

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="is+not+missing+constant"

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

5

Yehuda Katz Developer | Engine Yard (ph) 718.877.1325

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.

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.

6

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

7

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

8

Yehuda Katz Developer | Engine Yard (ph) 718.877.1325

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.

9

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 :slight_smile:

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

Andrew White

10

Yehuda Katz Developer | Engine Yard (ph) 718.877.1325

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 :slight_smile:

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

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.

11

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

12

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

13

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

Thanks

Ryan Kinderman

14

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