Deprecate creating properties whose names are already methodsadamleff/warn-on-dangerous-property-names

When creating a resource, a user can create a property that is the same
name as an already-existing Ruby method, such as `#hash`. In the case of
the `#hash` method, this can cause issues when attempting to adding
resources to other data structures, such as Arrays or Hashes. In other
examples, this could cause unexpected behavior that is incredibly
difficult to troubleshoot.

This change adds a deprecation warning in the case where a user adds
a property to a resource that the resource instance already responds to.
If y'all are OK with this approach, I'll be happy to write up the
deprecation doc for this for docs.chef.io.

Signed-off-by: Adam Leff <adam@leff.co>

9 files changed, 69 insertions, 7 deletions

diff --git a/Gemfile.lock b/Gemfile.lock
index 465a3d6c5b..d12d641d87 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -179,7 +179,7 @@ GEM
       mixlib-log (~> 1.3)
       rack (~> 2.0)
       uuidtools (~> 2.1)
-    cheffish (4.0.0)
+    cheffish (4.1.0)
       chef-zero (~> 5.0)
       net-ssh
     chefspec (5.3.0)
diff --git a/lib/chef/chef_class.rb b/lib/chef/chef_class.rb
index 3a395d979d..e61fd5e1d2 100644
--- a/lib/chef/chef_class.rb
+++ b/lib/chef/chef_class.rb
@@ -30,6 +30,7 @@ require "chef/platform/provider_priority_map"
 require "chef/platform/resource_priority_map"
 require "chef/platform/provider_handler_map"
 require "chef/platform/resource_handler_map"
+require "chef/deprecated"
 require "chef/event_dispatch/dsl"
 require "chef/deprecated"
 
diff --git a/lib/chef/deprecated.rb b/lib/chef/deprecated.rb
index 3a988fdfa3..2d352e4b1a 100644
--- a/lib/chef/deprecated.rb
+++ b/lib/chef/deprecated.rb
@@ -156,6 +156,26 @@ class Chef
       end
     end
 
+    class PropertyNameCollision < Base
+      def id
+        11
+      end
+
+      def target
+        "property_name_collision.html"
+      end
+    end
+
+    class LaunchdHashProperty < Base
+      def id
+        12
+      end
+
+      def target
+        "launchd_hash_property.html"
+      end
+    end
+
     class ChefPlatformMethods < Base
       def id
         13
diff --git a/lib/chef/property.rb b/lib/chef/property.rb
index 8d8ed352b3..8fa290251a 100644
--- a/lib/chef/property.rb
+++ b/lib/chef/property.rb
@@ -518,6 +518,13 @@ class Chef
       # be using the existing getter/setter to manipulate it instead.
       return if !instance_variable_name
 
+      # We deprecate any attempt to create a property that already exists as a
+      # method in some Classes that we know would cause our users problems.
+      # For example, creating a `hash` property could cause issues when adding
+      # a Chef::Resource instance to an data structure that expects to be able
+      # to call the `#hash` method and get back an appropriate Fixnum.
+      emit_property_redefinition_deprecations
+
       # We prefer this form because the property name won't show up in the
       # stack trace if you use `define_method`.
       declared_in.class_eval <<-EOM, __FILE__, __LINE__ + 1
@@ -632,6 +639,30 @@ class Chef
 
     private
 
+    def emit_property_redefinition_deprecations
+      # We only emit deprecations if this property already exists as an instance method.
+      # Weeding out class methods avoids unnecessary deprecations such Chef::Resource
+      # defining a `name` property when there's an already-existing `name` method
+      # for a Module.
+      return unless declared_in.instance_methods.include?(name)
+
+      # Only emit deprecations for some well-known classes. This will still
+      # allow more advanced users to subclass their own custom resources and
+      # override their own properties.
+      return unless [ Object, BasicObject, Kernel, Chef::Resource ].include?(declared_in.instance_method(name).owner)
+
+      # Allow top-level Chef::Resource proprties, such as `name`, to be overridden.
+      # As of this writing, `name` is the only Chef::Resource property created with the
+      # `property` definition, but this will allow for future properties to be extended
+      # as needed.
+      return if Chef::Resource.properties.keys.include?(name)
+
+      # Emit the deprecation.
+      resource_name = declared_in.respond_to?(:resource_name) ? declared_in.resource_name : declared_in
+      Chef.deprecated(:property_name_collision, "Property `#{name}` of resource `#{resource_name}` overwrites an existing method. " \
+        "Please use a different property name. This will raise an exception in Chef 13.")
+    end
+
     def exec_in_resource(resource, proc, *args)
       if resource
         if proc.arity > args.size
