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.

10 files changed, 297 insertions, 196 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
diff --git a/spec/integration/client/client_spec.rb b/spec/integration/client/client_spec.rb
index d08935d854..24ce69ac03 100644
--- a/spec/integration/client/client_spec.rb
+++ b/spec/integration/client/client_spec.rb
@@ -430,7 +430,7 @@ EOM
       expect(run_complete).to be >= 0
 
       # Make sure there is exactly one result for each, and that it occurs *after* the complete message.
-      expect(match_indices(/nil currently does not overwrite the value of/, result.stdout)).to match([ be > run_complete ])
+      expect(match_indices(/An attempt was made to change x from \[\] to nil by calling x\(nil\). In Chef 12, this does a get rather than a set. In Chef 13, this will change to set the value to nil./, result.stdout)).to match([ be > run_complete ])
     end
   end
 
diff --git a/spec/unit/property/validation_spec.rb b/spec/unit/property/validation_spec.rb
index 7e84a651e2..5a40cce145 100644
--- a/spec/unit/property/validation_spec.rb
+++ b/spec/unit/property/validation_spec.rb
@@ -72,7 +72,7 @@ describe "Chef::Resource.property validation" do
     end
   end
 
-  def self.validation_test(validation, success_values, failure_values, getter_values=[], *tags)
+  def self.validation_test(validation, success_values, failure_values, *tags)
     with_property ":x, #{validation}", *tags do
       it "gets nil when retrieving the initial (non-set) value" do
         expect(resource.x).to be_nil
@@ -91,11 +91,52 @@ describe "Chef::Resource.property validation" do
           expect { resource.x v }.to raise_error Chef::Exceptions::ValidationFailed
         end
       end
-      getter_values.each do |v|
-        it "setting value to #{v.inspect} does not change the value" do
+      it "setting x to nil when it is already nil does not emit a warning" do
+        expect(resource.x nil).to be_nil
+        expect(resource.x).to be_nil
+      end
+      it "changing x to nil warns that the get will change to a set in Chef 13 and does not change the value" do
+        resource.instance_eval { @x = "default" }
+        expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
+          /An attempt was made to change x from "default" to nil by calling x\(nil\). In Chef 12, this does a get rather than a set. In Chef 13, this will change to set the value to nil./
+        Chef::Config[:treat_deprecation_warnings_as_errors] = false
+        expect(resource.x nil).to eq "default"
+        expect(resource.x).to eq "default"
+      end
+    end
+    if tags.include?(:nil_is_valid)
+      with_property ":x, #{validation}, default: nil" do
+        it "setting x to nil when it is already nil does not emit a warning" do
+          expect(resource.x nil).to be_nil
+          expect(resource.x).to be_nil
+        end
+        it "changing x to nil warns that the get will change to a set in Chef 13 and does not change the value" do
+          resource.instance_eval { @x = "default" }
+          expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
+            /An attempt was made to change x from "default" to nil by calling x\(nil\). In Chef 12, this does a get rather than a set. In Chef 13, this will change to set the value to nil./
           Chef::Config[:treat_deprecation_warnings_as_errors] = false
+          expect(resource.x nil).to eq "default"
+          expect(resource.x).to eq "default"
+        end
+      end
+    else
+      it "property :x, #{validation}, default: nil warns that the default is invalid" do
+        expect { resource_class.class_eval("property :x, #{validation}, default: nil", __FILE__, __LINE__) }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
+          /Default value nil is invalid for property x of resource chef_resource_property_spec_(\d+). 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 :x, \[ String, nil \], default: nil'\)./
+      end
+      context "With property :x, #{validation}, default: nil" do
+        before do
+          Chef::Config[:treat_deprecation_warnings_as_errors] = false
+          resource_class.class_eval("property :x, #{validation}, default: nil", __FILE__, __LINE__)
+          Chef::Config[:treat_deprecation_warnings_as_errors] = true
+        end
+
+        it "changing x to nil emits a warning that the value is invalid and does not change the value" do
           resource.instance_eval { @x = "default" }
-          expect(resource.x v).to eq "default"
+          expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
+            /nil is an invalid value for x of resource chef_resource_property_spec_(\d+). In Chef 13, this warning will change to an error./
+          Chef::Config[:treat_deprecation_warnings_as_errors] = false
+          expect(resource.x nil).to eq "default"
           expect(resource.x).to eq "default"
         end
       end
