Towards 5027 -- add options hash to lookupvar as with setvar

This patch adds an options hash to lookupvar analogous to the one taken by
setvar and uses it to pass in source location for error reporting.  It also
fixes the mechanism used by setvar (file was not being passed correctly), adds
line and file information to errors in templates, and extends/corrects tests.

As presently written it does not gather userful line numbers from inline
templates and there are no tests for the template line number generation.

9 files changed, 66 insertions, 59 deletions

diff --git a/lib/puppet/parser/ast/leaf.rb b/lib/puppet/parser/ast/leaf.rb
index b61634d6c..c8ebc9483 100644
--- a/lib/puppet/parser/ast/leaf.rb
+++ b/lib/puppet/parser/ast/leaf.rb
@@ -124,7 +124,7 @@ class Puppet::Parser::AST
     # not include syntactical constructs, like '$' and '{}').
     def evaluate(scope)
       parsewrap do
-        if (var = scope.lookupvar(@value)) == :undefined
+        if (var = scope.lookupvar(@value, :file => file, :line => line)) == :undefined
           var = :undef
         end
         var
@@ -141,7 +141,7 @@ class Puppet::Parser::AST
 
     def evaluate_container(scope)
       container = variable.respond_to?(:evaluate) ? variable.safeevaluate(scope) : variable
-      (container.is_a?(Hash) or container.is_a?(Array)) ? container : scope.lookupvar(container)
+      (container.is_a?(Hash) or container.is_a?(Array)) ? container : scope.lookupvar(container, :file => file, :line => line)
     end
 
     def evaluate_key(scope)
diff --git a/lib/puppet/parser/ast/vardef.rb b/lib/puppet/parser/ast/vardef.rb
index 6de1860c8..b766311dd 100644
--- a/lib/puppet/parser/ast/vardef.rb
+++ b/lib/puppet/parser/ast/vardef.rb
@@ -20,7 +20,7 @@ class Puppet::Parser::AST
         name = @name.safeevaluate(scope)
 
         parsewrap do
-          scope.setvar(name,value, :file => @file, :line => @line, :append => @append)
+          scope.setvar(name,value, :file => file, :line => line, :append => @append)
         end
       end
     end
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index 95b765b52..a61a8578f 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -223,21 +223,22 @@ class Puppet::Parser::Scope
   private :qualified_scope
 
   # Look up a variable.  The simplest value search we do.  
-  def lookupvar(name)
+  def lookupvar(name, options = {})
     table = ephemeral?(name) ? @ephemeral.last : @symtable
     # If the variable is qualified, then find the specified scope and look the variable up there instead.
     if name =~ /^(.*)::(.+)$/
       begin 
-        qualified_scope($1).lookupvar($2)
+        qualified_scope($1).lookupvar($2,options)
       rescue RuntimeError => e
-        warning "Could not look up qualified variable '#{name}'; #{e.message}"
+        location = (options[:file] && options[:line]) ? " at #{options[:file]}:#{options[:line]}" : ''
+        warning "Could not look up qualified variable '#{name}'; #{e.message}#{location}"
         :undefined
       end
     elsif ephemeral_include?(name) or table.include?(name)
       # We can't use "if table[name]" here because the value might be false
       table[name]
     elsif parent
-      parent.lookupvar(name)
+      parent.lookupvar(name,options)
     else
       :undefined
     end
diff --git a/lib/puppet/parser/templatewrapper.rb b/lib/puppet/parser/templatewrapper.rb
index 73fcb8aac..180a03dc9 100644
--- a/lib/puppet/parser/templatewrapper.rb
+++ b/lib/puppet/parser/templatewrapper.rb
@@ -18,9 +18,14 @@ class Puppet::Parser::TemplateWrapper
     @__scope__
   end
 