diff --git a/lib/chef/provider/launchd.rb b/lib/chef/provider/launchd.rb
index 0ec8d0cfe1..f3e0a00758 100644
--- a/lib/chef/provider/launchd.rb
+++ b/lib/chef/provider/launchd.rb
@@ -150,7 +150,7 @@ class Chef
       end
 
       def content
-        plist_hash = new_resource.hash || gen_hash
+        plist_hash = new_resource.plist_hash || gen_hash
         Plist::Emit.dump(plist_hash) unless plist_hash.nil?
       end
 
diff --git a/lib/chef/resource/apt_repository.rb b/lib/chef/resource/apt_repository.rb
index 8b87371824..b38bd1c8ec 100644
--- a/lib/chef/resource/apt_repository.rb
+++ b/lib/chef/resource/apt_repository.rb
@@ -38,7 +38,6 @@ class Chef
 
       property :cookbook, [String, nil, false], default: nil, desired_state: false, nillable: true, coerce: proc { |x| x ? x : nil }
       property :cache_rebuild, [TrueClass, FalseClass], default: true, desired_state: false
-      property :sensitive, [TrueClass, FalseClass], default: false, desired_state: false
 
       default_action :add
       allowed_actions :add, :remove
diff --git a/lib/chef/resource/launchd.rb b/lib/chef/resource/launchd.rb
index 6427b13f84..c78ffa3f0e 100644
--- a/lib/chef/resource/launchd.rb
+++ b/lib/chef/resource/launchd.rb
@@ -33,7 +33,7 @@ class Chef
       property :backup, [Integer, FalseClass]
       property :cookbook, String
       property :group, [String, Integer]
-      property :hash, Hash
+      property :plist_hash, Hash
       property :mode, [String, Integer]
       property :owner, [String, Integer]
       property :path, String
@@ -139,6 +139,18 @@ class Chef
       property :wait_for_debugger, [ TrueClass, FalseClass ]
       property :watch_paths, Array
       property :working_directory, String
+
+      # hash is an instance method on Object and needs to return a Fixnum.
+      def hash(arg = nil)
+        Chef.deprecated(:launchd_hash_property, "Property `hash` on the `launchd` resource has changed to `plist_hash`." \
+          "Please use `plist_hash` instead. This will raise an exception in Chef 13.")
+
+        set_or_return(
+          :plist_hash,
+          arg,
+          :kind_of => Hash
+        )
+      end
     end
   end
 end
diff --git a/lib/chef/resource/yum_repository.rb b/lib/chef/resource/yum_repository.rb
index 1c215b51ff..30a14a22d5 100644
--- a/lib/chef/resource/yum_repository.rb
+++ b/lib/chef/resource/yum_repository.rb
@@ -58,7 +58,6 @@ class Chef
       property :repo_gpgcheck, [TrueClass, FalseClass]
       property :report_instanceid, [TrueClass, FalseClass]
       property :repositoryid, String, regex: /.*/, name_property: true
-      property :sensitive, [TrueClass, FalseClass], default: false
       property :skip_if_unavailable, [TrueClass, FalseClass]
       property :source, String, regex: /.*/
       property :sslcacert, String, regex: /.*/
diff --git a/spec/unit/provider/launchd_spec.rb b/spec/unit/provider/launchd_spec.rb
index 3e45433c62..693801f99b 100644
--- a/spec/unit/provider/launchd_spec.rb
+++ b/spec/unit/provider/launchd_spec.rb
@@ -185,8 +185,8 @@ XML
       end
 
       describe "hash is passed" do
-        it "should produce the test_plist from the hash" do
-          new_resource.hash test_hash
+        it "should produce the test_plist content from the plist_hash property" do
+          new_resource.plist_hash test_hash
           expect(provider.content?).to be_truthy
           expect(provider.content).to eql(test_plist)
         end