Enforce that parameters position with sorbet-static

[Morriar]

Motivation

There are three mismatch between sorbet-static and sorbet-runtime regarding parameters:

1. sorbet-runtime ask for required parameters to be defined before the optional ones
2. sorbet-runtime ask for required keyword parameters to be defined before the optional ones
3. sorbet-runtime ask for the parameters order in the method to be the same than the signature

None of these checks are implemented in sorbet-static yet.

This pull-request implements 2. and 3..
For 1., it is possible to implement it but it conflicts with some definitions in socket.rbi.

Test plan

Few examples from the tests:

sig {params(x: T1, y: T2, z: T3).void}
def f2(x:, y: T2.new, z:); end
#                     ^^ error: Malformed `sig`. Required parameter `z` must be declared before all the optional ones

sig { params(a: Integer, b: String).void }
def f2(b, a); end
#      ^ error: Bad parameter ordering for `b`, expected `a` instead
#         ^ error: Bad parameter ordering for `a`, expected `b` instead

sig {params(v: T1, w: T2, x: T1, y: T2, z: T3).void}
def f3(v, w = T2.new, *y, z:, x: T1.new); end
#                      ^ error: Bad parameter ordering for `y`, expected `x` instead
#                         ^^ error: Bad parameter ordering for `z`, expected `y` instead
#                             ^^ error: Bad parameter ordering for `x`, expected `z` instead

See included automated tests.

Closes #174.

[DarkDimius]

While I do like the proposed requirement, as it makes the code more readable.
That said ruby does accept those method definitions. Thus we should prohibit them outright.

WDYT about making this into a separate error class that is enabled in strict?

BTW: thank you for great tests

[DarkDimius]

due to ^^^ I expect this will break a lot of people.
Let's make it less invasive by making it a:

• separate error code
• that's only enabled at strict and higher

[Morriar]

@DarkDimius:

WDYT about making this into a separate error class that is enabled in strict?

The problem we have with the current state is that sorbet-runtime will raise for errors not caught by sorbet-static. So we're at risk of something raising in production while we could not catch it on CI.

To avoid this problem we have two cops ran on CI especially for the parameters ordering:

• https://github.com/Shopify/rubocop-sorbet/blob/master/lib/rubocop/cop/sorbet/parameters_ordering_in_signature.rb
• https://github.com/Shopify/rubocop-sorbet/blob/master/lib/rubocop/cop/sorbet/keyword_argument_ordering.rb

My goal with this PR was to remove the need of these cops.

Moving this check to strict or strong doesn't really fixes it for us as false and true are still at risk of raising in production.

I added a commit to bind these errors to another class especially for the parameters ordering: 5045: BadParameterOrdering.

WDYT about leaving this in false level and let users blacklist it if needed?

[DarkDimius]

WDYT about leaving this in false level and let users blacklist it if needed?

our current policy is that the moment you blacklist an error, you're not supported, thus I'd like to not introduce a common error that open-source folks will blacklist.

Moving this check to strict or strong doesn't really fixes it for us as false and true are still at risk of raising in production.

Let me carefully see how many breackages does this have in open-source & in our repo to get indication if this is problematic in practice and get inspiration for how can we address all usecases here.

[DarkDimius]

Hi Alexandre,
Would you kindly rebase and run ./tools/scripts/update_exp_files.sh?
Other than that, I think we can merge! 🎉
Thank you!

[Morriar]

^ @DarkDimius rebased on master.

Thank you!

[Morriar]

@DarkDimius is there something else to do with this pull request?

[DarkDimius]

Working on landing it

[DarkDimius]

Thank you, sorry it took so long!