Fix nil with properties:

1. Warn when default values are invalid.
2. Never validate nil (on set or get) if there is no default.
3. Emit "will be invalid in Chef 13" warning when setting an invalid nil value.

5 files changed, 110 insertions, 38 deletions

diff --git a/lib/chef/exceptions.rb b/lib/chef/exceptions.rb
index f9a717471c..08d916064f 100644
--- a/lib/chef/exceptions.rb
+++ b/lib/chef/exceptions.rb
@@ -71,6 +71,7 @@ class Chef
     class RoleNotFound < RuntimeError; end
     class DuplicateRole < RuntimeError; end
     class ValidationFailed < ArgumentError; end
+    class CannotValidateStaticallyError < ArgumentError; end
     class InvalidPrivateKey < ArgumentError; end
     class MissingKeyAttribute < ArgumentError; end
     class KeyCommandInputError < ArgumentError; end
diff --git a/lib/chef/mixin/params_validate.rb b/lib/chef/mixin/params_validate.rb
index 841cf9e6a2..3585def853 100644
--- a/lib/chef/mixin/params_validate.rb
+++ b/lib/chef/mixin/params_validate.rb
@@ -22,7 +22,6 @@ require "chef/delayed_evaluator"
 class Chef
   module Mixin
     module ParamsValidate
-
       # Takes a hash of options, along with a map to validate them.  Returns the original
       # options hash, plus any changes that might have been made (through things like setting
       # default values in the validation map)
@@ -333,6 +332,7 @@ class Chef
       def _pv_name_property(opts, key, is_name_property=true)
         if is_name_property
           if opts[key].nil?
+            raise CannotValidateStaticallyError, "name_property cannot be evaluated without a resource." if self == Chef::Mixin::ParamsValidate
             opts[key] = self.instance_variable_get(:"@name")
           end
         end
@@ -403,6 +403,7 @@ class Chef
         to_be.each do |tb|
           case tb
           when Proc
+            raise CannotValidateStaticallyError, "is: proc { } must be evaluated once for each resource" if self == Chef::Mixin::ParamsValidate
             return true if instance_exec(value, &tb)
           when Property
             validate(opts, { key => tb.validation_options })
@@ -436,12 +437,21 @@ class Chef
       #
       def _pv_coerce(opts, key, coercer)
         if opts.has_key?(key.to_s)
+          raise CannotValidateStaticallyError, "coerce must be evaluated for each resource." if self == Chef::Mixin::ParamsValidate
           opts[key.to_s] = instance_exec(opts[key], &coercer)
         elsif opts.has_key?(key.to_sym)
+          raise CannotValidateStaticallyError, "coerce must be evaluated for each resource." if self == Chef::Mixin::ParamsValidate
           opts[key.to_sym] = instance_exec(opts[key], &coercer)
         end
       end
 
+      # We allow Chef::Mixin::ParamsValidate.validate(), but we will raise an
+      # error if you try to do anything requiring there to be an actual resource.
+      # This way, you can statically validate things if you have constant validation
+      # (which is the norm).
+      extend self
+
+
       # Used by #set_or_return to avoid emitting a deprecation warning for
       # "value nil" and to keep default stickiness working exactly the same
       # @api private
diff --git a/lib/chef/property.rb b/lib/chef/property.rb
index 8b5c6560a9..437ee10147 100644
--- a/lib/chef/property.rb
+++ b/lib/chef/property.rb
@@ -87,16 +87,21 @@ class Chef
     #     is fully initialized.
     #
     def initialize(**options)
-      options.each { |k,v| options[k.to_sym] = v if k.is_a?(String) }
+      options.each { |k,v| options[k.to_sym] = v; options.delete(k) if k.is_a?(String) }
+      @options = options
+      options[:name] = options[:name].to_sym if options[:name]
+      options[:instance_variable_name] = options[:instance_variable_name].to_sym if options[:instance_variable_name]
+
 
       # Replace name_attribute with name_property
       if options.has_key?(:name_attribute)
         # If we have both name_attribute and name_property and they differ, raise an error
         if options.has_key?(:name_property)
-          raise ArgumentError, "Cannot specify both name_property and name_attribute together on property #{options[:name]}#{options[:declared_in] ? " of resource #{options[:declared_in].resource_name}" : ""}."
+          raise ArgumentError, "Cannot specify both name_property and name_attribute together on property #{self}."
         end
         # replace name_property with name_attribute in place
         options = Hash[options.map { |k,v| k == :name_attribute ? [ :name_property, v ] : [ k,v ] }]
+        @options = options
       end
 
       # Only pick the first of :default, :name_property and :name_attribute if
