Multi-level inheritance from Shoes::Widget fails

[atownley]

I'm trying to do something similar to the following with Policeman (1739) on MacOS:

#! /usr/bin/env open -a Shoes.app

class A < Shoes::Widget; end

class B < A
  def initialize(options = {}, &block)
    para "B"
  end
end

Shoes.app do
  b
end

Doing so gives me undefined method []=' for nil:NilClass in shoes.rb:541:ininherited'. The method in question looks like this:

535   class Types::Widget
536     @types = {}
537     def self.inherited subc
538       methc = subc.to_s[/(^|::)(\w+)$/, 2].
539               gsub(/([A-Z]+)([A-Z][a-z])/,'\1_\2').
540               gsub(/([a-z\d])([A-Z])/,'\1_\2').downcase
541       @types[methc] = subc
542       Shoes.class_eval %{
543         def #{methc}(*a, &b)
544           a.unshift Widget.instance_variable_get("@types")[#{methc.dump}]
545           widget(*a, &b)
546         end
547       }
548     end
549   end
550 end

From looking at it, I'm guessing that the reference to @types from a class method is causing the problem. I would've thought this was a bad idea. If you change the above code to the following:

535   class Types::Widget
536     @@types = {}
537     def self.inherited subc
538       methc = subc.to_s[/(^|::)(\w+)$/, 2].
539               gsub(/([A-Z]+)([A-Z][a-z])/,'\1_\2').
540               gsub(/([a-z\d])([A-Z])/,'\1_\2').downcase
541       @@types[methc] = subc
542       Shoes.class_eval %{
543         def #{methc}(*a, &b)
544 #          a.unshift Widget.instance_variable_get("@types")[#{methc.dump}]
545           a.unshift Widget.class_variable_get("@@types")[#{methc.dump}]
546           widget(*a, &b)
547         end
548       }
549     end
550   end

It seems to work as expected. I've never messed with this part of Ruby voodoo before, so I'm not sure if this is an acceptable fix or not. However, it seems to make the above test case actually work.

Looking forward to your thoughts.

[steveklabnik]

Cool!

Shoes Widgets are still very much a half a feature, and you're totally right, the reference to @types is the wrong thing. However, I hate class variables for a few reasons, so I'd totally accept this as a fix, but with a class instance variable instead. Can you give this a shot?

class << self; attr_accessor :types; end

and then just self.types inside the method.

If you're not familliar, http://martinfowler.com/bliki/ClassInstanceVariable.html

Do this in a pull request, and I'll totally merge it in.

[steveklabnik]

Actually, reviewing further, it already is a class instance variable:

rbx-head :007 > class A
rbx-head :008?>   @types = []
rbx-head :009?>   end
 => [] 
rbx-head :010 > A.new.instance_variables
 => [] 
rbx-head :011 > A.instance_variables
 => ["@types"] 
rbx-head :012 > A.class_variables
 => [] 

Soooooo hrm. Okay. Work this up as a pull request with the @@types and we'll see what everyone thinks.

[atownley]

I hadn't ever had an occasion to look into class instance variables before. I only use class variables extremely rarely, but it seems to me that what Widget is trying to do is exactly the case Fowler describes in the article about class variables:

Class variables, often referred to as static variables, are shared across all instances of a class. Every instance points to same value and any changes are seen by all.

It seems to me that in this case, what Widget needs is exactly to implement the Registry pattern across all instances of the Widget class, regardless of where they are in the hierarchy. Alternatively, it seems that it might be just a side effect of the instantiation order of derived classes by the interpreter. I don't know enough about the internals to comment further.

I'll send you a pull request later today, because I really need this for how I'm trying to use Shoes. How do you guys manage release schedules for the binaries? Is there a roadmap, or is it more ad-hoc than that?

Thanks for the feedback,

ast

[steveklabnik]

. I don't know enough about the internals to comment further.

Yeah, honestly, I just hate class variables with enough of a passion that I knee-jerk against them, because they can really get you into trouble. They are occasionally sometimes useful, though, and this may be one of those cases.

Is there a roadmap, or is it more ad-hoc than that?

It is currently more ad-hoc, but I would like it to become more regular. The biggest sticking point right now is windows. If I can solve the 1.9.2 issue, then we can get to stable releases, but right now, things are a little wonky, and it's impeding movement forward.

[atownley]

No worries. Totally understand both points.

Where's the best place to put the simple test case above? It doesn't really seem like a good sample, but I think it should be there somewhere to demonstrate the issue is actually resolved...

[steveklabnik]

Yes. We have a spec directory for rspec tests, and so it'd be great to have a test there.

I don't have time right this second to talk about how to turn it into a test, but play around with it and see if you can get something going. :)

[atownley]

Um, right. All I see in the spec directory is a set of tests for homebrew. I've no idea how to integrate a GUI test into rspec.

The only Ruby test frameworks I've used are RUnit and Testy. I pretty much use Testy for all my Ruby tests these days.

The reason for my question is that I don't see any Shoes-specific, GUI test cases anywhere. I guess I'm just blind... ;)

Also bear in mind that at the present time, I can't actually build Shoes on either of my machines, so anything that requires a full build just isn't going to happen for a while. Sorry. I'm already a week or so behind on what I'm trying to actually do with Shoes, so I have to ship something before I can go back and attack the build issues.

[steveklabnik]

No worries at all. I'm getting on a plane in a few hours, I'll cook something up. :D

guess I'm just blind

No, it's that it's hard, and the features directory doesn't have a whole lot in it yet. We're just getting around to writing tests, but this one doesn't actually need to use the GUI...

Never heard of testy, I'll check it out.

[wasnotrice]

This is strange to me. The point of using a class instance variable, it seems, is that the subtypes do not inherit the value (i.e. the registry is only in the inherited class). That sounds like "the right thing".

And it should work:

[1] pry(main)> class A
[1] pry(main)*   @types = {}
[1] pry(main)*   def self.inherited subc
[1] pry(main)*     @types[subc.name.downcase] = subc
[1] pry(main)*   end  
[1] pry(main)* end  
=> nil
[2] pry(main)> class B < A; end
=> nil
[3] pry(main)> B.new
=> #<B:0x007fd9691ef2b8>
[4] pry(main)> A.instance_variable_get :@types
=> {"b"=>B}
[5] pry(main)> B.instance_variable_get :@types
=> nil

Now, if we use a class variable:

[1] pry(main)> class A
[1] pry(main)*   @@types = {}  
[1] pry(main)*   def self.inherited subc  
[1] pry(main)*     @@types[subc.name.downcase] = subc    
[1] pry(main)*   end  
[1] pry(main)* end  
=> nil
[2] pry(main)> class B < A; end
=> nil
[3] pry(main)> B.new
=> #<B:0x007fcd88a8c750>
[4] pry(main)> A.class_variable_get :@@types
=> {"b"=>B}
[5] pry(main)> B.class_variable_get :@@types
=> {"b"=>B}

So I guess I'm not sure why it's not working as expected, but is using a class variable just masking the problem?

[steveklabnik]

Yeah, that's what I'm concerned about. I plan on looking at it on the plane I'm boarding in 3 hours. :)

[atownley]

First off, I don't understand what the big deal is here in this case. Why is it not the right thing to have Widget maintain a list of all the inherited types, regardless of the level of inheritance? The way that the above is coded assumes that Widget is the registry of all derived types. The class instance variable breaks this once you get into another derived type.

The above example from @wasnotrice doesn't illustrate the problem because it only has one level of inheritance.

This is an example that illustrates the problem:

$ cat /tmp/foo.rb
class A
  @types = {}
  def self.inherited subc
    @types[subc.name.downcase] = subc
  end
end

class B < A; end

class C < B
  def initialize
    puts "C"
  end
end

C.new

And when run:

$ ruby /tmp/foo.rb
/tmp/foo.rb:4:in `inherited': undefined method `[]=' for nil:NilClass (NoMethodError)
    from /tmp/foo.rb:10

Instrumenting it a bit further confirms what I suspected. The class instance variable idiom breaks here because you're defining the instance variable for class A - and only class A - yet when inheriting from class B, you're still executing the same #inherited method (due to inheritance), but the variable referenced in the base class method doesn't actually exist. Naturally, this is the definition of what class instance variables do, and this is why the idiom breaks in this case.

I understand you guys seem allergic to class variables, but I don't see any other way to solve this problem without rethinking the way that Widget manages the registry of helper methods.

Here's the modified test case:

$ cat /tmp/foo.rb
class A
  @types = {}
  def self.inherited subc
    puts "Subclass: #{subc}"
    @types[subc.name.downcase] = subc
  end
end

class B < A; end

class C < B
  def initialize
    puts "C"
  end
end

C.new

and the output:

$ ruby /tmp/foo.rb
Subclass: B
Subclass: C
/tmp/foo.rb:5:in `inherited': undefined method `[]=' for nil:NilClass (NoMethodError)
    from /tmp/foo.rb:11

Re @steveklabnik on tests, yes, I know GUI testing is hard. I spent about 7 years doing automated GUI testing on Windows, and it's still a nightmare on most platforms. While the issue itself doesn't need the GUI (as we've shown above), I wasn't aware that you could instantiate Widget instances without having the UI involved. If you can, then the test case becomes trivial. If you can't, then you need to ensure that the instance exists/or no exception was thrown.

Looking forward to see what you come up with, and have a good flight to wherever you're going! :)

[steveklabnik]

At the airport. yay free wifi.

So here's the deal, basically: This code was written by @why. Long ago. He's no longer with us. This feature was half written. Nobody has really touched this code or knows exactly how or why it works. Widgets were always a bit experimental.

So the combination of 1) code I'm not sure about 2) what other things it'll impact 3) a Ruby feature I dislike and 4) no way to ask the author means that I just want to be a biiiit cautious about it. Your code seems great, but I don't like to merge things I don't yet fully understand.

