Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Contract error when mixing positional and keyword arguments with defaults #304

Closed
vlad-pisanov opened this issue Jul 23, 2024 · 12 comments · Fixed by #305
Closed

Contract error when mixing positional and keyword arguments with defaults #304

vlad-pisanov opened this issue Jul 23, 2024 · 12 comments · Fixed by #305

Comments

@vlad-pisanov
Copy link
Contributor

I'm having trouble defining a contract for a method that takes two optional arguments -- a positional one and a keyword one. Consider:

class C
  include Contracts

  Contract String, KeywordArgs[b: Optional[String]] => Any
  def foo(a = 'a', b: 'b')
    p [a, b]
  end
end

c = C.new
c.foo('a', b: 'b') # Works ✔️
c.foo('a')         # Works ✔️
c.foo(b: 'b')      # Contract error ❌
c.foo              # Contract error ❌

The stack trace is

Contract violation for argument 1 of 1: (ParamContractError)
        Expected: String,
        Actual: {:b=>"b"}
        Value guarded in: C::foo
        With Contract: String, KeywordArgs => Any
        At: c.rb:9

      fail data[:contracts].failure_exception.new(failure_msg(data), data)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        from /Users/main/.gem/ruby/3.3.2/gems/contracts-0.17/lib/contracts.rb:197:in `failure_callback'
        from /Users/main/.gem/ruby/3.3.2/gems/contracts-0.17/lib/contracts/method_handler.rb:144:in `block in redefine_method'

It seems Contracts wants me to provide the positional argument even if a default is specified in the method signature? 🤔

@PikachuEXE
Copy link
Collaborator

I don't even know how it handles positional optional arguments
I see nothing from https://egonschiele.github.io/contracts.ruby/
Have you used contract on methods with positional optional arguments only?

@vlad-pisanov
Copy link
Contributor Author

It is indeed not documented, but it works as expected when only positional arguments are used:

class C
  include Contracts

  Contract String, String => Any
  def goo(a = 'a', b = 'b')
    p [a, b]
  end
end

c = C.new
c.goo           # Works ✔️
c.goo('a')      # Works ✔️
c.goo('a', 'b') # Works ✔️

The issue seems to be in KeywordArgs, because if I replace the original example with Any, everything works again:

class C
  include Contracts

  Contract String, Any => Any
  def hoo(a = 'a', b: 'b')
    p [a, b]
  end
end

c = C.new
c.hoo('a', b: 'b') # Works ✔️
c.hoo('a')         # Works ✔️
c.hoo(b: 'b')      # Works ✔️
c.hoo              # Works ✔️

@PikachuEXE
Copy link
Collaborator

I think it's a deeper issue of lacking a contract for optional positional argument
From my limited understand it works like this

  1. Declare contracts, e.g. Contract contract_at_pos_1, contract_at_pos_2, contract_keyword_args
  2. When called, check against contracts according to position: arg1 => contract_at_pos_1, arg2 => contract_at_pos_2...

It's fine for methods with optional arguments only coz it works like:
Contracts declared: contract_at_pos_1, contract_at_pos_2
When some arguments not passed, it checks like: arg1 => contract_at_pos_1, no arg2 = no check

But if optional positional argument is mixed with keyword arguments
Contracts declared: contract_at_pos_1, contract_keyword_args
It checks like: keyword_args => contract_at_pos_1

I am guessing from
https://github.com/egonSchiele/contracts.ruby/blob/v0.17/lib/contracts/call_with.rb#L51-L81

@PikachuEXE
Copy link
Collaborator

Please check #305 and see if you want to add more examples to ensure I don't break anything
Use it in your app for a while to ensure nothing breaks

@PikachuEXE
Copy link
Collaborator

I won't merge #305 until some has tested it~

@vlad-pisanov
Copy link
Contributor Author

Thanks for the patch! I'll test it later this week.

@vlad-pisanov
Copy link
Contributor Author

@PikachuEXE everything looks good! 🚀 No issues in our 4K+ file repo.

@PikachuEXE
Copy link
Collaborator

Let me release it next week if no other PR blocking

@PikachuEXE
Copy link
Collaborator

I forgot I don't have release right in this gem
@egonSchiele Please release a new version 0.17.1 thx
Changes: v0.17...master

@egonSchiele
Copy link
Owner

egonSchiele commented Sep 28, 2024 via email

@egonSchiele
Copy link
Owner

done

@PikachuEXE
Copy link
Collaborator

Tag on wrong commit 😅
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants