Attempting to sorbetize Diff::LCS

[halostatue]

I’m exploring Sorbet by attempting to apply it to Diff::LCS at halostatue/diff-lcs#67. Even if I get it working, it won’t be merged in the current release version because Diff::LCS has a wide support range (1.8.7 to current Ruby). I should have been committing each set of successful changes as different commits, but I didn’t.

I have turned on typed: true in a few files, but the latest file that I added it to (lib/diff/lcs.rb) appears to have some likely unfixable issues with how I understand Sorbet.

Here’s some general feedback:

1. Tuple specifications are useful, especially for type aliases and what would have been *args. https://github.com/halostatue/diff-lcs/blob/sorbet/lib/diff/lcs/change.rb#L10-L16

2. I originally had these constructors as def initialize(*args), then I shifted to my current form with def initialize(action, position, element, *). Sorbet didn’t like the last argument without a specification (even though that’s Ruby for “I really don’t care”), so I had to give this throwaway arg a name (_). It would be nice to be able to recognize bare * and ** as the throwaway values they are.

3. Dynamic splats. I had to change these from dynamic splats to explicit parameter splits because Sorbet didn’t like dynamic splats (the code here was previously *(arr[0...3])). https://github.com/halostatue/diff-lcs/blob/sorbet/lib/diff/lcs/change.rb#L74-L85

4. Unsupported method undef (quick bug report: typo here, Unsuppored method). I could this by creating a Diff::LCS::AbstractChange class that doesn’t contain position or element, but this is code that I started in ~2004 or so. https://github.com/halostatue/diff-lcs/blob/sorbet/lib/diff/lcs/change.rb#L139-L144

This seems fairly reasonable to me, but I will acknowledge that this may be a code smell.

lib/diff/lcs/change.rb:143: Unsuppored method: undef https://srb.help/3008
     143 |  undef :position
            ^^^^^^^^^^^^^^^

lib/diff/lcs/change.rb:144: Unsuppored method: undef https://srb.help/3008
     144 |  undef :element
            ^^^^^^^^^^^^^^

5. Module support. This is one of a couple of things that’s going to make adopting Sorbet very hard, because while Diff::LCS is meant to be used as module/class methods (Diff::LCS.diff, etc.) it can also be mixed into random access sequences (e.g., Array or String). So the following errors don’t really make sense to me, because the Diff::LCS module itself should not include Kernel.

lib/diff/lcs.rb:130: Method `respond_to?` does not exist on `Diff::LCS` https://srb.help/7003
     130 |    if respond_to?(:replace)
                 ^^^^^^^^^^^^^^^^^^^^^
  Did you mean to `include Kernel` in this module?
    https://github.com/sorbet/sorbet/tree/343e75f47cd770744456c155e5a3ee6c5130b64d/rbi/core/kernel.rbi#L571: Did you mean: `Kernel#respond_to?`?
     571 |  def respond_to?(arg0,include_all=false); end
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

lib/diff/lcs.rb:131: Method `replace` does not exist on `Diff::LCS` https://srb.help/7003
     131 |      replace(patch!(patchset))
                ^^^^^^^^^^^^^^^^^^^^^^^^^

6. Class/Module name typing is going to work very poorly for some cases. I have defined a type alias for SequenceT = T.type_alias { T.any(Diff::LCS, Enumerable, String) }, but it’s not a correct alias, and it’s not correct. The code mentioned in the error below is suboptimal because it uses .count (it had been using .size, which is the correct type). What I need to say here is something that amounts to:

• This method works on Strings, Arrays, or other Enumerables that implement #size and #[] (and probably #[]=, but I’m not 100% sure; it also needs to support #[]/#[]= in the same way that Array does, for replacement or insertion). Really what I’m looking for is something trait-ish, but more duck typing than the name typing/resolution here. I’m really not sure how to indicate this. I could do some runtime checking of included on Diff::LCS to make sure that #size, #[], and #[]= are defined on the class, but that doesn’t help with Sorbet.

This is also currently blocking me because I’ve gone from 2 errors (and tests pass) to 89 errors and the tests don’t look like they’re going to pass.

lib/diff/lcs.rb:298: Method `count` does not exist on `Diff::LCS` component of `T.any(T::Enumerable[T.untyped], Diff::LCS)` https://srb.help/7003
     298 |    a_size = seq1.count
                       ^^^^^^^^^^

7. What are the expected best practices for gem authors? Should we only require 'sorbet-runtime' in our tests? Should we somehow create .rbi files and leave our code unannotated otherwise?

There are other errors, but this is by and large the stuff that’s currently blocking me with no clear way forward.

[DarkDimius]

Ack, thank you.

This is also currently blocking me because

You should be able to unblock yourself by using T.unsafe(seq1).count and similar. It is true that adopting sorbet

had to give this throwaway arg a name (_)

The decision that we have made when developing Sorbet is that passing arguments to methods that aren't used is a smell. It leads to confusion of "what does this method need" and thus "what does it do/what does it guarantee". I understand that with a smaller and well understood codebase that doesn't change much, these may not sound like an issue, but with dozens millions of lines of code that evolves quickly these are important concerns to address.

So the following errors don’t really make sense to me, because the Diff::LCS module itself should not include Kernel.

We believe this is valid error because respond_to isn't guaranteed to exist:

[1] pry(main)> s = BasicObject.new
=> #<BasicObject:0x3fe1515fde7c>
[2] pry(main)> s.respond_to(:s)
NoMethodError: undefined method `respond_to' for #<BasicObject:0x00007fc2a2bfbcf8>
from (pry):2:in `__pry__'

Indeed, it might be too strict and the backdoor is T.unsafe

Really what I’m looking for is something trait-ish, but more duck typing than the name typing/resolution here. I’m really not sure how to indicate this.

We went back & forward on this one in early days of Sorbet. At this point, it's very unlikely we'll add structural types. We have found that for existing libraries using T.unsafe as a term / T.untyped typed is an option, while not having structural types leads to new code that is safer and easier to understand for engineers. This also mimics experience of other typecheckers in the area.

What are the expected best practices for gem authors?

TBH, we aren't a good place to ask: Stripe does not use gems(we are a monorepo place). AFAIK Shoppify was working on sharing how they use gems in their systems(that, if I'm not mistaking, can be summarized by require 'sorbet-runtime' in tests & prod, but they disable typechecking overhead in prod(Stripe doesn't))

[jez]

Thank you for the feedback!

Feel free to open bugs if they are worth tracking separately.