[wasnotrice]

@atownley thanks for illustrating why my example doesn't apply (and explaining why the class instance variable doesn't work). Your solutions seems right to me.

This issue really illustrates the problem with having no tests. We can't know whether this change breaks anything. @steveklabnik maybe the test case in the original issue here is a good model for getting some of these features under test.

[atownley]

No worries, guys. Not trying to be a pain, just need this to work given how I'm trying to use Shoes! ;)

Thought I'd also share a bit more analysis just in case anyone else wonders what's happening and why this is the right solution. Naturally, you could obviously fix the bug simply by following Fowler's example and ensuring that the variable was initialized if it didn't already exist, but I don't think that's useful in this case. Since the #inherited method is defining methods on the Shoes class dynamically at compile time based on the registry of all widgets to instantiate the derived class via a helper alias, the only way it can do this is by accessing a global registry of all Widget classes. Obviously, Widget is already a global object, so it's a good candidate for holding this registry.

If the class instance idiom were used, the reference to the derived subclass name when the method in Shoes was called would return a nil reference, and the mechanism would still break--just in a different place.

Here's another quick example:

$ cat /tmp/foo.rb
class A
  def self.inherited subc
    puts "Subclass: #{subc}"
    @types ||= {}
    @types[subc.name.downcase] = subc
    puts "@types: #{@types.inspect}"
  end

  def self.types
    @types
  end
