Merge pull request #305 from egonSchiele/fix/keyword-arguments-with-optional-positional-arguments

Fix keyword arguments contract when used with optional positional arguments

Author: PikachuEXE

features/builtin_contracts/keyword_args_with_optional_positional_args.feature

@@ -0,0 +1,76 @@
+Feature: KeywordArgs when used with optional positional arguments
+
+  Checks that the argument is an options hash, and all required keyword arguments are present, and all values pass their respective contracts
+
+  ```ruby
+  Contract Any, KeywordArgs[:number => Num, :description => Optional[String]] => Any
+  ```
+
+  Background:
+    Given a file named "keyword_args_with_optional_positional_args_usage.rb" with:
+    """ruby
+    require "contracts"
+    C = Contracts
+
+    class Example
+      include Contracts::Core
+
+      Contract C::Any, String, C::KeywordArgs[b: C::Optional[String]] => Symbol
+      def foo(output, a = 'a', b: 'b')
+        p [a, b]
+        output
+      end
+    end
+    """
+
+  Scenario: Accepts arguments when only require arguments filled and valid
+    Given a file named "accepts_all_filled_valid_args.rb" with:
+    """ruby
+    require "./keyword_args_with_optional_positional_args_usage"
+    puts Example.new.foo(:output)
+    """
+    When I run `ruby accepts_all_filled_valid_args.rb`
+    Then output should contain:
+    """
+    ["a", "b"]
+    output
+    """
+
+  Scenario: Accepts arguments when all filled and valid
+    Given a file named "accepts_all_filled_valid_args.rb" with:
+    """ruby
+    require "./keyword_args_with_optional_positional_args_usage"
+    puts Example.new.foo(:output, 'c', b: 'd')
+    """
+    When I run `ruby accepts_all_filled_valid_args.rb`
+    Then output should contain:
+    """
+    ["c", "d"]
+    output
+    """
+
+  Scenario: Accepts arguments when only require arguments & optional keyword arguments filled and valid
+    Given a file named "accepts_all_filled_valid_args.rb" with:
+    """ruby
+    require "./keyword_args_with_optional_positional_args_usage"
+    puts Example.new.foo(:output, b: 'd')
+    """
+    When I run `ruby accepts_all_filled_valid_args.rb`
+    Then output should contain:
+    """
+    ["a", "d"]
+    output
+    """
+
+  Scenario: Accepts arguments when only require arguments & optional positional arguments filled and valid
+    Given a file named "accepts_all_filled_valid_args.rb" with:
+    """ruby
+    require "./keyword_args_with_optional_positional_args_usage"
+    puts Example.new.foo(:output, 'c')
+    """
+    When I run `ruby accepts_all_filled_valid_args.rb`
+    Then output should contain:
+    """
+    ["c", "b"]
+    output
+    """

lib/contracts.rb

@@ -53,7 +53,7 @@ class Contract < Contracts::Decorator
     end
   end
 
-  attr_reader :args_contracts, :ret_contract, :klass, :method
+  attr_reader :args_contracts, :kargs_contract, :ret_contract, :klass, :method
 
   def initialize(klass, method, *contracts)
     super(klass, method)
@@ -70,13 +70,18 @@ def initialize(klass, method, *contracts)
 
     # internally we just convert that return value syntax back to an array
     @args_contracts = contracts[0, contracts.size - 1] + contracts[-1].keys
+    # Extract contract for keyword arguments
+    @kargs_contract = args_contracts.find { |c| c.is_a?(Contracts::Builtin::KeywordArgs) }
+    args_contracts.delete(kargs_contract) if kargs_contract
 
     @ret_contract = contracts[-1].values[0]
 
     @args_validators = args_contracts.map do |contract|
       Contract.make_validator(contract)
     end
 
+    @kargs_validator = kargs_contract ? Contract.make_validator(kargs_contract) : nil
+
     @args_contract_index = args_contracts.index do |contract|
       contract.is_a? Contracts::Args
     end
@@ -94,16 +99,6 @@ def initialize(klass, method, *contracts)
 
     # ====
 
-    # == @has_options_contract
-    last_contract = args_contracts.last
-    penultimate_contract = args_contracts[-2]
-    @has_options_contract = if @has_proc_contract
-                              penultimate_contract.is_a?(Contracts::Builtin::KeywordArgs)
-                            else
-                              last_contract.is_a?(Contracts::Builtin::KeywordArgs)
-                            end
-    # ===
-
     @klass, @method = klass, method
   end
 
@@ -256,19 +251,6 @@ def maybe_append_block! args, blk
     true
   end
 
-  # Same thing for when we have named params but didn't pass any in.
-  # returns true if it appended nil
-  def maybe_append_options! args, kargs, blk
-    return false unless @has_options_contract
-
-    if @has_proc_contract && args_contracts[-2].is_a?(Contracts::Builtin::KeywordArgs)
-      args.insert(-2, kargs)
-    elsif args_contracts[-1].is_a?(Contracts::Builtin::KeywordArgs)
-      args << kargs
-    end
-    true
-  end
-
   # Used to determine type of failure exception this contract should raise in case of failure
   def failure_exception
     if pattern_match?

lib/contracts/call_with.rb

@@ -12,8 +12,20 @@ def call_with_inner(returns, this, *args, **kargs, &blk)
       # Explicitly append blk=nil if nil != Proc contract violation anticipated
       nil_block_appended = maybe_append_block!(args, blk)
 
-      # Explicitly append options={} if Hash contract is present
-      kargs_appended = maybe_append_options!(args, kargs, blk)
+      if @kargs_validator && !@kargs_validator[kargs]
+        data = {
+          arg:          kargs,
+          contract:     kargs_contract,
+          class:        klass,
+          method:       method,
+          contracts:    self,
+          arg_pos:      :keyword,
+          total_args:   args.size,
+          return_value: false,
+        }
+        return ParamContractError.new("as return value", data) if returns
+        return unless Contract.failure_callback(data)
+      end
 
       # Loop forward validating the arguments up to the splat (if there is one)
       (@args_contract_index || args.size).times do |i|
@@ -84,7 +96,6 @@ def call_with_inner(returns, this, *args, **kargs, &blk)
       # If we put the block into args for validating, restore the args
       # OR if we added a fake nil at the end because a block wasn't passed in.
       args.slice!(-1) if blk || nil_block_appended
-      args.slice!(-1) if kargs_appended
       result = if method.respond_to?(:call)
                  # proc, block, lambda, etc
                  method.call(*args, **kargs, &blk)