+  def script_line
+    # find which line in the template (if any) we were called from
+    caller.find { |l| l =~ /#{file}:/ }.first[/:(\d+):/,1]
+  end
+
   # Should return true if a variable is defined, false if it is not
   def has_variable?(name)
-    scope.lookupvar(name.to_s) != :undefined
+    scope.lookupvar(name.to_s, :file => file, :line => script_line) != :undefined
   end
 
   # Allow templates to access the defined classes
@@ -51,13 +56,13 @@ class Puppet::Parser::TemplateWrapper
   # the missing_method definition here until we declare the syntax finally
   # dead.
   def method_missing(name, *args)
-    value = scope.lookupvar(name.to_s)
+    value = scope.lookupvar(name.to_s,:file => file,:line => script_line)
     if value != :undefined
       return value
     else
       # Just throw an error immediately, instead of searching for
       # other missingmethod things or whatever.
-      raise Puppet::ParseError, "Could not find value for '#{name}'"
+      raise Puppet::ParseError.new("Could not find value for '#{name}'",@file,script_line)
     end
   end
 
@@ -97,6 +102,7 @@ class Puppet::Parser::TemplateWrapper
     result = nil
     benchmark(:debug, "Interpolated template #{template_source}") do
       template = ERB.new(self.string, 0, "-")
+      template.filename = file
       result = template.result(binding)
     end
 
diff --git a/spec/unit/parser/ast/arithmetic_operator_spec.rb b/spec/unit/parser/ast/arithmetic_operator_spec.rb
index 0aab6f080..9b54f09d8 100755
--- a/spec/unit/parser/ast/arithmetic_operator_spec.rb
+++ b/spec/unit/parser/ast/arithmetic_operator_spec.rb
@@ -60,14 +60,4 @@ describe Puppet::Parser::AST::ArithmeticOperator do
     operator.evaluate(@scope).should == 4.33
   end
 
-  it "should work for variables too" do
-    @scope.expects(:lookupvar).with("one").returns(1)
-    @scope.expects(:lookupvar).with("two").returns(2)
-    one = ast::Variable.new( :value => "one" )
-    two = ast::Variable.new( :value => "two" )
-
-    operator = ast::ArithmeticOperator.new :lval => one, :operator => "+", :rval => two
-    operator.evaluate(@scope).should == 3
-  end
-
 end
diff --git a/spec/unit/parser/ast/comparison_operator_spec.rb b/spec/unit/parser/ast/comparison_operator_spec.rb
index 3efe28bb6..060827390 100755
--- a/spec/unit/parser/ast/comparison_operator_spec.rb
+++ b/spec/unit/parser/ast/comparison_operator_spec.rb
@@ -95,8 +95,8 @@ describe Puppet::Parser::AST::ComparisonOperator do
     one = Puppet::Parser::AST::Variable.new( :value => "one" )
     two = Puppet::Parser::AST::Variable.new( :value => "two" )
 
-    @scope.expects(:lookupvar).with("one").returns(1)
-    @scope.expects(:lookupvar).with("two").returns(2)
+    one.expects(:safeevaluate).with(@scope).returns(1)
+    two.expects(:safeevaluate).with(@scope).returns(2)
 
     operator = Puppet::Parser::AST::ComparisonOperator.new :lval => one, :operator => "<", :rval => two
     operator.evaluate(@scope).should == true
diff --git a/spec/unit/parser/ast/leaf_spec.rb b/spec/unit/parser/ast/leaf_spec.rb
index 97c996b40..64fdcd659 100755
--- a/spec/unit/parser/ast/leaf_spec.rb
+++ b/spec/unit/parser/ast/leaf_spec.rb
@@ -107,7 +107,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do
 
   describe "when evaluating" do
     it "should evaluate the variable part if necessary" do
-      @scope.stubs(:lookupvar).with("a").returns(["b"])
+      @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns(["b"])
 
       variable = stub 'variable', :evaluate => "a"
       access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => variable, :key => 0 )
@@ -118,7 +118,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do
     end
 
     it "should evaluate the access key part if necessary" do
-      @scope.stubs(:lookupvar).with("a").returns(["b"])
+      @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns(["b"])
 
       index = stub 'index', :evaluate => 0
       access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => index )
@@ -129,7 +129,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do
     end
 
     it "should be able to return an array member" do
-      @scope.stubs(:lookupvar).with("a").returns(["val1", "val2", "val3"])
+      @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns(["val1", "val2", "val3"])
 
       access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => 1 )
 
@@ -153,7 +153,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do
     end
 
     it "should be able to return an hash value" do
-      @scope.stubs(:lookupvar).with("a").returns({ "key1" => "val1", "key2" => "val2", "key3" => "val3" })
+      @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns({ "key1" => "val1", "key2" => "val2", "key3" => "val3" })
 
       access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key2" )
 
@@ -177,7 +177,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do
     end
 
     it "should raise an error if the variable wasn't in the scope" do