@@ -109,17 +114,22 @@ class Chef
           options.delete(:name_property)
           preferred_default = :default
         end
-        Chef.log_deprecation("Cannot specify both default and name_property together on property #{options[:name]}#{options[:declared_in] ? " of resource #{options[:declared_in].resource_name}" : ""}. Only one (#{preferred_default}) will be obeyed. In Chef 13, this will become an error.")
+        Chef.log_deprecation("Cannot specify both default and name_property together on property #{self}. Only one (#{preferred_default}) will be obeyed. In Chef 13, this will become an error. Please remove one or the other from the property.")
       end
 
-      @options = options
-
-      options[:name] = options[:name].to_sym if options[:name]
-      options[:instance_variable_name] = options[:instance_variable_name].to_sym if options[:instance_variable_name]
+      # Validate the default early, so the user gets a good error message, and
+      # cache it so we don't do it again if so
+      begin
+        # If we can validate it all the way to output, do it.
+        @stored_default = input_to_stored_value(nil, default, is_default: true)
+      rescue Chef::Exceptions::CannotValidateStaticallyError
+        # If the validation is not static (i.e. has procs), we will have to
+        # coerce and validate the default each time we run
+      end
     end
 
     def to_s
-      name
+      "#{name}#{declared_in ? " of resource #{declared_in.resource_name}" : ""}"
     end
 
     #
@@ -253,14 +263,26 @@ class Chef
         return get(resource)
       end
 
-      if value.nil? && !explicitly_accepts_nil?(resource)
+      if value.nil?
         # In Chef 12, value(nil) does a *get* instead of a set, so we
         # warn if the value would have been changed. In Chef 13, it will be
         # equivalent to value = nil.
         result = get(resource)
-        if !result.nil?
-          Chef.log_deprecation("#{name} nil currently does not overwrite the value of #{name}. This will change in Chef 13, and the value will be set to nil instead. Please change your code to explicitly accept nil using \"property :#{name}, [MyType, nil]\", or stop setting this value to nil.")
+
+        # Warn about this becoming a set in Chef 13.
+        begin
+          input_to_stored_value(resource, value)
+          # If nil is valid, and it would change the value, warn that this will change to a set.
+          if !result.nil?
+            Chef.log_deprecation("An attempt was made to change #{name} from #{result.inspect} to nil by calling #{name}(nil). In Chef 12, this does a get rather than a set. In Chef 13, this will change to set the value to nil.")
+          end
+        rescue Chef::Exceptions::DeprecatedFeatureError
+          raise
+        rescue
+          # If nil is invalid, warn that this will become an error.
+          Chef.log_deprecation("nil is an invalid value for #{self}. In Chef 13, this warning will change to an error. Error: #{$!}")
         end
+
         result
       else
         # Anything else, such as myprop(value) is a set
@@ -291,16 +313,14 @@ class Chef
     #   this property, or if the value is required and not set.
     #
     def get(resource)
+      # If it's set, return it (and evaluate any lazy values)
       if is_set?(resource)
         value = get_value(resource)
-        if value.is_a?(DelayedEvaluator)
-          value = exec_in_resource(resource, value)
-          value = coerce(resource, value)
-          validate(resource, value)
-        end
-        value
+        value = stored_value_to_output(resource, value)
 
       else
+        # We are getting the default value.
+
         # If the user does something like this:
         #
         # ```
@@ -326,14 +346,14 @@ class Chef
         end
 
         if has_default?
-          value = default
-          if value.is_a?(DelayedEvaluator)
-            value = exec_in_resource(resource, value)
+          # If we were able to cache the stored_default, grab it.
+          if defined?(@stored_default)
+            value = @stored_default
+          else
+            # Otherwise, we have to validate it now.
+            value = input_to_stored_value(resource, default, is_default: true)
           end
-
-          value = coerce(resource, value)
-
-          # We don't validate defaults
+          value = stored_value_to_output(resource, value, is_default: true)
 
           # If the value is mutable (non-frozen), we set it on the instance
           # so that people can mutate it.  (All constant default values are
@@ -366,11 +386,7 @@ class Chef
     #   this property.
     #
     def set(resource, value)
-      unless value.is_a?(DelayedEvaluator)
-        value = coerce(resource, value)
-        validate(resource, value)
-      end
-      set_value(resource, value)
+      set_value(resource, input_to_stored_value(resource, value))
     end
 
     #
@@ -423,7 +439,10 @@ class Chef
     #
     def coerce(resource, value)
       if options.has_key?(:coerce)
-        value = exec_in_resource(resource, options[:coerce], value)
+        # If we have no default value, `nil` is never coerced or validated
+        unless !has_default? && value.nil?
+          value = exec_in_resource(resource, options[:coerce], value)
+        end
       end
       value
     end
@@ -442,7 +461,10 @@ class Chef
     #   this property.
     #
     def validate(resource, value)
-      resource.validate({ name => value }, { name => validation_options })
+      # If we have no default value, `nil` is never coerced or validated
+      unless value.nil? && !has_default?
+        (resource || Chef::Mixin::ParamsValidate).validate({ name => value }, { name => validation_options })
+      end
     end
 
     #
@@ -595,14 +617,53 @@ class Chef
           value = resource.instance_exec(*args, &proc)
         end
       else
-        value = proc.call
+        # If we don't have a resource yet, we can't exec in resource!
+        raise Chef::Exceptions::CannotValidateStaticallyError, "Cannot validate or coerce without a resource"
+      end
+    end
+
+    def input_to_stored_value(resource, value, is_default: false)
+      unless value.is_a?(DelayedEvaluator)
+        value = coerce_and_validate(resource, value, is_default: is_default)
       end
+      value
+    end
 
+    def stored_value_to_output(resource, value, is_default: false)
+      # Crack open lazy values before giving the result to the user
       if value.is_a?(DelayedEvaluator)
-        value = coerce(resource, value)
-        validate(resource, value)
+        value = exec_in_resource(resource, value)
+        value = coerce_and_validate(resource, value, is_default: is_default)
       end
       value
     end
+
+    # Coerces and validates the value. If the value is a default, it will warn
+    # the user that invalid defaults are bad mmkay, and return it as if it were
+    # valid.
+    def coerce_and_validate(resource, value, is_default: false)
+      result = coerce(resource, value)
+      begin
+        # If the input is from a default, we need to emit an invalid default warning on validate.
+        validate(resource, result)
+      rescue Chef::Exceptions::CannotValidateStaticallyError
+        # This one gets re-raised
+        raise
+      rescue
+        # Anything else is just an invalid default: in those cases, we just
+        # warn and return the (possibly coerced) value to the user.
+        if is_default
+          if value.nil?
+            Chef.log_deprecation("Default value nil is invalid for property #{self}. Possible fixes: 1. Remove 'default: nil' if nil means 'undefined'. 2. Set a valid default value if there is a reasonable one. 3. Allow nil as a valid value of your property (for example, 'property #{name.inspect}, [ String, nil ], default: nil'). Error: #{$!}")
+          else
+            Chef.log_deprecation("Default value #{value.inspect} is invalid for property #{self}. In Chef 13 this will become an error: #{$!}.")
+          end
+        else
+          raise
+        end
+      end
+
+      result
+    end
   end
 end
diff --git a/lib/chef/resource/chef_gem.rb b/lib/chef/resource/chef_gem.rb
index 233ae66026..655534684c 100644
--- a/lib/chef/resource/chef_gem.rb
+++ b/lib/chef/resource/chef_gem.rb
@@ -26,9 +26,9 @@ class Chef
 
       property :gem_binary, default: "#{RbConfig::CONFIG['bindir']}/gem",
                callbacks: {
-                 "The chef_gem resource is restricted to the current gem environment, use gem_package to install to other environments." => proc { false }
+                 "The chef_gem resource is restricted to the current gem environment, use gem_package to install to other environments." => proc { |v| v == properties[:gem_binary].default }
                }
-      property :compile_time, [ true, false ], default: lazy { Chef::Config[:chef_gem_compile_time] }, desired_state: false
+      property :compile_time, [ true, false, nil ], default: lazy { Chef::Config[:chef_gem_compile_time] }, desired_state: false
 
       def after_created
         # Chef::Resource.run_action: Caveat: this skips Chef::Runner.run_action, where notifications are handled
diff --git a/lib/chef/resource/file.rb b/lib/chef/resource/file.rb
index 0a530e27c9..52cb409226 100644
--- a/lib/chef/resource/file.rb
+++ b/lib/chef/resource/file.rb
@@ -49,7 +49,7 @@ class Chef
       allowed_actions :create, :delete, :touch, :create_if_missing
 
       property :path, String, name_property: true, identity: true
-      property :atomic_update, [ true, false ], desired_state: false, default: Chef::Config[:file_atomic_update]
+      property :atomic_update, [ true, false ], desired_state: false, default: lazy { Chef::Config[:file_atomic_update] }
       property :backup, [ Integer, false ], desired_state: false, default: 5
       property :checksum, [ /^[a-zA-Z0-9]{64}$/, nil ]
       property :content, [ String, nil ], desired_state: false