@@ -150,9 +191,12 @@ describe "Chef::Resource.property validation" do
         it "get succeeds" do
           expect(resource.x).to eq "default"
         end
-        it "set(nil) sets the value" do
-          expect(resource.x nil).to be_nil
-          expect(resource.x).to be_nil
+        it "set(nil) emits a warning that the value will be set, but does not set the value" do
+          expect { resource.x nil }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
+            /An attempt was made to change x from "default" to nil by calling x\(nil\). In Chef 12, this does a get rather than a set. In Chef 13, this will change to set the value to nil./
+          Chef::Config[:treat_deprecation_warnings_as_errors] = false
+          expect(resource.x nil).to eq "default"
+          expect(resource.x).to eq "default"
         end
         it "set to valid value succeeds" do
           expect(resource.x "str").to eq "str"
@@ -185,46 +229,41 @@ describe "Chef::Resource.property validation" do
   context "bare types" do
     validation_test "String",
       [ "hi" ],
-      [ 10 ],
-      [ nil ]
+      [ 10 ]
 
     validation_test ":a",
       [ :a ],
-      [ :b ],
-      [ nil ]
+      [ :b ]
 
     validation_test ":a, is: :b",
       [ :a, :b ],
-      [ :c ],
-      [ nil ]
+      [ :c ]
 
     validation_test ":a, is: [ :b, :c ]",
       [ :a, :b, :c ],
-      [ :d ],
-      [ nil ]
+      [ :d ]
 
     validation_test "[ :a, :b ], is: :c",
       [ :a, :b, :c ],
-      [ :d ],
-      [ nil ]
+      [ :d ]
 
     validation_test "[ :a, :b ], is: [ :c, :d ]",
       [ :a, :b, :c, :d ],
-      [ :e ],
-      [ nil ]
+      [ :e ]
 
     validation_test "nil",
-      [ nil ],
-      [ :a ]
+      [ ],
+      [ :a ],
+      :nil_is_valid
 
     validation_test "[ nil ]",
-      [ nil ],
-      [ :a ]
+      [ ],
+      [ :a ],
+      :nil_is_valid
 
     validation_test "[]",
       [],
-      [ :a ],
-      [ nil ]
+      [ :a ]
   end
 
   # is
@@ -232,35 +271,30 @@ describe "Chef::Resource.property validation" do
     # Class
     validation_test "is: String",
       [ "a", "" ],
-      [ :a, 1 ],
-      [ nil ]
+      [ :a, 1 ]
 
     # Value
     validation_test "is: :a",
       [ :a ],
-      [ :b ],
-      [ nil ]
+      [ :b ]
 
     validation_test "is: [ :a, :b ]",
       [ :a, :b ],
-      [ [ :a, :b ] ],
-      [ nil ]
+      [ [ :a, :b ] ]
 
     validation_test "is: [ [ :a, :b ] ]",
       [ [ :a, :b ] ],
-      [ :a, :b ],
-      [ nil ]
+      [ :a, :b ]
 
     # Regex
     validation_test "is: /abc/",
       [ "abc", "wowabcwow" ],
-      [ "", "abac" ],
-      [ nil ]
+      [ "", "abac" ]
 
     # Property
     validation_test "is: Chef::Property.new(is: :a)",
       [ :a ],
-      [ :b, nil ]
+      [ :b ]
 
     # RSpec Matcher
     class Globalses
@@ -269,31 +303,30 @@ describe "Chef::Resource.property validation" do
 
     validation_test "is: Globalses.eq(10)",
       [ 10 ],
-      [ 1 ],
-      [ nil ]
+      [ 1 ]
 
     # Proc
     validation_test "is: proc { |x| x }",
       [ true, 1 ],
-      [ false ],
-      [ nil ]
+      [ false ]
 
     validation_test "is: proc { |x| x > blah }",
       [ 10 ],
       [ -1 ]
 
     validation_test "is: nil",
-      [ nil ],
-      [ "a" ]
+      [ ],
+      [ "a" ],
+      :nil_is_valid
 
     validation_test "is: [ String, nil ]",
-      [ "a", nil ],
-      [ :b ]
+      [ "a" ],
+      [ :b ],
+      :nil_is_valid
 
     validation_test "is: []",
       [],
