From 63e1c71c50af2499fc56fe9c30488f26116207a4 Mon Sep 17 00:00:00 2001
From: Jack Nagel <jacknagel@gmail.com>
Date: Tue, 22 Oct 2013 13:07:08 -0500
Subject: [PATCH] Eagerly initialize formula specs

Declarations of dependencies, options, and resources in the DSL only
apply to specs that have already been initialized. For example, given
this snippet:

  url ...
  sha1 ...

  depends_on 'foo'

  devel do
    url ...
    sha1 ...
  end

The dependency 'foo' will be recorded for the stable spec, but not the
devel spec, since it was not initialized prior to the call to
depends_on.

While it is considered best practice to declare all specs (stable,
devel, head, and bottle) prior to other declarations, there is nothing
that enforces this ordering, so when it happens it can be confusing and
hard to debug.

To prevent this, we can initialize all specs up front. This comes with
a performance penalty for commands that load all formulae into memory,
but that is probably outweighed by what we gain in correctness.

Fixes Homebrew/homebrew#23425.
---
 Library/Homebrew/formula.rb           | 33 ++++++++-------------------
 Library/Homebrew/test/test_formula.rb | 26 +++++++++++++++++++++
 2 files changed, 36 insertions(+), 23 deletions(-)

diff --git a/Library/Homebrew/formula.rb b/Library/Homebrew/formula.rb
index af585e3613..59f8155533 100644
--- a/Library/Homebrew/formula.rb
+++ b/Library/Homebrew/formula.rb
@@ -56,7 +56,6 @@ class Formula
 
   def set_spec(name)
     spec = self.class.send(name)
-    return if spec.nil?
     if block_given? && yield(spec) || !spec.url.nil?
       spec.owner = self
       instance_variable_set("@#{name}", spec)
@@ -656,69 +655,57 @@ class Formula
     attr_rw :plist_startup, :plist_manual
 
     def specs
-      @specs ||= []
-    end
-
-    def create_spec(klass)
-      spec = klass.new
-      specs << spec
-      spec
+      @specs ||= [stable, devel, head, bottle].freeze
     end
 
     def url val, specs={}
-      @stable ||= create_spec(SoftwareSpec)
-      @stable.url(val, specs)
+      stable.url(val, specs)
     end
 
     def version val=nil
-      @stable ||= create_spec(SoftwareSpec)
-      @stable.version(val)
+      stable.version(val)
     end
 
     def mirror val
-      @stable ||= create_spec(SoftwareSpec)
-      @stable.mirror(val)
+      stable.mirror(val)
     end
 
     Checksum::TYPES.each do |cksum|
       class_eval <<-EOS, __FILE__, __LINE__ + 1
         def #{cksum}(val)
-          @stable ||= create_spec(SoftwareSpec)
-          @stable.#{cksum}(val)
+          stable.#{cksum}(val)
         end
       EOS
     end
 
     def build
-      @stable ||= create_spec(SoftwareSpec)
-      @stable.build
+      stable.build
     end
 
     def stable &block
+      @stable ||= SoftwareSpec.new
       return @stable unless block_given?
-      @stable ||= create_spec(SoftwareSpec)
       @stable.instance_eval(&block)
     end
 
     def bottle *, &block
+      @bottle ||= Bottle.new
       return @bottle unless block_given?
-      @bottle ||= create_spec(Bottle)
       @bottle.instance_eval(&block)
       @bottle.version = @stable.version
     end
 
     def devel &block
+      @devel ||= SoftwareSpec.new
       return @devel unless block_given?
-      @devel ||= create_spec(SoftwareSpec)
       @devel.instance_eval(&block)
     end
 
     def head val=nil, specs={}, &block
+      @head ||= HeadSoftwareSpec.new
       if block_given?
-        @head ||= create_spec(HeadSoftwareSpec)
         @head.instance_eval(&block)
       elsif val
-        @head ||= create_spec(HeadSoftwareSpec)
         @head.url(val, specs)
       else
         @head
diff --git a/Library/Homebrew/test/test_formula.rb b/Library/Homebrew/test/test_formula.rb
index bc1e703eb2..00f929e4ba 100644
--- a/Library/Homebrew/test/test_formula.rb
+++ b/Library/Homebrew/test/test_formula.rb
@@ -212,4 +212,30 @@ class FormulaTests < Test::Unit::TestCase
   ensure
     path.unlink
   end
+
+  def test_class_specs_are_always_initialized
+    f = formula { url 'foo-1.0' }
+
+    %w{stable devel head bottle}.each do |spec|
+      assert_kind_of SoftwareSpec, f.class.send(spec)
+    end
+  end
+
+  def test_incomplete_instance_specs_are_not_accessible
+    f = formula { url 'foo-1.0' }
+
+    %w{devel head bottle}.each { |spec| assert_nil f.send(spec) }
+  end
+
+  def test_honors_attributes_declared_before_specs
+    f = formula do
+      url 'foo-1.0'
+      depends_on 'foo'
+      devel { url 'foo-1.1' }
+    end
+
+    %w{stable devel head bottle}.each do |spec|
+      assert_equal 'foo', f.class.send(spec).deps.first.name
+    end
+  end
 end