end

class B < A; end

class C < B
  def initialize
    puts "C"
  end
end

C.new
puts "A @types: #{A.types.inspect}"
puts "B @types: #{B.types.inspect}"
puts "C @types: #{C.types.inspect}"

and the output:

$ ruby /tmp/foo.rb
Subclass: B
@types: {"b"=>B}
Subclass: C
@types: {"c"=>C}
C
A @types: {"b"=>B}
B @types: {"c"=>C}
C @types: nil

Again, perfectly in line with the expected behavior of class instance variables. The registry maintained by each type only contains references to the direct subtypes. Unfortunately, this isn't what the original code implies it really wants to do. It seems to want to create helpers for every Widget subclass so they're available just the same as the built-in methods. The only way to do this that I know is to have a registry of all subtypes and to centralize that registry somewhere convenient, like the root of the type tree.

Re @steveklabnik on Widget being experimental/half baked: I think there are potentially some other problems with this approach in larger projects with name collisions. If someone defines A::MyWiget and someone else defines B::MyWidget, I'm not actually sure which one you'll get when you call Shoes#my_widget. Maybe this approach will need to be flagged for a revisit at some stage.

I'm creating a few widgets in different namespaces, and it took me a while to figure out exactly what the relationship was between the magic helper method and the class names. Given the above potential conflict scenario, I'm not actually sure what the right answer is except to say "don't do that." Doesn't really seem like a robust solution... ;)