Merge branch 'jk/nil-for-reals'

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..f2b16d5067 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 == "#{RbConfig::CONFIG['bindir']}/gem" }
                }
-      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)