-      @scope.stubs(:lookupvar).with("a").returns(nil)
+      @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns(nil)
 
       access = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key2" )
 
@@ -190,7 +190,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do
     end
 
     it "should work with recursive hash access" do
-      @scope.stubs(:lookupvar).with("a").returns({ "key" => { "subkey" => "b" }})
+      @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns({ "key" => { "subkey" => "b" }})
 
       access1 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key")
       access2 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => access1, :key => "subkey")
@@ -199,7 +199,7 @@ describe Puppet::Parser::AST::HashOrArrayAccess do
     end
 
     it "should work with interleaved array and hash access" do
-      @scope.stubs(:lookupvar).with("a").returns({ "key" => [ "a" , "b" ]})
+      @scope.stubs(:lookupvar).with { |name,options| name == 'a'}.returns({ "key" => [ "a" , "b" ]})
 
       access1 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => "a", :key => "key")
       access2 = Puppet::Parser::AST::HashOrArrayAccess.new(:variable => access1, :key => 1)
@@ -332,16 +332,21 @@ end
 describe Puppet::Parser::AST::Variable do
   before :each do
     @scope = stub 'scope'
-    @var = Puppet::Parser::AST::Variable.new(:value => "myvar")
+    @var = Puppet::Parser::AST::Variable.new(:value => "myvar", :file => 'my.pp', :line => 222)
   end
 
   it "should lookup the variable in scope" do
-    @scope.expects(:lookupvar).with("myvar").returns(:myvalue)
+    @scope.expects(:lookupvar).with { |name,options| name == "myvar" }.returns(:myvalue)
+    @var.safeevaluate(@scope).should == :myvalue
+  end
+
+  it "should pass the source location to lookupvar" do
+    @scope.expects(:lookupvar).with { |name,options| name == "myvar" and options[:file] == 'my.pp' and options[:line] == 222 }.returns(:myvalue)
     @var.safeevaluate(@scope).should == :myvalue
   end
 
   it "should return undef if the variable wasn't set" do
-    @scope.expects(:lookupvar).with("myvar").returns(:undefined)
+    @scope.expects(:lookupvar).with { |name,options| name == "myvar" }.returns(:undefined)
     @var.safeevaluate(@scope).should == :undef
   end
 
diff --git a/spec/unit/parser/ast/vardef_spec.rb b/spec/unit/parser/ast/vardef_spec.rb
index a462e8ef5..5a851bb13 100755
--- a/spec/unit/parser/ast/vardef_spec.rb
+++ b/spec/unit/parser/ast/vardef_spec.rb
@@ -16,8 +16,7 @@ describe Puppet::Parser::AST::VarDef do
       name.expects(:safeevaluate).with(@scope)
       value.expects(:safeevaluate).with(@scope)
 
-      vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil,
-        :line => nil
+      vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil, :line => nil
       vardef.evaluate(@scope)
     end
 
@@ -27,8 +26,7 @@ describe Puppet::Parser::AST::VarDef do
 
       @scope.expects(:setvar).with { |name,value,options| options[:append] == nil }
 
-      vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil,
-        :line => nil
+      vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil, :line => nil
       vardef.evaluate(@scope)
     end
 
@@ -38,8 +36,17 @@ describe Puppet::Parser::AST::VarDef do
 
       @scope.expects(:setvar).with { |name,value,options| options[:append] == true }
 
-      vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil,
-        :line => nil, :append => true
+      vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => nil, :line => nil, :append => true
+      vardef.evaluate(@scope)
+    end
+
+    it "should call pass the source location to setvar" do
+      name = stub 'name', :safeevaluate => "var"
+      value = stub 'value', :safeevaluate => "1"
+
+      @scope.expects(:setvar).with { |name,value,options| options[:file] == 'setvar.pp' and options[:line] == 917 }
+
+      vardef = Puppet::Parser::AST::VarDef.new :name => name, :value => value, :file => 'setvar.pp', :line => 917
       vardef.evaluate(@scope)
     end
 
diff --git a/spec/unit/parser/templatewrapper_spec.rb b/spec/unit/parser/templatewrapper_spec.rb
index 4a713b8a8..0c7199ba1 100755
--- a/spec/unit/parser/templatewrapper_spec.rb
+++ b/spec/unit/parser/templatewrapper_spec.rb
@@ -17,6 +17,12 @@ describe Puppet::Parser::TemplateWrapper do
     @tw = Puppet::Parser::TemplateWrapper.new(@scope)
   end
 
