Warn when user sets a property of an inline resource to itself.

(User will expect "x x" to grab the parent property.)

6 files changed, 94 insertions, 17 deletions

diff --git a/lib/chef/chef_class.rb b/lib/chef/chef_class.rb
index 6a0d09ec96..5ccb8bb92b 100644
--- a/lib/chef/chef_class.rb
+++ b/lib/chef/chef_class.rb
@@ -205,17 +205,7 @@ class Chef
     # @api private this will likely be removed in favor of an as-yet unwritten
     #      `Chef.log`
     def log_deprecation(message, location=nil)
-      if !location
-        # Pick the first caller that is *not* part of the Chef gem, that's the
-        # thing the user wrote.
-        chef_gem_path = File.expand_path("../..", __FILE__)
-        caller(0..10).each do |c|
-          if !c.start_with?(chef_gem_path)
-            location = c
-            break
-          end
-        end
-      end
+      location ||= Chef::Log.caller_location
       # `run_context.events` is the primary deprecation target if we're in a
       # run. If we are not yet in a run, print to `Chef::Log`.
       if run_context && run_context.events
diff --git a/lib/chef/log.rb b/lib/chef/log.rb
index bf846c2072..93c0cbbe68 100644
--- a/lib/chef/log.rb
+++ b/lib/chef/log.rb
@@ -37,6 +37,20 @@ class Chef
       end
     end
 
+    #
+    # Get the location of the caller (from the recipe). Grabs the first caller
+    # that is *not* in the chef gem proper (allowing us to weed out internal
+    # calls and give the user a more useful perspective).
+    #
+    # @return [String] The location of the caller (file:line#) from caller(0..20), or nil if no non-chef caller is found.
+    #
+    def self.caller_location
+      # Pick the first caller that is *not* part of the Chef gem, that's the
+      # thing the user wrote.
+      chef_gem_path = File.expand_path("../..", __FILE__)
+      caller(0..20).select { |c| !c.start_with?(chef_gem_path) }.first
+    end
+
     def self.deprecation(msg=nil, location=caller(2..2)[0], &block)
       if msg
         msg << " at #{Array(location).join("\n")}"
diff --git a/lib/chef/property.rb b/lib/chef/property.rb
index 3c40c020b6..9aaa49c313 100644
--- a/lib/chef/property.rb
+++ b/lib/chef/property.rb
@@ -292,6 +292,16 @@ class Chef
         value
 
       else
+        # If we are still compiling this resource (rather than running an action
+        # on it), reading this property before it's been set is likely the wrong
+        # thing. Warn if it's a duplicate of the enclosing provider.
+        if resource.respond_to?(:enclosing_provider) && resource.enclosing_provider &&
+           !resource.currently_running_action &&
+           !name_property? &&
+           resource.enclosing_provider.respond_to?(name)
+           Chef::Log.warn("#{Chef::Log.caller_location}: property #{name} is declared in both #{resource} and #{resource.enclosing_provider}. Use new_resource.#{name} instead. At #{Chef::Log.caller_location}")
+        end
+
         if has_default?
           value = default
           if value.is_a?(DelayedEvaluator)
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index 863d357561..4864ad8816 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -569,6 +569,8 @@ class Chef
     #
     def run_action(action, notification_type=nil, notifying_resource=nil)
       # reset state in case of multiple actions on the same resource.
+      old_currently_running_action = @currently_running_action
+      @currently_running_action = action
       @elapsed_time = 0
       start_time = Time.now
       events.resource_action_start(self, action, notification_type, notifying_resource)
@@ -608,16 +610,23 @@ class Chef
           events.resource_failed(self, action, e)
           raise customize_exception(e)
         end
-      ensure
-        @elapsed_time = Time.now - start_time
-        # Reporting endpoint doesn't accept a negative resource duration so set it to 0.
-        # A negative value can occur when a resource changes the system time backwards
-        @elapsed_time = 0 if @elapsed_time < 0
-        events.resource_completed(self)
       end
+    ensure
+      @currently_running_action = old_currently_running_action
+      @elapsed_time = Time.now - start_time
+      # Reporting endpoint doesn't accept a negative resource duration so set it to 0.
+      # A negative value can occur when a resource changes the system time backwards
+      @elapsed_time = 0 if @elapsed_time < 0
+      events.resource_completed(self)
     end
 
     #
+    # If we are currently running an action, this shows the action we are running.
+    # If the resource is running multiple actions at once, this will show the most recent.
+    #
+    attr_reader :currently_running_action
+
+    #
     # Generic Ruby and Data Structure Stuff (for user)
     #
 
diff --git a/lib/chef/resource/action_class.rb b/lib/chef/resource/action_class.rb
index 12211418e9..32652dd72b 100644
--- a/lib/chef/resource/action_class.rb
+++ b/lib/chef/resource/action_class.rb
@@ -21,6 +21,10 @@ require 'chef/exceptions'
 class Chef
   class Resource
     module ActionClass
+      def to_s
+        "#{new_resource || "<no resource>"} action #{action ? action.inspect : "<no action>"}"
+      end
+
       #
       # If load_current_value! is defined on the resource, use that.
       #
diff --git a/spec/integration/recipes/resource_action_spec.rb b/spec/integration/recipes/resource_action_spec.rb
index 6f3f5ab47e..26ce54a1c5 100644
--- a/spec/integration/recipes/resource_action_spec.rb
+++ b/spec/integration/recipes/resource_action_spec.rb
@@ -360,4 +360,54 @@ describe "Resource.action" do
       expect(WeirdActionJackson.action_was).to eq :"a-b-c d"
     end
   end
+
+  context "With a resource with property x" do
+    class ResourceActionSpecWithX < Chef::Resource
+      resource_name :resource_action_spec_with_x
+      property :x, default: 20
+      action :set do
+        # Access x during converge to ensure that we emit no warnings there
+        x
+      end
+    end
+
+    context "And another resource with a property x and an action that sets property x to its value" do
+      class ResourceActionSpecAlsoWithX < Chef::Resource
+        resource_name :resource_action_spec_also_with_x
+        property :x
+        action :set_x_to_x do
+          resource_action_spec_with_x 'hi' do
+            x x
+          end
+        end
+        action :set_x_to_10 do
+          resource_action_spec_with_x 'hi' do
+            x 10
+          end
+        end
+      end
+
+      it "Using the enclosing resource to set x to x emits a warning that you're using the wrong x" do
+        recipe = converge {
+          resource_action_spec_also_with_x 'hi' do
+            x 1
+            action :set_x_to_x
+          end
+        }
+        warnings = recipe.logs.lines.select { |l| l =~ /warn/i }
+        expect(warnings.size).to eq 1
+        expect(warnings[0]).to match(/property x is declared in both resource_action_spec_with_x\[hi\] and resource_action_spec_also_with_x\[hi\] action :set_x_to_x. Use new_resource.x instead. At #{__FILE__}:#{__LINE__-19}/)
+      end
+
+      it "Using the enclosing resource to set x to 10 emits no warning" do
+        expect_recipe {
+          resource_action_spec_also_with_x 'hi' do
+            x 1
+            action :set_x_to_10
+          end
+        }.to emit_no_warnings_or_errors
+      end
+    end
+
+  end
 end