-      [ :a ],
-      [ nil ]
+      [ :a ]
   end
 
   # Combination
@@ -301,7 +334,7 @@ describe "Chef::Resource.property validation" do
     validation_test 'kind_of: String, equal_to: "a"',
       [ "a" ],
       [ "b" ],
-      [ nil ]
+      :nil_is_valid
   end
 
   # equal_to
@@ -310,37 +343,38 @@ describe "Chef::Resource.property validation" do
     validation_test "equal_to: :a",
       [ :a ],
       [ :b ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "equal_to: [ :a, :b ]",
       [ :a, :b ],
       [ [ :a, :b ] ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "equal_to: [ [ :a, :b ] ]",
       [ [ :a, :b ] ],
       [ :a, :b ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "equal_to: nil",
       [ ],
       [ "a" ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test 'equal_to: [ "a", nil ]',
       [ "a" ],
       [ "b" ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test 'equal_to: [ nil, "a" ]',
       [ "a" ],
       [ "b" ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "equal_to: []",
       [],
       [ :a ],
-      [ nil ]
+      :nil_is_valid
+
   end
 
   # kind_of
@@ -348,37 +382,37 @@ describe "Chef::Resource.property validation" do
     validation_test "kind_of: String",
       [ "a" ],
       [ :b ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "kind_of: [ String, Symbol ]",
       [ "a", :b ],
       [ 1 ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "kind_of: [ Symbol, String ]",
       [ "a", :b ],
       [ 1 ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "kind_of: NilClass",
       [ ],
       [ "a" ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "kind_of: [ NilClass, String ]",
       [ "a" ],
       [ :a ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "kind_of: []",
       [],
       [ :a ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "kind_of: nil",
       [],
       [ :a ],
-      [ nil ]
+      :nil_is_valid
   end
 
   # regex
@@ -386,53 +420,55 @@ describe "Chef::Resource.property validation" do
     validation_test "regex: /abc/",
       [ "xabcy" ],
       [ "gbh", 123 ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "regex: [ /abc/, /z/ ]",
       [ "xabcy", "aza" ],
       [ "gbh", 123 ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "regex: [ /z/, /abc/ ]",
       [ "xabcy", "aza" ],
       [ "gbh", 123 ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "regex: [ [ /z/, /abc/ ], [ /n/ ] ]",
       [ "xabcy", "aza", "ana" ],
       [ "gbh", 123 ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "regex: []",
       [],
       [ :a ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "regex: nil",
       [],
       [ :a ],
-      [ nil ]
+      :nil_is_valid
   end
 
   # callbacks
   context "callbacks" do
     validation_test 'callbacks: { "a" => proc { |x| x > 10 }, "b" => proc { |x| x%2 == 0 } }',
       [ 12 ],
-      [ 11, 4 ]
+      [ 11, 4 ],
+      :nil_is_valid
 
     validation_test 'callbacks: { "a" => proc { |x| x%2 == 0 }, "b" => proc { |x| x > 10 } }',
       [ 12 ],
-      [ 11, 4 ]
+      [ 11, 4 ],
+      :nil_is_valid
 
     validation_test 'callbacks: { "a" => proc { |x| x.nil? } }',
       [ ],
       [ "a" ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "callbacks: {}",
       [ :a ],
       [],
-      [ nil ]
+      :nil_is_valid
   end
 
   # respond_to
@@ -440,84 +476,84 @@ describe "Chef::Resource.property validation" do
     validation_test "respond_to: :split",
       [ "hi" ],
       [ 1 ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test 'respond_to: "split"',
       [ "hi" ],
       [ 1 ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "respond_to: :to_s",
       [ :a ],
       [],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "respond_to: [ :split, :to_s ]",
       [ "hi" ],
       [ 1 ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "respond_to: %w(split to_s)",
       [ "hi" ],
       [ 1 ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "respond_to: [ :to_s, :split ]",
       [ "hi" ],
       [ 1 ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "respond_to: []",
       [ :a ],
       [],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "respond_to: nil",
       [ :a ],
       [],
-      [ nil ]
+      :nil_is_valid
   end
 
   context "cannot_be" do
     validation_test "cannot_be: :empty",
       [ 1, [1,2], { a: 10 } ],
       [ [] ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test 'cannot_be: "empty"',
       [ 1, [1,2], { a: 10 } ],
       [ [] ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "cannot_be: [ :empty, :nil ]",
       [ 1, [1,2], { a: 10 } ],
       [ [] ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test 'cannot_be: [ "empty", "nil" ]',
       [ 1, [1,2], { a: 10 } ],
       [ [] ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "cannot_be: [ :nil, :empty ]",
       [ 1, [1,2], { a: 10 } ],
       [ [] ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "cannot_be: [ :empty, :nil, :blahblah ]",
       [ 1, [1,2], { a: 10 } ],
       [ [] ],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "cannot_be: []",
       [ :a ],
       [],
-      [ nil ]
+      :nil_is_valid
 
     validation_test "cannot_be: nil",
       [ :a ],
       [],
-      [ nil ]
+      :nil_is_valid
 
   end
 
@@ -549,9 +585,8 @@ describe "Chef::Resource.property validation" do
       it "if x is not specified, retrieval fails" do
         expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
       end
-      it "value nil is valid" do
-        expect(resource.x nil).to be_nil
-        expect(resource.x).to be_nil
+      it "value nil is not valid (required means 'not nil')" do
+        expect { resource.x nil }.to raise_error Chef::Exceptions::ValidationFailed
       end
       it "value '1' is valid" do
         expect(resource.x "1").to eq "1"
diff --git a/spec/unit/property_spec.rb b/spec/unit/property_spec.rb
index 1fe8abc706..ffdbccf019 100644
--- a/spec/unit/property_spec.rb
+++ b/spec/unit/property_spec.rb
@@ -203,7 +203,7 @@ describe "Chef::Resource.property" do
           expect(subresource_class.properties[:y]).not_to be_nil
         end
         it "y is not on the superclass" do
-          expect { resource_class.y 10 }.to raise_error
+          expect { resource_class.y 10 }.to raise_error NoMethodError
           expect(resource_class.properties[:y]).to be_nil
         end
       end
@@ -581,10 +581,17 @@ describe "Chef::Resource.property" do
     end
 
     context "validation of defaults" do
-      with_property ":x, String, default: 10" do
-        it "when the resource is created, no error is raised" do
-          resource
+      it "When a class is declared with property :x, String, default: 10, a warning is emitted" do
+        expect { resource_class.class_eval { property :x, String, default: 10 } }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
+          /Default value 10 is invalid for property x of resource chef_resource_property_spec_(\d+). In Chef 13 this will become an error: Option x must be one of: String!  You passed 10./
+      end
+      context "With property :x, String, default: 10" do
+        before do
+          Chef::Config[:treat_deprecation_warnings_as_errors] = false
+          resource_class.class_eval { property :x, String, default: 10 }
+          Chef::Config[:treat_deprecation_warnings_as_errors] = true
         end
+
         it "when x is set, no error is raised" do
           expect(resource.x "hi").to eq "hi"
           expect(resource.x).to eq "hi"
@@ -592,9 +599,6 @@ describe "Chef::Resource.property" do
         it "when x is retrieved, no validation error is raised" do
           expect(resource.x).to eq 10
         end
-        # it "when x is retrieved, a validation error is raised" do
-        #   expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
-        # end
       end
 
       with_property ":x, String, default: lazy { Namer.next_index }" do
@@ -605,40 +609,31 @@ describe "Chef::Resource.property" do
           expect(resource.x "hi").to eq "hi"
           expect(resource.x).to eq "hi"
         end
-        it "when x is retrieved, no validation error is raised" do
-          expect(resource.x).to eq 1
+        it "when x is retrieved, an invalid default warning is emitted and the value is returned" do
+          expect { resource.x }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
+            /Default value 1 is invalid for property x of resource chef_resource_property_spec_(\d+). In Chef 13 this will become an error: Option x must be one of: String!  You passed 1./
           expect(Namer.current_index).to eq 1
+          Chef::Config[:treat_deprecation_warnings_as_errors] = false
+          expect(resource.x).to eq 2
         end
-        # it "when x is retrieved, a validation error is raised" do
-        #   expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
-        #   expect(Namer.current_index).to eq 1
-        # end
       end
 
       with_property ":x, default: lazy { Namer.next_index.to_s }, is: proc { |v| Namer.next_index; true }" do
-        it "validation is not run at all on the default value" do
+        it "coercion and validation is only run the first time" do
           expect(resource.x).to eq "1"
-          expect(Namer.current_index).to eq 1
+          expect(Namer.current_index).to eq 2
+          expect(resource.x).to eq "1"
+          expect(Namer.current_index).to eq 2
         end
-        # it "validation is run each time" do
-        #   expect(resource.x).to eq '1'
-        #   expect(Namer.current_index).to eq 2
-        #   expect(resource.x).to eq '1'
-        #   expect(Namer.current_index).to eq 2
-        # end
       end
 
       with_property ":x, default: lazy { Namer.next_index.to_s.freeze }, is: proc { |v| Namer.next_index; true }" do
-        it "validation is not run at all on the default value" do
+        it "coercion and validation is run each time" do
           expect(resource.x).to eq "1"
-          expect(Namer.current_index).to eq 1
+          expect(Namer.current_index).to eq 2
+          expect(resource.x).to eq "3"
+          expect(Namer.current_index).to eq 4
         end
-        # it "validation is only run the first time" do
-        #   expect(resource.x).to eq '1'
-        #   expect(Namer.current_index).to eq 2
-        #   expect(resource.x).to eq '1'
-        #   expect(Namer.current_index).to eq 2
-        # end
       end
     end
 
@@ -716,12 +711,12 @@ describe "Chef::Resource.property" do
       end
 
       with_property ':x, proc { |v| Namer.next_index; true }, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
-        it "coercion is only run the first time x is retrieved, and validation is not run" do
+        it "coercion and validation is only run the first time x is retrieved" do
           expect(Namer.current_index).to eq 0
           expect(resource.x).to eq "101"
-          expect(Namer.current_index).to eq 1
+          expect(Namer.current_index).to eq 2
           expect(resource.x).to eq "101"
-          expect(Namer.current_index).to eq 1
+          expect(Namer.current_index).to eq 2
         end
       end
 
@@ -732,12 +727,12 @@ describe "Chef::Resource.property" do
           end
         end
         with_property ':x, Integer, coerce: proc { |v| "#{v}#{next_index}" }, default: 10' do
-          it "when x is retrieved, it is coerced and not validated" do
-            expect(resource.x).to eq "101"
+          it "when x is retrieved, it is coerced and emits an invalid default warning, but still returns the value" do
+            expect { resource.x }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
+              /Default value 10 is invalid for property x of resource chef_resource_property_spec_(\d+). In Chef 13 this will become an error: Option x must be one of: Integer!  You passed "101"./
+            Chef::Config[:treat_deprecation_warnings_as_errors] = false
+            expect(resource.x).to eq "102"
           end
-          # it "when x is retrieved, it is coerced before validating and fails" do
-          #   expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
-          # end
         end
         with_property ':x, String, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
           it "when x is retrieved, it is coerced before validating and passes" do
@@ -745,20 +740,20 @@ describe "Chef::Resource.property" do
           end
         end
         with_property ':x, Integer, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
-          it "when x is retrieved, it is coerced and not validated" do
-            expect(resource.x).to eq "101"
+          it "when x is retrieved, it is coerced and emits an invalid default warning; the value is still returned." do
+            expect { resource.x }.to raise_error Chef::Exceptions::DeprecatedFeatureError,
+              /Default value 10 is invalid for property x of resource chef_resource_property_spec_(\d+). In Chef 13 this will become an error: Option x must be one of: Integer!  You passed "101"./
+            Chef::Config[:treat_deprecation_warnings_as_errors] = false
+            expect(resource.x).to eq "102"
           end
-          # it "when x is retrieved, it is coerced before validating and fails" do
-          #   expect { resource.x }.to raise_error Chef::Exceptions::ValidationFailed
-          # end
         end
         with_property ':x, proc { |v| Namer.next_index; true }, coerce: proc { |v| "#{v}#{next_index}" }, default: lazy { 10 }' do
-          it "coercion is only run the first time x is retrieved, and validation is not run" do
+          it "coercion is only run the first time x is retrieved, and validation is run" do
             expect(Namer.current_index).to eq 0
             expect(resource.x).to eq "101"
-            expect(Namer.current_index).to eq 1
+            expect(Namer.current_index).to eq 2
             expect(resource.x).to eq "101"
-            expect(Namer.current_index).to eq 1
+            expect(Namer.current_index).to eq 2
           end
         end
       end
@@ -922,7 +917,8 @@ describe "Chef::Resource.property" do
         expect(Namer.current_index).to eq 1
       end
       it "does not emit a deprecation warning if set to nil" do
-        expect(resource.x nil).to eq "1"
+        # nil is never coerced
+        expect(resource.x nil).to be_nil
       end
       it "coercion sets the value (and coercion does not run on get)" do
         expect(resource.x 10).to eq "101"
diff --git a/spec/unit/provider/package/dpkg_spec.rb b/spec/unit/provider/package/dpkg_spec.rb
index e0bf2accde..f4c938ac7a 100644
--- a/spec/unit/provider/package/dpkg_spec.rb
+++ b/spec/unit/provider/package/dpkg_spec.rb
@@ -80,30 +80,26 @@ Conflicts: wget-ssl
       expect { provider.run_action(:purge) }.not_to raise_error
     end
 
-    it "should raise an exception if a source is nil when :install" do
-      new_resource.source nil
-      allow(::File).to receive(:exist?).with(source).and_return(false)
-      expect { provider.run_action(:install) }.to raise_error(Chef::Exceptions::Package)
-    end
+    context "when source is nil" do
+      let(:source) { nil }
 
-    it "should raise an exception if a source is nil when :upgrade" do
-      new_resource.source nil
-      allow(::File).to receive(:exist?).with(source).and_return(false)
-      expect { provider.run_action(:upgrade) }.to raise_error(Chef::Exceptions::Package)
-    end
+      it "should raise an exception if a source is nil when :install" do
+        expect { provider.run_action(:install) }.to raise_error(Chef::Exceptions::Package)
+      end
 
-    it "should not raise an exception if a source is nil when :remove" do
-      new_resource.source nil
-      allow(::File).to receive(:exist?).with(source).and_return(false)
-      expect(provider).to receive(:action_remove)
-      expect { provider.run_action(:remove) }.not_to raise_error
-    end
+      it "should raise an exception if a source is nil when :upgrade" do
+        expect { provider.run_action(:upgrade) }.to raise_error(Chef::Exceptions::Package)
+      end
 
-    it "should not raise an exception if a source is nil when :purge" do
-      new_resource.source nil
-      allow(::File).to receive(:exist?).with(source).and_return(false)
-      expect(provider).to receive(:action_purge)
-      expect { provider.run_action(:purge) }.not_to raise_error
+      it "should not raise an exception if a source is nil when :remove" do
+        expect(provider).to receive(:action_remove)
+        expect { provider.run_action(:remove) }.not_to raise_error
+      end
+
+      it "should not raise an exception if a source is nil when :purge" do
+        expect(provider).to receive(:action_purge)
+        expect { provider.run_action(:purge) }.not_to raise_error
+      end
     end
   end
 
diff --git a/spec/unit/resource_reporter_spec.rb b/spec/unit/resource_reporter_spec.rb
index aa162fd161..3dcd51deab 100644
--- a/spec/unit/resource_reporter_spec.rb
+++ b/spec/unit/resource_reporter_spec.rb
@@ -271,6 +271,7 @@ describe Chef::ResourceReporter do
           @bad_resource = Chef::Resource::File.new("/tmp/nameless_file.txt")
           allow(@bad_resource).to receive(:name).and_return(nil)
           allow(@bad_resource).to receive(:identity).and_return(nil)
+          allow(@bad_resource).to receive(:path).and_return(nil)
           @resource_reporter.resource_action_start(@bad_resource, :create)
           @resource_reporter.resource_current_state_loaded(@bad_resource, :create, @current_resource)
           @resource_reporter.resource_updated(@bad_resource, :create)
@@ -294,6 +295,7 @@ describe Chef::ResourceReporter do
           @bad_resource = Chef::Resource::File.new("/tmp/filename_as_hash.txt")
           allow(@bad_resource).to receive(:name).and_return({:foo=>:bar})
           allow(@bad_resource).to receive(:identity).and_return({:foo=>:bar})
+          allow(@bad_resource).to receive(:path).and_return({:foo=>:bar})
           @resource_reporter.resource_action_start(@bad_resource, :create)
           @resource_reporter.resource_current_state_loaded(@bad_resource, :create, @current_resource)
           @resource_reporter.resource_updated(@bad_resource, :create)