+  def mock_template(source=nil)
+    template_mock = mock("template", :result => "woot!")
+    ERB.expects(:new).with("template contents", 0, "-").returns(template_mock)
+    template_mock.expects(:filename=).with(source)
+  end
+
   it "should create a new object TemplateWrapper from a scope" do
     tw = Puppet::Parser::TemplateWrapper.new(@scope)
 
@@ -54,41 +60,38 @@ describe Puppet::Parser::TemplateWrapper do
   end
 
   it "should return the processed template contents with a call to result" do
-    template_mock = mock("template", :result => "woot!")
+    mock_template("/tmp/fake_template")
     File.expects(:read).with("/tmp/fake_template").returns("template contents")
-    ERB.expects(:new).with("template contents", 0, "-").returns(template_mock)
 
     @tw.file = @file
     @tw.result.should eql("woot!")
   end
 
   it "should return the processed template contents with a call to result and a string" do
-    template_mock = mock("template", :result => "woot!")
-    ERB.expects(:new).with("template contents", 0, "-").returns(template_mock)
-
+    mock_template
     @tw.result("template contents").should eql("woot!")
   end
 
   it "should return the contents of a variable if called via method_missing" do
-    @scope.expects(:lookupvar).with("chicken").returns("is good")
+    @scope.expects(:lookupvar).with { |name,options| name == "chicken"}.returns("is good")
     tw = Puppet::Parser::TemplateWrapper.new(@scope)
     tw.chicken.should eql("is good")
   end
 
   it "should throw an exception if a variable is called via method_missing and it does not exist" do
-    @scope.expects(:lookupvar).with("chicken").returns(:undefined)
+    @scope.expects(:lookupvar).with { |name,options| name == "chicken"}.returns(:undefined)
     tw = Puppet::Parser::TemplateWrapper.new(@scope)
     lambda { tw.chicken }.should raise_error(Puppet::ParseError)
   end
 
   it "should allow you to check whether a variable is defined with has_variable?" do
-    @scope.expects(:lookupvar).with("chicken").returns("is good")
+    @scope.expects(:lookupvar).with { |name,options| name == "chicken"}.returns("is good")
     tw = Puppet::Parser::TemplateWrapper.new(@scope)
     tw.has_variable?("chicken").should eql(true)
   end
 
   it "should allow you to check whether a variable is not defined with has_variable?" do
-    @scope.expects(:lookupvar).with("chicken").returns(:undefined)
+    @scope.expects(:lookupvar).with { |name,options| name == "chicken"}.returns(:undefined)
     tw = Puppet::Parser::TemplateWrapper.new(@scope)
     tw.has_variable?("chicken").should eql(false)
   end
@@ -114,9 +117,7 @@ describe Puppet::Parser::TemplateWrapper do
   end
 
   it "should set all of the scope's variables as instance variables" do
-    template_mock = mock("template", :result => "woot!")
-    ERB.expects(:new).with("template contents", 0, "-").returns(template_mock)
-
+    mock_template
     @scope.expects(:to_hash).returns("one" => "foo")
     @tw.result("template contents")
 
@@ -124,8 +125,7 @@ describe Puppet::Parser::TemplateWrapper do
   end
 
   it "should not error out if one of the variables is a symbol" do
-    template_mock = mock("template", :result => "woot!")
-    ERB.expects(:new).with("template contents", 0, "-").returns(template_mock)
+    mock_template
 
     @scope.expects(:to_hash).returns(:_timestamp => "1234")
     @tw.result("template contents")
@@ -133,13 +133,11 @@ describe Puppet::Parser::TemplateWrapper do
 
   %w{! . ; :}.each do |badchar|
     it "should translate #{badchar} to _ when setting the instance variables" do
-    template_mock = mock("template", :result => "woot!")
-    ERB.expects(:new).with("template contents", 0, "-").returns(template_mock)
-
-    @scope.expects(:to_hash).returns("one#{badchar}" => "foo")
-    @tw.result("template contents")
+      mock_template
+      @scope.expects(:to_hash).returns("one#{badchar}" => "foo")
+      @tw.result("template contents")
 
-    @tw.instance_variable_get("@one_").should == "foo"
-  end
+      @tw.instance_variable_get("@one_").should == "foo"
+    end
   end
 end